Skip to content

fix(ui-web): keep each conversation's staged files with its own draft - #619

Merged
0xKT merged 2 commits into
mainfrom
fix/composer_attachments_per_session
Sep 22, 2026
Merged

0xKT merged 2 commits into
mainfrom
fix/composer_attachments_per_session

Conversation

@0xKT

@0xKT 0xKT commented Sep 21, 2026 •

Copy link
Copy Markdown
Member

Summary

A file staged in the composer followed the reader into every other conversation: the tray was one page-wide array that a conversation switch never parked, loaded or cleared, so the chips stayed on screen, armed the send button, and rode out with the next message sent from the other conversation. An upload that landed or failed after a switch wrote whatever tray was open, which could leave a permanently "uploading" chip on the conversation it was staged in and refuse every later send there.

Two commits:

  1. fix(ui-web): the tray is keyed by the same owner the text draft already uses (the open conversation, or new for the first message of a draft): one map of trays with a single writer, swapped in by loadDraft alongside the text, re-keyed by claimDraft when a draft becomes a session, dropped with the conversation. Staging captures its owner once, so a late upload lands in the tray it was staged in. Only the open conversation's tray is painted, and empty-for-empty does not repaint.
  2. fix(ui-web): found by an adversarial pass over the served page. Consuming the owned text draft (a send, a slash command) had been made to drop the tray as well, so /compress, and /clear before its confirmation was answered, threw the staged files away. Consuming the text draft now takes the text only; the tray goes only when the conversation itself is deleted from the rail, and a send still empties it through takeAtts. Also, a failed upload wrote its transcript note into whichever conversation was open; the note stays in the conversation the file was staged in, and once the reader has moved on the failure is a toast instead.

Decisions: attachments are not persisted across a reload (an image chip's data URL alone can exceed the storage quota; an in-flight upload means nothing after a reload); an upload in flight is not cancelled by a switch, it lands in its own tray. The sub-agent direct-chat composer keeps its own per-instance state and is unchanged. Server side is untouched: uploads are agent-home rooted and cross-session valid, which is why the symptom was a wrong attachment rather than a broken one.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

  • npm test --prefix ui-web -- src/features/composer -- 117 passed (108 before). New cases in describe('composer drafts') and the tray describe: the tray follows its conversation across switches; a draft's tray is re-keyed when it becomes a session; a late upload lands in the tray it was staged in; a slash command keeps the staged file; a late failure while another conversation is open raises a toast and writes no transcript note there; a failure in the open conversation still writes the note. The slash and misrouted-note cases fail against the first commit's store.ts; the rest fail against the previous one.

  • npm test --prefix ui-web -- 188 files, 2473 passed.

  • npm run --prefix ui-web type-check and npm run --prefix ui-web lint -- clean (4 pre-existing warnings in untouched files).

  • Served page, isolated agent home, two conversations: before the fix a file staged in A shows in B, B's text-only message carries A's file and A's tray is emptied; after the fix B opens with an empty tray and a disarmed send, A keeps its chip and thumbnail, B's message carries no attachment, and a file staged on the new-task screen rides that draft's first message and is gone afterwards. Second pass: a file staged and /compress run keeps its chip; /clear cancelled at the confirmation keeps it; an upload refused after a switch (a 110 MB file against the 100 MB ceiling) shows a toast with the file name and reason, writes no note into the open conversation, and leaves no chip behind in the one it was staged in.

  • Relevant tests pass locally

  • Relevant lint / type checks pass locally

  • User-facing docs or screenshots are updated when needed

Risk

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Client-only change in one module; no wire or Python change. Staged attachments still vanish on a browser reload while text drafts survive it (unchanged from before). Rollback: revert the two commits in reverse order.

Related Issues

N/A

The composer's attachment tray was one page-wide array that no conversation
switch parked, loaded or cleared. A file staged in one conversation stayed on
screen after switching, armed the send button there, and rode out with the next
message sent from the other conversation, leaving the first one empty. An upload
that landed or failed after a switch wrote whatever tray was open, so a failed
chip could stay pinned to the conversation it was staged in and refuse every
later send there.

Key the tray by the same owner the text draft already uses: a module-level map
of trays with one writer, swapped in by loadDraft alongside the text, re-keyed
by claimDraft when a new draft becomes a session, and dropped with the draft.
The staging paths capture their owner once, so a late upload lands in the tray
it was staged in. Only the open conversation's tray is painted, and an empty
tray replacing an empty one does not repaint, so a rail click on
attachment-free conversations stays as cheap as before. A slash command drops
its draft's tray along with the text and repaints, so the screen and the store
cannot disagree.

Attachments are not persisted across a reload the way text drafts are: an image
chip's data URL alone can exceed the storage quota, and an upload still in
flight has no meaning after a reload.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@0xKT
0xKT requested a review from gloryfromca September 21, 2026 20:57

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the full github/main...HEAD diff and the surrounding composer, session-switch, draft-promotion, deletion, send, upload, and template-pick paths. I also checked the branch history, backward-compatible ownership behavior, the added tests for weakening/gaps, and the applicable AGENTS.md and Web UI architecture/domain rules. The per-owner tray is swapped, claimed, cleared, and updated consistently, including late upload completion/failure, without changing the wire boundary.

Verification:

  • npm test -- --run src/features/composer/ComposerPage.test.tsx (47 passed)
  • npm run type-check (passed)
  • npx eslint src/features/composer/store.ts src/features/composer/ComposerPage.test.tsx (passed)
  • npm test (188 files, 2472 tests passed)
  • git diff --check github/main...HEAD (passed)

@0xKT

0xKT commented Sep 22, 2026

Copy link
Copy Markdown
Member Author

Not a blocker -- the fix is right and I could not break it. One gap, and it is in the tests rather than the code: of the two places that capture owner before a round trip, only one has a test that notices when the capture is removed.

Measured on cb83185c merged onto the live tip 08be2a25.

What I retracted, and what noticed

Each line of behaviour put back the way it was before this PR, one at a time, against ComposerPage.test.tsx:

retraction                                                        tests that failed
loadDraft no longer swaps the tray on a session switch             7
traySet paints even when the key is not the open conversation      2
dropDraft no longer clears its tray                                2
claimDraft no longer migrates the 'new' tray                       1  (named for it)
runSlash paints before dropping the draft (the old order)          1  (named for it)
addFiles' drop() resolves the owner when the failure LANDS         1  (named for it)
addTemplate's .catch resolves the owner when the failure LANDS     0  <-- nothing noticed

The last two are the same shape in the same file. addFiles has drops a failed upload from the tray it was staged in; addTemplate has no sibling, so the identical guard can be deleted and the suite stays green.

It is worth more than tidiness because of what the ghost costs. A failed pick that clears the wrong tray leaves entry in the staged conversation with uploading: true forever, and send() refuses on attsPending() before it does anything else (store.ts:594) -- so that conversation can never send again, which is exactly the harm your own comment on addFiles names ("a ghost that refuses that conversation every later send").

Here is the missing test. It fails on the retraction above and passes on your code as written -- I checked both ways, 49 passed with it, 1 failed | 48 passed with the guard removed:

it('drops a failed template pick from the tray it was staged in', async () => {
  let fail: (e: unknown) => void = () => {}
  const { calls } = wire({
    ...uploader(),
    templates: {
      list: () => Promise.resolve({ templates: [], available: true }),
      pick: () => new Promise((_r, rej) => { fail = rej }),
      pages: () => Promise.resolve({ pages: [] }),
    } as unknown as ComposerSource['templates'],
  })
  mountTray()
  store.loadDraft('a')
  act(() => { store.addTemplate({ name: 'deck', label: 'Deck', size: 9 }) })
  expect(store.attsPending()).toBe(1)

  act(() => { store.loadDraft('b') })
  await act(async () => { fail({ data: { detail: 'no such template' } }); await flush() })

  act(() => { store.loadDraft('a') })
  expect(store.get().atts).toEqual([])
  expect(store.attsPending()).toBe(0)
  expect(calls.notes.length).toBe(1)
})

Two retractions that survived and should not count against you

  • addFiles/addTemplate on the SUCCESS path (traySet(owner, trayOf(owner).slice()) -> traySet(ownerKey(), ...)) survives, and I think that is correct rather than a gap: entry is mutated in place, so the staged tray already holds the finished attachment, and writing the current tray back unchanged is a no-op that traySet's own key !== ownerKey() guard then declines to paint. The capture only matters on the failure paths, which is where the tests are.
  • traySet's empty-replacing-empty guard survives every test, but its comment claims a repaint count, not a state -- nothing observable to assert. Fine as it stands.

Everything else I tried and could not break

trays never disagrees with what is painted for the open conversation; claimDraft moves the 'new' tray and leaves the paint alone because the same array was already on screen; dropDraft on a conversation you are not looking at writes the map and skips the paint; _resetForTests clears the map. The map is only ever emptied per key, so a conversation with unsent files holds them for the life of the tab -- which is the point, and bounded by conversations that actually staged something.


GATES OK head=cb83185c tip=08be2a25 base=c7f0a04f ruff/lint-imports/commit/large/lang rc=0; npm test 190 files / 2515 tests pass; npx tsc --noEmit exit 0; npm run gen:check in sync (194 methods); npm run build + ui-web/build.py both booted-DOM goldens OK at 243 nodes.

…load failure

The per-conversation tray made consuming the owned draft drop the tray as
well, so a slash command -- /compress, or /clear before its confirmation
was answered -- threw the staged files away. Consuming the text draft now
takes the text only; the tray goes only when the conversation itself is
deleted from the rail. A send still empties it through takeAtts.

A failed upload wrote its transcript note into whichever conversation was
open. The note stays in the conversation the file was staged in; once the
reader has moved on, the failure is a toast instead.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@0xKT

0xKT commented Sep 22, 2026

Copy link
Copy Markdown
Member Author

Second commit pushed, c63e307d, after an adversarial pass over the served page found two more holes in the same invariant.

# Finding Disposition
1 A slash command destroyed the staged files: consuming the owned draft (runSlash -> dropOwnedDraft -> dropDraft) dropped the tray as well, so /compress, and /clear before its confirmation was answered, lost the attachments (cancelling did not restore them). Introduced by the first commit. Fixed. Consuming the text draft takes the text only (dropDraftText); the tray goes only when the conversation itself is deleted from the rail (dropDraft), and a send still empties it through takeAtts.
2 A failed upload's transcript note landed in whichever conversation was open (addFiles / addTemplate catch handlers called transcriptNote unconditionally). Pre-existing, but the per-conversation tray is what made a failure after a switch reachable. Fixed. sayFailed(owner, ...) writes the note when the owner is still the open conversation and raises a toast (label plus detail) otherwise.

Tests: ComposerPage.test.tsx -- a slash command keeps the staged file and the tray still follows its conversation afterwards; a late failure while another conversation is open raises a toast and writes no note there; a failure in the open conversation still writes the note and no toast. All three fail against the first commit's store.ts. Composer suite 117 passed; whole npm test --prefix ui-web 188 files / 2473 passed; type-check and lint clean.

Served page: /compress with a file staged keeps the chip; /clear cancelled at its confirmation keeps it; an upload refused after a switch (110 MB against the 100 MB ceiling) shows the toast with the file name and reason, writes nothing into the open conversation, and leaves no chip in the one it was staged in.

PR description updated (two commits, the second pass in Verification).

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the delta from cb83185c12f7 and rechecked the full change against the composer, session deletion, slash-command, upload, template-pick, and transcript/toast callers. I also checked the applicable repository and Web UI architecture rules, history, backward compatibility, and whether the revised tests could mask the behavior. The split between consuming draft text and deleting a conversation now preserves staged files across slash commands while still clearing them on actual deletion, and owner-aware failure reporting avoids contaminating another conversation's transcript. The previously noted missing template mutation test does not expose a concrete defect because both async paths use the same captured owner and sayFailed helper.

Verification:

  • npm test -- --run src/features/composer/ComposerPage.test.tsx (48 passed)
  • npm run type-check (passed)
  • npx eslint src/features/composer/store.ts src/features/composer/ComposerPage.test.tsx (passed)
  • npm test (188 files, 2473 tests passed)
  • git diff --check github/main...HEAD (passed)

@0xKT

0xKT commented Sep 22, 2026

Copy link
Copy Markdown
Member Author

Not a blocker. Re-reviewed at c63e307d. Both fixes are real and each is pinned. First, though, something I owe you.

I passed the head that had the slash-command bug

I graded cb83185c accepted. On that head, running /compress -- or /clear before its confirmation was answered -- threw away the files the reader had staged. You found it; I did not.

I did mutation-test that exact line. Retracting the runSlash reordering failed clears the open tray when a slash command drops its draft, so code and tests agreed and I moved on. What I never asked was whether that test's name described behaviour anyone wanted. It did not, and the test you replaced it with says the opposite:

- it('clears the open tray when a slash command drops its draft')
+ it('leaves the staged files where they are when a slash command runs')

Mutation testing shows the tests grip the code. It cannot tell me the spec is right, and here I read a test name as the spec. That is on me, and it is the more useful half of this reply.

The new commit, retracted line by line

retraction                                                        tests that failed
dropOwnedDraft back to dropDraft (the bug above)                  1  leaves the staged files where they are when a slash command runs
a late failure writes a transcript note into whatever is open     2  says a late failure in a toast ... / drops a failed upload from the tray it was staged in
dropDraft (the rail delete) no longer clears the tray             1  forgets the files of a dropped session and leaves the others

Each bites, and the third is the one that shows the split is the right one rather than just a smaller hammer: dropDraftText for consuming a draft, dropDraft for the rail deleting the conversation, and the tray follows only the second. sayFailed reads well -- the note goes where the file was staged, and a toast once the reader has moved on, which is the only place a late failure can be said without landing in somebody else's transcript.

Still open from my last note

addTemplate's failure path still has no test. Retracting its owner capture -- traySet(owner, ...) to traySet(ownerKey(), ...) in the .catch -- leaves all 49 green, while the identical retraction in addFiles' drop() fails drops a failed upload from the tray it was staged in immediately. The test I offered last time still applies unchanged; the cost is still that a ghost left uploading blocks every later send in that conversation (send() refuses on attsPending(), store.ts:594).

Not a blocker, and not worth another round on its own -- folding it into whatever you touch next is fine.


GATES OK head=c63e307d tip=08be2a25 base=c7f0a04f ruff/lint-imports/commit/large/lang rc=0; npm test 190 files / 2516 tests pass; npx tsc --noEmit exit 0.

@0xKT
0xKT merged commit 3b63c6c into main Sep 22, 2026
23 of 26 checks passed
@0xKT
0xKT deleted the fix/composer_attachments_per_session branch September 22, 2026 06:24
LivXue pushed a commit that referenced this pull request Oct 12, 2026
…#619)

## Summary

A file staged in the composer followed the reader into every other
conversation: the tray was one page-wide array that a conversation
switch never parked, loaded or cleared, so the chips stayed on screen,
armed the send button, and rode out with the next message sent from the
other conversation. An upload that landed or failed after a switch wrote
whatever tray was open, which could leave a permanently "uploading" chip
on the conversation it was staged in and refuse every later send there.

Two commits:

1. `fix(ui-web)`: the tray is keyed by the same owner the text draft
already uses (the open conversation, or `new` for the first message of a
draft): one map of trays with a single writer, swapped in by `loadDraft`
alongside the text, re-keyed by `claimDraft` when a draft becomes a
session, dropped with the conversation. Staging captures its owner once,
so a late upload lands in the tray it was staged in. Only the open
conversation's tray is painted, and empty-for-empty does not repaint.
2. `fix(ui-web)`: found by an adversarial pass over the served page.
Consuming the owned text draft (a send, a slash command) had been made
to drop the tray as well, so `/compress`, and `/clear` before its
confirmation was answered, threw the staged files away. Consuming the
text draft now takes the text only; the tray goes only when the
conversation itself is deleted from the rail, and a send still empties
it through `takeAtts`. Also, a failed upload wrote its transcript note
into whichever conversation was open; the note stays in the conversation
the file was staged in, and once the reader has moved on the failure is
a toast instead.

Decisions: attachments are not persisted across a reload (an image
chip's data URL alone can exceed the storage quota; an in-flight upload
means nothing after a reload); an upload in flight is not cancelled by a
switch, it lands in its own tray. The sub-agent direct-chat composer
keeps its own per-instance state and is unchanged. Server side is
untouched: uploads are agent-home rooted and cross-session valid, which
is why the symptom was a wrong attachment rather than a broken one.

## Type

- [x] Fix
- [ ] Feature
- [ ] Docs
- [ ] CI / tooling
- [ ] Refactor
- [ ] Other

## Verification

- `npm test --prefix ui-web -- src/features/composer` -- 117 passed (108
before). New cases in `describe('composer drafts')` and the tray
describe: the tray follows its conversation across switches; a draft's
tray is re-keyed when it becomes a session; a late upload lands in the
tray it was staged in; a slash command keeps the staged file; a late
failure while another conversation is open raises a toast and writes no
transcript note there; a failure in the open conversation still writes
the note. The slash and misrouted-note cases fail against the first
commit's `store.ts`; the rest fail against the previous one.
- `npm test --prefix ui-web` -- 188 files, 2473 passed.
- `npm run --prefix ui-web type-check` and `npm run --prefix ui-web
lint` -- clean (4 pre-existing warnings in untouched files).
- Served page, isolated agent home, two conversations: before the fix a
file staged in A shows in B, B's text-only message carries A's file and
A's tray is emptied; after the fix B opens with an empty tray and a
disarmed send, A keeps its chip and thumbnail, B's message carries no
attachment, and a file staged on the new-task screen rides that draft's
first message and is gone afterwards. Second pass: a file staged and
`/compress` run keeps its chip; `/clear` cancelled at the confirmation
keeps it; an upload refused after a switch (a 110 MB file against the
100 MB ceiling) shows a toast with the file name and reason, writes no
note into the open conversation, and leaves no chip behind in the one it
was staged in.

- [x] Relevant tests pass locally
- [x] Relevant lint / type checks pass locally
- [ ] User-facing docs or screenshots are updated when needed

## Risk

- [x] Security impact considered
- [x] Backward compatibility considered
- [x] Rollback path is clear for risky changes

Client-only change in one module; no wire or Python change. Staged
attachments still vanish on a browser reload while text drafts survive
it (unchanged from before). Rollback: revert the two commits in reverse
order.

## Related Issues

N/A

---------

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants