fix: Show All button in DirectoryGroup now properly expands sessions - #161
fix: Show All button in DirectoryGroup now properly expands sessions#161edspencer wants to merge 2 commits into
Conversation
Fixes #150 - Add showAll state to track whether user clicked "Show all" - When showAll is true, render all filteredSessions instead of slicing to INITIAL_SESSIONS_SHOWN - Button now shows all locally-loaded sessions first, then fetches more from server if needed - Hide button after it's been clicked (when showAll is true) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a changeset file documenting a patch release and modifies DirectoryGroup component to fix the non-functional "Show All" button. Introduces local Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Deploying herdctl with
|
| Latest commit: |
dfc37c8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3bd151bf.herdctl.pages.dev |
| Branch Preview URL: | https://fix-150-show-all-button.herdctl.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web/src/client/src/components/all-chats/DirectoryGroup.tsx (1)
65-65: Add an explicit boolean type forshowAllstate.Line 65 currently relies on inference; prefer explicit typing for consistency with strict TS conventions.
Suggested diff
- const [showAll, setShowAll] = useState(false); + const [showAll, setShowAll] = useState<boolean>(false);As per coding guidelines,
**/*.{ts,tsx}: Use strict TypeScript with explicit types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/client/src/components/all-chats/DirectoryGroup.tsx` at line 65, The state declaration for showAll uses type inference; update it to an explicit boolean type to satisfy strict TS rules by changing the useState call that defines showAll and setShowAll (the useState import or invocation in DirectoryGroup.tsx) to declare useState<boolean>(...) so showAll is explicitly typed as boolean while preserving the initial value and existing usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/client/src/components/all-chats/DirectoryGroup.tsx`:
- Around line 80-85: handleShowAll currently calls loadMoreGroupSessions once
(which fetches limit:50 in all-chats-slice.ts) then hides the "Load more" button
via setShowAll(true), so groups with >50 remaining sessions become inaccessible;
update handleShowAll in DirectoryGroup.tsx to either (a) loop/await
loadMoreGroupSessions(group.encodedPath) until hasMoreOnServer is false so all
pages are fetched before hiding the button, or (b) keep the load-more control
visible (do not call setShowAll(true) or hide the button) and let the user
repeatedly invoke loadMoreGroupSessions while hasMoreOnServer remains true;
reference the symbols handleShowAll, loadMoreGroupSessions, and hasMoreOnServer
when making the change.
---
Nitpick comments:
In `@packages/web/src/client/src/components/all-chats/DirectoryGroup.tsx`:
- Line 65: The state declaration for showAll uses type inference; update it to
an explicit boolean type to satisfy strict TS rules by changing the useState
call that defines showAll and setShowAll (the useState import or invocation in
DirectoryGroup.tsx) to declare useState<boolean>(...) so showAll is explicitly
typed as boolean while preserving the initial value and existing usage.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/fix-show-all-button.mdpackages/web/src/client/src/components/all-chats/DirectoryGroup.tsx
| // First, show all locally-loaded sessions | ||
| setShowAll(true); | ||
| // Then fetch more from server if there are any | ||
| if (hasMoreOnServer) { | ||
| loadMoreGroupSessions(group.encodedPath); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Searching for loadMoreGroupSessions definition and usage =="
rg -nP --type=ts --type=tsx -C4 '\bloadMoreGroupSessions\b'
echo
echo "== Checking DirectoryGroup.tsx content around lines 80-85 and 154 =="
fd -e tsx -e ts "DirectoryGroup" | head -5Repository: edspencer/herdctl
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Finding DirectoryGroup.tsx =="
fd "DirectoryGroup" -e tsx -e ts
echo
echo "== Searching for loadMoreGroupSessions without type restriction =="
rg -n 'loadMoreGroupSessions' --max-count=20
echo
echo "== Checking pagination-related patterns =="
rg -n 'hasMore|nextCursor|offset|limit|page' | head -30Repository: edspencer/herdctl
Length of output: 2340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Reading all-chats-slice.ts loadMoreGroupSessions implementation =="
cat -n packages/web/src/client/src/store/all-chats-slice.ts | sed -n '100,150p'
echo
echo "== Reading DirectoryGroup.tsx around lines 80-85 and 154 =="
cat -n packages/web/src/client/src/components/all-chats/DirectoryGroup.tsx | sed -n '60,100p'
echo
echo "== Reading DirectoryGroup.tsx around line 154 =="
cat -n packages/web/src/client/src/components/all-chats/DirectoryGroup.tsx | sed -n '145,165p'Repository: edspencer/herdctl
Length of output: 4362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Checking for any pagination continuation or hasMore tracking =="
grep -n "sessionCount\|sessions.length\|hasMore" packages/web/src/client/src/store/all-chats-slice.ts | head -20
echo
echo "== Check if there's re-fetching logic elsewhere =="
rg -n "loadMoreGroupSessions" packages/web/src/client/src/store/ -A5 -B5
echo
echo "== Check API response structure =="
rg -n "fetchDirectoryGroupSessions" packages/web/src/client/src/ -A10 | head -40Repository: edspencer/herdctl
Length of output: 4916
Confirm pagination handles groups with >50 additional sessions.
loadMoreGroupSessions fetches with limit: 50 (all-chats-slice.ts:118), but handleShowAll (DirectoryGroup.tsx:79–85) calls it only once. If a group has more than 50 additional sessions beyond those already loaded, the button disappears after the first fetch (line 154), leaving remaining sessions inaccessible. Either loop until hasMoreOnServer is false, or preserve a visible "Load more" button when additional server data exists.
Also applies to: 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/client/src/components/all-chats/DirectoryGroup.tsx` around
lines 80 - 85, handleShowAll currently calls loadMoreGroupSessions once (which
fetches limit:50 in all-chats-slice.ts) then hides the "Load more" button via
setShowAll(true), so groups with >50 remaining sessions become inaccessible;
update handleShowAll in DirectoryGroup.tsx to either (a) loop/await
loadMoreGroupSessions(group.encodedPath) until hasMoreOnServer is false so all
pages are fetched before hiding the button, or (b) keep the load-more control
visible (do not call setShowAll(true) or hide the button) and let the user
repeatedly invoke loadMoreGroupSessions while hasMoreOnServer remains true;
reference the symbols handleShowAll, loadMoreGroupSessions, and hasMoreOnServer
when making the change.
edspencer
left a comment
edspencer
left a comment
There was a problem hiding this comment.
This PR fixes the "Show All" button in DirectoryGroup so that clicking it now renders all locally-loaded sessions (via new showAll state) instead of always slicing to the first 10 — resolving #150. It also fires a server fetch when more sessions exist remotely, and hides the button once showAll is set.
I read DirectoryGroup.tsx in full and traced loadMoreGroupSessions in store/all-chats-slice.ts (it fetches one page of limit: 50 at offset: group.sessions.length, merges results, and swallows errors internally so there's no unhandled-rejection risk from the un-awaited call). The local-display fix is correct. I found one correctness gap in how the button interacts with server pagination for large groups.
|
|
||
| {/* Show more button */} | ||
| {(hasMoreLoaded || hasMoreOnServer) && ( | ||
| {!showAll && (hasMoreLoaded || hasMoreOnServer) && ( |
There was a problem hiding this comment.
MEDIUM · correctness — "Show all" cannot reveal all sessions when the group has more than one server page
loadMoreGroupSessions fetches only a single page of limit: 50 per call (see store/all-chats-slice.ts:117-120, offset: group.sessions.length). handleShowAll calls it exactly once and simultaneously sets showAll = true, and this line hides the button whenever showAll is true (!showAll && ...).
So for a group where sessionCount > sessions.length + 50 (e.g. 10 loaded locally, 200 total), one click fetches 50 more (60 visible) and then permanently hides the button — the remaining ~140 sessions can never be loaded, even though the button label promised "Show all 200 sessions". Groups with up to initial + 50 sessions work fine; larger ones don't fully expand.
One fix is to keep the button visible while the server still has more, so repeated clicks keep paging (each click just fetches the next 50). Since handleShowAll already only fetches when hasMoreOnServer, re-clicking after showAll is set simply loads the next page:
| {!showAll && (hasMoreLoaded || hasMoreOnServer) && ( | |
| {((!showAll && hasMoreLoaded) || hasMoreOnServer) && ( |
|
Reviewed 2 changed files (5 hunks); ran the verify pass; 1 finding. |
|
Assessment while working through #150 in #455. The bug this PR fixes is still live on The approach here is right, and #455 uses it (local
#455 credits this PR in the commit message and adds regression tests ( |
…All Chats search (#170, #150, #145, #275) (#455) * fix(web): time out, retry, and correctly empty-state chat loads (#170) 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> * fix(web): expand a directory group beyond the first 10 sessions (#150) `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> * test(web): lock in the collapse-during-search fix (#145) 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> * fix(web): make the All Chats no-results state deterministic (#275) 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> * fix(web): attach WebSocket listeners before awaiting fleet status (#275) 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> * chore: changeset for the @herdctl/web bug fixes Co-Authored-By: Claude <noreply@anthropic.com> * fix(web): give UI-test agents a non-temp working directory (#275) 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> --------- Co-authored-by: HomeLab Agent <homelab-infra@valfenda.net> Co-authored-by: Claude <noreply@anthropic.com>
1 participant
Summary
Fixes #150
showAllstate to track whether user clicked "Show all" buttonshowAllis true, component renders allfilteredSessionsinstead of slicing toINITIAL_SESSIONS_SHOWNhasMoreOnServeris trueshowAllis true)Changes
useStatefrom ReactshowAllstate (defaults tofalse)filteredSessionsbased onshowAllstatehandleShowAllto set state and conditionally fetch from servershowAllis trueTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes