feat(@helm/web): read-only kanban dashboard and item detail (Session 6)#10
Merged
Conversation
api.ts wraps fetch with typed ApiResult<T> — ok/error discriminated union, network errors caught and returned (never throw to caller). Imports Product from @helm/shared and WorkflowStage from @helm/workflow; defines ItemState and WorkflowEvent locally mirroring apps/api response shapes. 6 tests covering success, HTTP error, and network error paths. Adds vitest + jsdom + @testing-library/react test infrastructure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
usePolling<T>(fetcher, intervalMs) fetches immediately then polls via setInterval; stable fetcherRef avoids re-creating the effect on re-renders; intervalMs=null fetches once. Kanban renders dynamic columns from product.workflow.stages_enabled, filters items per stage, shows relative time and item count badges. Items error shown inline; product error shown full-page. Polling every 5s with cleanup on unmount. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ItemDetail polls GET /api/items/:id every 5s, renders stage badge, full transition history table (newest first), and back-to-board link. Error state shown full-page. App.tsx replaces the WebSocket placeholder with BrowserRouter + two routes: / → Kanban, /items/:id → ItemDetail. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
6 tests: initial loading state, re-fetch on interval tick, interval cleanup on unmount, error on fetch failure, error cleared on recovery, and intervalMs=null fetch-once behaviour. Uses vi.useFakeTimers() to control setInterval without real time delays. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the WebSocket status UI with BrowserRouter routes and adds a typed API client, a reusable usePolling hook, Kanban and ItemDetail views that poll the API, plus Vitest/jsdom test setup and tests for the API client and polling hook. ChangesWeb UI with routing and polling
Sequence DiagramsequenceDiagram
participant User
participant App
participant Kanban
participant ItemDetail
participant usePolling
participant api
participant Server
User->>App: navigate to /
App->>Kanban: render
Kanban->>usePolling: call usePolling(api.getProduct)
usePolling->>api: fetcher() product
api->>Server: GET /api/product
Server-->>api: Product JSON
api-->>usePolling: ApiResult{data}
usePolling-->>Kanban: {data, loading:false}
Kanban->>usePolling: call usePolling(api.listItems)
usePolling->>api: fetcher() items
api->>Server: GET /api/items
Server-->>api: ItemState[] JSON
api-->>usePolling: ApiResult{data}
usePolling-->>Kanban: {data, loading:false}
Kanban-->>User: render board by stage
Note over usePolling: after interval tick (5s)
usePolling->>api: fetcher() items
api->>Server: GET /api/items
Server-->>api: ItemState[] JSON
api-->>usePolling: ApiResult{data}
usePolling-->>Kanban: {data, loading:false}
Kanban-->>User: re-render board
User->>App: click item card
App->>ItemDetail: navigate /items/:id
ItemDetail->>usePolling: call usePolling(api.getItem(id))
usePolling->>api: fetcher() single item
api->>Server: GET /api/items/{id}
Server-->>api: ItemState JSON
api-->>usePolling: ApiResult{data}
usePolling-->>ItemDetail: {data, loading:false}
ItemDetail-->>User: render item detail + history
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/vite.config.ts (1)
1-1: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse Vitest-aware config typing for the
testblock.Import
defineConfigfromvitest/config(or add Vitest config type reference) sotestoptions are properly typed in tooling.Suggested change
-import { defineConfig } from 'vite'; +import { defineConfig } from 'vitest/config';Also applies to: 17-21
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/vite.config.ts` at line 1, The Vite config's test block lacks Vitest-aware typing; replace or augment the current import of defineConfig from 'vite' so the config is typed for Vitest by importing defineConfig from 'vitest/config' (or add a Vitest config type reference) and ensure the existing defineConfig(...) call that contains the test: { ... } block uses that Vitest-aware defineConfig import so IDE/typecheck picks up Vitest options correctly; update any other occurrences of defineConfig/imports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/package.json`:
- Line 26: The dependency "`@types/react-router-dom`" listed in package.json
conflicts with the installed react-router-dom@7.15.1 which now includes built-in
TypeScript types; remove the "`@types/react-router-dom`" entry from package.json
(the exact string "`@types/react-router-dom`": "^5.3.3") and then run your package
manager install (npm/yarn/pnpm) to update node_modules and lockfile so
TypeScript uses the bundled types from react-router-dom@7.15.1.
- Around line 28-32: The package dependency mismatch is that
"`@vitest/coverage-v8`" is at major version 4 while "vitest" is at major version
2; update the dependency entry for "`@vitest/coverage-v8`" in package.json to a
v2-compatible version (e.g., "^2.0.0" or the latest 2.x) so both packages share
the same major version, then reinstall (npm/yarn/pnpm install) to update
lockfiles; target the "`@vitest/coverage-v8`" key to locate and change the
version.
In `@apps/web/src/hooks/usePolling.test.ts`:
- Around line 22-128: Add a new test to apps/web/src/hooks/usePolling.test.ts
that covers the rejection path of the fetcher: renderHook(() =>
usePolling(fetcher, 1_000)) with fetcher = vi.fn().mockRejectedValue(new
Error('boom')) (or mockRejectedValue('boom')), call act to let the initial fetch
run, then assert result.current.error equals the rejection message,
result.current.data is null, and result.current.loading is false; also advance
timers with vi.advanceTimersByTime(...) and assert no additional successful data
is populated (use fetcher call counts) to lock behavior. Ensure the test
references usePolling, fetcher, renderHook, act, vi.advanceTimersByTime and
checks result.current.error/loading/data to locate the relevant code.
In `@apps/web/src/hooks/usePolling.ts`:
- Around line 30-55: Wrap the body of doFetch with a try/catch/finally to catch
and log/handle any exceptions thrown by fetcherRef.current(), and introduce an
in-flight guard (e.g., local boolean like isFetching or a ref) to prevent
concurrent runs so subsequent setInterval ticks skip while a fetch is pending;
check the cancelled flag before writing state in both success and error paths
and only call setLoading(false) in finally when the fetch was the active one
(and not after cancellation). Update references to doFetch, fetcherRef,
cancelled, intervalMs, id, setData, setError, and setLoading accordingly so
thrown errors are handled and overlapping requests cannot produce
stale/out-of-order state writes.
In `@apps/web/src/lib/api.test.ts`:
- Around line 67-72: The test "URL-encodes the externalId" (using api.getItem
and mockFetch) incorrectly uses 'issue_1' which contains no characters that
require encoding; update the test to pass an externalId containing characters
that must be encoded (e.g., spaces, slashes, question/ampersand or non-ASCII)
and then assert that mockFetch was called with a URL containing
encodeURIComponent(externalId) (i.e., the percent-encoded form) rather than the
raw input, so the test fails if encoding is removed.
In `@apps/web/src/lib/api.ts`:
- Line 40: The BASE_URL initialization uses the nullish coalescing operator
only, which allows whitespace-only VITE_API_URL values to pass through; update
the assignment for BASE_URL to read the env var via (import.meta as { env?: {
VITE_API_URL?: string } }).env?.VITE_API_URL, apply .trim() and a truthiness
check, and fall back to '' when the trimmed value is empty; modify the BASE_URL
constant accordingly so any whitespace-only or empty env value becomes the
empty-string fallback.
In `@apps/web/src/views/ItemDetail.tsx`:
- Around line 34-35: The history table rows conditionally render a sixth cell
only when event.note exists, which breaks alignment with the five-column header;
update the row rendering in ItemDetail.tsx (the JSX that outputs {event.note &&
<td ...>{event.note}</td>}) to always output a note <td> (render event.note or
an empty/placeholder string) so the column structure remains consistent, and
apply the same change to the other occurrence of this pattern later in the file
(the second history-row rendering block around the other {event.note && ...}
usage).
In `@apps/web/src/views/Kanban.tsx`:
- Around line 80-82: The render repeatedly filters allItems per stage; instead
compute a single grouping once (e.g., groupedItems via allItems.reduce or a Map)
keyed by stage id (use the same key as stages from
product?.workflow.stages_enabled) and then use groupedItems.get(stageId) or
groupedItems[stageId] inside the column rendering (replace allItems.filter(...)
uses around the stages loop and the occurrences noted near lines 102-107).
Ensure groupedItems falls back to an empty array for missing keys and compute it
outside the stages.map so it is reused across renders.
---
Outside diff comments:
In `@apps/web/vite.config.ts`:
- Line 1: The Vite config's test block lacks Vitest-aware typing; replace or
augment the current import of defineConfig from 'vite' so the config is typed
for Vitest by importing defineConfig from 'vitest/config' (or add a Vitest
config type reference) and ensure the existing defineConfig(...) call that
contains the test: { ... } block uses that Vitest-aware defineConfig import so
IDE/typecheck picks up Vitest options correctly; update any other occurrences of
defineConfig/imports accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20f8f7fb-8df1-4305-9adf-a854d3b523f6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/web/package.jsonapps/web/src/App.tsxapps/web/src/hooks/usePolling.test.tsapps/web/src/hooks/usePolling.tsapps/web/src/lib/api.test.tsapps/web/src/lib/api.tsapps/web/src/test-setup.tsapps/web/src/views/ItemDetail.tsxapps/web/src/views/Kanban.tsxapps/web/vite.config.ts
1. Remove conflicting @types/react-router-dom@5 (RR v7 ships own types)
2. Align @vitest/coverage-v8 to ^2.1.9 (v4 requires Vite 6+)
3. Add rejection test for usePolling (fetcher throws vs returns {ok:false})
4. Harden usePolling: inFlight guard prevents overlapping requests;
try-catch maps thrown errors to error state (never-throw parity)
5. URL-encoding test now uses 'issue/1 with space' to actually exercise
encodeURIComponent
6. VITE_API_URL: trim() + truthiness check (empty-string-in-input category)
7. ItemDetail note column: always rendered with "—" fallback so thead/tbody
column counts stay consistent
8. Kanban: useMemo groups items by stage once per render instead of
running filter() once per stage per render
Addresses CodeRabbit review comments on PR #10.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/hooks/usePolling.ts (1)
52-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
setLoading(false)with cancellation check infinally.On Line 54,
setLoading(false)runs even after cleanup setscancelled = true. Keep cancellation behavior consistent across all state writes.Proposed fix
} finally { inFlight = false; - setLoading(false); + if (!cancelled) setLoading(false); }#!/bin/bash # Verify that setLoading(false) is currently unguarded in finally. rg -n -C3 'finally|setLoading\(false\)|cancelled' apps/web/src/hooks/usePolling.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/hooks/usePolling.ts` around lines 52 - 55, In the usePolling hook's finally block, avoid calling setLoading(false) after cleanup has marked cancelled=true; change the finally so that setLoading(false) is only called when cancelled is false (e.g., if (!cancelled) setLoading(false)). Keep the assignment inFlight = false as-is or adjust similarly if you want it guarded, but at minimum wrap the setLoading(false) call with the cancelled check to prevent state updates after cancellation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/views/ItemDetail.tsx`:
- Around line 82-83: The heading and the monospace subtext both render the same
prop item.externalId; update the <h1> in ItemDetail.tsx to show the
human-friendly field (e.g., item.name, item.title, or item.displayName) instead
of item.externalId, leaving item.externalId only in the monospace <p> for the
technical identifier, or if no human-friendly field exists, remove the duplicate
element so the identifier is only shown once.
In `@apps/web/src/views/Kanban.tsx`:
- Around line 29-30: The UI currently renders item.externalId twice (first <p>
with className "truncate text-sm font-medium text-gray-900" and second <p> with
"mt-0.5 line-clamp-2 text-xs text-gray-500"), which is redundant; update the
second element in the Kanban view to show the appropriate descriptive field
(e.g., item.description or item.name) instead of item.externalId, or remove the
second <p> entirely if no descriptive field exists — keep the first <p> as the
primary ID display and ensure the "line-clamp-2" element reflects multi-line
descriptive content when present.
- Around line 10-19: The relativeTime helper currently computes diff using new
Date(iso).getTime() which yields NaN for malformed ISO strings; update
relativeTime to validate the parsed timestamp (e.g., const ts = new
Date(iso).getTime(); if (isNaN(ts)) return a safe fallback like 'unknown' or
'invalid date') before computing diff, and also handle negative diffs (future
dates) if needed; modify the function body around the new Date(iso).getTime()
usage to perform this check and return the fallback early.
---
Duplicate comments:
In `@apps/web/src/hooks/usePolling.ts`:
- Around line 52-55: In the usePolling hook's finally block, avoid calling
setLoading(false) after cleanup has marked cancelled=true; change the finally so
that setLoading(false) is only called when cancelled is false (e.g., if
(!cancelled) setLoading(false)). Keep the assignment inFlight = false as-is or
adjust similarly if you want it guarded, but at minimum wrap the
setLoading(false) call with the cancelled check to prevent state updates after
cancellation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08dbb08b-4784-4a79-8fd5-074849d65a42
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
apps/web/package.jsonapps/web/src/hooks/usePolling.test.tsapps/web/src/hooks/usePolling.tsapps/web/src/lib/api.test.tsapps/web/src/lib/api.tsapps/web/src/views/ItemDetail.tsxapps/web/src/views/Kanban.tsx
1. ItemDetail: externalId was shown twice; replace redundant <p> with productSlug as contextual subtitle (ItemState has no title field) 2. Kanban ItemCard: externalId was shown twice; replace second line with currentStage so the card conveys two distinct pieces of information 3. relativeTime: guard against NaN from malformed ISO strings with isNaN(date.getTime()) → return 'unknown' Addresses CodeRabbit review comments on PR #10. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/views/ItemDetail.tsx`:
- Around line 7-12: The formatDate function can produce "Invalid Date" when
given malformed ISO strings; update formatDate to parse the input into a Date,
check validity via isNaN(date.getTime()) (or Number.isNaN), and return a safe
fallback (e.g., empty string or hyphen) when invalid, otherwise return
date.toLocaleString(...) as before; reference the formatDate function to locate
the change and ensure the fallback is used whenever the Date is invalid.
- Around line 105-107: The current key for HistoryRow uses
`${event.at}-${event.toStage}` and falls back to the unstable array index from
the reversed array; to fix it, create a stable, collision-resistant key: if each
event has a unique id (e.g., event.id) use that as the key in HistoryRow;
otherwise attach the original index before reversing (e.g., item.history.map((e,
idx) => ({ e, idx })).reverse().map(({ e, idx }) => <HistoryRow
key={`${e.at}-${e.toStage}-${idx}`} ... />)) so keys remain stable across
renders and do not depend on the reversed array index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: beb62aca-7f0a-43b9-bfcb-792b856c719d
📒 Files selected for processing (2)
apps/web/src/views/ItemDetail.tsxapps/web/src/views/Kanban.tsx
1. formatDate: guard against malformed ISO strings with Number.isNaN check — same defensive pattern applied to relativeTime in round 2 2. History row key: replace at+toStage with at+fromStage+toStage+ triggeredBy to avoid collisions when two events share the same timestamp and destination stage Addresses CodeRabbit review comments on PR #10. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/views/ItemDetail.tsx`:
- Around line 57-68: The current ItemDetail render returns the full-page error
UI whenever error is truthy, which replaces previously loaded content on
transient poll failures; change the condition to only render the full-page error
when error is present AND item is falsy (i.e., if (error && !item) ...) so that
existing item data remains visible on transient errors, and instead render an
inline warning when error && item; apply the same pattern to the other error
branches in this file (the similar checks around the other error render blocks)
and update any JSX that currently shows the full-page error to use the inline
warning component when item exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01f94bb1-9e0c-4943-8304-bb216b91b14f
📒 Files selected for processing (1)
apps/web/src/views/ItemDetail.tsx
…etail Full-page error replaced content even when prior data was available, making brief network hiccups look like hard outages. Now: full-page error only on first-load failure (no data yet); inline amber banner when polling fails but last-known data is present. Matches the pattern already used in Kanban (itemsError shown inline without hiding the board). Addresses CodeRabbit review comment on PR #10. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/views/ItemDetail.tsx`:
- Around line 46-47: The polling starts even when the route param id is missing
because fetcher falls back to '' and triggers failing requests; update the
fetcher and polling usage in ItemDetail (the fetcher created via useCallback and
the usePolling call) to short-circuit when id is falsy: return a noop/undefined
result or disable polling by passing null/false/etc. to usePolling (or skip
calling it) when id is not present, so requests to api.getItem are only made
once a valid id exists. Ensure you reference the fetcher (useCallback(() =>
api.getItem(...))) and the usePolling(...) invocation when implementing the
guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd2f6370-7b13-4557-b1a9-9e8bc999dd53
📒 Files selected for processing (1)
apps/web/src/views/ItemDetail.tsx
id ?? '' triggered repeated GET /api/items/ failures when the route
param was absent. Now: returns {ok:false} immediately if id is falsy
and disables the polling interval (intervalMs=null) for the same case,
so no network requests are made until a valid id is present.
Addresses CodeRabbit review comment on PR #10.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
This was referenced May 17, 2026
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.
Summary
src/lib/api.ts): typedApiResult<T>wrapper aroundfetch— ok/error discriminated union, network errors returned (never thrown). ImportsProductfrom@helm/shared,WorkflowStagefrom@helm/workflow;ItemState/WorkflowEventdefined locally mirroring API response shapes.usePolling<T>(src/hooks/usePolling.ts): fetches immediately then polls viasetInterval; stablefetcherRefavoids re-running the effect on re-renders;intervalMs=nullfor fetch-once behaviour. Interval cleared on unmount.src/views/Kanban.tsx): columns fromproduct.workflow.stages_enabled, items filtered per stage, relative time badges, polling every 5s. Items error shown inline; product error shown full-page.src/views/ItemDetail.tsx): stage badge, transition history table (newest first), back-to-board link, same 5s polling pattern./→ Kanban,/items/:id→ ItemDetail.Test plan
pnpm --filter @helm/web test— 12 tests pass (6 API client + 6 polling hook)pnpm --filter @helm/web build—tsc --noEmit+ Vite build clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
API
Tests
Chores