Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 163b873286
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+70
to
+72
| const entry = await getStoreValue<unknown>(key); | ||
| if (isDraftEntry(entry) && entry.expiresAt > now) continue; | ||
| await deleteStoreValue(key); |
There was a problem hiding this comment.
Because the startup sweep runs fire-and-forget while the UI is already usable, a user can begin editing a key after this loop reads its old expired or malformed entry but before deleteStoreValue completes. In that ordering, the queued save may write a fresh draft first and this unconditional deletion then removes it, losing the newly typed text. Revalidate that the stored entry is still the one inspected, or run the sweep before draft writers become active.
Useful? React with 👍 / 👎.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Resolve code review warnings on the unsent-draft caching feature: - draftCache: the write scheduler kept a single debounce timer, so scheduling a save for a new key silently cancelled a pending save for the previous key (e.g. switching sessions in the chat pane dropped the prior session's latest keystrokes). Track the pending key and flush — rather than cancel — when a write targets a different key. - draftCache: replace the scheduler's cancel() with flush(), and call it from SessionChatPane/ProjectSection onDestroy so text typed within the debounce window of teardown is persisted instead of dropped. - BranchCardSessionManager: guard loadNewSessionDraftForMode with try/catch so a failed store read degrades to an empty draft instead of leaving the new-session modal unopened via an unhandled rejection, matching the other draft-load call sites. Adds scheduler unit tests for debouncing, cross-key flushing, and flush-on-close. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Resolve the remaining code review comments on unsent-draft caching: - SessionChatPane: bouncing between sessions faster than the write debounce (A → B → A) read A's stale stored value while its newest keystrokes were still pending, restored that stale text, and then let the save effect write it back — superseding the pending newer write, so the newest text reached neither the composer nor the store. Flush the writer before reading. The load now also catches store failures instead of leaving an unhandled rejection, and re-enables saving when it fails so further keystrokes still persist. - NewSessionModal: two mode switches in quick succession (⌘1→⌘2→⌘3) could file one mode's draft under another mode's key — while switch #1 awaited its draft load, `prompt` still held the outgoing mode's text but `currentMode` had already advanced, so switch #2 saved the former under the latter. Skip the outgoing save when a switch is already in flight; that switch has already saved the correct owner's draft. - BranchCardSessionManager: `draft ?? ''` blanked the draft still held in memory when the store read failed, and a close-then-instant-reopen could read the store before the close's immediate save landed. Flush before reading, and fall back to the held draft — but only when it belongs to the mode being opened, which also stops openNewSessionReferring from leaking another mode's text into commit mode. - draftCache: loadDraft only expires the key it reads, and draft keys are scoped to a session/branch/project id, so drafts outliving their owner were never read again and sat in ~/.staged/preferences.json forever. Add sweepExpiredDrafts(), run once at startup, backed by a new listStoreKeys() (with a matching web-mode `list_preference_keys` arm). Adds sweep unit tests covering expiry, namespace isolation, and dropping unparseable entries. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Resolve all five findings from the code review of commit af4d634: - NewSessionModal: the draft-save $effect guarded on two non-reactive locals before reading any signals. Every mode switch scheduled the effect and then ran it while the guard tripped, so the run read zero reactive values, Svelte truncated its dependency list to empty, and the effect never ran again — after the first ⌘1/⌘2/⌘3 switch typing no longer reached onDraftChange, so debounced autosave was dead for the life of the modal and only close/submit persisted. Read prompt/mode/imageIds into a local before the guard, and make `switchingMode` $state so the effect re-fires when the gate reopens and schedules text typed during a load window. - SessionChatPane: `loadedDraftSessionId` was a plain let, so the save effect never re-ran when a finished draft load opened its gate — a keystroke typed during the load window was never scheduled and was lost if the user then stopped typing. Make it $state. - BranchCardSessionManager: openNewSession awaited a writer flush plus a store read before showing the dialog, lagging the click by up to two IPC round-trips (an HTTP hop in web mode). Open the modal synchronously seeded with the held draft and let the modal fetch the stored draft itself via loadDraftForMode — preserving the flush-before-read and openNewSessionRequest generation checks from af4d634, adding a don't-clobber-typing check, and still clearing the held prompt when it belongs to a different mode. - NewSessionModal: the mount-focus effect tracked `prefilled`, which flips when the first keystroke overwrites a prefill — the re-run yanked the caret to the end mid-edit. Gate it with a one-shot flag. - web_server: list_preference_keys maps serialization errors into the Result like the neighbouring arms instead of unwrapping. Adds jsdom component tests for the modal's draft autosave via a new "component" vitest project (svelte plugin, stubbed leaf components). The regression test was verified to fail against the pre-fix effect; the others cover populating a synchronous open from the stored draft and preserving text typed during the initial load. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Resolve the four actionable comments from the code review of bfb7a1d8:
- NewSessionModal: closing the modal during an in-flight mode switch —
after currentMode advanced but before the load replaced `prompt` —
paired the outgoing mode's text with the new mode, so the parent's
immediate save clobbered the new mode's stored draft; and nothing
superseded the switch, which fired one more onDraftChange against the
closed modal. Track `promptMode` (the mode `prompt`'s text belongs
to, which lags `currentMode` during a switch) and have close, submit,
outgoing switch saves, and the save effect read it; close and submit
now also bump modeSwitchGeneration so superseded loads are no-ops.
Submit is ignored while promptMode lags currentMode, since it would
pair one mode's text with the other mode's session type.
- NewSessionModal: the switch-in-flight skip also suppressed the
outgoing save when only the *initial* draft load was in flight, where
no prior save existed — text typed during the load window and
followed by ⌘2 was lost from both the store and the held draft. The
owner-mode attribution makes the outgoing save correct mid-flight, so
the skip is gone and that save always runs.
- ProjectSection: loadProjectSessionDraft was the one draft-load site
that read the store without awaiting projectDraftWriter.flush(), so
a quick reopen after submit (clearNow) or close-with-cleared-text
(saveNow('')) could read before the pending delete landed and
resurrect the just-cleared prompt. Flush before reading, matching
loadNewSessionDraftForMode and loadReplyDraft.
- draftCache: the write scheduler swallowed queue errors for the chain
while handing the same rejected promise to callers, several of which
void it — teardown failures surfaced as unhandled rejections and
mid-session failures were never reported. enqueue now catches and
console.errors, so queue promises never reject and every failure is
logged, matching the load paths.
Adds component regression tests for the close-during-switch owner
attribution and the save-during-initial-load scenario (both verified
to fail pre-fix), a scheduler test that write failures resolve, log,
and don't block later writes (also verified pre-fix), and a
flush-waits-for-in-flight-clearNow test covering the reopen read.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Resolve the three actionable comments from the code review of 1080f849:
- SessionChatPane: the loadDraft failure path re-enabled the save gate
while the composer could still hold the previous session's text. On a
switch A -> B with B's load failing, setting loadedDraftSessionId
($state) re-fired the save effect immediately, scheduling A's text
under B's key and overwriting B's stored draft. Suppress exactly that
first save-effect run when the composer still holds the text from
before the load began (inherited text); text typed during the load
window was written while B was displayed, so it still saves, and the
success path's write-back (which refreshes the TTL) is untouched.
- BranchCardSessionManager: openNewSessionReferring set
newSessionMode = 'commit' before its awaited flush+read, so a
concurrent openNewSession('commit') click saw mode === newSessionMode
and kept a draftPrompt that actually belonged to the held mode,
seeding commit mode with e.g. note's text. The assignment was
redundant - it's set again after the generation guard, and the branch
reads the captured heldMode/heldPrompt - so it's gone, closing the
window.
- ProjectSection: the project-session modal was the last open path that
awaited store IPC (flush + read) before showing the dialog. It now
opens synchronously seeded with the held draft and passes
loadDraftForMode so NewSessionModal fetches the authoritative stored
draft itself, with its existing generation and don't-clobber-typing
checks - matching the BranchCard flow from bfb7a1d8. The flush-before-
read stays inside the load function, and the referring path reuses it.
Adds jsdom component regression tests: SessionChatPane tests that a
failed load neither files the previous session's text under the new
session's key nor drops text typed during the failing load (plus a
reactiveProps test helper for post-mount prop switches), and
BranchCardSessionManager tests that a commit open racing the referring
load gets a clean composer while an unraced referring open still builds
on the stored commit draft. The two misattribution tests were verified
to fail against the pre-fix code.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Resolve the single actionable comment from the code review of a5c74ed0: opening the new-session modal in a different mode than the held draft cleared draftPrompt but not draftImageIds, so images attached while drafting (e.g.) a note seeded the commit modal via initialImageIds — violating the branch's per-mode draft invariant. Images aren't persisted to the store, so the in-memory list is the only copy and can simply be cleared: - BranchCardSessionManager.openNewSession: clear draftImageIds alongside draftPrompt when the requested mode differs from the held one. - openNewSessionReferring needed the same treatment: its fresh-open path targets commit mode while draftImageIds may still hold another mode's images. Clear them when the held mode isn't commit — the same rule resolveOpenPrompt applies to the held prompt. Held commit-mode images still survive a referring open. Extends the manager component tests: different-mode opens (plain and referring) get a clean image list while same-mode/commit-mode opens keep the held images. Both clearing tests were verified to fail against the pre-fix code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Resolve the two remaining comments from the code review of 063330b6: - ProjectSection: handleProjectNoteNewSessionReferring awaited its flush+read with no supersede guard. A plain "New project note" click landing in that window opened the modal synchronously without the ref; when the referring handler resumed it saved the ref-appended prompt to the store, which the already-open modal (it reads initialPrompt once) never displayed and whose next keystroke's save overwrote - the ref was silently lost. Add a projectSessionOpenRequest counter mirroring BranchCardSessionManager.openNewSessionRequest: every open bumps it, and a referring open that finds itself overtaken becomes a no-op, matching the BranchCard flow. - NewSessionModal: the mode-switch load overwrote `prompt` wholesale when it landed, so keystrokes typed between ⌘1/⌘2/⌘3 and the load resolving were dropped from both modes - the outgoing save had already been snapshotted and the save effect is gated by switchingMode. Snapshot the prompt once the gate closes (keystrokes before that were still shown under the outgoing mode and reach its key through the save effect) and, when the load lands, diff it against the live prompt with a new insertedText() helper: the inserted part belongs to the incoming mode, so it is placed after the stored draft, or stands alone when there is none (a prefill would bury it). The existing post-load onDraftChange and the reopened save effect then file it under the new mode's key. The NewSessionModal component suite was failing wholesale at HEAD: main's f912579 (#956) added Dialog.ResizeHandle to the modal after the dialog mock was written. The mock now stubs it. Adds jsdom component regression tests: a ProjectSection suite (with a TimelineContextMenu stub exposing the referring trigger) covering a plain open racing the referring load - verified to fail pre-fix - plus the unraced referring prompt; and NewSessionModal tests that text typed during a mode-switch load is appended to the stored draft and kept when there is none, both verified to fail pre-fix. insertedText gets unit tests for appends, mid-string inserts, replacements, and deletions. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Resolve the three comments from the code review of 6475fe97:
- NewSessionModal: loadInitialDraft was the one load path that still
discarded a stored draft when the user typed during the load window —
`prompt !== promptWhenLoadStarted` skipped cachedDraft entirely, then
`switchingMode = false` re-fired the save effect and overwrote the
stored draft with just the typed text. BranchCard.openNewSession seeds
'' whenever the held draft belongs to another mode, and in web mode the
load is a flush plus an HTTP read, so a fast typist could permanently
lose that mode's stored draft. Bring it in line with switchMode: when
the seed is not the stored draft, place insertedText(seed, prompt)
after the stored draft (same newline join), or apply the stored draft
alone when nothing was typed; the reopened save effect then files the
combined value under the mode's key. When the seed already is the
stored draft (the normal case — the caller holds what the store holds)
edits made during the load sit on top of it and the prompt is left
alone, so nothing is appended twice. With no stored draft the typed
text still wins over a prefill, as before.
- sessionModalHelpers: document that insertedText is a prefix/suffix
diff, not a general-purpose one — a replacement whose typed text
shares boundary characters with the replaced text loses them
('note text' → 'new' yields 'ew', 'fix bug' → 'fix bag' yields 'a') —
and that this is accepted for the single store round-trip it covers.
- Component tests: the `$lib/components/ui/dialog` mock was a closed
object copied into NewSessionModal and ProjectSection's suites, so any
new Dialog.* subcomponent on main (as ResizeHandle did in #956) failed
both wholesale with an opaque "not a component" error. Extract
testStubs/dialogMock.ts, which keeps Root/Content/Title as passthroughs
and returns the empty stub for any other PascalCase name via a Proxy
(`has` included, since vitest checks `prop in module`; `then`,
`default` and `__esModule` stay absent), and use it from both suites.
Adds jsdom component tests: the initial-load regression (seeded '' with
a stored draft, typed during the pending load → the save carries the
stored draft followed by the typed text; verified to fail pre-fix), no
stored draft → typed text still wins, and seed-equals-stored-draft →
edits are kept without duplication. The old "typed text wins over the
stored draft" test pinned the data-loss behaviour and is replaced by
these. insertedText gets a unit test documenting the boundary-character
limitation.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Resolve the three polish comments from the code review of f5a853a9:
- NewSessionModal: a prefilled open selects all, so the natural gesture
during the initial load window is typing a replacement over the
prefill — and loadInitialDraft diffed that against the seed with
insertedText, which drops boundary characters the replacement shares
with the prefill ('Commit the changes' → 'add tests' yielded
'add test'). Unlike switchMode's rare partial-edit case this is the
common prefilled-open gesture, so only diff while the seed is still
contained in the live prompt; once it's gone the whole prompt was
typed and carries over intact after the stored draft. switchMode is
left as is: its seed is the outgoing mode's text with the caret at
the end, so a partial delete-and-type there would otherwise carry
leftover outgoing text into the new mode.
- testStubs/dialogMock: dialog/index.ts also exports Root/Content/Title
as Dialog/DialogContent/DialogTitle, which the Proxy resolved to the
empty stub. Add the aliases to the explicit passthrough map so a
component switching to the alias names fails loudly in tests instead
of silently rendering nothing inside the dialog.
- BranchCardSessionManager.openNewSessionReferring and
ProjectSection.handleProjectNoteNewSessionReferring: document that the
immediate save must stay synchronous with opening the modal, since
loadInitialDraft only keeps the seeded prompt when the stored draft
equals it — an await inserted between the two would let the load read
the pre-ref draft and replace the prompt with it, minus the ref.
Adds a jsdom component regression test: prefilled open with a stored
draft, the prefill replaced during the pending load by text sharing its
trailing character, resolve → the saved prompt is the stored draft
followed by the full replacement (verified to fail pre-fix with the
trailing 's' dropped).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Resolve the two comments from the code review of 9619b4a1: the ordering notes beside the immediate saveNow in BranchCardSessionManager.openNewSessionReferring and ProjectSection.handleProjectNoteNewSessionReferring claimed that an await between opening the modal and the save would let loadInitialDraft read the pre-ref draft and "replace the prompt with it, minus the ref". That isn't what the code does: with the seed X + '\n' + ref and the store holding X, `prompt.includes(X)` is true, insertedText recovers the ref, and the prompt is rebuilt byte-identical to buildReferringPrompt's output; with an empty held draft the stored value is null and the prompt is left alone. Reword both comments (same text at both sites) to the actual reason: keeping the save in the same tick as the open lets loadInitialDraft hit its equality fast path (stored draft === seed), so the ref is preserved exactly rather than the open depending on the insertedText prefix/suffix heuristic. Comment-only; no behaviour change. `pnpm check` and the two affected component suites pass. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
ProjectSection now pauses the poller when its layer is hidden, so the component test's poller stub needs the method or the effect throws. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Rebase onto main, which added lastChatMessageAt and chatReadAt to ProjectNote in #1005 (surface note revisions and chat replies on note rows). The completedNote fixture in ProjectSection.component.test.ts is the only hand-rolled ProjectNote literal that #1005 could not update, since the file exists only on this branch, so the PR's merge-commit typecheck failed while the branch tip stayed green. Set both fields to null; the new noteTimestamps/noteChatState helpers only read note fields, so no other test changes are needed. svelte-check and the ProjectSection component suite pass. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
matt2e
force-pushed
the
store-unsent-messages
branch
from
November 1, 2026 00:41
163b873 to
87c7575
Compare
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.
Summary
Unsent prompt text typed into the session modal, the project new-session modal, and the session chat reply box is now cached in the persistent store, so closing the modal, switching modes, or restarting the app no longer loses what was typed.
draftCachemodule (src/lib/shared/draftCache.ts):loadDraft/saveDraft/clearDraftstore{ value, updatedAt, expiresAt }entries under a shareddraft:namespace with a 7-day TTL, plus a debouncedcreateDraftWriteSchedulerwhose write queue never rejects and flushes a pending write when the target key changes rather than dropping it.App.sveltecallssweepExpiredDrafts()once at boot to delete expired or malformeddraft:*entries whose owning session/branch/project may be gone. This needed a newlistStoreKeys()on the persistent store, backed by the Tauri store'skeys()and a newlist_preference_keyscommand in the web server.BranchCardSessionManager+NewSessionModal): drafts are keyed per branch and per mode (commit/review), image attachments are held per mode, the modal opens synchronously and loads the stored draft itself, and keystrokes typed while a load or mode switch is in flight are preserved (via a newinsertedTextprefix/suffix helper for the mode-switch case). Drafts are cleared on submit and saved immediately on close.ProjectSection): same pattern keyed by project id, with a load generation counter so a stale load or a racing open cannot overwrite text typed in the meantime or cross draft owners.SessionChatPane): the typed reply is cached per session and restored on reopen.NewSessionModal,ProjectSection,SessionChatPane, andBranchCardSessionManager.vitest.config.tsis split intounitandcomponentprojects so*.component.test.tsfiles run under jsdom with the Svelte plugin; small stub components live insessions/testStubs/.Notes
The pre-push CI run originally failed because the
ProjectSectiontest's poller stub lackedsetPaused, which main added in the layer-stacking perf work; the final commit adds it.transport.test.tsis untouched here and only flaked under heavy machine load.🤖 Generated with Claude Code