fix(desktop): polish list pages — Usage, Memory, and Web Search - #1479
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Approved. The production fix is in the right places: DataTable owns its scroll boundary, and the page CSS only handles the grid and text constraints that belong to those pages. The remaining settings-row overflow is correctly left to the shared follow-up instead of being patched here.
I left two inline comments about states that the Story and E2E do not actually render. Those are coverage gaps, not objections to the implementation.
…e2e and stories Review follow-up for apache#1479 (both inline comments): - The floor containment test never rendered the two long-content branches: the default fixture has no request logs (requests DataTable never mounts) and no Tavily key (the page stops at the no-key message). Two new tests lock the actual fixes — the request table scrolls inside its own container while the page stays contained (settings-usage fixture, refresh-until-seeded to dodge the boot race), and the hostile-width results (bare-URL title, long snippet) wrap inside their cards. - Web search results come from a typed offline fixture (web-search-e2e-fixture.ts, mirrors the story rows) answered AFTER the real validation/enablement gates; the settings-search scenario now seeds a configured Tavily key so the query controls are enabled. - UsageEmpty story now opens on the actual EmptyState: it reuses the details-on requests-tab settings instead of default settings whose showDetails:false rendered another summary baseline. Settings e2e 15/15 locally; desktop unit tests 2845/2845.
|
@Astro-Han The follow-up commit (c6804b7) seems to be waiting for a maintainer to approve the workflow run — no check suite has been created on it (fork PRs appear to be gated now; base-repo PRs are still triggering fine). Local verification: settings e2e 15/15, desktop unit 2845/2845, format/typecheck clean. Could you hit "Approve and run" when you get a chance? |
Sub-issue of apache#1303, same shape as the apache#1361 metric-pages pass. Based on the Story fixtures first — two of three pages had no usable baseline ---------------------------------------------------------------- - Memory: the story bridge had no `memory` / `workspaceInstructions` / `app.openPath` channels at all, so the story booted into two error toasts and an empty page — no entry list, backup candidates, or prompt preview ever rendered. The bridge now serves a clean empty state by default; `MemoryPopulated` covers the list surfaces with deliberately long titles, contents, and tag sets. - Usage: `logs` was `[]` and the requests tab defaults to its summary-only Alert, so **no story ever rendered a DataTable** — the issue's headline theme had no baseline. `UsageRequestsPopulated` pins hostile-width rows (dated preview model id, namespaced MCP tool name, full UUID session); `UsageEmpty` covers the all-empty tabs. - Web search: the result list only renders after a live query. `WebSearchResults` drives the real query path via a play step against a configured-key bridge (built by hand — `mergeSettings` treats the masked apiKey sentinel as "keep current" and would drop it), with a long title, a bare-URL title, and a long snippet. Layout fixes (all found by measuring the new baselines) ------------------------------------------------------- - Usage page + tab panel: pin the implicit single grid column to `minmax(0, 1fr)`. With the implicit `auto` track, every block's min-content propagated into the track — the five-tab bar (~453px intrinsic) and the requests table dragged the page into horizontal scroll even though both scroll within themselves. - Usage tab bar: five nowrap tabs are wider than the whole content column at the 480px window floor; the bar scrolls within itself (tabs stay one line by design). - Usage filters: was a five-track grid (~724px min-content) behind a 900px *viewport* breakpoint; between the breakpoint and ~1100px the grid overflowed the page because the settings column is narrower than the viewport. Same lesson as apache#1361: wrap structurally instead of guessing a threshold — the filter input grows from its old 260px track, everything else falls to the next line when it no longer fits. - Usage summary: `repeat(4, minmax(0, 1fr))` → `auto-fit`, the same hard-track division apache#1361 fixed on the Permissions/Health summaries (tiles collapsed to ~32px at the floor). Full-width layout unchanged. - Memory prompt-preview header: the trailing cluster (injection Chip + copy button) cannot shrink; it wraps under the title at the floor. - Web search: three unbreakable-token fixes (`overflow-wrap: anywhere` on result titles that are bare URLs, result source hostnames, and the TAVILY_API_KEY env-var hint), `min-width: 0` on the query/key inputs (a text input never shrinks below its intrinsic size width on its own), and the mono nowrap timestamp may wrap in the narrow layout. Routed to apache#1360, not patched here --------------------------------- The Memory page still overflows at the 480px floor through the shared settings-rows primitive: a `.settingsFormRow`'s min-content (~306px — label + status Chip + switch + paddings) exceeds the ~190px content column. That is the shared-primitive row-wrapping problem apache#1362 names for the form-row pages, so it is documented on apache#1360 rather than hacked per-page; the e2e contract asserts Memory per-surface (prompt-preview header) instead of whole-page containment, with a comment explaining why. Verification ------------ - Two e2e contract tests: containment + tab-bar/summary geometry at the 480px floor for Usage and Web search, per-surface for Memory; plus a full-width guard so `auto-fit` cannot silently cost the wide layout. Confirmed they fail against origin/main's CSS. - Full settings e2e suite 13/13; desktop unit suite 2845/2845. - Probed 7 stories x 480-1280px in Storybook: zero horizontal overflow beyond the documented settings-rows remainder. - Verified in the live app at 1280px and 480px on all three pages. Closes apache#1364.
…e2e and stories Review follow-up for apache#1479 (both inline comments): - The floor containment test never rendered the two long-content branches: the default fixture has no request logs (requests DataTable never mounts) and no Tavily key (the page stops at the no-key message). Two new tests lock the actual fixes — the request table scrolls inside its own container while the page stays contained (settings-usage fixture, refresh-until-seeded to dodge the boot race), and the hostile-width results (bare-URL title, long snippet) wrap inside their cards. - Web search results come from a typed offline fixture (web-search-e2e-fixture.ts, mirrors the story rows) answered AFTER the real validation/enablement gates; the settings-search scenario now seeds a configured Tavily key so the query controls are enabled. - UsageEmpty story now opens on the actual EmptyState: it reuses the details-on requests-tab settings instead of default settings whose showDetails:false rendered another summary baseline. Settings e2e 15/15 locally; desktop unit tests 2845/2845.
c6804b7 to
89681fb
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Approving 89681fb. The E2E now reaches the request table and long Web Search results, and UsageEmpty opens on the actual empty state. All required checks pass.
One follow-up: settings.spec.ts calls .toPass() without a timeout. With the locked Playwright 1.61.1, this defaults to 0 and does not inherit expect.timeout, so the refresh block runs once. Please set a timeout on the call or configure expect.toPass.timeout. This is not blocking.
Sub-issue of #1303 — the visual/layout audit of the list/long-content pages (Usage / Memory / Web Search), same shape as the #1361 metric-pages pass. Based on #1478 (the
DataTablescroller fix this pass traced to the shared primitive); the first commit here is that branch. Closes #1364.Two of three pages had no usable baseline
memory/workspaceInstructions/app.openPathchannels — the story booted into two error toasts and an empty page; entry list, backup candidates and prompt preview never renderedlogs: []+ requests tab defaults to its summary Alert — no story ever rendered a DataTable, so the issue's headline theme ("DataTable overflow") had no baseline; that is also why the primitive defect in #1478 went unseenNew variants per the issue's expected deliverables (empty vs populated vs long-content):
UsageRequestsPopulated(hostile-width rows: dated preview model id, namespaced MCP tool name, full-UUID session) /UsageEmpty/ existingUsage(summary-only Alert)MemoryPopulated(long title/content/tag sets, archived group, backup-candidate rows) /Memory(now a clean empty state)WebSearchResults— a play step drives the real query path against a configured-key bridge, with a long title, a bare-URL title, and a long snippet. One wrinkle worth noting: the bridge settings are built by hand becausemergeSettingstreats the maskedapiKeysentinel as "keep current key" and silently drops it.What the audit found (all measured, 480–1280px)
Usage — the page grid's implicit
autotrack let every block's min-content propagate: the five-tab bar (~453px intrinsic) and the requests table dragged the page into horizontal scroll even though both should scroll within themselves. Pinned the single column tominmax(0, 1fr)(page + tab panel), gave the tab bar its ownoverflow-x: auto, and replaced two hard-track grids:repeat(4, minmax(0, 1fr))— the same hard-track division fix(desktop): polish metric pages — Permissions and Health #1361 fixed on Permissions/Health (~32px tiles at the floor) →auto-fit, full-width layout unchanged (locked by a wide-layout guard).Web search — three unbreakable tokens (
TAVILY_API_KEYhint, bare-URL result titles, long result hostnames) each widened the page at the floor →overflow-wrap: anywhere;min-width: 0on the query/key inputs (a text input never shrinks below its intrinsicsizewidth on its own); the mono nowrap timestamp may wrap in the narrow layout.Memory — the prompt-preview header's trailing cluster (injection Chip + copy button) cannot shrink → wraps under the title.
Routed to #1360, not patched here
Memory still overflows at the 480px floor through the shared
settings-rowsprimitive: a.settingsFormRow's min-content (~306px) exceeds the ~190px content column. That is exactly #1362's declared "row wrapping" scope, so it is documented on the routing issue rather than hacked per-page — and the e2e contract deliberately asserts Memory per-surface (prompt-preview header) instead of whole-page containment, with a comment explaining why.Verification
auto-fitcannot cost the wide layout). Confirmed they fail against origin/main's CSS (toHaveCSS('overflow-x', 'auto')on the tab bar).Live-app screenshots
Usage and Web search at the 480px floor (summary folds, tab bar scrolls within itself, tokens wrap):
The new populated baselines — requests DataTable scrolling in its own container at 1280px, and the Memory entry surfaces that had never rendered in a story: