feat(@helm/web): multi-product dashboard with tabs and URL routing (Session 8)#13
Merged
Merged
Conversation
listProducts() → GET /api/products getProductBySlug(slug) → GET /api/products/:slug listItemsForProduct(slug) → GET /api/products/:slug/items Legacy methods (getProduct, listItems, getItem) kept for backward compat until cleanup PR. 6 new tests covering happy paths, URL encoding, 404, network error, and empty response. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Renders horizontal tabs from GET /api/products, marks active tab via useParams slug, links to /products/:slug. Polls every 5s to surface newly-registered products without reload. Loading: blank bar (no layout shift). Error: inline ⚠ message. Empty registry: renders null. 6 tests covering: render, active marking, hrefs, error, empty, re-poll. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…roduct App.tsx: /products/:slug → Kanban, /products/:slug/items/:externalId → ItemDetail. ProductsRedirect resolves first product slug and navigates; shows empty state if registry is empty. LegacyItemRedirect maps old /items/:id to the new path for backward compat. Kanban: reads slug from useParams, calls getProductBySlug + listItemsForProduct, renders ProductTabs above the board. Product not found → inline error with back link. ItemCard links updated to /products/:slug/items/:externalId. ItemDetail: reads slug + externalId from useParams, back link resolves to /products/:slug. Error and loading states unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
useMemo was called after early returns in Kanban — violates Rules of Hooks. Moved before loading/error guards so it's always called unconditionally. Tests added: - Kanban.test.tsx: 5 cases — columns, item card, correct href, error+back link, empty items. Uses MemoryRouter with /products/:slug route. - App.test.tsx: 2 cases — empty registry shows empty state, redirect to /products/:firstSlug when products exist. 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 (3)
📝 WalkthroughWalkthroughMulti-product routing is introduced: client API gained product-scoped methods, ProductTabs provides product navigation, App adds redirect helpers and new routes, and Kanban/ItemDetail were refactored to use slug-scoped data. Tests were added across components and API client. ChangesMulti-product routing migration
Sequence DiagramssequenceDiagram
participant Router as React Router
participant ProductTabs
participant API as api.listProducts
participant Browser as Browser DOM
Router->>ProductTabs: slug param
ProductTabs->>API: fetch products (polling every 5s)
alt loading
ProductTabs->>Browser: render placeholder
else error
ProductTabs->>Browser: render error message
else empty
ProductTabs->>Browser: return null
else success
API-->>ProductTabs: Product[] result
ProductTabs->>Browser: render nav tabs, mark active by slug
Browser->>Router: on tab click navigate to /products/:slug
end
sequenceDiagram
participant Router as React Router
participant Kanban
participant API as api slug-scoped
participant ProductTabs
participant Board as Kanban Board
Router->>Kanban: :slug param
Kanban->>API: getProductBySlug(slug)
Kanban->>API: listItemsForProduct(slug)
API-->>Kanban: Product, ItemState[]
Kanban->>ProductTabs: render tabs
Kanban->>Board: render stages + items with slug
Board->>Router: item link to /products/:slug/items/:externalId
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: 7
🤖 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/App.test.tsx`:
- Around line 57-67: The test currently only checks rendered content but should
assert the redirected URL; after rendering App() and waiting for the UI, add an
assertion that the browser pathname equals the expected product path (e.g.
expect(window.location.pathname).toBe(`/products/${PRODUCTS[0].slug}`)); keep
the existing mocks (mockListProducts, mockGetProductBySlug,
mockListItemsForProduct) and the render(<App />) + vi.waitFor block and add this
pathname check alongside the heading assertion to verify the actual redirect.
In `@apps/web/src/App.tsx`:
- Line 39: The redirect constructs raw path segments from product identifiers
which can break routes for reserved characters; update the two Navigate usages
in App.tsx (the one that uses products[0]!.product.slug and the other at the
mentioned location that builds a path from a product id/slug) to encode path
segments with encodeURIComponent (e.g., apply encodeURIComponent to the slug or
id before interpolating into the `/products/...` path) so navigation uses safe,
encoded URLs.
- Line 16: The product registry fetch currently treats failures as empty results
because App.tsx only checks usePolling(fetchProducts) for data/loading; update
the flow so fetch errors are surfaced and handled instead of falling through to
the “no products” redirect. Modify usePolling (or the call site) to return an
error value (e.g., { data, loading, error }) when fetchProducts fails, then in
App.tsx where products/loading are used (the call to usePolling, and the
subsequent branches around lines referenced) add an explicit check for error and
route to an error UI/stop the redirect logic (or show a retry) rather than
treating error as an empty products array; reference the
usePolling(fetchProducts) call, the fetchProducts function, and the
redirect/empty-state branch in App.tsx when making these changes.
In `@apps/web/src/components/ProductTabs.tsx`:
- Line 38: The product tab URL is built using the raw slug
(product.product.slug) which can break for slugs containing reserved URL
characters; update the link construction in ProductTabs (where
`to={`/products/${product.product.slug}`}` is used) to encode the slug with
encodeURIComponent before interpolation so the resulting path is a valid URL
(e.g., use encodeURIComponent(product.product.slug) when creating the `to` value
and any other places in this component that build product URLs).
In `@apps/web/src/lib/api.test.ts`:
- Around line 103-108: The test named "URL-encodes the slug" doesn't actually
exercise encoding because it passes a plain slug; update it to use a slug with
characters that require encoding (e.g., a slug with spaces or ampersands), call
api.getProductBySlug(slug) as before, and assert that mockFetch was called with
a URL containing encodeURIComponent(slug) (or the explicit percent-encoded
string) instead of the raw slug; reference the existing test harness
variables/mockFetch and the api.getProductBySlug function when making this
change.
In `@apps/web/src/views/ItemDetail.tsx`:
- Around line 53-58: The component currently fetches the item via
fetcher/api.getItem using externalId but never validates that the returned item
belongs to the route's slug; add a check that item.productSlug === slug before
rendering details and handle mismatches by either navigating to the correct
product-scoped URL (e.g., replace/redirect to
`/products/${item.productSlug}/items/${externalId}`) or setting an
error/NotFound state; implement this check after the usePolling result (watch
item and slug) — update backLink only when slug is confirmed or use the item's
productSlug for backLink when redirecting.
In `@apps/web/src/views/Kanban.tsx`:
- Line 29: The link builds a route using raw slug and item.externalId
(to={`/products/${slug}/items/${item.externalId}`}) which can break for reserved
characters; update the Kanban.tsx link construction to URL-encode those dynamic
segments by wrapping slug and item.externalId with encodeURIComponent before
interpolating into the to prop (i.e., use encodeURIComponent(slug) and
encodeURIComponent(item.externalId)) so the generated path is safe for routing.
🪄 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: e5c1c062-c63e-415a-b404-bef835956b33
📒 Files selected for processing (9)
apps/web/src/App.test.tsxapps/web/src/App.tsxapps/web/src/components/ProductTabs.test.tsxapps/web/src/components/ProductTabs.tsxapps/web/src/lib/api.test.tsapps/web/src/lib/api.tsapps/web/src/views/ItemDetail.tsxapps/web/src/views/Kanban.test.tsxapps/web/src/views/Kanban.tsx
1. App.test.tsx: assert window.location.pathname === '/products/helm' to verify the redirect contract directly (not just rendered content) 2. App.tsx ProductsRedirect: expose error from usePolling so registry failures show "Failed to load products" instead of "no products" empty state. LegacyItemRedirect: redirect to / on error. 3+4. App.tsx + ProductTabs.tsx: encodeURIComponent on slug in Navigate and tab Link hrefs 5. api.test.ts: getProductBySlug URL-encoding test uses 'helm/playground with space' to actually exercise encodeURIComponent 6. ItemDetail.tsx: validate item.productSlug === slug — mismatch shows "Item does not belong to this product." error instead of rendering wrong item silently 7. Kanban.tsx: encodeURIComponent on slug + externalId in ItemCard href Addresses CodeRabbit review comments on PR #13. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/App.test.tsx`:
- Around line 28-30: The helper function ok<T>(data: T) returns the payload
directly, exposing mutable references; modify ok to defensive-copy mutable
inputs before returning by detecting arrays and plain objects and returning a
shallow copy (e.g., spread for objects and slice/[…array] for arrays) while
returning primitives as-is; update the function named ok to perform this shallow
cloning so tests don't share mutable state across calls.
In `@apps/web/src/lib/api.test.ts`:
- Around line 120-126: Update the test for listItemsForProduct to use a product
slug containing reserved/unsafe characters (e.g., spaces or symbols) so we
verify URL encoding; call api.listItemsForProduct with that raw slug, then
assert that the captured calledUrl (from mockFetch.mock.calls[0][0]) contains
the encoded form (use the expected encodeURIComponent result string) and still
assert result.ok, ensuring the test will catch regressions in the URL
construction inside listItemsForProduct.
In `@apps/web/src/views/ItemDetail.tsx`:
- Around line 58-59: The backLink is built from the decoded slug (const backLink
= slug ? `/products/${slug}` : '/products') which can produce invalid URLs for
slugs with reserved characters; update the ItemDetail component to encode the
slug when constructing backLink (use encodeURIComponent(slug)) so the generated
path is safe, and ensure the same encoded-backLink is used for the back
navigation points where backLink is referenced.
🪄 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: c1ff32f5-68df-45a7-baff-a15d2a74af0b
📒 Files selected for processing (6)
apps/web/src/App.test.tsxapps/web/src/App.tsxapps/web/src/components/ProductTabs.tsxapps/web/src/lib/api.test.tsapps/web/src/views/ItemDetail.tsxapps/web/src/views/Kanban.tsx
1. ItemDetail.tsx: encode slug in backLink (missed in previous round —
inconsistent with App.tsx and Kanban.tsx encoding)
2. App.test.tsx: shallow-copy data in ok() helper so mutable references
don't leak between tests
3. api.test.ts: listItemsForProduct URL encoding test uses slug with
reserved chars ('helm/playground with space') to actually exercise
encodeURIComponent
Addresses CodeRabbit review comments on PR #13.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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):listProducts(),getProductBySlug(slug),listItemsForProduct(slug)targeting the new/api/productsendpoints. Legacy methods kept for backward compat.ProductTabs(src/components/ProductTabs.tsx): horizontal tabs fromGET /api/products, polls every 5s, marks active tab viauseParamsslug, links to/products/:slug.src/App.tsx):/products/:slug→ Kanban,/products/:slug/items/:externalId→ ItemDetail.ProductsRedirectresolves first product slug; shows empty state if registry is empty.LegacyItemRedirectmaps old/items/:idto new path for backward compat.useParams, callsgetProductBySlug+listItemsForProduct, rendersProductTabsabove board. Invalid slug → error state with back link.useMemomoved before early returns (Rules of Hooks fix).slug+externalIdfromuseParams, back link resolves to/products/:slug.Test plan
pnpm --filter @helm/web test— 32 tests passpnpm --filter @helm/web build— clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests