feat(llc, persistence): add offline support for reactions - #2847
feat(llc, persistence): add offline support for reactions#2847VelikovPetar wants to merge 10 commits into
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds durable pending-operation storage for reaction mutations, queues retriable offline reaction sends/deletes, and replays them in order after websocket recovery. Persistence includes Drift schema, DAO, mappings, client methods, mocks, and tests. ChangesOffline reaction operations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StreamChatClient
participant PendingOperationReplayer
participant ChatPersistenceClient
participant StreamChatAPI
StreamChatClient->>PendingOperationReplayer: replay on connectionRecovered
PendingOperationReplayer->>ChatPersistenceClient: getPendingOperations
PendingOperationReplayer->>StreamChatAPI: replay reaction operation
StreamChatAPI-->>PendingOperationReplayer: success or terminal response
PendingOperationReplayer->>ChatPersistenceClient: deletePendingOperation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
# Conflicts: # packages/stream_chat/CHANGELOG.md
| // optimistic `ownReactions` for messages that previously had none. | ||
| state?.replaceMessage(message); | ||
| } | ||
| rethrow; |
There was a problem hiding this comment.
Not entirely sure if we should always rethrow here, maybe just in the non-retry-able case?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2847 +/- ##
==========================================
+ Coverage 72.77% 72.87% +0.09%
==========================================
Files 428 433 +5
Lines 27602 27702 +100
==========================================
+ Hits 20088 20187 +99
- Misses 7514 7515 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/stream_chat_persistence/lib/src/dao/pending_operation_dao.dart (1)
19-23: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider bounding the queue.
getPendingOperationsloads the entire table into memory and the queue has no cap, so a long offline session (or an operation that keeps failing retriably) can grow it without bound. Alimiton the read, or a retention cap/TTL on insert, would keep replay predictable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat_persistence/lib/src/dao/pending_operation_dao.dart` around lines 19 - 23, Bound the pending-operation queue in getPendingOperations by applying an explicit maximum read limit while preserving ascending id order, using the project’s established queue-size configuration or constant if available. Ensure replay remains predictable without loading the entire pendingOperations table into memory.packages/stream_chat_persistence/lib/src/db/drift_chat_database.dart (1)
61-82: 🗄️ Data Integrity & Integration | 🔵 TrivialDestructive migration will silently drop unreplayed pending operations on future schema bumps.
onUpgradedeletes and recreates every table (including the newpending_operationstable) wheneverschemaVersionchanges. For the other tables this is a harmless cache eviction, butpending_operationsnow holds durable, not-yet-acknowledged user mutations — a future schema bump before replay would silently drop a user's queued reaction/delete with no error surfaced.Consider special-casing
pending_operationsin the migration (e.g. preserve/re-insert its rows acrossonUpgrade), or at minimum documenting this data-loss risk for maintainers bumpingschemaVersiongoing forward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat_persistence/lib/src/db/drift_chat_database.dart` around lines 61 - 82, Update the MigrationStrategy.onUpgrade flow so schema bumps do not silently delete durable rows from the pending_operations table: preserve and restore its pending operations while recreating the other tables, or otherwise explicitly retain that table and migrate it safely. Keep the existing destructive cache migration for non-durable tables, and ensure queued mutations remain available for replay after upgrades.packages/stream_chat/test/src/client/pending_operation_replayer_test.dart (1)
38-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated reaction pending-operation test scaffolding across two files.
addOpandstubSendReactionOkare copy-pasted verbatim between the two suites; the shared root cause is a missing common test helper for building/stubbing reaction pending operations.
packages/stream_chat/test/src/client/pending_operation_replayer_test.dart#L38-L62: keep as the canonical definition (or move) and have the other site import it.packages/stream_chat/test/src/client/client_test.dart#L5470-L5489: replace this local copy with an import of the shared helper instead of redefiningaddOp/stubSendReactionOk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat/test/src/client/pending_operation_replayer_test.dart` around lines 38 - 62, The reaction pending-operation helpers addOp and stubSendReactionOk are duplicated across the test suites. Keep or extract the canonical shared definitions from packages/stream_chat/test/src/client/pending_operation_replayer_test.dart:38-62, then update packages/stream_chat/test/src/client/client_test.dart:5470-5489 to import and reuse them, removing its local copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stream_chat/lib/src/client/channel.dart`:
- Around line 1685-1695: Update _enqueuePendingOperation to return whether
insertPendingOperation succeeded instead of swallowing the outcome. At both call
sites, including sendReaction, only retain the optimistic state when enqueue
succeeds; when it fails, fall back to state?.replaceMessage(message) so the
local reaction is reconciled.
In `@packages/stream_chat/lib/src/client/pending_operation_replayer.dart`:
- Around line 89-110: Validate operation.targetMessageId before returning either
deferred replay closure in _replayCallFor, and treat a null value as malformed
so the existing caller drops it through its malformed-payload handling. Replace
the deferred non-null assertions used by sendReaction and deleteReaction with
the validated value while preserving the existing replay behavior for valid
operations.
In `@packages/stream_chat/lib/src/db/chat_persistence_client.dart`:
- Around line 359-367: Update the doc comments for insertPendingOperation,
getPendingOperations, and deletePendingOperation to explicitly warn that the
default no-op implementations silently discard pending operations and can
prevent optimistic reaction replay; instruct custom ChatPersistenceClient
implementations to override all three methods together when persistence is
enabled.
---
Nitpick comments:
In `@packages/stream_chat_persistence/lib/src/dao/pending_operation_dao.dart`:
- Around line 19-23: Bound the pending-operation queue in getPendingOperations
by applying an explicit maximum read limit while preserving ascending id order,
using the project’s established queue-size configuration or constant if
available. Ensure replay remains predictable without loading the entire
pendingOperations table into memory.
In `@packages/stream_chat_persistence/lib/src/db/drift_chat_database.dart`:
- Around line 61-82: Update the MigrationStrategy.onUpgrade flow so schema bumps
do not silently delete durable rows from the pending_operations table: preserve
and restore its pending operations while recreating the other tables, or
otherwise explicitly retain that table and migrate it safely. Keep the existing
destructive cache migration for non-durable tables, and ensure queued mutations
remain available for replay after upgrades.
In `@packages/stream_chat/test/src/client/pending_operation_replayer_test.dart`:
- Around line 38-62: The reaction pending-operation helpers addOp and
stubSendReactionOk are duplicated across the test suites. Keep or extract the
canonical shared definitions from
packages/stream_chat/test/src/client/pending_operation_replayer_test.dart:38-62,
then update packages/stream_chat/test/src/client/client_test.dart:5470-5489 to
import and reuse them, removing its local copies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18d78a56-10bc-41d9-8129-154bb6dd524d
📒 Files selected for processing (25)
packages/stream_chat/CHANGELOG.mdpackages/stream_chat/lib/src/client/channel.dartpackages/stream_chat/lib/src/client/client.dartpackages/stream_chat/lib/src/client/pending_operation_replayer.dartpackages/stream_chat/lib/src/client/reaction_pending_operation.dartpackages/stream_chat/lib/src/core/models/pending_operation.dartpackages/stream_chat/lib/src/db/chat_persistence_client.dartpackages/stream_chat/lib/stream_chat.dartpackages/stream_chat/test/src/client/channel_test.dartpackages/stream_chat/test/src/client/client_test.dartpackages/stream_chat/test/src/client/pending_operation_replayer_test.dartpackages/stream_chat/test/src/core/models/pending_operation_test.dartpackages/stream_chat/test/src/mocks.dartpackages/stream_chat_persistence/CHANGELOG.mdpackages/stream_chat_persistence/lib/src/dao/dao.dartpackages/stream_chat_persistence/lib/src/dao/pending_operation_dao.dartpackages/stream_chat_persistence/lib/src/dao/pending_operation_dao.g.dartpackages/stream_chat_persistence/lib/src/db/drift_chat_database.dartpackages/stream_chat_persistence/lib/src/db/drift_chat_database.g.dartpackages/stream_chat_persistence/lib/src/entity/entity.dartpackages/stream_chat_persistence/lib/src/entity/pending_operations.dartpackages/stream_chat_persistence/lib/src/mapper/mapper.dartpackages/stream_chat_persistence/lib/src/mapper/pending_operation_mapper.dartpackages/stream_chat_persistence/lib/src/stream_chat_persistence_client.dartpackages/stream_chat_persistence/test/src/dao/pending_operation_dao_test.dart
- Roll back the optimistic reaction when enqueueing the pending operation fails, so the local state can no longer diverge from the server. - Validate targetMessageId eagerly in the replayer so a malformed operation is dropped instead of being retried forever. - Document the default no-op pending-operation methods on ChatPersistenceClient and group them at the bottom of the public surface. - Add persistence-client delegation tests for the pending-operation methods. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the nitpicks — decisions on each: Bound the queue ( Destructive migration drops pending ops ( Duplicated test helpers: leaving the small duplication. The two suites test different layers (replayer semantics vs. client wiring) and the helpers are tiny. |
…fline_support' into feature/FLU-506_add_reactions_offline_support
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stream_chat_persistence/lib/src/db/drift_chat_database.g.dart`:
- Around line 4275-4574: The generated Drift artifact was edited directly
instead of being regenerated. Update the source Drift table and database
definitions corresponding to PendingOperations, then run the project’s Drift
code-generation command to recreate drift_chat_database.g.dart and keep the
table, PendingOperationEntity, and PendingOperationsCompanion definitions
synchronized.
- Around line 4315-4358: Regenerate the Drift-generated table code with Drift
2.33.0 so PendingOperationsTable.validateIntegrity includes payload metadata
validation and the required-column missing check for payload. Ensure the
generated handling covers both provided values and inserts that omit the
required converted payload column.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 624dca50-9bf9-44ce-84bb-7c2169f8ad9a
📒 Files selected for processing (2)
packages/stream_chat_persistence/lib/src/dao/pending_operation_dao.g.dartpackages/stream_chat_persistence/lib/src/db/drift_chat_database.g.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_chat_persistence/lib/src/dao/pending_operation_dao.g.dart
1 participant
Submit a pull request
Linear: FLU-506
Github Issue: #
CLA
Description of the pull request
Adds offline support for reactions. When offline storage is enabled and a reaction add/remove fails with a transient/offline error, the optimistic change is kept and queued for replay on reconnect instead of being reverted.
Low-level client (
stream_chat):PendingOperationmodel andReactionPendingOperation(add/delete) describing a queued reaction op.PendingOperationReplayerthat replays queued operations independently (per-op, single attempt) when connectivity is restored.Channel.sendReaction/Channel.deleteReactionnow enqueue a pending operation on a retriable error (when persistence is enabled) rather than rolling back the optimistic state.ChatPersistenceClientinterface extended with pending-operation persistence methods.Persistence (
stream_chat_persistence):PendingOperationsDrift table,PendingOperationDao, entity, and mapper.DriftChatDatabaseandStreamChatPersistenceClient.Tests:
sample_appE2E reactions integration test.Screenshots / Videos
No UI changes.
Summary by CodeRabbit