feat(chat): restore an archived chat when a new message is sent - #294
feat(chat): restore an archived chat when a new message is sent#294NickTitle wants to merge 4 commits into
Conversation
Co-authored-by: Goose <opensource@block.xyz>
Co-authored-by: Goose <opensource@block.xyz>
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. The exact PR comparison introduces two blocking archive-lifecycle defects: dispatch is not reliably gated on a durable restore, and rejected or cancelled send attempts can still reactivate a chat. The project code-review and user wes-review skills were explicitly loaded and applied as distinct Engineering passes. Final self-check covered every changed send and steer flow; accessibility; i18n/localization completeness; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; design-system and project rules; duplicate and overlapping findings; and concrete evidence plus user effect for blocking severity. The supplied GitHub evidence was inspected: all completed checks passed, while Tauri Rust checks (macOS) was in progress; required checks independently govern merge readiness. This changes send behavior and archive state through non-visual library code, but does not directly change an eligible graphical Berd interface, so no visual-evidence requirement applies.
Deterministic publication result: 2 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| sessionId: string, | ||
| ): Promise<void> { | ||
| const sessionStore = useChatSessionStore.getState(); | ||
| if (!sessionStore.getSession(sessionId)?.archivedAt) { |
There was a problem hiding this comment.
🤖 P1 · Make restoration an authoritative gate (blocking)
This check treats the optimistic local archive flag as proof that the session is active. unarchiveSession clears archivedAt before its backend request settles, so a concurrent dispatch or steer skips the restore entirely; if restoration never completes, that later path can still proceed. The helper also swallows a restoration failure, and it does not verify that a newer archive action won before dispatch. A prompt can therefore cross the transport boundary while the backend session is still or once again archived.
User effect: A new message or agent reply can remain hidden in Archived, fail unpredictably, or appear to start while the original send is still stuck restoring the chat.
Recommended fix: Own this lifecycle in the session store with an ensure-active operation that coalesces an in-flight restore, waits for durable success, verifies that no newer archive action won, and propagates failure instead of dispatching. Keep the message intent available and surface the shared send error when restoration fails.
Test: Use a deferred unarchive request to prove a concurrent send or steer cannot dispatch early, then cover rejection, never-settling state, and a newer archive winning the race; each path must leave the prompt undispatched unless the session is durably active.
| // local transcript state so a retained queued record can retry without | ||
| // duplicating the user turn. | ||
| throwIfAborted(signal); | ||
| await restoreArchivedSessionBeforeSend(sessionId); |
There was a problem hiding this comment.
🤖 P2 · Restore only accepted sends (blocking)
Restoration runs before prepare, even though that callback can reject a superseded queued send at the final reversible boundary, and before the post-prepare abort check. The steer path likewise restores before its empty-input and attachment-budget guards. These paths can persist an unarchive even though no user message is accepted or dispatched.
User effect: A chat can unexpectedly move from Archived back to Active after a cancelled, stale, blank, or oversized send attempt that produced no new message.
Recommended fix: Move restoration after each path's no-op, validation, preparation, ownership, and cancellation checks, at the final pre-dispatch boundary, while preserving the durable restore gate from the first finding.
Test: For an archived session, cover a preparation rejection, cancellation before dispatch, an empty steer, and an oversized steer; assert that no unarchive request occurs, the chat remains archived, and no prompt is dispatched.
Co-authored-by: Goose <opensource@block.xyz>
Co-authored-by: Goose <opensource@block.xyz>
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
COMMENT. A fresh static review of the exact three-dot comparison c5058ae...1a52dc5 found no new publishable findings. The revision fixes durable restore failure handling, rejected preparation, and empty or oversized steer admission. Two remaining proposed concerns are suppressed because they are the same underlying issues already covered by unresolved automated threads with no substantive human reply; those unresolved threads prevent approval under the publication policy. The project code-review and user wes-review skills were explicitly loaded and applied as distinct Engineering passes. Final self-check covered foreground, queued, background, voice, CLI, and steer flows; accessibility; i18n/localization completeness; navigation and consent guards; async, error, never-completes, lifecycle, cancellation, and race behavior; test honesty; design-system and project rules; duplicate and overlapping findings; and concrete evidence plus user effect for blocking severity. Supplied GitHub evidence is structurally valid: all ten listed checks completed successfully, while the combined status was pending; required checks independently govern merge readiness. Changed files contain no eligible graphical Berd frontend source, so no visual-evidence requirement applies.
Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 2 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
Summary
Sending a new message to an archived chat now restores it.
dispatchPrompt— the single choke point for every prompt send (composer sends, queued-message drains, andberdctl session send) — checks the target session'sarchivedAtbefore preparing the prompt. When the chat is archived, it runs the existingunarchiveSessionstore action first, which clears the flag optimistically and unarchives on the backend before the prompt goes out.Before: a message sent into an archived chat was dispatched, but the chat stayed in the Archived list, so the conversation kept replying while hidden. After: the send clears the archive flag (local + backend), so the chat moves back to the active list at the moment the message is sent.
Details:
prepare?.(), so the backend session is unarchived before any target transition or dispatch.Related issue
none found
Testing
src/features/chat/lib/sendCore.test.ts:archivedAtclears)vitest run src/features/chat/lib/ src/features/chat/stores/→ 903 tests pass (16 insendCore.test.ts)vitest run src/features/berdctl→ 293 tests pass (coversberdctl session send, which reaches the same restore path)tsc --noEmitandbiome checkclean; pre-push hooks (check, fmt-check, clippy, tauri-check) green