Fix/issue 78 like count drift clean#79
Open
1amKhush wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new optimistic-state module and tests, integrates optimistic like/unlike handling into the like component, implements clamped counts, snapshot-based rollback that only reverts when an optimistic update was applied, and defers authoritative count resyncs after failures. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant Optim as Optimistic Module
participant Pub as Publisher/Signer
participant Relay as Relay/Server
participant Store as Authoritative Count
UI->>Optim: capture snapshot (LikeUiState)
UI->>Optim: applyOptimisticLike/unlike -> updated UI state
UI->>Pub: publish like/unlike mutation
Pub->>Relay: send mutation
alt mutation succeeds
Relay->>Store: authoritative update
Relay-->>UI: success
UI->>UI: updateLikeCount(result.totalCount) [clamp]
else mutation fails
Relay-->>Pub: error
Pub-->>UI: error
UI->>Optim: rollbackOptimisticLikeState(current, snapshot, didApply)
UI->>UI: restore isLiked/likeCount (clamped)
UI->>Store: queueAuthoritativeCountResync()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Resolves #78 by making like/unlike failure handling deterministic and preventing like-count drift when optimistic updates were not actually applied.
Changes:
- Added pure helpers for clamping, optimistic updates, and rollback decisioning (
optimistic-state.ts). - Refactored like/unlike mutation error handling to use snapshot-based rollback and clamp fetched totals (
nostr-like.ts). - Added unit tests covering clamp behavior and failure-before/after-optimistic rollback semantics (
optimistic-state.test.ts).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/nostr-like-button/optimistic-state.ts | Introduces pure helpers for clamping counts and performing deterministic rollback decisions. |
| src/nostr-like-button/nostr-like.ts | Uses helpers to clamp relay totals and to rollback only when an optimistic update was applied; adds best-effort resync. |
| src/nostr-like-button/tests/optimistic-state.test.ts | Adds regression tests for the optimistic-state helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/nostr-like-button/nostr-like.ts (1)
169-179: Coalesce resync requests instead of dropping them.The boolean guard prevents concurrent refreshes, but it also discards any failure that happens while an earlier resync is still in flight. If that earlier fetch started before the later failure, its result can already be stale and no follow-up fetch will run. A small
needsResyncflag or loop would preserve the latest reconciliation request without allowing overlap.♻️ One way to keep the latest resync request
private isResyncingLikeCount = false; +private needsLikeCountResync = false; private queueAuthoritativeCountResync(): void { - if (this.isResyncingLikeCount) return; - - this.isResyncingLikeCount = true; + this.needsLikeCountResync = true; + if (this.isResyncingLikeCount) return; + + this.isResyncingLikeCount = true; void (async () => { try { - await this.updateLikeCount(); + while (this.needsLikeCountResync) { + this.needsLikeCountResync = false; + await this.updateLikeCount(); + } } finally { this.isResyncingLikeCount = false; } })(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nostr-like-button/nostr-like.ts` around lines 169 - 179, The current queueAuthoritativeCountResync uses isResyncingLikeCount to block concurrent runs but drops any resync requests while a run is in-flight; change it to coalesce requests by adding a needsResync flag and looping: when queueAuthoritativeCountResync is called, set needsResync = true and if isResyncingLikeCount return; otherwise set isResyncingLikeCount = true and run a loop that clears needsResync, calls await this.updateLikeCount(), then if needsResync is true continue another iteration, finally set isResyncingLikeCount = false; reference and update the existing isResyncingLikeCount, queueAuthoritativeCountResync, and updateLikeCount symbols.src/nostr-like-button/__tests__/optimistic-state.test.ts (1)
11-55: Add one component-level regression test for theNostrLikewiring.This suite proves the pure helper, but it would still pass if
handleLike()/handleUnlike()setdidApplyOptimisticUpdateat the wrong time or skipped the resync path. A mocked failure test aroundsrc/nostr-like-button/nostr-like.tswould cover the actual bug surface this PR is fixing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nostr-like-button/__tests__/optimistic-state.test.ts` around lines 11 - 55, Add a component-level regression test exercising the NostrLike component wiring (src/nostr-like-button/nostr-like.ts) that simulates a failed server mutation after an optimistic update: mount or render NostrLike, trigger handleLike() (and/or handleUnlike()) to apply the optimistic update, mock the mutation to reject, and assert that the component calls the rollback/resync path and that didApplyOptimisticUpdate is interpreted correctly (i.e., state is restored to the snapshot when didApplyOptimisticUpdate=true and current state is clamped when didApplyOptimisticUpdate=false). Ensure the test mocks the network/mutation layer used by NostrLike so the failure occurs after the optimistic change, and assert final DOM/state matches the expected post-rollback values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nostr-like-button/nostr-like.ts`:
- Around line 165-204: The optimistic-update flag is being set only after await
ndkEvent.publish(), so the catch path that calls handleLikeMutationFailure
(which relies on didApplyOptimisticUpdate and rollbackOptimisticLikeState) is
nearly never exercised; move the code that applies the optimistic UI change and
flips didApplyOptimisticUpdate to before calling ndkEvent.publish(), or
alternatively allow updateLikeCount() errors to propagate (remove its internal
try/catch) so post-publish refresh failures hit the outer catch and trigger
handleLikeMutationFailure; update references in the publish flow and tests
accordingly (look for ndkEvent.publish, rollbackOptimisticLikeState,
didApplyOptimisticUpdate, updateLikeCount, handleLikeMutationFailure, and
queueAuthoritativeCountResync).
---
Nitpick comments:
In `@src/nostr-like-button/__tests__/optimistic-state.test.ts`:
- Around line 11-55: Add a component-level regression test exercising the
NostrLike component wiring (src/nostr-like-button/nostr-like.ts) that simulates
a failed server mutation after an optimistic update: mount or render NostrLike,
trigger handleLike() (and/or handleUnlike()) to apply the optimistic update,
mock the mutation to reject, and assert that the component calls the
rollback/resync path and that didApplyOptimisticUpdate is interpreted correctly
(i.e., state is restored to the snapshot when didApplyOptimisticUpdate=true and
current state is clamped when didApplyOptimisticUpdate=false). Ensure the test
mocks the network/mutation layer used by NostrLike so the failure occurs after
the optimistic change, and assert final DOM/state matches the expected
post-rollback values.
In `@src/nostr-like-button/nostr-like.ts`:
- Around line 169-179: The current queueAuthoritativeCountResync uses
isResyncingLikeCount to block concurrent runs but drops any resync requests
while a run is in-flight; change it to coalesce requests by adding a needsResync
flag and looping: when queueAuthoritativeCountResync is called, set needsResync
= true and if isResyncingLikeCount return; otherwise set isResyncingLikeCount =
true and run a loop that clears needsResync, calls await this.updateLikeCount(),
then if needsResync is true continue another iteration, finally set
isResyncingLikeCount = false; reference and update the existing
isResyncingLikeCount, queueAuthoritativeCountResync, and updateLikeCount
symbols.
🪄 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
Run ID: 3b2efcc6-d92d-446b-8596-36a230c4c562
📒 Files selected for processing (3)
src/nostr-like-button/__tests__/optimistic-state.test.tssrc/nostr-like-button/nostr-like.tssrc/nostr-like-button/optimistic-state.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #78
This PR fully resolves the like-count drift bug in failed like/unlike mutation paths.
When a like or unlike operation fails before optimistic state is applied, the UI no longer performs rollback arithmetic that can incorrectly increment or decrement the displayed count. The implementation now restores state deterministically and keeps count values non-negative.
Root Cause
The previous catch-path rollback assumed optimistic mutation had already been applied.If a failure occurred earlier (for example signer denial or publish failure), rollback logic could still run and drift local count.
What Changed
Added pure optimistic-state helpers in
optimistic-state.tsUpdated mutation and failure handling in
nostr-like.tsExpanded regression coverage in
optimistic-state.test.tsImplementation Details
Like/unlike flows now snapshot pre-mutation UI state.
A local didApplyOptimisticUpdate flag controls rollback behavior.
Rollback is state-based (restore snapshot) rather than arithmetic.
Fetched relay totals are clamped before assigning to displayed count.
Added best-effort authoritative count resync after failed mutations, with an in-flight guard to prevent redundant resync calls.
Behavior After This Change
Failure-before-optimistic: local count does not drift.
Failure-after-optimistic: local state is restored to pre-mutation snapshot.
Displayed like count never drops below zero.
Failed mutations attempt relay-backed reconciliation to converge UI state.
Testing
Executed targeted regression tests:
optimistic-state.test.tsResult:
1 test file passed
6 tests passed
Summary by CodeRabbit
New Features
Tests