Repository navigation
fix(web): blank drafts no longer pile up in the sidebar - #240
Conversation
Opening the app at / starts a draft in the most recently updated project. A phone lands on / every visit, so the sidebar collected a blank "New thread" card per project. Paint an empty draft only while it is open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code quality review of e2813e6d95ec63e9c0a2b49c54d8389ad1472292.
The fix is small and the targeted tests pass (sidebarV2DraftRows.test.ts, newAgentDraft.test.ts: 17/17). It still has two structural problems I'd fix before merging:
-
There are now two rules for which drafts matter.
composerDraftStorealready decides which draft sessions to keep: the project's mapped draft, a draft that is promoting, a draft with user content, and a draft with an open preview. BothpartializeComposerDraftStoreStateand the replacement path insetLogicalProjectDraftThreadIduse that set. This PR adds a different rule in the sidebar (has content, or is open). The two now disagree: a draft with an open preview and no text is one the store deliberately keeps, but the sidebar now hides it once you leave it. The visibility rule should come from the same criteria as the store's, not a fresh condition built from different parts. -
The predicate is in the wrong layer, and it costs an unfenced edit to an upstream line.
hasUserContentis passed in as a callback fromSidebar.tsx. That callback is the fourthuseComposerDraftStore.getState().getComposerDraft(draftId)lookup at that call site, and it made the upstream../composerDraftStoreimport line change with nofork:beginfence. That's sync-conflict surface for no benefit. Five existing tests also had to addhasUserContent: () => true, openThreadKey: null, which suggests the input shape is wrong. A simpler version: givelistSidebarDraftRowsonecomposerDraftFor(draftId)lookup (or callcomposerDraftHasUserContentinsidecustom/sidebarV2DraftRows.ts). Then the content check lives in the fork module, theSidebar.tsximport goes back to upstream's line, and the call site shrinks.
Smaller point: the memo comments at Sidebar.tsx:2515 and :2541 still say the getState() snapshot is only for prompt titles. It now also decides whether a row shows at all, which is why visibility only updates on route change. Say so there, since that is the invariant this fix relies on.
No file crosses 1k lines because of this PR (Sidebar.tsx is already 5.4k; another reason to keep new logic out of it).
Reviewed by Claude Opus 5.5 in T3 Code.
| const threadRef = scopeThreadRef(draft.environmentId, draft.threadId); | ||
| if (input.hasServerShell(threadRef)) continue; | ||
| const draftId = draftIdValue as DraftId; | ||
| if (!input.hasUserContent(draftId) && scopedThreadKey(threadRef) !== input.openThreadKey) { |
There was a problem hiding this comment.
This adds a second rule for whether a draft matters, next to the store's own (partializeComposerDraftStoreState / setLogicalProjectDraftThreadId in composerDraftStore.ts: mapped, promoting, has content, open preview). They disagree. A draft with an open preview and no text is kept on purpose by the store, but this hides it once it isn't the route. If it's no longer the project's mapped draft, nothing in the sidebar leads back to it. Can the visibility check reuse the store's retention criteria (minus "mapped", which is what this PR is trying to stop showing) as one exported predicate, so the two can't drift apart?
There was a problem hiding this comment.
Resolved in 61320f1 by removing the sidebar filter entirely. The fix moved to the source: custom/landingDraft.ts remembers only the draft / opens and discards it once the route leaves it untouched. The untouched check now mirrors the store's own retention reasons (composer content, open preview) plus split/floating membership, and the sidebar no longer has a second visibility rule.
| draftId: DraftId, | ||
| draft: DraftSessionState, | ||
| ) => string | null | undefined; | ||
| readonly hasUserContent: (draftId: DraftId) => boolean; |
There was a problem hiding this comment.
Why is this an injected callback? Its only real implementation is composerDraftHasUserContent(getComposerDraft(draftId)). That's a pure helper the fork module can import itself, and five tests now carry hasUserContent: () => true boilerplate. I'd replace this with one composerDraftFor: (draftId) => ComposerThreadDraftState | null input and derive the content check (and the model selection, which already reads the same composer draft) here. That removes moving pieces instead of adding another lambda.
There was a problem hiding this comment.
Resolved in 61320f1 by removing the sidebar filter entirely. The fix moved to the source: custom/landingDraft.ts remembers only the draft / opens and discards it once the route leaves it untouched. The injected hasUserContent input and its test boilerplate are gone.
| import { Tooltip, TooltipPopup, TooltipProvider, TooltipTrigger } from "./ui/tooltip"; | ||
| import { MiddleTruncate } from "./ui/middle-truncate"; | ||
| import { useComposerDraftStore, useThreadHasUnsentDraft } from "../composerDraftStore"; | ||
| import { |
There was a problem hiding this comment.
This rewrites an upstream import line without a fork:begin sidebar-v2-draft-rows fence. That's exactly the kind of hunk that conflicts on the next upstream sync. If the content check moves into custom/sidebarV2DraftRows.ts (see the comment there), this import goes back to upstream's version and the hunk disappears.
There was a problem hiding this comment.
Resolved in 61320f1 by removing the sidebar filter entirely. The fix moved to the source: custom/landingDraft.ts remembers only the draft / opens and discards it once the route leaves it untouched. Sidebar.tsx is back to the base version, so the unfenced import hunk is gone.
| (occurrence) => occurrence.label, | ||
| ), | ||
| hasUserContent: (draftId) => | ||
| composerDraftHasUserContent(useComposerDraftStore.getState().getComposerDraft(draftId)), |
There was a problem hiding this comment.
This is the fourth useComposerDraftStore.getState().getComposerDraft(draftId) at this call site. It also turns the memo's getState() snapshot into a visibility rule, not just a title rule: the comments at 2515 and 2541 still say the snapshot is only for prompt titles. Please update them. Visibility only updates when routeThreadKey changes, and that is the invariant this fix depends on.
There was a problem hiding this comment.
Resolved in 61320f1 by removing the sidebar filter entirely. The fix moved to the source: custom/landingDraft.ts remembers only the draft / opens and discards it once the route leaves it untouched. The memo is back to the base version and only snapshots titles again.
NoahHendrickson
left a comment
There was a problem hiding this comment.
Reviewed commit e2813e6d95ec63e9c0a2b49c54d8389ad1472292 against base e63be3e7f92f64f7e095131a6ec6b39442ea3dda. Found one P2 regression, detailed inline.
Validation: 172 tests passed across sidebarV2DraftRows.test.ts, newAgentDraft.test.ts, the sidebar draft-row fork guard, and composerDraftStore.test.ts. An additional isolated reproduction failed because a preserved draft with an open browser tab was absent from the returned sidebar rows. No source changes were made.
| const threadRef = scopeThreadRef(draft.environmentId, draft.threadId); | ||
| if (input.hasServerShell(threadRef)) continue; | ||
| const draftId = draftIdValue as DraftId; | ||
| if (!input.hasUserContent(draftId) && scopedThreadKey(threadRef) !== input.openThreadKey) { |
There was a problem hiding this comment.
[P2] Keep drafts with open browser tabs visible
composerDraftHasUserContent does not include open preview sessions, so this also hides drafts that still own browser tabs. For example, open a page in an otherwise empty draft A, then use New agent and assign draft B to the same project. setLogicalProjectDraftThreadId deliberately preserves A because draftThreadHasOpenPreview(A) is true (covered by the existing "keeps a draft with an open page when remapping a project to a new draft thread" test), but this filter removes its sidebar row once B is open. A no longer has the project mapping either, so New thread returns to B rather than restoring A; the user loses the sidebar entry for returning to or discarding A and its tabs. Please retain drafts with open preview sessions in the visibility predicate and cover this remapping case. I reproduced it with the real draft and preview stores: A remained stored, while listSidebarDraftRows returned only B.
There was a problem hiding this comment.
Fixed in 61320f1. A landing draft with an open preview session counts as touched and is kept; the sidebar filter that hid it is gone.
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code review of e2813e6d95ec63e9c0a2b49c54d8389ad1472292.
The filter fixes the phone pile-up. But "open" here means "on the route", and that is narrower than "on screen". Three findings are inline. Each was checked against the source.
One reported issue was dropped after checking. The concern was that a failed background send hides the restored draft because restoreFailedDraftThread runs before the prompt is put back. Both writes happen in the same synchronous block in ChatView.tsx, so the next render sees the restored content.
| const threadRef = scopeThreadRef(draft.environmentId, draft.threadId); | ||
| if (input.hasServerShell(threadRef)) continue; | ||
| const draftId = draftIdValue as DraftId; | ||
| if (!input.hasUserContent(draftId) && scopedThreadKey(threadRef) !== input.openThreadKey) { |
There was a problem hiding this comment.
Empty drafts in a split pane disappear from the sidebar while they are still on screen.
In split mode, focus is a URL change: pressing into a pane navigates to it (SplitThreadPanes.tsx header). So routeThreadKey is only the focused pane, not every visible one.
How to reproduce:
- Click Split agent on thread T. A new empty draft D opens in the partner pane.
- Click back into T's pane. The route becomes T.
- D is empty and is not
openThreadKey, so it is dropped here. buildSidebarSplitModelcan no longer find D inthreads, so the pair row falls back to a single T card. D's card vanishes even though D is on screen.
The floating pane is affected too, because pressing into it does not take the route. An empty floated draft is never painted.
A fix is to treat every draft that is mounted in a pane (the split pair members plus floating) as open, not just the route thread.
There was a problem hiding this comment.
Fixed in 61320f1. The sidebar filter is gone, and a landing draft held by a split pair, the floating pane, or an in-flight Split agent (usePendingSplitStore source, since pairing lands after the route resolves) is kept.
| useComposerDraftStore.getState().getComposerDraft(draftId)?.prompt ?? "", | ||
| (occurrence) => occurrence.label, | ||
| ), | ||
| hasUserContent: (draftId) => |
There was a problem hiding this comment.
Whether a row shows now depends on composer content, but this memo does not track content.
The memo reads content from getState() and only recomputes on draftThreadsByThreadKey, routeThreadKey, serverThreadKeys and projectDefaultModelByKey. Before this PR, stale content could only mislabel a row. Now it can hide one.
How to reproduce: type into an empty draft that is visible but not the route, for example a floated draft (pressing into a float does not take the route). The row stays hidden until some unrelated navigation happens.
The comment above the memo ("Prompt titles are read from getState()... Leaving the draft (routeThreadKey) is what refreshes the label") also no longer covers what the snapshot decides. A cheap fix is to subscribe to a derived boolean per draft (has content or not). That flips once per draft, not on every keystroke, so it keeps the no-rebuild-on-typing property.
There was a problem hiding this comment.
Fixed in 61320f1. Visibility no longer depends on composer content; the memo is back to the base version.
| * draft in the most recently updated project, so a client that opens there | ||
| * every time (a phone) would otherwise collect a blank "New thread" card per | ||
| * project as that project changes. The store keeps one draft per project and | ||
| * the next New thread there reuses it, so hiding it loses nothing. |
There was a problem hiding this comment.
"Hiding it loses nothing" isn't quite true.
The only way back to a hidden empty draft is New thread in that project. That goes through the emptyStoredDraftThread path in useHandleNewThread.ts (around lines 204–246). With no explicit workspace options and the draft not open, that path resets the draft to branch: null, worktreePath: null, the default env mode and the default runtime mode.
How to reproduce: pick worktree mode on branch feat/x for a draft, don't type a prompt, and open another thread. The row disappears. Clicking New thread brings the draft back without the setup. Before this PR, clicking the visible row kept it. The hidden draft also loses its card's Discard action, so the user can't see or remove it (a one-way door, though bounded at one per project).
An alternative is to fix it where the drafts are created: drop or don't keep the draft that _chat.index creates on landing if the user leaves it untouched. That solves the phone case without hiding drafts the user made on purpose (Split agent, New thread with branch or worktree options, a New agent draft assigned from the pill). If the filter stays, treat a draft with a non-default workspace context as having content.
There was a problem hiding this comment.
Agreed, and taken: 61320f1 moves cleanup to the draft _chat.index creates on landing and leaves deliberately made drafts (Split agent, New thread with options, drafts with tabs) alone. Only a landing draft the user never touched is discarded.
There was a problem hiding this comment.
Follow-up in 25e2b1e: a landing draft whose env mode, branch or worktree changed since the landing also counts as taken up, so a workspace set up on it without a prompt is kept.
Review found the sidebar filter hid drafts that were still on screen (split and floating panes), drafts holding preview tabs, and drafts with a chosen workspace. Revert it and clean up at the source: remember the draft that `/` opens and discard it once the route leaves it untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
Brings in #240 (landing draft cleanup) and #241 (design mode fixes). Upstream moved the right panel's width hook from ChatView into ChatRightPanel, so design mode's panel-room floor now reaches it through a fenced minWidth prop; manifest watch entry and guard follow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The landing-draft header and a guard header were over four lines, the panel-room effect had no stated reason, and the boot browser test cleaned up in afterEach instead of a disposable. All four tripped the lint-owned ratchet once custom merged in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Opening the web app at
/starts a draft in the most recently updated project (upstream's landing behavior), and Sidebar V2 paints every unsent draft as a "New thread" card. A phone lands on/every visit, so as the most recent project changes it collects one blank draft card per project.The draft the landing opens is now remembered (in localStorage, since phones reload rather than resume). When the route resolves somewhere else, that draft is discarded if it is still untouched: no composer content, no open preview tab, and not held by a split or floating pane or an in-flight Split agent, and still on the env mode, branch and worktree the landing gave it. A landing draft the user took up is forgotten and kept like any other. Drafts the user creates are never tracked.
An earlier revision hid empty drafts in the sidebar instead; review showed that also hid drafts still on screen in split/floating panes, drafts holding preview tabs, and drafts with a chosen workspace, so it was reverted in favor of fixing the source.
🤖 Generated with Claude Code — Claude Opus 5.5 in T3 Code