fix(accounts): align pending comment failure state with plebbit-js#48
Merged
tomcasaburi merged 3 commits intomasterfrom Mar 26, 2026
Merged
fix(accounts): align pending comment failure state with plebbit-js#48tomcasaburi merged 3 commits intomasterfrom
tomcasaburi merged 3 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis pull request fixes terminal publish failure handling for pending account comments. Instead of relying on a 20-minute timeout fallback, the system now immediately classifies comments as failed when the terminal state pair ( Changes
Sequence DiagramsequenceDiagram
participant Comment
participant PublishSession
participant AccountsStore
participant Hook
Comment->>PublishSession: publish()
PublishSession->>PublishSession: ⚠️ Error occurs
PublishSession->>AccountsStore: error event
AccountsStore->>AccountsStore: recordPublishCommentError()
AccountsStore->>AccountsStore: Update error & errors[]
PublishSession->>PublishSession: emit statechange: "stopped"
PublishSession->>PublishSession: emit publishingstatechange: "failed"
PublishSession->>AccountsStore: statechange & publishingstatechange events
AccountsStore->>AccountsStore: Update comment.state = "stopped"
AccountsStore->>AccountsStore: Update comment.publishingState = "failed"
Hook->>AccountsStore: getAccountCommentsStates()
AccountsStore->>Hook: Detect terminal pair (failed + stopped)
Hook->>Hook: Set state = "failed"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
tomcasaburi
force-pushed
the
fix/comment-publish-failure-review
branch
from
4fb355d to
222af7e
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/accounts/accounts-actions.ts (1)
1124-1270:⚠️ Potential issue | 🟠 MajorScope failure callbacks to the active session and close failed sessions.
Line 1124 intentionally detaches the old publication before a retry, but Lines 1210-1215 and 1267-1270 still call
onErroreven when that emitter is no longer the active session. A late error from a replaced or abandoned comment can still surface as a user-visible failure. Also, the newstate === "stopped"+publishingState === "failed"pair never callscleanupPublishSessionOnTerminal(), so terminal failures remain active and can be overwritten by later events.🔧 Suggested session-lifecycle fix
+ const finishFailedSession = () => + queueMicrotask(() => cleanupPublishSessionOnTerminal(publishSessionId)); + activeComment.on("error", (error: Error) => { - publishCommentOptions.onError?.( - recordPublishCommentError(error, activeComment), - activeComment, - ); + if (!getActiveSessionForComment(activeComment)) return; + const normalizedError = recordPublishCommentError(error, activeComment); + finishFailedSession(); + publishCommentOptions.onError?.(normalizedError, activeComment); }); activeComment.on("statechange", (state: string) => { const session = getActiveSessionForComment(activeComment); if (!session) return; const currentIndex = session.currentIndex; accountsStore.setState(({ accountsComments }) => maybeUpdateAccountComment(accountsComments, account.id, currentIndex, (ac, acc) => { - ac[currentIndex] = { ...acc, state }; + const nextAccountComment = { ...acc, state }; + ac[currentIndex] = nextAccountComment; + if ( + nextAccountComment.state === "stopped" && + nextAccountComment.publishingState === "failed" + ) { + finishFailedSession(); + } }), ); }); activeComment.on("publishingstatechange", async (publishingState: string) => { const session = getActiveSessionForComment(activeComment); if (!session) return; const currentIndex = session.currentIndex; accountsStore.setState(({ accountsComments }) => maybeUpdateAccountComment(accountsComments, account.id, currentIndex, (ac, acc) => { - ac[currentIndex] = { ...acc, publishingState }; + const nextAccountComment = { ...acc, publishingState }; + ac[currentIndex] = nextAccountComment; + if ( + nextAccountComment.state === "stopped" && + nextAccountComment.publishingState === "failed" + ) { + finishFailedSession(); + } }), ); publishCommentOptions.onPublishingStateChange?.(publishingState); }); @@ } catch (error) { - publishCommentOptions.onError?.( - recordPublishCommentError(error, activeComment), - activeComment, - ); + if (!getActiveSessionForComment(activeComment)) return; + const normalizedError = recordPublishCommentError(error, activeComment); + finishFailedSession(); + publishCommentOptions.onError?.(normalizedError, activeComment); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/accounts/accounts-actions.ts` around lines 1124 - 1270, The error/state/publishing callbacks need to be scoped to the current publish session: in the activeComment "error", "statechange" and "publishingstatechange" handlers only call publishCommentOptions.onError or mutate state if getActiveSessionForComment(activeComment) returns the same session that created this handler (and the session is not abandoned via isPublishSessionAbandoned); additionally when you detect the terminal failed pair (state === "stopped" && publishingState === "failed") call cleanupPublishSessionOnTerminal(publishSessionId) so the session is closed and cannot be overwritten by later events; update the try/catch publish() path similarly to verify the session is still active before invoking publishCommentOptions.onError(recordPublishCommentError(...), activeComment).
🧹 Nitpick comments (1)
src/hooks/accounts/accounts.ts (1)
325-330: Create a shared type for publish metadata properties instead of ad-hoc casting.The
AccountComment & { error?, errors?, publishingState?, state? }pattern appears at lines 325–330 and 546–547 in this file. Extract this shape into a dedicated interface (e.g.,AccountCommentWithPublishMetadata) insrc/types.tsand use it consistently. This reduces duplication and makes TypeScript upgrades safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/accounts/accounts.ts` around lines 325 - 330, Replace the ad-hoc intersection type used where accountComment is cast (AccountComment & { error?: Error; errors?: Error[]; publishingState?: string; state?: string }) by declaring a shared exported interface named AccountCommentWithPublishMetadata that extends AccountComment and defines those four optional properties, add that interface to your central types module and export it, then import and use AccountCommentWithPublishMetadata wherever the inline cast is used (e.g., the accountComment variable casts) to remove duplication and ensure consistent typing.
🤖 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/stores/accounts/accounts-actions.test.ts`:
- Around line 2176-2200: The test flakes because it emits events on commentRef
before the mocked createComment assigns it; change the test to wait
deterministically for commentRef to be set after calling
accountsActions.publishComment instead of using a fixed timeout: create a
Promise that resolves when your vi.spyOn(account.plebbit,
"createComment").mockImplementation assigns commentRef (or otherwise hook into
the mock to signal completion), await that promise after the act that calls
accountsActions.publishComment, and only then emit the "statechange" and
"publishingstatechange" events on commentRef; this ensures publishComment and
the createComment mock have completed before emitting events.
---
Outside diff comments:
In `@src/stores/accounts/accounts-actions.ts`:
- Around line 1124-1270: The error/state/publishing callbacks need to be scoped
to the current publish session: in the activeComment "error", "statechange" and
"publishingstatechange" handlers only call publishCommentOptions.onError or
mutate state if getActiveSessionForComment(activeComment) returns the same
session that created this handler (and the session is not abandoned via
isPublishSessionAbandoned); additionally when you detect the terminal failed
pair (state === "stopped" && publishingState === "failed") call
cleanupPublishSessionOnTerminal(publishSessionId) so the session is closed and
cannot be overwritten by later events; update the try/catch publish() path
similarly to verify the session is still active before invoking
publishCommentOptions.onError(recordPublishCommentError(...), activeComment).
---
Nitpick comments:
In `@src/hooks/accounts/accounts.ts`:
- Around line 325-330: Replace the ad-hoc intersection type used where
accountComment is cast (AccountComment & { error?: Error; errors?: Error[];
publishingState?: string; state?: string }) by declaring a shared exported
interface named AccountCommentWithPublishMetadata that extends AccountComment
and defines those four optional properties, add that interface to your central
types module and export it, then import and use
AccountCommentWithPublishMetadata wherever the inline cast is used (e.g., the
accountComment variable casts) to remove duplication and ensure consistent
typing.
🪄 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: f1511482-1225-488c-8ae2-a8922762bce5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
README.mdpackage.jsonsrc/hooks/accounts/accounts.test.tssrc/hooks/accounts/accounts.tssrc/stores/accounts/accounts-actions.test.tssrc/stores/accounts/accounts-actions.ts
Member
Author
|
Addressed the remaining valid review point in the latest commit: stale publish-session errors are now ignored once a session is replaced or reaches the terminal failed pair, and regression tests cover both the retry and terminal-failure paths. Local verification: yarn build, yarn test, and yarn test:coverage:hooks-stores (still non-zero only because of the repo’s pre-existing sub-100% hooks/stores baseline). |
Member
Author
|
The remaining non-correctness review items are being declined. The / assignee updates were included intentionally because the username change to was requested during this PR and those files are repo-managed mirrors; the docstring coverage warning is not a repository gate and adding unrelated docstrings here would be noise; the shared-type suggestion in is a non-blocking cleanup and not required for this bug fix. |
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
1 participant
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.
Align pending account comment failure handling with the terminal publication semantics used by
plebbit-js.Closes #47
Note
Medium Risk
Changes affect comment publishing/session lifecycle and how failures are surfaced, which can impact user-visible state and error callbacks. Risk is moderated by substantial new/updated tests covering edge cases like retries and terminal state handling.
Overview
Pending account comments now transition to
failedimmediately whenplebbit-jsreports a terminal failure (publishingState === "failed"+state === "stopped") or when publish errors are recorded, instead of waiting solely on the 20-minute fallback; docs were updated to clarify this behavior.accountsActions.publishCommentnow records errors onto the pending account comment, dedupes/normalizes error reporting, and ignores stale events from replaced retry comments (session/comment identity checks + deferred cleanup on terminal failure). Tests were updated/added to cover the new state derivation, error persistence, stale error suppression, and callback expectations.Written by Cursor Bugbot for commit c6043fe. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests