refactor: listing views#282
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
WalkthroughThis PR adds shared listing primitives, route-search-driven list state, and new detail/list routes for bridges and skills, then rewires loops, tasks, vault, and network surfaces onto the new row/page composition. It also updates generated route metadata, tests, and storybook coverage. ChangesListing UI System and Route Migration
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
React Doctor found no issues. 🎉
|
![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
🧹 Nitpick comments (13)
web/src/lib/listing-search.ts (1)
1-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a dedicated unit test for these shared parsers.
This module is the single source of truth for URL search parsing across bridges/loops/skills routes (per the file's own comment), but no test file is provided in this cohort. Direct unit tests for
normalizeListingSearchValueandparseListingViewedge cases (empty string, whitespace, array values, invalidview) would be cheap and guard a shared contract used by multiple routes.As per coding guidelines, "Write unit tests with high code coverage for critical paths."
🤖 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 `@web/src/lib/listing-search.ts` around lines 1 - 22, Add a dedicated unit test module for the shared parsers in listing-search.ts to cover the contract used by bridges/loops/skills. Focus tests on normalizeListingSearchValue and parseListingView, including empty string, whitespace-only input, non-string/array values, and invalid view values returning undefined. Keep the tests targeted to these exported helpers so the single-source-of-truth behavior stays protected against route drift.Source: Coding guidelines
web/src/components/design-system-showcase.tsx (1)
1517-1524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hardcoded array index
SECTIONS[9]for the section label.
SECTIONS[9]is a magic index that silently breaks if entries are reordered, inserted, or removed elsewhere in the file — the label would then point to the wrong section without any type error. Prefer looking up the entry by its stableid(e.g."listing-row").♻️ Suggested fix
- label={<SectionLink section={SECTIONS[9]}>ListingRow</SectionLink>} + label={ + <SectionLink section={SECTIONS.find(s => s.id === "listing-row")!}> + ListingRow + </SectionLink> + }🤖 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 `@web/src/components/design-system-showcase.tsx` around lines 1517 - 1524, The ListingRowSection label currently uses a hardcoded SECTIONS[9] lookup, which is brittle and can drift if the array order changes; update ListingRowSection to resolve the section by its stable id instead of a numeric index. Use the existing SectionLink and SECTIONS data in design-system-showcase.tsx to find the entry whose id matches "listing-row", then pass that entry to SectionLink so the label remains correct even if SECTIONS is reordered.web/src/systems/loops/lib/__tests__/loop-catalog.test.ts (1)
1-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new
loopStatusesandhasActiveLoopFiltershelpers.This file tests
matchesLoopFilter/groupLoopCatalogthoroughly for the newstatusdimension, but the two other new exports fromloop-catalog.ts—loopStatuses(drives filter-bar options) andhasActiveLoopFilters(drives the empty-state "Clear filters" UX) — have no direct test here.As per path instructions,
web/**/*.{test,spec}.{js,ts,jsx,tsx}should "Write unit tests with high code coverage for critical paths."✅ Suggested additional test cases
it("Should derive distinct last-run statuses, sorted", () => { expect(loopStatuses(loopCatalogFixtures)).toEqual(["running", "watching"]); }); it("Should report active filters from query text or any chip", () => { expect(hasActiveLoopFilters("", { kind: "all", category: null, status: null })).toBe(false); expect(hasActiveLoopFilters("x", { kind: "all", category: null, status: null })).toBe(true); expect(hasActiveLoopFilters("", { kind: "workspace", category: null, status: null })).toBe(true); });🤖 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 `@web/src/systems/loops/lib/__tests__/loop-catalog.test.ts` around lines 1 - 121, The loop-catalog test suite is missing coverage for the new `loopStatuses` and `hasActiveLoopFilters` helpers. Add focused tests in `loop-catalog.test.ts` that verify `loopStatuses(loopCatalogFixtures)` returns distinct, sorted last-run statuses, and that `hasActiveLoopFilters` returns true when either the query text is non-empty or any filter chip in the loop filter object is active, using the existing `loopCatalogFixtures` and `matchesLoopFilter`/`groupLoopCatalog` patterns as a guide.Source: Path instructions
web/src/systems/loops/components/__tests__/loop-catalog.test.tsx (1)
96-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a navigation-isolation test for the card view's Run button.
The row view has a dedicated test ("Should launch a run inline without navigating to the detail row"), but the cards-grid test only checks rendering, not that clicking
LoopRunButtoninsideLoopCatalogCardavoids triggering the card'sLinknavigation.🤖 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 `@web/src/systems/loops/components/__tests__/loop-catalog.test.tsx` around lines 96 - 101, Add a navigation-isolation test for the cards view’s Run button in the LoopCatalogCard/LoopRunButton path. The existing cards-grid test only verifies rendering, so extend the Harness-based test to click the Run action inside the card and assert onRun is called without triggering Link navigation or rendering the detail route. Use the existing loop-catalog-card-grid and loop-catalog-card-software-delivery selectors plus the LoopCatalogCard and LoopRunButton behavior to locate the right interaction.web/src/systems/loops/components/catalog/loop-catalog-card.tsx (1)
19-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
sourceLabellogic to avoid duplication.The
entry.source === "workspace" ? "Workspace" : "Read-only"ternary at line 21 is duplicated verbatim inloop-catalog-row.tsx(line 36). Both files already import shared helpers (loopCategory) fromlib/loop-catalog.ts; consider adding aloopSourceLabel(entry)helper there for both card and row to consume.♻️ Proposed helper extraction
+// web/src/systems/loops/lib/loop-catalog.ts +export function loopSourceLabel(entry: LoopCatalogEntry): string { + return entry.source === "workspace" ? "Workspace" : "Read-only"; +}- const sourceLabel = entry.source === "workspace" ? "Workspace" : "Read-only"; + const sourceLabel = loopSourceLabel(entry);🤖 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 `@web/src/systems/loops/components/catalog/loop-catalog-card.tsx` around lines 19 - 58, The `LoopCatalogCard` component is hardcoding the `entry.source === "workspace" ? "Workspace" : "Read-only"` label inline, duplicating the same logic used by the row view. Move this mapping into a shared helper in `lib/loop-catalog.ts` (similar to `loopCategory`) such as a source-label helper, then update `LoopCatalogCard` to consume it so both card and row share the same source label logic.web/src/hooks/routes/use-loops-catalog.ts (1)
56-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the search-type assertion in
web/src/hooks/routes/use-loops-catalog.ts
useNavigate({ from: "/loops" })should already carry the/loopssearch type fromvalidateLoopsSearch, socurrent as LoopsRouteSearch | undefinedcan be dropped and the updater can stay type-safe end to end.🤖 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 `@web/src/hooks/routes/use-loops-catalog.ts` around lines 56 - 61, In `useLoopsCatalog`, remove the unnecessary search-type assertion inside the `navigate` search updater and rely on the `/loops` search type inferred from `useNavigate({ from: "/loops" })` and `validateLoopsSearch`. Update the updater callback so it accepts the typed current search directly, keeping `LoopsRouteSearch` safety end to end without casting. Make the change in the `use-loops-catalog` hook where `navigate` is called.Source: Coding guidelines
web/src/systems/network/components/threads/threads-list.tsx (1)
112-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkeleton doesn't reserve space for the new leading icon.
The real row now renders a
ListingRow.Icon(MessagesSquare) viaListingRow.Link, butThreadsListSkeletononly renders three text-line placeholders with no icon/avatar block, which can cause a layout shift when the skeleton resolves into real content.As per coding guidelines, "For
web/andpackages/ui/UI work, verify changes withagh-ui-screenshotbefore completion and cite the capture; tests verify code, not pixels." Please confirm this skeleton-vs-row layout was verified visually.♻️ Proposed fix
<SkeletonRows count={5} data-testid="network-thread-list-skeleton" rowClassName="border-b border-line px-4 py-3" > + <Skeleton className="size-4 shrink-0 rounded-sm" /> <Skeleton className="h-3.5 w-2/3" /> <Skeleton className="h-3 w-full" /> <Skeleton className="h-3 w-3/4" /> </SkeletonRows>🤖 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 `@web/src/systems/network/components/threads/threads-list.tsx` around lines 112 - 124, The ThreadsListSkeleton in ThreadsList is missing a leading icon/avatar placeholder, so it no longer matches the real row layout rendered by ListingRow.Link and ListingRow.Icon. Update ThreadsListSkeleton to reserve space for the new MessagesSquare icon block and keep the text placeholders aligned with the final row structure. Also verify the UI visually with agh-ui-screenshot and cite the capture in your completion notes so the skeleton-to-row transition is confirmed without layout shift.Source: Coding guidelines
web/src/hooks/routes/use-bridge-detail-page.ts (1)
1-479: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd comments for the more intricate business rules.
The hook aggregates a lot of derived state (scope-based list filters, progressive
selectedBridge/detailLoadingfallback, secret-input-key prefixing) with no explanatory comments. As per path instructions forweb/**/*.{js,ts,jsx,tsx}: "Write comprehensive comments explaining complex logic and business rules."🤖 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 `@web/src/hooks/routes/use-bridge-detail-page.ts` around lines 1 - 479, Add explanatory comments in useBridgeDetailPage for the non-obvious business rules: how bridgeListFilters switches between global and workspace scope, why selectedBridge falls back from bridgeDetailQuery to listBridgeSummary and how that affects detailLoading, and how secretInputValues are namespaced via bridgeSecretDraftKey/selective prefix filtering for the active bridge. Keep the comments close to the relevant logic so future changes to useBridge, useBridges, and secret binding handling are easier to follow.Source: Path instructions
web/src/hooks/routes/use-bridges-page.ts (1)
84-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid filtering bridges twice.
visibleBridgesandfilteredBridgeCountare computed here, but the route still passespage.bridgesintoBridgeListPanel, which filters the list again with the same inputs. Either pass the prefiltered list/count through or drop these derived values from the hook.🤖 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 `@web/src/hooks/routes/use-bridges-page.ts` around lines 84 - 112, The bridges list is being filtered twice: use the precomputed values from useBridgesPage instead of recomputing them later in BridgeListPanel, or remove the derived values here if they are not needed. Update the route and BridgeListPanel wiring so visibleBridges and filteredBridgeCount are the single source of truth, and keep the totalBridgeCount/visible count calculations aligned with the same filter inputs in useBridgesPage.web/src/routes/_app/__tests__/-skills.test.tsx (1)
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the
hasChildMatch/<Outlet />branch.
useChildMatchesis mocked to always return[], so the new "render<Outlet />and hide tabs/actions when a child route matches" logic inskills.tsx(added in this PR) is never exercised here.As per coding guidelines,
web/**/*.{test,spec}.{js,ts,jsx,tsx}: "Write unit tests with high code coverage for critical paths."it("renders Outlet and hides tabs/actions when a child route is active", () => { vi.mocked(useChildMatches).mockReturnValueOnce([{} as never]); render(<SkillsPage />); expect(screen.getByTestId("skills-outlet")).toBeInTheDocument(); expect(screen.queryByTestId("skills-tabs")).not.toBeInTheDocument(); });Also applies to: 49-50
🤖 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 `@web/src/routes/_app/__tests__/-skills.test.tsx` at line 75, Add test coverage for the new `hasChildMatch` branch in `-skills.test.tsx` by varying the `useChildMatches` mock instead of always returning an empty array. Update the `SkillsPage`/`skills.tsx` tests to render a child-route case, assert that the `<Outlet />` is shown, and verify tabs/actions are hidden when `useChildMatches()` returns a match. Keep the existing no-child-route case too so both paths are covered.Source: Coding guidelines
web/src/hooks/routes/use-skills-page.ts (1)
33-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate search-value normalization logic.
This
normalizeSearchValuere-implements the same trim/blank-to-undefined logic asnormalizeListingSearchValuein@/lib/listing-search.ts(already used by this route'svalidateSkillsSearch), and is duplicated a third time inuse-skill-detail-page.ts. Three independent copies of the same normalization rule risk silently diverging.♻️ Proposed consolidation
-function normalizeSearchValue(value: string | null | undefined): string | undefined { - const trimmed = value?.trim(); - return trimmed ? trimmed : undefined; -} +import { normalizeListingSearchValue } from "`@/lib/listing-search`";Then replace call sites with
normalizeListingSearchValue(nextQuery).🤖 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 `@web/src/hooks/routes/use-skills-page.ts` around lines 33 - 36, The search-value trimming logic is duplicated in normalizeSearchValue and already exists as normalizeListingSearchValue in the shared listing-search helper used by validateSkillsSearch. Remove the local normalizeSearchValue in use-skills-page and update its call sites to use normalizeListingSearchValue(nextQuery) instead, and apply the same consolidation anywhere else this route or related hooks are re-implementing the same rule (including use-skill-detail-page).web/src/systems/skill/components/skill-list-panel.tsx (1)
65-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated source-derivation logic across rows/cards.
const source = skill.provenance?.precedence_tier ?? skill.source;is repeated identically inSkillListingRow(Line 66) andSkillCatalogCard(Line 116), and the same expression also appears inskill-detail-panel.tsx(Line 406). Consider extracting a single helper (e.g.deriveSkillDisplaySource(skill)) intoskill-formatters.tsto avoid drift if the derivation rule changes.♻️ Proposed helper extraction
// skill-formatters.ts +export function deriveSkillDisplaySource(skill: SkillPayload): string { + return skill.provenance?.precedence_tier ?? skill.source; +}// skill-list-panel.tsx - const source = skill.provenance?.precedence_tier ?? skill.source; + const source = deriveSkillDisplaySource(skill);🤖 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 `@web/src/systems/skill/components/skill-list-panel.tsx` around lines 65 - 116, Extract the repeated skill source derivation into a shared helper so the logic stays consistent across `SkillListingRow`, `SkillCatalogCard`, and the matching usage in `skill-detail-panel.tsx`. Move the `skill.provenance?.precedence_tier ?? skill.source` rule into a formatter/helper such as `deriveSkillDisplaySource(skill)` in `skill-formatters.ts`, then update each row/card component to call that helper instead of inlining the expression.web/src/systems/skill/lib/skill-list-filters.ts (1)
35-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo dedicated unit tests for these pure filter/parse helpers.
Sibling filter libs in this PR (e.g. loop-list-filters, vault-list-filters per the stack outline) ship dedicated
__tests__files, butskill-list-filters.ts's chip-building/parsing/filtering functions are only exercised indirectly throughskill-list-panel.test.tsx. Direct unit tests would better cover edge cases (invalid chip values, empty chips array, case-sensitivity ofasSkillSource).🤖 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 `@web/src/systems/skill/lib/skill-list-filters.ts` around lines 35 - 124, The pure helper functions in skill-list-filters.ts are missing direct unit coverage, so add a dedicated __tests__ file for buildSkillFilterFields, skillFiltersToChips, applySkillFilterChips, filterInstalledSkills, parseSkillSourceFilter, and parseSkillEnabledFilter. Cover the edge cases called out in the review: empty chips, invalid chip values, and the case-sensitive behavior of asSkillSource/asSkillEnabled, using the existing exported symbols to locate and exercise the logic directly.
🤖 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 `@web/src/hooks/routes/use-bridge-detail-page.ts`:
- Around line 146-152: Scope the page-level error in use-bridge-detail-page so
BridgeDetailPanel only receives bridgeDetailQuery.error and does not treat
bridgeRoutesQuery.error or bridgeSecretBindingsQuery.error as fatal. Update the
detailError logic to preserve a loaded bridge detail view even when route or
secret-binding queries fail, and handle those failures within the
route/secret-binding sections instead of merging them into the top-level error.
In `@web/src/routes/_app/skills`.$name.tsx:
- Around line 54-72: The `skills.$name.tsx` route is computing `isLoading` in a
way that can never be true once the early `page.isLoadingDetail &&
!page.selectedSkill` guard has passed, so `SkillDetailPanel` never sees the
background-refresh loading state. Update the `SkillDetailPanel` props to
preserve the real detail-loading signal for cases where `selectedSkill` already
exists, and use a separate condition from the initial full-page spinner logic so
the panel can still show loading during refetches.
In `@web/src/systems/loops/components/catalog/loop-run-button.tsx`:
- Around line 5-32: `LoopRunButton` is using a custom prop shape even though it
renders a single native button, so native attributes like disabled and aria-*
cannot be passed through. Update `LoopRunButtonProps` to extend the intrinsic
button props for the root element, keep the existing loopName/onRun/className
API, and forward the remaining props onto the `<button>` in `LoopRunButton`
while still merging `className` with the existing styles.
In `@web/src/systems/skill/components/__tests__/skill-list-panel.test.tsx`:
- Around line 63-71: The test in skill-list-panel.test.tsx claims to verify name
sorting, but it only checks presence of rows. Update the assertion in the
renderPanel-based test to verify the actual DOM order of the skill rows, using
the skill item test ids (for example via getAllByTestId and index/order checks),
so the behavior in filterInstalledSkills is truly covered.
In `@web/src/systems/tasks/components/__tests__/task-card.test.tsx`:
- Around line 1-21: The test file uses React.ReactNode in the
`@tanstack/react-router` mock without importing React, which will fail typecheck.
Update the task-card test setup by importing type ReactNode from react and use
that in the Link mock’s children cast instead of referencing React.ReactNode
directly.
In `@web/src/systems/tasks/components/__tests__/tasks-list-row.test.tsx`:
- Around line 1-21: The test file uses React.ReactNode in the mocked Link
component without importing React, so the type reference is undefined. Update
the mock in tasks-list-row.test.tsx by either adding a React type import and
casting children to ReactNode, or by switching to a React namespace import and
using that consistently inside the vi.mock("`@tanstack/react-router`") stub.
In `@web/src/systems/tasks/components/__tests__/tasks-list-surface.test.tsx`:
- Around line 6-23: The mock Link component in the tasks list surface test is
using React.ReactNode without importing React, so update the test to import the
React node type explicitly and use that type for the children cast. Adjust the
vi.mock("`@tanstack/react-router`") block in tasks-list-surface.test.tsx so the
Link signature still matches the mocked component while referencing the imported
ReactNode type instead of React.ReactNode.
In `@web/src/systems/tasks/components/tasks-list-sort.tsx`:
- Line 11: The `tasks-list-sort.tsx` component is importing `TaskListSortKey`
from the route-level `use-tasks-page` hook, which reverses the intended
dependency flow. Move `TaskListSortKey` into a lower-level shared location such
as `systems/tasks/types.ts` or `lib/`, update `tasks-list-sort.tsx` to import it
from there, and have `use-tasks-page.ts` consume that shared type instead. Keep
cross-system access going through the public `@/systems/<domain>` barrel only.
---
Nitpick comments:
In `@web/src/components/design-system-showcase.tsx`:
- Around line 1517-1524: The ListingRowSection label currently uses a hardcoded
SECTIONS[9] lookup, which is brittle and can drift if the array order changes;
update ListingRowSection to resolve the section by its stable id instead of a
numeric index. Use the existing SectionLink and SECTIONS data in
design-system-showcase.tsx to find the entry whose id matches "listing-row",
then pass that entry to SectionLink so the label remains correct even if
SECTIONS is reordered.
In `@web/src/hooks/routes/use-bridge-detail-page.ts`:
- Around line 1-479: Add explanatory comments in useBridgeDetailPage for the
non-obvious business rules: how bridgeListFilters switches between global and
workspace scope, why selectedBridge falls back from bridgeDetailQuery to
listBridgeSummary and how that affects detailLoading, and how secretInputValues
are namespaced via bridgeSecretDraftKey/selective prefix filtering for the
active bridge. Keep the comments close to the relevant logic so future changes
to useBridge, useBridges, and secret binding handling are easier to follow.
In `@web/src/hooks/routes/use-bridges-page.ts`:
- Around line 84-112: The bridges list is being filtered twice: use the
precomputed values from useBridgesPage instead of recomputing them later in
BridgeListPanel, or remove the derived values here if they are not needed.
Update the route and BridgeListPanel wiring so visibleBridges and
filteredBridgeCount are the single source of truth, and keep the
totalBridgeCount/visible count calculations aligned with the same filter inputs
in useBridgesPage.
In `@web/src/hooks/routes/use-loops-catalog.ts`:
- Around line 56-61: In `useLoopsCatalog`, remove the unnecessary search-type
assertion inside the `navigate` search updater and rely on the `/loops` search
type inferred from `useNavigate({ from: "/loops" })` and `validateLoopsSearch`.
Update the updater callback so it accepts the typed current search directly,
keeping `LoopsRouteSearch` safety end to end without casting. Make the change in
the `use-loops-catalog` hook where `navigate` is called.
In `@web/src/hooks/routes/use-skills-page.ts`:
- Around line 33-36: The search-value trimming logic is duplicated in
normalizeSearchValue and already exists as normalizeListingSearchValue in the
shared listing-search helper used by validateSkillsSearch. Remove the local
normalizeSearchValue in use-skills-page and update its call sites to use
normalizeListingSearchValue(nextQuery) instead, and apply the same consolidation
anywhere else this route or related hooks are re-implementing the same rule
(including use-skill-detail-page).
In `@web/src/lib/listing-search.ts`:
- Around line 1-22: Add a dedicated unit test module for the shared parsers in
listing-search.ts to cover the contract used by bridges/loops/skills. Focus
tests on normalizeListingSearchValue and parseListingView, including empty
string, whitespace-only input, non-string/array values, and invalid view values
returning undefined. Keep the tests targeted to these exported helpers so the
single-source-of-truth behavior stays protected against route drift.
In `@web/src/routes/_app/__tests__/-skills.test.tsx`:
- Line 75: Add test coverage for the new `hasChildMatch` branch in
`-skills.test.tsx` by varying the `useChildMatches` mock instead of always
returning an empty array. Update the `SkillsPage`/`skills.tsx` tests to render a
child-route case, assert that the `<Outlet />` is shown, and verify tabs/actions
are hidden when `useChildMatches()` returns a match. Keep the existing
no-child-route case too so both paths are covered.
In `@web/src/systems/loops/components/__tests__/loop-catalog.test.tsx`:
- Around line 96-101: Add a navigation-isolation test for the cards view’s Run
button in the LoopCatalogCard/LoopRunButton path. The existing cards-grid test
only verifies rendering, so extend the Harness-based test to click the Run
action inside the card and assert onRun is called without triggering Link
navigation or rendering the detail route. Use the existing
loop-catalog-card-grid and loop-catalog-card-software-delivery selectors plus
the LoopCatalogCard and LoopRunButton behavior to locate the right interaction.
In `@web/src/systems/loops/components/catalog/loop-catalog-card.tsx`:
- Around line 19-58: The `LoopCatalogCard` component is hardcoding the
`entry.source === "workspace" ? "Workspace" : "Read-only"` label inline,
duplicating the same logic used by the row view. Move this mapping into a shared
helper in `lib/loop-catalog.ts` (similar to `loopCategory`) such as a
source-label helper, then update `LoopCatalogCard` to consume it so both card
and row share the same source label logic.
In `@web/src/systems/loops/lib/__tests__/loop-catalog.test.ts`:
- Around line 1-121: The loop-catalog test suite is missing coverage for the new
`loopStatuses` and `hasActiveLoopFilters` helpers. Add focused tests in
`loop-catalog.test.ts` that verify `loopStatuses(loopCatalogFixtures)` returns
distinct, sorted last-run statuses, and that `hasActiveLoopFilters` returns true
when either the query text is non-empty or any filter chip in the loop filter
object is active, using the existing `loopCatalogFixtures` and
`matchesLoopFilter`/`groupLoopCatalog` patterns as a guide.
In `@web/src/systems/network/components/threads/threads-list.tsx`:
- Around line 112-124: The ThreadsListSkeleton in ThreadsList is missing a
leading icon/avatar placeholder, so it no longer matches the real row layout
rendered by ListingRow.Link and ListingRow.Icon. Update ThreadsListSkeleton to
reserve space for the new MessagesSquare icon block and keep the text
placeholders aligned with the final row structure. Also verify the UI visually
with agh-ui-screenshot and cite the capture in your completion notes so the
skeleton-to-row transition is confirmed without layout shift.
In `@web/src/systems/skill/components/skill-list-panel.tsx`:
- Around line 65-116: Extract the repeated skill source derivation into a shared
helper so the logic stays consistent across `SkillListingRow`,
`SkillCatalogCard`, and the matching usage in `skill-detail-panel.tsx`. Move the
`skill.provenance?.precedence_tier ?? skill.source` rule into a formatter/helper
such as `deriveSkillDisplaySource(skill)` in `skill-formatters.ts`, then update
each row/card component to call that helper instead of inlining the expression.
In `@web/src/systems/skill/lib/skill-list-filters.ts`:
- Around line 35-124: The pure helper functions in skill-list-filters.ts are
missing direct unit coverage, so add a dedicated __tests__ file for
buildSkillFilterFields, skillFiltersToChips, applySkillFilterChips,
filterInstalledSkills, parseSkillSourceFilter, and parseSkillEnabledFilter.
Cover the edge cases called out in the review: empty chips, invalid chip values,
and the case-sensitive behavior of asSkillSource/asSkillEnabled, using the
existing exported symbols to locate and exercise the logic directly.
🪄 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: CHILL
Plan: Pro
Run ID: a1f93e95-d04e-4c1f-9a42-8efd9b21499b
⛔ Files ignored due to path filters (3)
.claude/prompts/listing-row-promotion.txtis excluded by!.claude/**docs/qa/state.csvis excluded by!**/*.csvpackages/ui/README.mdis excluded by!**/*.md
📒 Files selected for processing (98)
packages/ui/src/components/custom/__tests__/listing-row.test.tsxpackages/ui/src/components/custom/__tests__/listing-toolbar.test.tsxpackages/ui/src/components/custom/__tests__/review-row.test.tsxpackages/ui/src/components/custom/index.tspackages/ui/src/components/custom/listing-page.tsxpackages/ui/src/components/custom/listing-row.tsxpackages/ui/src/components/custom/listing-toolbar.tsxpackages/ui/src/components/custom/review-row.tsxpackages/ui/src/components/custom/stories/listing-page.stories.tsxpackages/ui/src/components/custom/stories/listing-row.stories.tsxpackages/ui/src/components/custom/stories/listing-toolbar.stories.tsxpackages/ui/src/components/custom/stories/review-row.stories.tsxpackages/ui/src/index.tsweb/src/components/design-system-showcase.tsxweb/src/hooks/routes/use-bridge-detail-page.tsweb/src/hooks/routes/use-bridges-page.tsweb/src/hooks/routes/use-loops-catalog.tsweb/src/hooks/routes/use-skill-detail-page.tsweb/src/hooks/routes/use-skills-page.tsweb/src/lib/listing-search.tsweb/src/routeTree.gen.tsweb/src/routes/_app/-tasks-route.tsxweb/src/routes/_app/__tests__/-bridges.$id.test.tsxweb/src/routes/_app/__tests__/-bridges.test.tsxweb/src/routes/_app/__tests__/-loops.test.tsxweb/src/routes/_app/__tests__/-skills.test.tsxweb/src/routes/_app/__tests__/-tasks.router.integration.test.tsxweb/src/routes/_app/__tests__/-vault.test.tsxweb/src/routes/_app/bridges.$id.tsxweb/src/routes/_app/bridges.tsxweb/src/routes/_app/loop-runs.tsxweb/src/routes/_app/loops.tsxweb/src/routes/_app/skills.$name.tsxweb/src/routes/_app/skills.tsxweb/src/routes/_app/vault.tsxweb/src/storybook/route-story-registry.tsweb/src/systems/bridges/components/__tests__/bridge-list-panel.test.tsxweb/src/systems/bridges/components/bridge-detail-panel.tsxweb/src/systems/bridges/components/bridge-list-filters.tsxweb/src/systems/bridges/components/bridge-list-panel.tsxweb/src/systems/bridges/components/stories/bridge-list-panel.stories.tsxweb/src/systems/bridges/index.tsweb/src/systems/bridges/lib/bridge-list-filters.tsweb/src/systems/bridges/routes/bridges.stories.tsxweb/src/systems/loops/components/__tests__/loop-catalog.test.tsxweb/src/systems/loops/components/catalog/loop-catalog-card.tsxweb/src/systems/loops/components/catalog/loop-catalog-filters.tsxweb/src/systems/loops/components/catalog/loop-catalog-row.tsxweb/src/systems/loops/components/catalog/loop-catalog.tsxweb/src/systems/loops/components/catalog/loop-run-button.tsxweb/src/systems/loops/components/stories/loop-catalog.stories.tsxweb/src/systems/loops/components/stories/loop-status-pill.stories.tsxweb/src/systems/loops/index.tsweb/src/systems/loops/lib/__tests__/loop-catalog.test.tsweb/src/systems/loops/lib/__tests__/loop-list-filters.test.tsweb/src/systems/loops/lib/loop-catalog.tsweb/src/systems/loops/lib/loop-list-filters.tsweb/src/systems/network/components/directs/__tests__/directs-list.test.tsxweb/src/systems/network/components/directs/directs-list.tsxweb/src/systems/network/components/threads/__tests__/threads-list.test.tsxweb/src/systems/network/components/threads/threads-list.tsxweb/src/systems/skill/components/__tests__/marketplace-view.test.tsxweb/src/systems/skill/components/__tests__/skill-list-panel.test.tsxweb/src/systems/skill/components/marketplace-view.tsxweb/src/systems/skill/components/skill-detail-panel.tsxweb/src/systems/skill/components/skill-list-filters.tsxweb/src/systems/skill/components/skill-list-panel.tsxweb/src/systems/skill/components/stories/marketplace-view.stories.tsxweb/src/systems/skill/components/stories/skill-detail-panel.stories.tsxweb/src/systems/skill/components/stories/skill-list-panel.stories.tsxweb/src/systems/skill/index.tsweb/src/systems/skill/lib/skill-list-filters.tsweb/src/systems/skill/routes/skills.stories.tsxweb/src/systems/tasks/components/__tests__/task-card.test.tsxweb/src/systems/tasks/components/__tests__/tasks-list-row.test.tsxweb/src/systems/tasks/components/__tests__/tasks-list-surface.test.tsxweb/src/systems/tasks/components/stories/task-overview-components.stories.tsxweb/src/systems/tasks/components/stories/tasks-list-row.stories.tsxweb/src/systems/tasks/components/stories/tasks-list-surface.stories.tsxweb/src/systems/tasks/components/task-card.tsxweb/src/systems/tasks/components/task-group.tsxweb/src/systems/tasks/components/tasks-inbox-view.tsxweb/src/systems/tasks/components/tasks-list-filters.tsxweb/src/systems/tasks/components/tasks-list-page-head.tsxweb/src/systems/tasks/components/tasks-list-row.tsxweb/src/systems/tasks/components/tasks-list-sort.tsxweb/src/systems/tasks/components/tasks-list-surface.tsxweb/src/systems/tasks/index.tsweb/src/systems/vault/components/__tests__/vault-secrets-list.test.tsxweb/src/systems/vault/components/index.tsweb/src/systems/vault/components/stories/vault-secrets-list.stories.tsxweb/src/systems/vault/components/vault-list-filters.tsxweb/src/systems/vault/components/vault-secrets-list.tsxweb/src/systems/vault/components/vault-secrets-row.tsxweb/src/systems/vault/components/vault-secrets-table.tsxweb/src/systems/vault/index.tsweb/src/systems/vault/lib/__tests__/vault-list-filters.test.tsweb/src/systems/vault/lib/vault-list-filters.ts
💤 Files with no reviewable changes (7)
- packages/ui/src/components/custom/stories/review-row.stories.tsx
- packages/ui/src/components/custom/review-row.tsx
- web/src/systems/tasks/components/tasks-list-page-head.tsx
- packages/ui/src/components/custom/tests/review-row.test.tsx
- web/src/systems/vault/components/vault-secrets-table.tsx
- web/src/systems/tasks/components/stories/tasks-list-surface.stories.tsx
- web/src/routes/_app/-tasks-route.tsx
Round 001 remediation (16 valid, 4 invalid): - Scope bridge detail top-level error to bridgeDetailQuery.error so a transient route/secret-binding failure no longer blanks a loaded bridge. - Replace magic SECTIONS[n] indices in the design showcase with stable id-based lookups (sectionById). - Drop the redundant loops search-type cast; share normalizeListingSearchValue across the loops/skills routes. - Extract loopSourceLabel and deriveSkillDisplaySource to their lib formatters; relocate TaskListSortKey into systems/tasks to restore the adapters->lib->hooks->components flow. - Make LoopRunButton extend native <button> props; reserve the leading icon well in the threads-list skeleton. - Add coverage: listing-search + skill-list-filters unit suites, loopStatuses/hasActiveLoopFilters/loopSourceLabel, skills Outlet branch, card-view Run isolation, and skill-list sort order. Invalid (documented): the skills.$name isLoading suggestion regresses the panel's full-screen loading gate; the three "React.ReactNode needs a React import" claims are false (type-only UMD-global reference, typecheck clean). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/systems/loops/lib/loop-catalog.ts (1)
17-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the label from
loopKindinstead of re-checkingsource.
loopSourceLabelre-implements the samesource === "workspace"branch already captured byloopKindjust above. Deriving fromloopKind(entry)keeps the classification in one place.♻️ Proposed refactor
export function loopSourceLabel(entry: Pick<LoopCatalogEntry, "source">): string { - return entry.source === "workspace" ? "Workspace" : "Read-only"; + return loopKind(entry) === "workspace" ? "Workspace" : "Read-only"; }🤖 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 `@web/src/systems/loops/lib/loop-catalog.ts` around lines 17 - 21, `loopSourceLabel` is duplicating the workspace/read-only classification by checking `source` directly; update it to derive the label from `loopKind(entry)` instead. Use the existing `loopKind` helper in `loop-catalog.ts` so the editability source is classified in one place, and keep `loopSourceLabel` as a simple mapping from that result to the final label.
🤖 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.
Nitpick comments:
In `@web/src/systems/loops/lib/loop-catalog.ts`:
- Around line 17-21: `loopSourceLabel` is duplicating the workspace/read-only
classification by checking `source` directly; update it to derive the label from
`loopKind(entry)` instead. Use the existing `loopKind` helper in
`loop-catalog.ts` so the editability source is classified in one place, and keep
`loopSourceLabel` as a simple mapping from that result to the final label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac46408a-4767-487c-b81a-fb4a94c6b63c
📒 Files selected for processing (23)
web/src/components/design-system-showcase.tsxweb/src/hooks/routes/use-bridge-detail-page.tsweb/src/hooks/routes/use-loops-catalog.tsweb/src/hooks/routes/use-skills-page.tsweb/src/hooks/routes/use-tasks-page.tsweb/src/lib/__tests__/listing-search.test.tsweb/src/routes/_app/__tests__/-skills.test.tsxweb/src/systems/loops/components/__tests__/loop-catalog.test.tsxweb/src/systems/loops/components/catalog/loop-catalog-card.tsxweb/src/systems/loops/components/catalog/loop-catalog-row.tsxweb/src/systems/loops/components/catalog/loop-run-button.tsxweb/src/systems/loops/lib/__tests__/loop-catalog.test.tsweb/src/systems/loops/lib/loop-catalog.tsweb/src/systems/network/components/threads/threads-list.tsxweb/src/systems/skill/components/__tests__/skill-list-panel.test.tsxweb/src/systems/skill/components/skill-detail-panel.tsxweb/src/systems/skill/components/skill-list-panel.tsxweb/src/systems/skill/lib/__tests__/skill-list-filters.test.tsweb/src/systems/skill/lib/skill-formatters.tsweb/src/systems/tasks/components/tasks-list-sort.tsxweb/src/systems/tasks/components/tasks-list-surface.tsxweb/src/systems/tasks/index.tsweb/src/systems/tasks/types.ts
✅ Files skipped from review due to trivial changes (2)
- web/src/lib/tests/listing-search.test.ts
- web/src/systems/skill/lib/tests/skill-list-filters.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- web/src/systems/loops/components/catalog/loop-catalog-card.tsx
- web/src/systems/loops/components/catalog/loop-run-button.tsx
- web/src/systems/tasks/index.ts
- web/src/systems/tasks/components/tasks-list-sort.tsx
- web/src/systems/skill/components/skill-detail-panel.tsx
- web/src/systems/network/components/threads/threads-list.tsx
- web/src/systems/skill/components/tests/skill-list-panel.test.tsx
- web/src/systems/loops/components/tests/loop-catalog.test.tsx
- web/src/systems/loops/components/catalog/loop-catalog-row.tsx
- web/src/hooks/routes/use-bridge-detail-page.ts
- web/src/hooks/routes/use-skills-page.ts
- web/src/hooks/routes/use-loops-catalog.ts
- web/src/systems/tasks/components/tasks-list-surface.tsx
- web/src/systems/skill/components/skill-list-panel.tsx
- web/src/routes/_app/tests/-skills.test.tsx
This was referenced Jul 9, 2026
Merged
This was referenced Jul 12, 2026
This was referenced Jul 12, 2026
This was referenced Jul 20, 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 by CodeRabbit