fix(web): chat load timeouts/retry, Show all sessions, deterministic All Chats search (#170, #150, #145, #275) - #455
Merged
Merged
Conversation
Loading an existing chat session had three gaps: - `fetchChatMessages` (and the ad hoc `fetchSessionByPath` equivalent) had no timeout or abort, so a hung API call — proxy timeout, huge transcript, dead server — left the feed on "Loading messages..." forever, with a page reload as the only escape. Both now run under a 15s AbortController and report a clear "Timed out loading messages" error. - An *existing* session that loaded zero messages showed "Send a message to start the conversation", which reads as "this chat is new" when in fact the transcript could not be read. `MessageFeed` now takes the session id and shows "No messages found for this session" for that case, while genuinely new chats keep the original prompt. - There was no way to retry a failed load. Both the error banner and the empty state now offer a Retry that re-runs the fetch in place. Closes #170 Co-Authored-By: Claude <noreply@anthropic.com>
`sessionsToShow` was hardcoded to `filteredSessions.slice(0, INITIAL_SESSIONS_SHOWN)` with no state tracking whether the user had asked for more. Clicking "Show all" called `loadMoreGroupSessions`, which fetched the next page from the server and updated the store — but the render slice never moved, so a directory group could never display more than 10 sessions no matter how many were loaded. Add a local `showAll` flag: the click reveals every already-loaded session immediately and only hits the server when the group is partially loaded. The button stays visible (as "Load more (N remaining)") while the server still has more, so groups larger than one page can be paged through, and is disabled while a fetch is in flight. Builds on the approach in #161 by @edspencer, which could not be applied directly: the file has since moved its search helper to lib/session-utils, and that version hid the button as soon as `showAll` was set, capping a group at the first server page. Closes #150 Co-Authored-By: Claude <noreply@anthropic.com>
The reported bug — collapsing a directory group while a search was active immediately re-expanded it, because `expandedGroups` sat in the auto-expand effect's dependency array and `toggleAllChatsGroup` always produces a fresh `Set` — was already fixed on main, but nothing stopped it regressing. Add a component test that searches, collapses a group, and asserts it stays collapsed. Verified it fails (in fact, renders an infinite effect loop) when `expandedGroups` is put back in the dependency array. Closes #145 Co-Authored-By: Claude <noreply@anthropic.com>
The "no results" journey rendered one of two different messages depending on the host machine's session history, which is why the Playwright spec had to accept either and was ultimately skipped in CI. The cause is a real inconsistency, not just a test problem. `groupMatchesQuery` kept a group whose *directory path or agent name* matched the query, but `DirectoryGroup` then filtered that group's sessions by session fields only — so the group survived with zero rows and rendered its own "No sessions match your search". Searching for a directory name therefore produced a list of empty groups instead of results. Resolve it in one place: - Move `groupMatchesQuery` into lib/session-utils alongside a new `groupHeaderMatchesQuery`, so the page and the group agree on what matched. - A group whose header matched shows ALL of its sessions (they are all relevant); a group that matched only via its sessions shows just those. - A group that matches neither is dropped entirely, so the single top-level "No matching sessions" state is the only no-results message users can see. The spec is un-skipped in CI and now waits for the seeded session to appear in `/api/chat/all` before searching. It previously waited on the text "talker", which also renders in the layout sidebar — so it could proceed with an empty group list and never render any no-results message at all. Refs #275 Co-Authored-By: Claude <noreply@anthropic.com>
The raw ping/pong browser probe timed out consistently in CI even at 30s while
every higher-level WebSocket journey passed. That was a server bug, not a
flaky test.
`WebSocketHandler.handleConnection` awaited `fleetManager.getFleetStatus()` to
send the initial snapshot, and only afterwards called `socket.on("message",
...)`. Any frame a client sent the instant its socket opened arrived at a
socket with no "message" listener and was silently dropped. On a cold or
loaded server — a freshly booted fleet, a CI runner — that window is wide
enough to hit for real, and it affects any early client frame (the dashboard's
keepalive ping, subscribe, chat:send), not just the probe.
Attach the message/close/error listeners synchronously, before the first
await. A consequence is that `pong` may now arrive before `fleet:status`, so
the probe waits for both instead of assuming an ordering, retries its
connection, and is un-skipped in CI.
Refs #275
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
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 |
Deploying herdctl with
|
| Latest commit: |
2879b05
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://240b76ee.herdctl.pages.dev |
| Branch Preview URL: | https://fix-web-bug-cluster.herdctl.pages.dev |
CI proved the All Chats specs were never testing what they claimed. Core's session discovery deliberately skips temp paths — `isTempDirectory` in `state/session-discovery.ts` filters `/tmp/`, `/private/tmp/`, `/var/folders/` and `os.tmpdir()` — and the harness put every agent's working directory under `mkdtemp(tmpdir())`. A harness-seeded session therefore could NEVER appear in the machine-wide All Chats listing, on any machine. That is the real reason the two specs diverged between environments: - "a completed chat appears in the All Chats directory listing" asserted on the text "talker", which also renders in the layout sidebar. It passed everywhere while proving nothing about the session list. - The no-results spec passed on a developer machine only because the developer's own ~/.claude session groups were present and all filtered out by the impossible query. On a clean CI runner there were no groups at all, so the page sat on its base empty state and rendered no no-results message. Move only the agent workspaces out of the temp root, into an alphanumeric-only directory under $HOME (removed on teardown). Config, state and scratch stay in tmpRoot. The names avoid non-alphanumerics because `encodePathForCli` maps every such character to "-" and the listing decodes it back by turning every "-" into "/", so anything else round-trips lossily. The harness now also exposes `agentEncodedPath` (core's exact grouping key) and `agentDisplayWorkdir` (the lossy decode the UI actually renders), so the specs can assert on a group precisely instead of on an ambiguous agent name. Refs #275 Co-Authored-By: Claude <noreply@anthropic.com>
5 tasks
Merged
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
1 participant
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.
Fixes a cluster of long-open
@herdctl/webbugs. One commit per issue, each independently reviewable.fix(web): time out, retry, and correctly empty-state chat loadsfix(web): expand a directory group beyond the first 10 sessionstest(web): lock in the collapse-during-search fixfix(web): make the All Chats no-results state deterministicfix(web): attach WebSocket listeners before awaiting fleet statusfix(web): give UI-test agents a non-temp working directory#170 — ChatView had no timeout, no retry, and a misleading empty state
All three gaps in the issue were still present. Fixed as proposed, plus the ad hoc session path which had the same problems:
fetchChatMessagesandfetchAdhocChatMessagesnow run under a 15sAbortControllerand reportTimed out loading messages after 15s. Previously a hung call left the feed on "Loading messages..." forever.MessageFeedtakes asessionId. An existing session with zero messages now shows "No messages found for this session" instead of "Send a message to start the conversation"; genuinely new chats are unchanged.ChatViewandAdhocChatView. On failure the active session is kept so retry knows what to re-fetch.#150 — "Show all" never expanded past 10 sessions
Still a live bug on
main. Adds theshowAllflag from #161, with two corrections that PR needed:showAllwas set, which caps a group at the first server page (a 100-session group would stop at 60). The button now stays asLoad more (N remaining)while the server has more.loadMoreGroupSessionsis awaited with a disabled/loading button, so the click isn't a no-op-looking dead control.#161 can't be applied as-is (the file moved its search helper to
lib/session-utilsin #160), so its approach is reimplemented and credited in the commit message. Recommend closing #161 as superseded.#145 — groups re-expanded on toggle during search
Already fixed on
main—expandedGroupswas removed from the effect deps and abiome-ignoreadded. Nothing locked it in, so this adds a component test that searches, collapses a group, and asserts it stays collapsed. Verified it fails (infinite effect loop) when the dep is restored.#275 — two CI-fragile test-ui specs
Both turned out to have real causes. Neither is fixed by loosening assertions; both specs are un-skipped and now pass in CI (28/28, no skips).
1. Raw ws probe — a server bug, not a flaky test.
WebSocketHandler.handleConnectionawaitedgetFleetStatus()before callingsocket.on("message", ...), so any frame a client sent the instant its socket opened arrived at a listener-less socket and was silently dropped (wsdoes not buffer). On a cold or loaded server the window is wide enough to hit for real, and it affects any early client frame — the dashboard keepalive ping,subscribe,chat:send— not just the probe. Listeners are now attached synchronously before the firstawait.pongmay consequently arrive beforefleet:status, so the probe waits for both rather than assuming an ordering. It now passes in CI in ~525ms (previously timed out at 30s).2. All Chats no-results — two layered causes.
Product:
groupMatchesQuerykept a group whose directory path or agent name matched, butDirectoryGroupthen filtered its sessions by session fields only — so the group survived with zero rows and rendered its own "No sessions match your search". Searching a directory name produced a list of empty groups instead of results. Unified inlib/session-utils: a group whose header matches shows all its sessions; a group matching only via sessions shows just those; a group matching neither is dropped entirely, so the single top-level "No matching sessions" is the only no-results message.Test harness (found by pushing this PR and reading the CI failure): core's session discovery deliberately skips temp paths —
isTempDirectoryinstate/session-discovery.tsfilters/tmp/,/private/tmp/,/var/folders/andos.tmpdir()— and the harness put every agent working directory undermkdtemp(tmpdir()). A harness-seeded session could therefore never appear in the All Chats listing, on any machine. That is the real source of the environment divergence:a completed chat appears in the All Chats directory listingasserted on the texttalker, which also renders in the layout sidebar. It passed everywhere while proving nothing.~/.claudegroups were present and all filtered out by the impossible query. On a clean CI runner there were no groups at all, so the page sat on its base empty state.Only the agent workspaces move out of the temp root (to an alphanumeric-only dir under
$HOME, removed on teardown); config and state stay intmpRoot. The harness now exposesagentEncodedPath(core's exact grouping key) andagentDisplayWorkdir(the lossy decode the UI renders) so specs assert on a group precisely instead of on an ambiguous agent name.Verification
pnpm typecheck✅ ·pnpm build✅ · Lint & Format ✅ · Unit Tests ✅ · Build + Smoke ✅ · Web UI Tests ✅ (28 passed, 0 skipped)DirectoryGroup.test.tsx,AllChatsPage.test.tsx,MessageFeed.test.tsx,chat-slice.test.ts, plus aws-handlertest that sends a message while the fleet-status promise is still pending.@herdctl/webis at 195 passing unit tests (was 141).@herdctl/core/herdctl(5s-timeout tests inhooks/__tests__/hook-executor.test.ts,scheduler/__tests__/scheduler.test.ts,cli/src/__tests__/smoke.test.ts— different tests each run). Nothing in this PR touches those packages; worth a separate look at test timeouts on the CI runner.Changeset:
@herdctl/webpatch.🤖 Generated with Claude Code