Repository navigation
fix(chat): keep unconfirmed legacy sends and share the pane's command list (#1591) - #2164
Conversation
…onfirmed (#1591) A bridge without "submit" takes a message as raw input and sends no receipt (SubmitResult unconfirmed). The composer, Send now on a held copy and the held queue's Send now treated that as delivered: the draft or the recovery row went, although nothing said the pane got it. The text now stays as an uncertain pending row (Not confirmed, Copy, Discard saved copy), never sent again from here.
… list another component loaded (#1591) The command cache kept only the read started last, so while the terminal's own read was still on its way the box's answered read was not shared, and a receipt for a terminal command could not open the peek. An answer is now kept unless a newer read's answer already is, PaneTerminal reads it at the receipt, and usePaneCommands draws it on its first render. The browser script covers both catalog orders, and the claude-terminal menu case gets back the checks #1580 dropped (headings, badge text and fit, ranking).
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit details
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 |
|
Merging 85cf605: omo-native-code-reviewer and Codex both pass this head. This touches queued input, and both reviewers checked it as such: no send is resent, dropped or delivered to the wrong pane, and the new tests fail on main. The required checks are green, and CodeRabbit did not review this head. Codex's MEDIUM (with a bridge that sends no |
…mands once (#2560) <!-- See CONTRIBUTING.md for checks, translations, contract tests and the changelog (a file in changelog.d/, never CHANGELOG.md). --> ## Change Fixes #2559. A pane opened in Chat sent the same `GET /api/pane/commands` 4 times at once. A chat opened again after a minute drew its skill mentions as plain text until a new read answered. Its terminal-only commands showed as plain bubbles. Both come from the pane's command cache, so one PR fixes both. - `src/lib/paneCommandCache.ts`: - `cachedPaneCommands` shares the read on its way, per PC, pane, agent and invalidation generation. The terminal, the chat and the composer mount together and send one request. - A read begun after `invalidatePaneCommands` has a new generation, so it starts fresh. The older answer is still never kept (#1441). - The entry goes when the read settles. A failed read fails every caller that shares it, and the next call asks again (the retry in `usePaneCommands` does that). - The read-order check from #2164 (`reads`, `kept.read < read`) goes. It kept a retry's older answer from replacing a refresh's newer one. With one read per list and generation, two reads of the same generation never overlap, so it cannot trigger. - `paneCommandsNow` serves the last list at any age (stale while revalidate). `cachedPaneCommands` still asks again after a minute, and that answer replaces the list. An invalidation still clears it. - `src/components/ChatView.tsx`: the skill matcher is a `useMemo` over the list of `usePaneCommands`. `src/lib/useSkillMatcher.ts` goes. It had no synchronous path and sent a read of its own. - `scripts/terminal-autoswitch-demo-regression.ts`: the catalog race (#1591) waited for 1 or 2 held extra reads. They no longer exist, so the script waits for the one shared read in "all" mode. It now asserts that the page sent one command read in both modes. - Changelog fragment: `changelog.d/chat-pane-commands-one-read.md`. Reference app: none. This changes no UI design. The chat keeps the command list it knows while a new read is on its way, and draws with it. I did not find how the Codex or Claude apps cache their own command lists, so I name none. The herdr stall behind the slow first rows (about 250 ms when RPC connections overlap) is `herdrdev/herdr#4572`. This PR does not fix it. It removes 3 of the 4 overlapping command reads. ## Validation - Red/green, `bun test src/lib/paneCommandCache.test.ts src/lib/usePaneCommands.hook.test.ts`: - With the new tests and `src/lib/paneCommandCache.ts` from `main`: 7 pass, 3 fail. - "the components of a pane that mount together share one read…": 4 fetches, expected 1. - "paneCommandsNow serves the last list at any age…": `null` 61 s after the read, expected the list. - "lets a retry join the refresh read…": 2 reads, expected 1. - With this PR: 10 pass, 0 fail. - The single-read test also fails when the shared read ignores the invalidation generation: the read after `invalidatePaneCommands` then joins the old one. - Targeted unit tests, 83 pass, 0 fail in 12 files: `src/lib/paneCommandCache.test.ts`, `src/lib/usePaneCommands.hook.test.ts`, `src/lib/skillMentions.test.ts`, `src/lib/useAgentSkills.hook.test.ts`, `src/components/ComposerRestored.test.tsx`, `src/components/ChatTurn.test.ts`, `src/components/ChatFallback.test.ts`, `src/components/ChatView.stoppedTurn.test.ts`, `src/app-hooks.test.ts`, `src/conventions.test.ts`, `scripts/conventions.test.ts`, `scripts/changelog-fold.test.ts`. - `bun run typecheck`: pass. - Browser regressions on the demo transport, headless Chromium, one at a time, on this PR and on `main` (fbb97b6): - `scripts/agent-skill-menu-demo-regression.ts` (the `/` menus and skill chips of 14 agent cases): pass on both, the same output. - `scripts/command-outcome-demo-regression.ts` (sent command rows, "Runs in the terminal"): 23 PASS on both, the same output. - `scripts/composer-fit-demo-regression.ts` (skill completion in the composer): 27 PASS on both, the same output. - `scripts/terminal-autoswitch-demo-regression.ts`: the catalog race now asserts one command read. I ran a copy that presses `ControlOrMeta+Shift+` where the script presses `Control+Shift+`, because Mod is Meta on a Mac. That copy is not in this PR. - This PR: 72 PASS, exit 0, in 2 of 2 runs. - This PR's script on `main`'s app: 68 PASS, then the new assertion fails as intended: "the pane's components share one read of its commands", actual 4, expected 1. - `main`'s own script on `main`: 3 runs, 3 failures (62, 64 and 66 PASS before them). Each was a 10 s timeout at a different step of the "screens" section. This PR's script passed that section on `main`'s app. I read this as flakiness under load on this Mac, but I did not prove it. - My reopen demo: the real `ChatView` over a fake bridge, a 300 ms command list, and the clock 61 s ahead. On `main`, the reopened chat painted 2 frames with no skill chip, then 3 chips. With this PR, 3 chips in the first frame. On the first open, `main` sent 2 command reads (this fixture mounts only `ChatView`), this PR 1. - Live check with the lag harness, in an isolated herdr session (`bun run check run`) with real Claude Code (haiku) panes. Two Ovis servers used that one session: this PR and `main` (fbb97b6). Headless Chromium at 1280×800, 4 runs, the servers in turns (A,B then B,A). `GET /api/pane/commands` per step: | Step | `main` | This PR | |---|---|---| | Page load on a Claude pane | 4 | 1 | | First open of a tab with a Claude pane and a shell split | 5 (4 + 1) | 2 (1 + 1) | | Switch to another workspace (idle Claude) | 4 | 1 | | New tab with agent | 4 | 1 (one run: 2, the second began 300 ms after the first answered) | | Cold load on the tab with the split | 5 | 2 | | Switch to a tab or workspace shown before | 0 | 0 | The counts were the same in every run. Timings are indicative only, because other timing runs used this Mac at the same time. Time to a settled chat showed no clear change (median of 4: first open of the tab 276 against 276 ms, workspace switch 305 against 315 ms). - My earlier live A/B on `main` feb3ae0 (14 desktop and 6 phone pairs, the same harness with an RPC probe) agrees. Command reads per open went from 4 to 1, and up to 8 to 1 for a new agent tab. herdr RPCs per step went down in every pair. Desktop medians: 70 → 52 on a first open, 72 → 51 on a workspace switch, 108 → 87 on a new agent tab, 96 → 72 on a cold load. Each Claude pane open downloaded 51 KB less. Time to a settled chat did not change beyond the noise. The conversation read still meets the herdr stall (`herdrdev/herdr#4572`) about as often. - Not run: `bun run check fast`. On this Mac it hangs on `server/usage-login-private`. These files also fail on `main` here, so they are not part of this check: `scripts/check`, `scripts/self-hosted-runner`, `scripts/install-star`, `scripts/ci-browser`, `scripts/ci-browser-parallel`, `server/codex`, `server/codex-processes`, `server/session-history`, `server/letta`, `server/remote-entry`, `server/core/composer-framing`, `server/usage-login-private`, and sometimes `server/bridge`. - Changelog fragment: `changelog.d/chat-pane-commands-one-read.md`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01HabHntUPvy5yA9iU3G1m5y Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Fixes the items of #1591 that were still open on main. #1805 fixed only the "before the capability snapshot" half of its MEDIUM.
Change
Unconfirmed sends (the MEDIUM). A bridge without
submittakes a message as raw input and sends no receipt (HerdrSocket.submit→{ ok: true, unconfirmed: true }). Three paths treated that as delivered, although nothing said the pane got the message:useComposerSends.composerSend: the composer cleared the draft.useComposerSends.actOnPending, Send now on a held copy:removeCopydropped its recovery row.useTerminalSends.sendComposerText, the held queue's Send now: the queue row went.The text now stays as a pending row in the
uncertainstate, with the new error codesubmit_unconfirmed(PendingMessageStore.keepUnconfirmed, orfail(..., uncertain)for the held copy). This is the existing row: "Not confirmed. Check the terminal before sending again." with Copy and Discard saved copy, and no Send now. So it is never sent again from here, and a reload keeps it. The composer box still lets the text go, since the message did leave. No new strings.Catalog race (LOWs 2 and 5). Composer, ChatView and PaneTerminal each read the pane's command list through
cachedPaneCommands. The cache kept only the read started last, so while the terminal's own read was on its way, the box's answered read was shared with nobody. A receipt for a terminal command then found no command and opened no peek.paneCommandsNowreads the list at once. PaneTerminal'sshowSentCommanduses it at the receipt.usePaneCommandsuses it on its first render, so a chat shown again draws its "Ran in terminal" rows at once instead of as plain bubbles first.Tests (LOWs 3 and 6).
src/lib/useComposerSends.test.ts(new): composer send and held-copy Send now, each answered unconfirmed and confirmed.src/lib/paneCommandCache.test.tsandsrc/lib/usePaneCommands.hook.test.ts: the shared answer and the first render.scripts/terminal-autoswitch-demo-regression.tsholds the pane's command reads:later: the box's read answered, the terminal's still waiting.all: nothing answered when the message leaves, the 40f6493 order, which nothing covered.scripts/skill-menu-cases/claude-terminal.tsgets back the checks feat(chat): hand terminal commands to the terminal lens #1580 dropped: heading order, badge text and fit, and ranking.Not changed here. A
sentframe from another connection for a pane's shared first message still counts as delivery for Send now. That only switches this view, and telling the two apart needs the server'spending-resultto say which action dispatched the message, a WS contract change. Filed as #2162.Design precedent
No new UI. The not-confirmed state is the row Ovis already uses for a send whose delivery it cannot prove (Pi transcript receipts, a dropped connection). The Codex and Claude apps likewise keep a message whose delivery is not confirmed visible with its text, rather than treating it as sent.
Verification
bun scripts/terminal-autoswitch-demo-regression.ts:latercase fails (the menu never gets the list, because the box's answer is not shared).allpasses on main (40f6493 fixed it). With 40f6493's ref update removed in a throwaway tree, it fails, so the case guards that fix.bun scripts/agent-skill-menu-demo-regression.ts claude-terminal: passes with the restored checks, 42 captures at 1280, 390 and 320 px, light and dark.bun run check fast: exit 0. Unit 7910 pass, 0 fail. Typecheck and build pass.submit, each chat message leaves a "Not confirmed" row until discarded. This is the intended trade-off: matching transcript text to clear them is what the pending store refuses to do.sendComposerText) has no unit test of its own. Its submit function is internal to the hook.usePaneCommandsreads the cache during render without subscribing to it. This is harmless: the list is what its own read brings.All captures come from the demo, never a live herdr session. Client-only change: no server,
shared/or remote bundle change.