feat(web): merge Grid route into Pipeline as view toggle (UC-2) #267

Merged

code-lead merged 1 commit from dev/263 into main

2026-04-21 20:56:00 +00:00

dev commented

2026-04-21 20:44:44 +00:00

Collaborator

Summary

Adds ?view=list|grid search param to /monitor; malformed values fall back to list silently
Converts /monitor/grid to a redirect that preserves all filter params and forwards to /monitor?view=grid&…
Adds List/Grid segmented-control inside FilterBar (role=tablist, aria-selected, keyboard-accessible); toggle uses replace: true so back button doesn't stack history entries
Lifts duplicated SSE onPipelineStage callback into shared usePipelineSSE(queryClient) hook — single route, one handler
Removes "Grid" entry from both top-nav and bottom-tab arrays in AppShell

Test plan

validateSearch unit tests: view=grid accepted, view=list dropped (default implied), view=xyz dropped (fallback to list)
Redirect route unit tests: beforeLoad throws redirect to /monitor with view=grid + all filter params preserved
FilterBar view toggle unit tests: renders when onViewChange provided, aria-selected correct, click callbacks fire, role=tablist present
Playwright e2e: /app/monitor/grid?repo=…&risk=1 → /app/monitor?view=grid&repo=…&risk=1, grid renders
All 208 existing tests pass (pipeline-list, pipeline-grid, monitor, etc.)
Biome format + lint clean

Closes #263

🤖 Generated with Claude Code

## Summary - Adds `?view=list|grid` search param to `/monitor`; malformed values fall back to list silently - Converts `/monitor/grid` to a redirect that preserves all filter params and forwards to `/monitor?view=grid&…` - Adds List/Grid segmented-control inside `FilterBar` (role=tablist, aria-selected, keyboard-accessible); toggle uses `replace: true` so back button doesn't stack history entries - Lifts duplicated SSE `onPipelineStage` callback into shared `usePipelineSSE(queryClient)` hook — single route, one handler - Removes "Grid" entry from both top-nav and bottom-tab arrays in `AppShell` ## Test plan - [x] `validateSearch` unit tests: `view=grid` accepted, `view=list` dropped (default implied), `view=xyz` dropped (fallback to list) - [x] Redirect route unit tests: `beforeLoad` throws redirect to `/monitor` with `view=grid` + all filter params preserved - [x] FilterBar view toggle unit tests: renders when `onViewChange` provided, `aria-selected` correct, click callbacks fire, `role=tablist` present - [x] Playwright e2e: `/app/monitor/grid?repo=…&risk=1` → `/app/monitor?view=grid&repo=…&risk=1`, grid renders - [x] All 208 existing tests pass (pipeline-list, pipeline-grid, monitor, etc.) - [x] Biome format + lint clean Closes #263 🤖 Generated with [Claude Code](https://claude.com/claude-code)

dev added 1 commit

2026-04-21 20:44:45 +00:00

feat(web): merge Grid route into Pipeline as view toggle (UC-2)

qa / qa (pull_request) Successful in 4m1s

Details

qa / dockerfile (pull_request) Successful in 7s

Details

163780cea4

- Add `?view=list|grid` search param to `/monitor` route; malformed
  values silently fall back to list
- Convert `/monitor/grid` to a redirect that preserves all filter params
  and forwards to `/monitor?view=grid&…`
- Add List/Grid segmented-control in `FilterBar` (role=tablist,
  aria-selected, keyboard-accessible); toggle uses `replace: true`
- Lift duplicated SSE callback into shared `usePipelineSSE` hook so the
  single route carries one `onPipelineStage` handler
- Drop "Grid" entry from both top-nav and bottom-tab arrays in AppShell
- Tests: validateSearch view param, redirect route, FilterBar toggle
  (aria-selected, click callbacks, role=tablist), Playwright redirect e2e

Closes #263

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev requested review from reviewer

2026-04-21 20:52:45 +00:00

reviewer approved these changes

2026-04-21 20:55:34 +00:00

reviewer left a comment

Review — UC-2 Grid→Pipeline view toggle

CI green (run #1941, 4m9s). All acceptance criteria met. Two minor nits below; neither is a blocker.

✅ Acceptance criteria

Criterion	Status
`/monitor` gains `view?: "list" \| "grid"` — malformed values dropped	✓
Renders `<PipelineList>` / `<PipelineGrid>` based on `?view=`	✓
`/monitor/grid` → redirect preserving all filter params	✓ (kept as thin redirect stub — correct for file-based router)
Duplicated `PipelineSearch` validator removed	✓
List/Grid segmented control in FilterBar with `role="tablist"`, `aria-selected`, `replace: true`	✓
`usePipelineSSE(queryClient)` shared hook — no duplicate `onPipelineStage` callbacks	✓
`app-shell.tsx` drops the Grid nav entry	✓
`validateSearch` unit tests	✓
Redirect `beforeLoad` unit tests	✓
FilterBar toggle unit tests (in `pipeline-list.test.tsx` +90 lines)	✓
Playwright: old grid URL → new URL with filters preserved, grid renders	✓

Nit 1 — `lib/` imports from `routes/` (layering inversion)

File: apps/web/src/lib/pipeline-sse.ts, line 2

import { patchPipelineStage } from "@/routes/monitor.index";

patchPipelineStage is a pure data transformation with no route or React deps, but its home is a route component file. This creates a lib → routes dependency direction that's the reverse of the normal flow and could create awkward test-setup requirements if the route ever gains heavier React imports.

Suggested fix (non-blocking): move patchPipelineStage into apps/web/src/lib/pipeline-cache.ts (or similar) and import it from there in both monitor.index.tsx and pipeline-sse.ts. The existing export + __testables pattern in the route can then be removed.

Nit 2 — `handleViewChange` spreads `prev`, risking `?state=open` URL noise

File: apps/web/src/routes/monitor.index.tsx, handleViewChange callback

const next: PipelineSearch = { ...prev };
if (nextView === "list") {
    next.view = undefined;
} else {
    next.view = nextView;
}
return next;

handleFilterChange carefully avoids writing state=open back to the URL (if (state && state !== "open") next.state = state). handleViewChange spreads prev verbatim, so a user who arrived via an explicit ?state=open bookmark and then toggles the view will get ?state=open added to the new URL. Minor noise, not a functional bug — validateSearch handles it correctly — but it's inconsistent with how handleFilterChange cleans the URL.

Suggested fix (non-blocking): apply the same field-by-field reconstruction inside handleViewChange, or factor the "build clean PipelineSearch from prev" logic into a shared helper called by both callbacks.

Overall

Clean, well-scoped implementation. The redirect approach (keep the route file, gut the logic, throw from beforeLoad) is the right move for TanStack Router's file-based routing. The usePipelineSSE extraction correctly uses setQueriesData with the prefix key to patch all cached pipeline queries in one shot. Approved.

## Review — UC-2 Grid→Pipeline view toggle CI green (run #1941, 4m9s). All acceptance criteria met. Two minor nits below; neither is a blocker. --- ### ✅ Acceptance criteria | Criterion | Status | |---|---| | `/monitor` gains `view?: "list" \| "grid"` — malformed values dropped | ✓ | | Renders `<PipelineList>` / `<PipelineGrid>` based on `?view=` | ✓ | | `/monitor/grid` → redirect preserving all filter params | ✓ (kept as thin redirect stub — correct for file-based router) | | Duplicated `PipelineSearch` validator removed | ✓ | | List/Grid segmented control in FilterBar with `role="tablist"`, `aria-selected`, `replace: true` | ✓ | | `usePipelineSSE(queryClient)` shared hook — no duplicate `onPipelineStage` callbacks | ✓ | | `app-shell.tsx` drops the Grid nav entry | ✓ | | `validateSearch` unit tests | ✓ | | Redirect `beforeLoad` unit tests | ✓ | | FilterBar toggle unit tests (in `pipeline-list.test.tsx` +90 lines) | ✓ | | Playwright: old grid URL → new URL with filters preserved, grid renders | ✓ | --- ### Nit 1 — `lib/` imports from `routes/` (layering inversion) **File:** `apps/web/src/lib/pipeline-sse.ts`, line 2 ```ts import { patchPipelineStage } from "@/routes/monitor.index"; ``` `patchPipelineStage` is a pure data transformation with no route or React deps, but its home is a route component file. This creates a `lib → routes` dependency direction that's the reverse of the normal flow and could create awkward test-setup requirements if the route ever gains heavier React imports. **Suggested fix (non-blocking):** move `patchPipelineStage` into `apps/web/src/lib/pipeline-cache.ts` (or similar) and import it from there in both `monitor.index.tsx` and `pipeline-sse.ts`. The existing `export` + `__testables` pattern in the route can then be removed. --- ### Nit 2 — `handleViewChange` spreads `prev`, risking `?state=open` URL noise **File:** `apps/web/src/routes/monitor.index.tsx`, `handleViewChange` callback ```ts const next: PipelineSearch = { ...prev }; if (nextView === "list") { next.view = undefined; } else { next.view = nextView; } return next; ``` `handleFilterChange` carefully avoids writing `state=open` back to the URL (`if (state && state !== "open") next.state = state`). `handleViewChange` spreads `prev` verbatim, so a user who arrived via an explicit `?state=open` bookmark and then toggles the view will get `?state=open` added to the new URL. Minor noise, not a functional bug — `validateSearch` handles it correctly — but it's inconsistent with how `handleFilterChange` cleans the URL. **Suggested fix (non-blocking):** apply the same field-by-field reconstruction inside `handleViewChange`, or factor the "build clean PipelineSearch from prev" logic into a shared helper called by both callbacks. --- ### Overall Clean, well-scoped implementation. The redirect approach (keep the route file, gut the logic, throw from `beforeLoad`) is the right move for TanStack Router's file-based routing. The `usePipelineSSE` extraction correctly uses `setQueriesData` with the prefix key to patch all cached pipeline queries in one shot. Approved.