feat(llc, persistence): add offline support for reactions - #2847
feat(llc, persistence): add offline support for reactions#2847VelikovPetar wants to merge 13 commits into
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 💡 1🛠️ Fix failing CI checks 💡
🧪 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.83% 72.93% +0.09%
==========================================
Files 428 433 +5
Lines 27658 27758 +100
==========================================
+ Hits 20145 20244 +99
- Misses 7513 7514 +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
# Conflicts: # packages/stream_chat/test/src/client/client_test.dart
2 participants
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