feat: add local dashboard SPA with React + Vite#39
Merged
WellDunDun merged 36 commits intodevfrom Mar 14, 2026
Merged
Conversation
Introduces a minimal React SPA at apps/local-dashboard/ with two routes: overview (KPIs, skill health grid, evolution feed) and per-skill drilldown (pass rate, invocation breakdown, evaluation records). Consumes existing dashboard-server API endpoints with SSE live updates, explicit loading/ error/empty states, and design tokens matching the current dashboard. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new Local Dashboard React SPA (Vite + TypeScript + shadcn/ui) and server changes to serve SPA assets and new SQLite-backed v2 API endpoints (/api/v2/overview, /api/v2/skills/:name). Includes client types, data hooks with 15s polling, UI pages/components, materialized SQLite queries, and SPA build/config files. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant SPA as Local Dashboard SPA
participant Server as dashboard-server
participant SQLite as SQLite (materialized)
Browser->>SPA: GET / (index.html & assets)
SPA->>Server: GET /api/v2/overview
Server->>SQLite: Query materialized overview
SQLite-->>Server: Overview rows
Server-->>SPA: 200 JSON OverviewResponse
SPA->>Browser: render Overview
Note over SPA,Server: SPA refetches /api/v2/overview every 15s
SPA->>Server: GET /api/v2/skills/:name
Server->>SQLite: Read per-skill materialized data + evolution_audit
SQLite-->>Server: Skill report payload
Server-->>SPA: 200 JSON SkillReportResponse or 404
SPA->>Browser: render SkillReport (KPIs, EvidenceViewer, EvolutionTimeline)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/package.json`:
- Around line 6-9: Add a new npm script named "typecheck" under the existing
"scripts" object so CI can run TypeScript-only checks; specifically add
"typecheck": "tsc --noEmit" (or your project-specific tsc flags) alongside
"dev", "build", and "preview" in the scripts section to run the TypeScript
compiler without emitting files for CI validation.
In `@apps/local-dashboard/src/api.ts`:
- Around line 24-26: The empty catch block "catch { // ignore parse errors }"
silently swallows JSON/parse failures; change it to "catch (err)" and log the
error and the malformed payload (or at least a descriptive message) so you can
diagnose bad events during development—use console.warn or the module's logger
and gate the extra verbosity behind a development check (e.g., NODE_ENV !==
'production') so production noise is avoided.
- Around line 29-37: The onerror handler in createSSEConnection currently always
reconnects after 3s causing an unbounded retry loop; modify createSSEConnection
to track retries (e.g., retryCount) and implement an exponential backoff
(baseDelay * 2^retryCount) with a configurable maxRetries, only scheduling
setTimeout to reconnect if retryCount < maxRetries, and otherwise log a final
failure and avoid reassigning cleanupRef; reset retryCount to 0 on a successful
connection/open event so transient errors use backoff but recovered connections
restore normal behavior.
In `@apps/local-dashboard/src/hooks/useSkillReport.ts`:
- Around line 19-21: The load callback in useSkillReport returns early when
skillName is falsy but leaves state set to "loading", causing a stuck loader;
update the early-return path inside load (in the useSkillReport hook) to
explicitly setState to a non-loading state (e.g., "idle" or "empty") before
returning, so consumers won't remain stuck; ensure you reference the same
setState and skillName symbols and keep the rest of the useCallback logic
intact.
In `@apps/local-dashboard/src/pages/Overview.tsx`:
- Around line 34-40: The deriveStatus logic is duplicated between deriveStatus
in Overview.tsx (accepting a MonitoringSnapshot) and the similar implementation
in SkillReport.tsx; extract a single shared utility function (e.g.,
deriveStatus) in a common module (utility/health or similar), standardize its
signature to accept a consistent input shape (either the MonitoringSnapshot type
or a small DTO with skill_checks, regression_detected, pass_rate) and update
both Overview.tsx and SkillReport.tsx to import and call this shared function
instead of their local copies; ensure you export the type or adapter helper if
SkillReport.tsx uses different params so both callers convert to the unified
signature.
- Around line 67-70: The avgPassRate calculation currently averages all
snapshots including ungraded ones; update the logic where avgPassRate is
computed to first filter snapshots for graded entries (e.g., snapshots.filter(s
=> s.skill_checks >= 5)), then compute the mean of s.pass_rate over that
filtered array, and return null if the filtered list is empty; adjust the symbol
references in Overview.tsx (avgPassRate and snapshots) accordingly so only
graded snapshots contribute to the average.
In `@apps/local-dashboard/src/pages/SkillReport.tsx`:
- Around line 8-30: The three utility functions formatRate, timeAgo, and
deriveStatus are duplicated; extract them into a shared utils.ts module, export
them, and replace the local copies in SkillReport.tsx and Overview.tsx with
imports from that module; reconcile the deriveStatus signatures (ensure the
exported deriveStatus uses the correct parameter types—passRate: number, checks:
number, regression: boolean—and the SkillHealthStatus return type used by both
files) and update any call sites to match the unified signature and types.
- Around line 59-61: The current filter in skillEvolution uses a fragile
substring match on overview.evolution[].details with decodedName; change it to
either match a dedicated identifier field on the evolution entries (preferred:
e.g., evolution.skillId or evolution.name) if available, otherwise replace the
includes check with a case-insensitive word-boundary regex that safely escapes
decodedName before building the regex (e.g., new
RegExp(`\\b${escapeRegExp(decodedName)}\\b`, 'i')), and add a small escapeRegExp
helper to sanitize decodedName; update the filter in the skillEvolution
assignment to use that stricter match.
In `@apps/local-dashboard/src/styles.css`:
- Around line 136-139: The .btn currently defines a hover state but lacks
explicit keyboard focus styling; update the CSS to add clear focus styles (use
.btn:focus and preferably .btn:focus-visible) that mirror or complement the
hover state — e.g., set a visible outline or box-shadow and adjust
border-color/background to ensure high contrast and a persistent visible
indicator for keyboard users; ensure you remove any outline:none elsewhere that
would suppress focus and apply these rules to the .btn selector to improve
accessibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67fca465-dcee-4eb7-8da5-00e256d8a32c
📒 Files selected for processing (18)
apps/local-dashboard/HANDOFF.mdapps/local-dashboard/index.htmlapps/local-dashboard/package.jsonapps/local-dashboard/src/App.tsxapps/local-dashboard/src/api.tsapps/local-dashboard/src/components/KpiCard.tsxapps/local-dashboard/src/components/Layout.tsxapps/local-dashboard/src/components/LoadingState.tsxapps/local-dashboard/src/components/StatusPill.tsxapps/local-dashboard/src/hooks/useOverview.tsapps/local-dashboard/src/hooks/useSkillReport.tsapps/local-dashboard/src/main.tsxapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/pages/SkillReport.tsxapps/local-dashboard/src/styles.cssapps/local-dashboard/src/types.tsapps/local-dashboard/tsconfig.jsonapps/local-dashboard/vite.config.ts
apps/local-dashboard/src/api.ts
Outdated
Comment on lines
+29
to
+37
| source.onerror = () => { | ||
| source.close(); | ||
| // reconnect after 3s | ||
| setTimeout(() => { | ||
| const cleanup = createSSEConnection(onData); | ||
| // store cleanup for when outer cleanup is called | ||
| cleanupRef = cleanup; | ||
| }, 3000); | ||
| }; |
There was a problem hiding this comment.
Unbounded reconnection loop on persistent failures.
The onerror handler reconnects unconditionally after 3s. If the server is down or returns a permanent error (404, etc.), this will retry indefinitely without backoff, potentially flooding the console with errors and wasting resources.
Consider adding a max retry count or exponential backoff.
🛠️ Suggested improvement with retry limit
-export function createSSEConnection(onData: (payload: OverviewPayload) => void): () => void {
- const source = new EventSource(`${BASE}/api/events`);
+export function createSSEConnection(
+ onData: (payload: OverviewPayload) => void,
+ retries = 0
+): () => void {
+ const MAX_RETRIES = 5;
+ const source = new EventSource(`${BASE}/api/events`);
source.addEventListener("data", (e) => {
try {
const payload = JSON.parse(e.data) as OverviewPayload;
onData(payload);
} catch {
// ignore parse errors
}
});
source.onerror = () => {
source.close();
- // reconnect after 3s
- setTimeout(() => {
- const cleanup = createSSEConnection(onData);
- // store cleanup for when outer cleanup is called
- cleanupRef = cleanup;
- }, 3000);
+ if (retries < MAX_RETRIES) {
+ const delay = Math.min(3000 * 2 ** retries, 30000);
+ setTimeout(() => {
+ const cleanup = createSSEConnection(onData, retries + 1);
+ cleanupRef = cleanup;
+ }, delay);
+ }
};
let cleanupRef = () => source.close();
return () => cleanupRef();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/api.ts` around lines 29 - 37, The onerror handler in
createSSEConnection currently always reconnects after 3s causing an unbounded
retry loop; modify createSSEConnection to track retries (e.g., retryCount) and
implement an exponential backoff (baseDelay * 2^retryCount) with a configurable
maxRetries, only scheduling setTimeout to reconnect if retryCount < maxRetries,
and otherwise log a final failure and avoid reassigning cleanupRef; reset
retryCount to 0 on a successful connection/open event so transient errors use
backoff but recovered connections restore normal behavior.
Extract shared utils (deriveStatus, formatRate, timeAgo), add SSE exponential backoff with max retries, filter ungraded skills from avg pass rate, fix stuck loading state for undefined skillName, use word-boundary regex for evolution filtering, add focus-visible styles, add typecheck script, and add Vite env types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/api.ts`:
- Around line 34-46: The reconnect logic schedules a setTimeout in
source.onerror but the returned cleanup only closes the current EventSource
(cleanupRef) and doesn’t cancel that pending timer; modify createSSEConnection
to track the reconnect timer id (e.g., let reconnectTimer: number | null = null)
and when scheduling setTimeout assign it to reconnectTimer, update cleanupRef to
both clearTimeout(reconnectTimer) and close the source, and ensure any
subsequent scheduled reconnects check whether cleanup has been called (or set
reconnectTimer to null) so no new EventSource is created after unsubscribe;
update references in source.onerror, cleanupRef, and the return closure
accordingly.
In `@apps/local-dashboard/src/hooks/useSkillReport.ts`:
- Around line 19-49: The load() function lacks a stale-request guard so an
earlier async completion can overwrite state for a new skill; modify
useSkillReport to track and ignore stale loads (e.g., use a ref like
currentLoadId or an AbortController) by incrementing/setting the token before
calling fetchOverview()/fetchSkillEvaluations(skillName) in load(), and verify
the token/abort state before calling setData, setState, or setError (and/or
early-return if aborted) so only the most recent invocation of load() updates
state; update the useEffect/load dependencies accordingly to ensure the guard is
created and checked around Promise.all completion.
In `@apps/local-dashboard/src/pages/SkillReport.tsx`:
- Around line 13-15: Remove the redundant decode step: drop the decodedName
variable and stop calling decodeURIComponent; use the route param directly (the
name returned from useParams) when calling useSkillReport (replace
useSkillReport(decodedName) with useSkillReport(name) and remove the decodedName
constant). This ensures you rely on React Router's already-decoded params and
avoids double-decoding issues.
In `@apps/local-dashboard/src/styles.css`:
- Line 22: The CSS custom property --mono currently lists unquoted font family
names causing stylelint failures; update the declaration for --mono to wrap
SFMono-Regular, Consolas, and Menlo in quotes (e.g., "SFMono-Regular",
"Consolas", "Menlo") while keeping the rest of the stack (SF Mono, "Liberation
Mono", monospace) intact so the variable --mono uses properly quoted font names.
In `@apps/local-dashboard/src/utils.ts`:
- Around line 3-9: The function signature for deriveStatus(passRate: number,
checks: number, regression: boolean): SkillHealthStatus is not formatted with
the project's Biome style; run the Biome formatter or manually reformat the
deriveStatus declaration to the project's wrapped/multiline signature style
(preserving parameter names and return type SkillHealthStatus) so CI passes —
locate the deriveStatus function in utils.ts and apply Biome formatting (or run
`biome format` locally) to update the signature shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84aa6938-84d6-467f-9005-5c1e1c48680a
📒 Files selected for processing (8)
apps/local-dashboard/package.jsonapps/local-dashboard/src/api.tsapps/local-dashboard/src/hooks/useSkillReport.tsapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/pages/SkillReport.tsxapps/local-dashboard/src/styles.cssapps/local-dashboard/src/utils.tsapps/local-dashboard/src/vite-env.d.ts
Cancel pending SSE reconnect timers on cleanup, add stale-request guard to useSkillReport, remove redundant decodeURIComponent (React Router already decodes), quote font names in CSS for stylelint, and format deriveStatus signature for Biome. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrate SPA from old JSONL-reading /api/data endpoints to new SQLite-backed /api/v2/* endpoints. Add v2 server routes for overview and per-skill reports. Replace SSE with 15s polling. Rewrite types to match materialized query shapes from queries.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/HANDOFF.md`:
- Around line 9-11: Add a language identifier to the fenced code block in
HANDOFF.md that contains the pipeline diagram ("JSONL logs →
materializeIncremental() → SQLite → getOverviewPayload() /
getSkillReportPayload() → /api/v2/* → SPA") to satisfy markdownlint MD040; edit
the opening ``` fence and append a language token such as text or plaintext so
the block becomes ```text (or ```plaintext).
In `@apps/local-dashboard/src/hooks/useOverview.ts`:
- Around line 13-38: The polling can overlap because setInterval repeatedly
calls load() while previous fetchOverview() calls may still be in flight; update
useOverview to serialize polls by adding an in-flight guard and staleness check:
add a ref like requestIdRef (or inFlightRef + latestId) that increments before
calling fetchOverview() inside load(), store the id locally, and only apply
setData/setState/setError if the response id matches the latest id; also
set/clear an AbortController or mark inFlightRef to ignore completions on
unmount so late responses don't update state. Target symbols: load, pollTimer,
fetchOverview, POLL_INTERVAL_MS, setData, setState, setError.
In `@apps/local-dashboard/src/hooks/useSkillReport.ts`:
- Around line 27-30: The current empty-state check in useSkillReport (the if
that sets setState("not-found")) only inspects report.usage.total_checks and
report.evidence and thus treats audit-only skills with evolution or
pending_proposals as missing; update that condition to also consider
report.evolution and report.pending_proposals (e.g., require those
arrays/collections to be empty as well) before calling setState("not-found") so
skills with proposal history or evolution are not misclassified.
In `@apps/local-dashboard/src/pages/Overview.tsx`:
- Around line 57-58: The Sessions KPI is using the wrong field
(overview.counts.telemetry) — update the value prop for the KpiCard with label
"Sessions" to use overview.counts.sessions instead; locate the KpiCard
invocation in Overview.tsx (the line with KpiCard label="Sessions") and replace
the telemetry field reference with sessions so the card shows unique session
count.
In `@cli/selftune/dashboard-server.ts`:
- Around line 550-554: The current refreshV2Data calls
materializeIncremental(db) inline so any exception aborts /api/v2/*; wrap the
call in a try/catch inside refreshV2Data, log the error (e.g., logger.error or
processLogger.error) and do NOT update lastV2MaterializedAt on failure so the
last-good snapshot continues to be served; only set lastV2MaterializedAt =
Date.now() after materializeIncremental(db) completes successfully, and do not
rethrow the exception so reads fall back to the existing materialized DB.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3cfeb4d-0ad8-4982-8eab-593ddb853c1f
📒 Files selected for processing (10)
apps/local-dashboard/HANDOFF.mdapps/local-dashboard/src/api.tsapps/local-dashboard/src/hooks/useOverview.tsapps/local-dashboard/src/hooks/useSkillReport.tsapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/pages/SkillReport.tsxapps/local-dashboard/src/styles.cssapps/local-dashboard/src/types.tsapps/local-dashboard/src/utils.tscli/selftune/dashboard-server.ts
- Add language identifier to HANDOFF.md fenced code block (MD040) - Prevent overlapping polls in useOverview with in-flight guard and sequential setTimeout - Broaden empty-state check in useSkillReport to include evolution/proposals - Fix Sessions KPI to use counts.sessions instead of counts.telemetry - Wrap materializeIncremental in try/catch to preserve last good snapshot on failure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 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 the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/package.json`:
- Line 7: The "dev" script in package.json uses shell-specific backgrounding
(`&` and `kill $!`) which fails on Windows; replace it with a cross-platform
runner (e.g., add "concurrently" to devDependencies and update the "dev" script)
to run "bun run cli/selftune/index.ts dashboard --serve --port 7888" and "vite"
in parallel and ensure proper termination; update the package.json "dev" script
to invoke concurrently (or use npm-run-all) so both processes start across
platforms without relying on shell job control.
In `@apps/local-dashboard/src/components/Sidebar.tsx`:
- Line 23: The prop type for counts is too permissive; update the Sidebar
component's counts declaration (where counts: Record<string, number>) to a
stricter type like Partial<Record<SkillHealthStatus, number>> using the existing
SkillHealthStatus enum/type, and adjust any places using counts (e.g., rendering
or arithmetic in Sidebar) to handle possibly undefined values (coalesce to 0 or
guard) so the code compiles and remains type-safe.
In `@apps/local-dashboard/src/styles.css`:
- Line 286: The CSS uses color-mix(in srgb, var(--border) 50%, transparent)
which has limited browser support; add a widely-supported fallback and a
feature-detection override: set a fallback border-bottom using an rgba/hex
blended value (e.g., border-bottom: 1px solid rgba(...)) or a fallback custom
property, then wrap the existing color-mix rule in an `@supports`(...) check to
apply it only when supported; update the selector containing the current
border-bottom rule so the fallback is applied first and the color-mix version
overrides it inside the `@supports` block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 126b5186-7fd5-4ca4-8d05-5e35d8356f63
📒 Files selected for processing (12)
.gitignoreapps/local-dashboard/.gitignoreapps/local-dashboard/HANDOFF.mdapps/local-dashboard/package.jsonapps/local-dashboard/src/components/ActivityTimeline.tsxapps/local-dashboard/src/components/Layout.tsxapps/local-dashboard/src/components/Sidebar.tsxapps/local-dashboard/src/hooks/useOverview.tsapps/local-dashboard/src/hooks/useSkillReport.tsapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/styles.csscli/selftune/dashboard-server.ts
apps/local-dashboard/src/styles.css
Outdated
| display: flex; | ||
| gap: 0.5rem; | ||
| padding: 0.4rem 0; | ||
| border-bottom: 1px solid color-mix(in srgb, var(--border) 50%, transparent); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
color-mix() has limited browser support.
color-mix(in srgb, ...) requires Safari 16.2+, Chrome 111+, Firefox 113+. For a local dev dashboard this is likely acceptable, but worth noting if broader browser support is needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/styles.css` at line 286, The CSS uses color-mix(in
srgb, var(--border) 50%, transparent) which has limited browser support; add a
widely-supported fallback and a feature-detection override: set a fallback
border-bottom using an rgba/hex blended value (e.g., border-bottom: 1px solid
rgba(...)) or a fallback custom property, then wrap the existing color-mix rule
in an `@supports`(...) check to apply it only when supported; update the selector
containing the current border-bottom rule so the fallback is applied first and
the color-mix version overrides it inside the `@supports` block.
…r types, CSS compat - Use concurrently for cross-platform dev script instead of shell backgrounding - Tighten Sidebar counts prop to Partial<Record<SkillHealthStatus, number>> - Replace color-mix() with rgba fallback for broader browser support Co-Authored-By: Claude Opus 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 the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/components/Sidebar.tsx`:
- Around line 4-10: STATUS_OPTIONS currently omits the "UNKNOWN" member of
SkillHealthStatus, so add an option { label: "Unknown", value: "UNKNOWN" } to
the STATUS_OPTIONS array (where STATUS_OPTIONS is defined) so the filter and
counts include UNKNOWN statuses; if omission was intentional instead add a clear
code comment in the STATUS_OPTIONS block explaining why "UNKNOWN" is excluded
and reference StatusPill.tsx to confirm styling/label consistency.
In `@apps/local-dashboard/src/styles.css`:
- Around line 112-123: The CSS uses the hardcoded header height (56px) in
multiple places (.sidebar-left, .sidebar-right, .dashboard-layout) which couples
components to a magic number; define a single CSS custom property (e.g.,
--header-height) at a global scope (root or .dashboard-layout container) and
replace occurrences of 56px and calc(100vh - 56px) in .sidebar-left,
.sidebar-right and .dashboard-layout to reference that variable (e.g., top:
var(--header-height) and height: calc(100vh - var(--header-height))) so changing
the header height only requires updating the variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cb6b9ae-a110-4032-a1af-2a5e0ce5f9d3
📒 Files selected for processing (3)
apps/local-dashboard/package.jsonapps/local-dashboard/src/components/Sidebar.tsxapps/local-dashboard/src/styles.css
- Add UNKNOWN to STATUS_OPTIONS so all SkillHealthStatus values are filterable - Extract hardcoded 56px header height to --header-h CSS variable Co-Authored-By: Claude Opus 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 the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/components/Sidebar.tsx`:
- Around line 26-37: The collapsed state in Sidebar.tsx must be lifted to the
page layout so grid sizing and mobile rules use the same source of truth: move
useState(false) out of Sidebar into the parent layout component (the component
that renders the grid and main pane), replace Sidebar's internal state with
props like collapsed and onToggle (or setCollapsed) and have the layout pass
those props into Sidebar; then update the layout to apply a class or
data-collapsed attribute based on collapsed and use that to change the grid
column width (instead of relying solely on .sidebar-left class) and to restore
the mobile toggle visibility when collapsed (adjust the mobile CSS rules to show
.sidebar-toggle when data-collapsed is true). Ensure function/prop names
referenced are Sidebar, collapsed, setCollapsed/onToggle and the parent layout
component that controls the page grid.
In `@apps/local-dashboard/src/styles.css`:
- Around line 217-220: Add a new CSS rule for the UNKNOWN filter so the selected
pill uses a status-specific appearance: create a
`.filter-pill.active.filter-unknown` rule (matching the `filter-unknown` value
emitted from Sidebar.tsx) and set its background, color, and border-color to the
appropriate variables (e.g., the neutral/unknown token colors used elsewhere
like --slate-bg / --text-muted / --text-soft or the project's chosen unknown
variables) so it no longer falls back to the generic blue active styling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 274ecadc-1143-4b8a-a7ea-c0f39824bd45
📒 Files selected for processing (3)
apps/local-dashboard/src/components/Sidebar.tsxapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/styles.css
Comment on lines
+26
to
+37
| const [collapsed, setCollapsed] = useState(false); | ||
|
|
||
| return ( | ||
| <aside className={`sidebar-left ${collapsed ? "collapsed" : ""}`}> | ||
| <button | ||
| type="button" | ||
| className="sidebar-toggle" | ||
| onClick={() => setCollapsed(!collapsed)} | ||
| aria-label={collapsed ? "Expand sidebar" : "Collapse sidebar"} | ||
| > | ||
| {collapsed ? "\u203A" : "\u2039"} | ||
| </button> |
There was a problem hiding this comment.
Collapse state is not wired into the layout.
collapsed only changes the <aside> class, but the parent grid still reserves the full var(--sidebar-w) column in apps/local-dashboard/src/styles.css, so the main pane never expands. It also creates a mobile dead-end: if this is collapsed before crossing the 768px breakpoint, the mobile rules hide both .sidebar-left.collapsed and .sidebar-toggle, so the filters become unreachable until reload. Hoist this state to the page layout and drive both the grid columns and the mobile reset from the same source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/components/Sidebar.tsx` around lines 26 - 37, The
collapsed state in Sidebar.tsx must be lifted to the page layout so grid sizing
and mobile rules use the same source of truth: move useState(false) out of
Sidebar into the parent layout component (the component that renders the grid
and main pane), replace Sidebar's internal state with props like collapsed and
onToggle (or setCollapsed) and have the layout pass those props into Sidebar;
then update the layout to apply a class or data-collapsed attribute based on
collapsed and use that to change the grid column width (instead of relying
solely on .sidebar-left class) and to restore the mobile toggle visibility when
collapsed (adjust the mobile CSS rules to show .sidebar-toggle when
data-collapsed is true). Ensure function/prop names referenced are Sidebar,
collapsed, setCollapsed/onToggle and the parent layout component that controls
the page grid.
- Lift collapsed state from Sidebar to Overview so grid columns resize properly - Add .sidebar-collapsed grid rules at all breakpoints - Fix mobile: collapsed sidebar no longer creates dead-end (shows inline) - Add .filter-pill.active.filter-unknown CSS rule for UNKNOWN status Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/local-dashboard/src/pages/Overview.tsx (1)
34-40:⚠️ Potential issue | 🟠 MajorReset the collapsed state when entering mobile layout.
Line 38 keeps
sidebarCollapsedacross breakpoints. Combined withapps/local-dashboard/src/components/Sidebar.tsxLines 40-76 andapps/local-dashboard/src/styles.cssLines 770-791, a user can collapse the sidebar on desktop, resize below 768px, and end up with an empty sidebar plus no visible toggle to reopen it.📱 Minimal fix
-import { useMemo, useState } from "react"; +import { useEffect, useMemo, useState } from "react"; @@ export function Overview() { const { data, state, error, retry } = useOverview(); const [search, setSearch] = useState(""); const [statusFilter, setStatusFilter] = useState<SkillHealthStatus | "ALL">("ALL"); const [sidebarCollapsed, setSidebarCollapsed] = useState(false); + + useEffect(() => { + const mq = window.matchMedia("(max-width: 768px)"); + const expandOnMobile = () => { + if (mq.matches) setSidebarCollapsed(false); + }; + expandOnMobile(); + mq.addEventListener("change", expandOnMobile); + return () => mq.removeEventListener("change", expandOnMobile); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/local-dashboard/src/pages/Overview.tsx` around lines 34 - 40, The Overview component preserves sidebarCollapsed across breakpoints causing the sidebar to stay hidden on mobile; add an effect in Overview that watches viewport width (e.g., via window.matchMedia('(max-width: 767px)') or window resize) and calls setSidebarCollapsed(false) when entering the mobile breakpoint so the collapsed state is reset on small screens; ensure the effect runs on mount and cleans up its listener and only toggles when transitioning into mobile to avoid unnecessary state updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/components/Sidebar.tsx`:
- Around line 62-69: The filter buttons in the Sidebar component expose
selection only via CSS; add a semantic state for screen readers by adding
aria-pressed to the button element (set to statusFilter === opt.value) so each
pill announces its pressed state; update the button in Sidebar.tsx (the element
using key={opt.value}, onClick={() => onStatusFilterChange(opt.value)}, and
className with filter-pill) to include aria-pressed={statusFilter === opt.value}
so assistive tech can detect the active filter.
---
Duplicate comments:
In `@apps/local-dashboard/src/pages/Overview.tsx`:
- Around line 34-40: The Overview component preserves sidebarCollapsed across
breakpoints causing the sidebar to stay hidden on mobile; add an effect in
Overview that watches viewport width (e.g., via window.matchMedia('(max-width:
767px)') or window resize) and calls setSidebarCollapsed(false) when entering
the mobile breakpoint so the collapsed state is reset on small screens; ensure
the effect runs on mount and cleans up its listener and only toggles when
transitioning into mobile to avoid unnecessary state updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4b4cac9-0894-48b5-8dac-71a38c3f37fb
📒 Files selected for processing (3)
apps/local-dashboard/src/components/Sidebar.tsxapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/styles.css
Comment on lines
+62
to
+69
| <button | ||
| key={opt.value} | ||
| type="button" | ||
| className={`filter-pill ${statusFilter === opt.value ? "active" : ""} ${opt.value !== "ALL" ? `filter-${opt.value.toLowerCase()}` : ""}`} | ||
| onClick={() => onStatusFilterChange(opt.value)} | ||
| > | ||
| {opt.label} | ||
| {count !== undefined && <span className="filter-count">{count}</span>} |
There was a problem hiding this comment.
Expose the active filter state semantically.
At Lines 62-69, the selected pill is only indicated by CSS. Screen readers will announce all of these as identical buttons, so the current filter is not discoverable. Add aria-pressed to mirror statusFilter === opt.value.
♿ Minimal fix
return (
<button
key={opt.value}
type="button"
className={`filter-pill ${statusFilter === opt.value ? "active" : ""} ${opt.value !== "ALL" ? `filter-${opt.value.toLowerCase()}` : ""}`}
+ aria-pressed={statusFilter === opt.value}
onClick={() => onStatusFilterChange(opt.value)}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| key={opt.value} | |
| type="button" | |
| className={`filter-pill ${statusFilter === opt.value ? "active" : ""} ${opt.value !== "ALL" ? `filter-${opt.value.toLowerCase()}` : ""}`} | |
| onClick={() => onStatusFilterChange(opt.value)} | |
| > | |
| {opt.label} | |
| {count !== undefined && <span className="filter-count">{count}</span>} | |
| <button | |
| key={opt.value} | |
| type="button" | |
| className={`filter-pill ${statusFilter === opt.value ? "active" : ""} ${opt.value !== "ALL" ? `filter-${opt.value.toLowerCase()}` : ""}`} | |
| aria-pressed={statusFilter === opt.value} | |
| onClick={() => onStatusFilterChange(opt.value)} | |
| > | |
| {opt.label} | |
| {count !== undefined && <span className="filter-count">{count}</span>} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/components/Sidebar.tsx` around lines 62 - 69, The
filter buttons in the Sidebar component expose selection only via CSS; add a
semantic state for screen readers by adding aria-pressed to the button element
(set to statusFilter === opt.value) so each pill announces its pressed state;
update the button in Sidebar.tsx (the element using key={opt.value}, onClick={()
=> onStatusFilterChange(opt.value)}, and className with filter-pill) to include
aria-pressed={statusFilter === opt.value} so assistive tech can detect the
active filter.
- Dashboard server now serves built SPA from apps/local-dashboard/dist/ at / - Legacy dashboard moved to /legacy/ route - SPA fallback for client-side routes (e.g. /skills/:name) - Static asset serving with content-hashed caching for /assets/* - Path traversal protection on static file serving - Add build:dashboard script to root package.json - Include apps/local-dashboard/dist/ in published files - Falls back to legacy dashboard if SPA build not found Co-Authored-By: Claude Opus 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 the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 932-987: The GET /api/v2/skills/:name handler should return 404
when the requested skill doesn't exist: after calling refreshV2Data() and
getSkillReportPayload(db, skillName), detect the "not found" signal that
getSkillReportPayload uses (e.g., null/undefined or a sentinel field) and if
missing return Response.json({ error: "Skill not found" }, { status: 404,
headers: corsHeaders() }); otherwise continue to load evolution and pending
proposals as currently implemented; update the handler around
getSkillReportPayload to perform this presence check before querying
evolution_audit and building the final response.
- Around line 670-685: The current path traversal check using
filePath.startsWith(spaDir) is unsafe and can be bypassed; update the check in
the SPA static asset handler to ensure resolved filePath is truly inside spaDir
by computing the relative path (using path.relative) between spaDir and filePath
and verifying it does not start with ".." and is not absolute, then proceed with
existsSync(filePath) and statSync(filePath). Replace the startsWith(spaDir)
condition in the block that resolves filePath (the variables and functions
involved: spaDir, filePath, resolve, path.relative, existsSync, statSync) so
only files under spaDir are served.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dfa5fd43-6efe-48f1-ac8e-c1e9f4391496
📒 Files selected for processing (3)
apps/local-dashboard/HANDOFF.mdcli/selftune/dashboard-server.tspackage.json
Migrate dashboard to shadcn theme system with proper light/dark support. Dark mode uses selftune site colors (navy/cream/copper), light mode uses standard shadcn defaults. Add ThemeProvider with localStorage persistence, sun/moon toggle in site header, and SVG logo with currentColor for both themes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use path.relative() + isAbsolute() instead of startsWith() for the SPA static asset path check to prevent directory traversal bypass. Return 404 from /api/v2/skills/:name when the skill has no usage data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/App.tsx`:
- Around line 24-27: DashboardShell and Overview both call useOverview causing
duplicate polling; lift the single source of truth by calling useOverview only
in DashboardShell and pass its result down as props (e.g., overviewData) to
Overview and any sidebar consumers instead of letting Overview call useOverview
itself; update Overview to accept the overview data/handlers (rename prop to
overviewData or similar) and remove its internal useOverview call so only
DashboardShell controls polling and state.
In `@apps/local-dashboard/src/components/ActivityTimeline.tsx`:
- Line 9: Remove the unused import of Separator from the ActivityTimeline
component: locate the import statement "import { Separator } from
'@/components/ui/separator'" in ActivityTimeline.tsx and delete it (or, if you
plan to use a separator, add its usage where appropriate). Ensure no other
references to Separator exist in the ActivityTimeline component or its JSX
before committing.
In `@apps/local-dashboard/src/components/skill-health-grid.tsx`:
- Around line 488-497: handleDragEnd currently closes over dataIds causing
stale-index lookups inside the setData updater; fix by capturing active.id and
over.id into local constants, then call setData(prev => { const oldIndex =
prev.findIndex(item => item.id === activeId); const newIndex =
prev.findIndex(item => item.id === overId); if (oldIndex === -1 || newIndex ===
-1) return prev; return arrayMove(prev, oldIndex, newIndex); }) so indices are
derived from the fresh prev state (referencing handleDragEnd, dataIds, setData,
arrayMove, active.id/over.id).
In `@apps/local-dashboard/src/components/theme-provider.tsx`:
- Around line 10-13: Change the context to allow undefined by initializing
ThemeProviderContext with createContext<ThemeProviderState |
undefined>(undefined) so useContext(ThemeProviderContext) can be correctly
guarded, and update the logic that reads from localStorage (the code that casts
to Theme) to validate the retrieved string against the allowed Theme union
(e.g., "light","dark","system") before using or calling setTheme/applying
classes; if the stored value is invalid, fall back to a safe default ("system")
or ignore it. Ensure checks reference ThemeProviderContext, ThemeProviderState,
useContext, and the function that reads localStorage so the guard at the
consumer (the undefined check) becomes reachable and invalid persisted values
never get applied to the DOM.
In `@apps/local-dashboard/src/components/theme-toggle.tsx`:
- Line 13: The onClick currently toggles only between "dark" and "light" losing
a "system" preference; update the handler (or add a helper like toggleTheme) to
detect when theme === "system" and either (a) resolve the current system
appearance via the existing appearance detection and toggle to the opposite
while allowing a subsequent click to return to "system", or (b) implement a
three-state cycle ["light","dark","system"] so users can return to the system
preference; update references to setTheme and theme in the ThemeToggle component
accordingly.
- Around line 14-16: The Button that wraps SunIcon and MoonIcon in
theme-toggle.tsx lacks relative positioning, so MoonIcon.absolute is not
overlaying correctly; update the Button's className (or the Button component's
base classes) to include "relative" so the absolute-positioned MoonIcon is
positioned against that Button, ensuring the SunIcon/MoonIcon overlay animations
behave as intended.
In `@apps/local-dashboard/src/components/ui/chart.tsx`:
- Around line 235-239: The tooltip rendering currently uses a falsy check
"item.value &&" which suppresses valid zero values; update the conditional that
guards rendering of item.value (in the chart tooltip JSX) to explicitly check
for null/undefined — e.g., use "item.value !== undefined && item.value !== null"
or "item.value != null" — so that 0 is rendered correctly while still skipping
absent values.
In `@apps/local-dashboard/src/components/ui/dropdown-menu.tsx`:
- Line 91: The Tailwind class string in dropdown-menu.tsx contains an invalid
selector `data-[variant=destructive]:*:[svg]:text-destructive`; locate the class
definition inside the DropdownMenu component (the long class string around
"group/dropdown-menu-item...") and remove or replace the malformed
segment—either delete `data-[variant=destructive]:*:[svg]:text-destructive`
because `[&_svg]` already covers svg styling, or replace it with a valid
selector such as `data-[variant=destructive]:[&_svg]:text-destructive` (or
`data-[variant=destructive]:*:data-[slot=svg]:text-destructive`) to achieve the
intended destructive svg color.
In `@apps/local-dashboard/src/components/ui/select.tsx`:
- Around line 163-165: The JSX for the scroll arrow icons is inconsistently
formatted: close the self-closing tags for ChevronUpIcon and ChevronDownIcon
inline (remove the extra blank line) so they match other icon usages in this
file; update the JSX inside SelectPrimitive.ScrollUpArrow (ChevronUpIcon) and
inside SelectScrollDownButton (ChevronDownIcon) to use a single-line
self-closing element (e.g., <ChevronUpIcon /> and <ChevronDownIcon />) to
maintain consistent formatting.
- Around line 126-132: The SelectPrimitive.ItemIndicator currently uses both the
render prop and children which conflicts with Base UI's API; update
SelectPrimitive.ItemIndicator to remove the render prop and pass the CheckIcon
as children (leave CheckIcon with its className), and if the span
wrapper/positioning is required move those styles to the parent
SelectPrimitive.Item or to a CSS rule rather than using render; target the
SelectPrimitive.ItemIndicator and CheckIcon in your change.
In `@apps/local-dashboard/src/components/ui/skeleton.tsx`:
- Around line 1-13: The file is using React.ComponentProps<"div"> in the
Skeleton component but React is not imported; add an import for React so the
type is in scope (e.g., import React from "react" or import type React from
"react") at the top of the file, keeping the existing cn import and leaving the
Skeleton function signature and export unchanged.
In `@apps/local-dashboard/src/components/ui/sonner.tsx`:
- Around line 1-10: The Toaster component is importing useTheme from
"next-themes" which can diverge from the app's dashboard theme state; change
Toaster to read theme from the local dashboard provider's hook (the exported
useTheme in the ThemeProvider file) instead of from "next-themes". Update the
import in the Toaster component to import useTheme from the local provider
module (the same module that exports ThemeProvider and its useTheme hook), and
keep the rest of Toaster (props handling and casting theme to
ToasterProps["theme"]) unchanged so toast styling follows the dashboard
provider's persisted theme.
In `@apps/local-dashboard/src/components/ui/toggle-group.tsx`:
- Around line 43-46: The Tailwind gap syntax is invalid: replace the class token
gap-[--spacing(var(--gap))] in the ToggleGroup className with gap-[var(--gap)]
and ensure the inline style sets the CSS variable with units (update the style
prop that sets "--gap" from a unitless spacing to a string with units, e.g.
compute `${spacing * 0.25}rem` or accept a precomputed value), so update both
the style assignment and the className string around cn(...) to use
gap-[var(--gap)] and a unit-bearing "--gap" value.
In `@apps/local-dashboard/src/hooks/use-mobile.ts`:
- Around line 6-18: The hook currently initializes isMobile as undefined causing
!!isMobile to be false until the effect runs; change the initialization to seed
from the current media query (use window.matchMedia(`(max-width:
${MOBILE_BREAKPOINT - 1}px)`).matches when window is defined) and in the effect
use the same mql.matches (or the event.matches) to update setIsMobile, keeping
the addEventListener/removeEventListener on mql and leaving the rest of
useEffect intact (reference: isMobile, setIsMobile, React.useEffect, mql,
MOBILE_BREAKPOINT).
In `@apps/local-dashboard/src/lib/utils.ts`:
- Around line 1-5: The helper cn has import/formatting issues causing Biome/CI
failures; run Biome (or your formatter) and apply its autofixes, ensure imports
are organized and spaced correctly (imports from "clsx" and "tailwind-merge"
grouped and sorted) and adjust the implementation to call clsx with a spread
(cn(...inputs: ClassValue[]) { return twMerge(clsx(...inputs)) }) so the inputs
are passed correctly; re-run Biome/CI to verify the file formats cleanly.
In `@apps/local-dashboard/src/pages/SkillReport.tsx`:
- Around line 139-142: passRateGood is using a hardcoded 0.7 threshold while
deriveStatus uses 0.8, causing inconsistent signals; update the code to compute
the "good" flag from the derived status (e.g., set passRateGood = status ===
'HEALTHY' or use a shared THRESHOLD constant) so the UI KPI and status badge
come from the same source; change any other occurrences in the block (lines
referencing passRateGood or direct 0.7 checks between 148-170) to the same
approach so all checks remain consistent with deriveStatus and STATUS_CONFIG.
In `@apps/local-dashboard/tsconfig.json`:
- Line 18: The tsconfig currently only includes "src", so vite.config.ts is
excluded from tsc checks; either add "vite.config.ts" (or "vite.config.*.ts") to
the "include" array in this tsconfig.json so tsc --noEmit typechecks the Vite
config, or create a separate tsconfig.node.json (with Node types) that includes
vite.config.ts and update the repository's "typecheck" npm script to run tsc
--noEmit against both tsconfig.json and tsconfig.node.json; reference the
"include" property, vite.config.ts, tsconfig.node.json, and the package.json
"typecheck" script when making the change.
In `@apps/local-dashboard/vite.config.ts`:
- Around line 26-30: The current manualChunks object uses the exact
"@base-ui/react" key so subpath imports (e.g., "@base-ui/react/input" used in
src/components/ui/input.tsx) won't be bundled into "vendor-ui"; change
manualChunks to the function form and detect module ids that
startWith("@base-ui/react") (or match a prefix regex) and return "vendor-ui" for
those ids (alongside returning the existing "vendor-react" and "vendor-table"
logic for their prefixes) so all Base UI subpath imports are grouped into the
vendor-ui chunk.
- Around line 8-10: The ESM config uses __dirname in resolve.alias
(path.resolve(__dirname, "./src")) which breaks under type: "module"; replace
the __dirname usage with the ESM fileURLToPath/URL pattern: import fileURLToPath
and use fileURLToPath(new URL(..., import.meta.url)) to derive the project
directory or directly resolve the "./src" URL, then pass that result into
path.resolve for the alias. Update the import statements accordingly so
vite.config.ts uses fileURLToPath and import.meta.url instead of __dirname.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 347eb2fc-6692-4894-93bf-34db53d814b3
⛔ Files ignored due to path filters (4)
apps/local-dashboard/package-lock.jsonis excluded by!**/package-lock.jsonapps/local-dashboard/public/favicon.pngis excluded by!**/*.pngapps/local-dashboard/public/logo.pngis excluded by!**/*.pngapps/local-dashboard/public/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (40)
apps/local-dashboard/components.jsonapps/local-dashboard/index.htmlapps/local-dashboard/package.jsonapps/local-dashboard/src/App.tsxapps/local-dashboard/src/components/ActivityTimeline.tsxapps/local-dashboard/src/components/app-sidebar.tsxapps/local-dashboard/src/components/section-cards.tsxapps/local-dashboard/src/components/site-header.tsxapps/local-dashboard/src/components/skill-health-grid.tsxapps/local-dashboard/src/components/theme-provider.tsxapps/local-dashboard/src/components/theme-toggle.tsxapps/local-dashboard/src/components/ui/avatar.tsxapps/local-dashboard/src/components/ui/badge.tsxapps/local-dashboard/src/components/ui/breadcrumb.tsxapps/local-dashboard/src/components/ui/button.tsxapps/local-dashboard/src/components/ui/card.tsxapps/local-dashboard/src/components/ui/chart.tsxapps/local-dashboard/src/components/ui/checkbox.tsxapps/local-dashboard/src/components/ui/drawer.tsxapps/local-dashboard/src/components/ui/dropdown-menu.tsxapps/local-dashboard/src/components/ui/input.tsxapps/local-dashboard/src/components/ui/label.tsxapps/local-dashboard/src/components/ui/select.tsxapps/local-dashboard/src/components/ui/separator.tsxapps/local-dashboard/src/components/ui/sheet.tsxapps/local-dashboard/src/components/ui/sidebar.tsxapps/local-dashboard/src/components/ui/skeleton.tsxapps/local-dashboard/src/components/ui/sonner.tsxapps/local-dashboard/src/components/ui/table.tsxapps/local-dashboard/src/components/ui/tabs.tsxapps/local-dashboard/src/components/ui/toggle-group.tsxapps/local-dashboard/src/components/ui/toggle.tsxapps/local-dashboard/src/components/ui/tooltip.tsxapps/local-dashboard/src/hooks/use-mobile.tsapps/local-dashboard/src/lib/utils.tsapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/pages/SkillReport.tsxapps/local-dashboard/src/styles.cssapps/local-dashboard/tsconfig.jsonapps/local-dashboard/vite.config.ts
apps/local-dashboard/src/App.tsx
Outdated
Comment on lines
+24
to
+27
| function DashboardShell() { | ||
| const [search, setSearch] = useState("") | ||
| const [statusFilter, setStatusFilter] = useState<SkillHealthStatus | "ALL">("ALL") | ||
| const { data } = useOverview() |
There was a problem hiding this comment.
Avoid double polling by sharing one useOverview source.
Line 27 starts one polling loop in DashboardShell, and Overview starts another loop internally (Line 83 render path). This duplicates API traffic and can desync sidebar data vs main content transiently.
Use a single useOverview instance (lift to shell or context) and pass the result to Overview and sidebar consumers.
Also applies to: 83-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/App.tsx` around lines 24 - 27, DashboardShell and
Overview both call useOverview causing duplicate polling; lift the single source
of truth by calling useOverview only in DashboardShell and pass its result down
as props (e.g., overviewData) to Overview and any sidebar consumers instead of
letting Overview call useOverview itself; update Overview to accept the overview
data/handlers (rename prop to overviewData or similar) and remove its internal
useOverview call so only DashboardShell controls polling and state.
| variant="ghost" | ||
| size="icon" | ||
| className="size-8" | ||
| onClick={() => setTheme(theme === "dark" ? "light" : "dark")} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider "system" theme handling.
The toggle only cycles between "dark" and "light". If the theme was "system", the first click resolves to the opposite of the current appearance, which loses the system preference. This may be intentional, but consider if users should be able to return to system preference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/components/theme-toggle.tsx` at line 13, The onClick
currently toggles only between "dark" and "light" losing a "system" preference;
update the handler (or add a helper like toggleTheme) to detect when theme ===
"system" and either (a) resolve the current system appearance via the existing
appearance detection and toggle to the opposite while allowing a subsequent
click to return to "system", or (b) implement a three-state cycle
["light","dark","system"] so users can return to the system preference; update
references to setTheme and theme in the ThemeToggle component accordingly.
Comment on lines
+1
to
+5
| import { clsx, type ClassValue } from "clsx" | ||
| import { twMerge } from "tailwind-merge" | ||
|
|
||
| export function cn(...inputs: ClassValue[]) { | ||
| return twMerge(clsx(inputs)) |
There was a problem hiding this comment.
Fix the Biome failure in this helper.
CI is already red on import organization/formatting for this file. Run Biome on it before merge.
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-3: Biome: The imports are not sorted. Safe fix available: organize imports.
[error] 1-7: Biome formatting check failed. Import order and minor formatting adjustments needed (e.g., semicolons, spacing).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/lib/utils.ts` around lines 1 - 5, The helper cn has
import/formatting issues causing Biome/CI failures; run Biome (or your
formatter) and apply its autofixes, ensure imports are organized and spaced
correctly (imports from "clsx" and "tailwind-merge" grouped and sorted) and
adjust the implementation to call clsx with a spread (cn(...inputs:
ClassValue[]) { return twMerge(clsx(...inputs)) }) so the inputs are passed
correctly; re-run Biome/CI to verify the file formats cleanly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/local-dashboard/src/hooks/use-mobile.ts (1)
6-18:⚠️ Potential issue | 🟡 MinorSeed
isMobilefrommatchMediaand usemql.matchesupdates.Line 6 + Line 18 force an initial
falseon mobile until the effect runs, causing a desktop-first render flip. Also, Line 11 and Line 14 should read from the query result (mql.matches/event.matches) instead ofwindow.innerWidth.Proposed fix
export function useIsMobile() { - const [isMobile, setIsMobile] = React.useState<boolean | undefined>(undefined); + const [isMobile, setIsMobile] = React.useState<boolean>(() => + typeof window !== "undefined" && + window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`).matches + ); React.useEffect(() => { const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`); - const onChange = () => { - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); + const onChange = (event: MediaQueryListEvent) => { + setIsMobile(event.matches); }; mql.addEventListener("change", onChange); - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); + setIsMobile(mql.matches); return () => mql.removeEventListener("change", onChange); }, []); - return !!isMobile; + return isMobile; }#!/bin/bash # Read-only verification: confirm no undefined-init / boolean coercion / innerWidth-based updates remain. rg -n -C2 'useState<.*undefined>|return !!isMobile|window\.innerWidth < MOBILE_BREAKPOINT|MediaQueryListEvent|mql\.matches' apps/local-dashboard/src/hooks/use-mobile.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/local-dashboard/src/hooks/use-mobile.ts` around lines 6 - 18, Initialize the isMobile state from matchMedia instead of undefined and stop reading window.innerWidth in the effect: set initial state using window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`).matches (guarded for SSR with typeof window !== 'undefined'), then in useEffect create const mql = window.matchMedia(...) and have onChange read event.matches (or mql.matches) to call setIsMobile; also return the boolean isMobile directly (no !! coercion) so the hook renders correct mobile state on first paint and updates from mql.matches/event.matches only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/local-dashboard/src/hooks/use-mobile.ts`:
- Around line 6-18: Initialize the isMobile state from matchMedia instead of
undefined and stop reading window.innerWidth in the effect: set initial state
using window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`).matches
(guarded for SSR with typeof window !== 'undefined'), then in useEffect create
const mql = window.matchMedia(...) and have onChange read event.matches (or
mql.matches) to call setIsMobile; also return the boolean isMobile directly (no
!! coercion) so the hook renders correct mobile state on first paint and updates
from mql.matches/event.matches only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: da2ddcbb-f7e9-4c1d-b83c-e28acf70cf61
📒 Files selected for processing (4)
apps/local-dashboard/src/hooks/use-mobile.tsapps/local-dashboard/src/lib/utils.tsapps/local-dashboard/vite.config.tscli/selftune/dashboard-server.ts
…harden theme/config - Lift useOverview to DashboardShell, pass as prop to Overview (no double polling) - Fix stale closure in drag handler by deriving indices from prev state - Validate localStorage theme values, use undefined context default - Add relative positioning to theme toggle button for MoonIcon overlay - Fix falsy check hiding zero values in chart tooltip - Fix invalid Tailwind selectors in dropdown-menu and toggle-group - Use ESM-safe fileURLToPath instead of __dirname in vite.config - Switch manualChunks to function form for Base UI subpath matching - Align pass-rate threshold with deriveStatus in SkillReport - Use local theme provider in sonner instead of next-themes - Add missing React import in skeleton, remove unused Separator import - Include vite.config.ts in tsconfig for typecheck coverage - Fix inconsistent JSX formatting in select scroll buttons Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (2)
apps/local-dashboard/src/components/ui/select.tsx (1)
126-132:⚠️ Potential issue | 🟠 Major
ItemIndicatorstill mixesrenderandchildren(API conflict).At Line 126-Line 132,
SelectPrimitive.ItemIndicatorpasses bothrenderandchildren. Keep a single pattern to avoid undefined/ignored rendering behavior.Suggested fix (use
renderonly)- <SelectPrimitive.ItemIndicator - render={ - <span className="pointer-events-none absolute right-2 flex size-4 items-center justify-center" /> - } - > - <CheckIcon className="pointer-events-none" /> - </SelectPrimitive.ItemIndicator> + <SelectPrimitive.ItemIndicator + render={ + <span className="pointer-events-none absolute right-2 flex size-4 items-center justify-center"> + <CheckIcon className="pointer-events-none" /> + </span> + } + />For `@base-ui/react/select` v1.3.0, does Select.ItemIndicator support using both `render` prop and JSX children at the same time, or must only one be used?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/local-dashboard/src/components/ui/select.tsx` around lines 126 - 132, SelectPrimitive.ItemIndicator currently supplies both the render prop and JSX children (CheckIcon), causing an API conflict; pick one pattern — recommended: use render only. Remove the JSX children (the CheckIcon) from inside SelectPrimitive.ItemIndicator and instead include that icon markup inside the existing render prop element passed to SelectPrimitive.ItemIndicator; alternatively, if you prefer the children API, delete the render prop entirely and keep the CheckIcon as the sole child of SelectPrimitive.ItemIndicator.apps/local-dashboard/src/components/theme-toggle.tsx (1)
13-13:⚠️ Potential issue | 🟡 MinorHandle
"system"explicitly in the toggle path.Line 13 collapses
"system"to"dark". On a dark OS, the first click becomes a no-op, and this control still has no way back to the system preference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/local-dashboard/src/components/theme-toggle.tsx` at line 13, The toggle currently treats "system" like "dark", causing a noop when the OS is dark and no way to return to system; update the onClick handler used with setTheme to explicitly handle theme === "system" by reading the OS preference via window.matchMedia('(prefers-color-scheme: dark)').matches and toggling to the opposite concrete theme, and implement a clear cycle that includes "system" (e.g., light -> dark -> system -> light) so users can return to the system preference; update the onClick logic in the component that calls setTheme (the theme-toggle.tsx handler referencing theme and setTheme) to compute the next state explicitly rather than collapsing "system" to "dark".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/App.tsx`:
- Around line 30-45: The mapping + sorting in skillNavItems duplicates
deriveSkillCards/deriveStatus logic; extract a shared helper (e.g.,
deriveSkillNavItems or reuse deriveSkillCards from Overview.tsx) that accepts
data.skills and returns the normalized items (name, status via deriveStatus,
passRate with null when total_checks===0, checks) and the same sort (passRate
null treated as 1, then checks desc); then replace the inline map/sort in
App.tsx (skillNavItems) and Overview.tsx to call that helper so both use the
single implementation.
In `@apps/local-dashboard/src/components/ActivityTimeline.tsx`:
- Line 67: The Tabs defaultValue currently chooses between "pending" and
"timeline" only; change the default logic in the Tabs component (defaultValue
prop) to pick the first non-empty dataset among pendingProposals, evolution, and
unmatchedQueries (use pendingProposals.length > 0 ? "pending" : evolution.length
> 0 ? "timeline" : unmatchedQueries.length > 0 ? "unmatched" : "timeline") so
the tab with data opens by default; update any labels/identifiers ("pending",
"timeline", "unmatched") used by the Tabs and TabPanels to match this selection.
In `@apps/local-dashboard/src/components/skill-health-grid.tsx`:
- Around line 697-699: The pager currently shows "Page 1 of 0" when there are no
rows; guard the pagination UI by checking the table's row count or page count
before rendering. In skill-health-grid.tsx, wrap the existing div that uses
table.getState().pagination.pageIndex and table.getPageCount() with a
conditional that only renders when table.getRowModel().rows.length > 0 (or
table.getPageCount() > 0), or alternatively clamp the displayed count with const
pageCount = Math.max(1, table.getPageCount()) and use
Math.min(currentPageIndex+1, pageCount) to avoid showing "of 0". Ensure you
reference table.getRowModel().rows.length, table.getPageCount(), and
table.getState().pagination.pageIndex when making the change.
- Around line 458-461: dataIds are being derived from the raw data array which
diverges from the visible order in table.getRowModel().rows (which includes
sorting/filtering/pagination), causing DnD mismatches and handleDragEnd calling
arrayMove with -1 indices; change the memo that builds dataIds to map over
table.getRowModel().rows (e.g., table.getRowModel().rows.map(r =>
r.original.name or r.id) so ids match the visible order), then in handleDragEnd
compute source and destination indices by looking up items in the same row model
(not raw data), guard against indexOf returning -1 (skip/return early if either
index is -1) before calling arrayMove to avoid corrupting the array.
In `@apps/local-dashboard/src/components/theme-provider.tsx`:
- Line 1: The code uses the React namespace type React.ReactNode but only
imports named hooks from "react", causing a TS error; fix by importing the type
ReactNode directly (e.g., add import type { ReactNode } from "react") and update
any type annotations that reference React.ReactNode to use ReactNode
instead—check the ThemeProvider component and any usages in this file
(createContext/useContext, ThemeProvider props) and replace React.ReactNode with
the imported ReactNode.
In `@apps/local-dashboard/src/components/ui/chart.tsx`:
- Line 189: The mapped Tooltip and Legend items use unstable keys
(key={item.dataKey} and key={item.value}) which can be undefined or duplicate;
update the mapping functions that render tooltip items and legend entries to
include an index-based fallback so keys are always stable and unique (e.g.,
combine the original property with the map index or a constant prefix when
item.dataKey or item.value is falsy). Locate the JSX that renders tooltip items
(where key={item.dataKey}) and the legend render map (where key={item.value})
and replace those keys with a deterministic composite keyed by the property plus
the loop index or a distinct prefix to guarantee uniqueness.
- Around line 80-98: The CSS attribute selector built in the style block uses
unquoted interpolation of id and can break with spaces/special chars; change the
selector to quote/escape the id (e.g., use a quoted attribute value or
CSS.escape on id) so the selector becomes [data-chart="..."] (refer to THEMES,
colorConfig and id in the style generation). Also replace unstable React keys
that currently use item.dataKey and item.value with stable keys (use a unique
identifier from the payload if available or fallback to the item index) where
those items are rendered to avoid reconciliation bugs.
In `@apps/local-dashboard/src/components/ui/dropdown-menu.tsx`:
- Line 1: Remove the unnecessary "use client" directive from the top of the
dropdown-menu.tsx file (it’s not needed for the Vite + React SPA); delete the
top-line string literal and verify the DropdownMenu component and any related
exports (e.g., the component/function named in this file) work without
server-component annotations and no client-only APIs are relied upon.
- Line 44: The dropdown menu's Tailwind class list uses "outline-none" which
removes focus outlines in forced-colors/high-contrast modes; update the class
string passed to cn in the DropdownMenu component (the className prop expression
where cn(...) is called) replacing "outline-none" with "outline-hidden" (and
search for other occurrences of "outline-none" in the same component to update
them as well) so it follows the Tailwind v4 accessibility migration.
In `@apps/local-dashboard/src/components/ui/sonner.tsx`:
- Around line 1-3: The file uses React.CSSProperties but never imports React;
fix by adding a type import from React (e.g., import type { CSSProperties } from
"react") and replace any occurrences of React.CSSProperties with CSSProperties;
update any relevant type annotations (such as the Toaster/Sonner wrapper using
ToasterProps) to reference CSSProperties directly so type-checking succeeds.
In `@apps/local-dashboard/src/components/ui/toggle-group.tsx`:
- Line 45: The Tailwind variants target non-existent
data-vertical/data-horizontal attributes; update them to match the actual prop
data-orientation by using arbitrary attribute variants. Replace occurrences of
data-vertical: and data-horizontal: with data-[orientation=vertical]: and
data-[orientation=horizontal]:, and replace group-data-vertical/ and
group-data-horizontal/ with group-[data-orientation=vertical]/ and
group-[data-orientation=horizontal]/ respectively in the class strings used by
the ToggleGroup component (where data-orientation={orientation} is set) so
orientation-specific styles apply.
In `@apps/local-dashboard/src/pages/SkillReport.tsx`:
- Line 233: The row background class on the TableRow in SkillReport.tsx uses a
light-only utility ("bg-red-50/50") which will be invisible in dark mode; update
the class logic for the element rendered by TableRow (key
`${rec.session_id}-${i}`) to use a dark-mode-aware variant (for example add a
dark:bg-red-900/40 or similar dark: utility) or switch to a CSS variable that
maps to different colors in light/dark, and preferably apply it conditionally
(e.g., via your existing className expression or a small helper like clsx) so
the highlighted rows remain visible in both themes.
In `@apps/local-dashboard/vite.config.ts`:
- Around line 24-40: The rollupOptions.output.manualChunks block in
vite.config.ts is poorly formatted and causing CI failures; run the Biome
formatter on this file and clean up the manualChunks function formatting (ensure
consistent indentation, proper trailing commas, and stable line breaks) for the
rollupOptions, output, and manualChunks declarations so the linter/formatter
passes.
---
Duplicate comments:
In `@apps/local-dashboard/src/components/theme-toggle.tsx`:
- Line 13: The toggle currently treats "system" like "dark", causing a noop when
the OS is dark and no way to return to system; update the onClick handler used
with setTheme to explicitly handle theme === "system" by reading the OS
preference via window.matchMedia('(prefers-color-scheme: dark)').matches and
toggling to the opposite concrete theme, and implement a clear cycle that
includes "system" (e.g., light -> dark -> system -> light) so users can return
to the system preference; update the onClick logic in the component that calls
setTheme (the theme-toggle.tsx handler referencing theme and setTheme) to
compute the next state explicitly rather than collapsing "system" to "dark".
In `@apps/local-dashboard/src/components/ui/select.tsx`:
- Around line 126-132: SelectPrimitive.ItemIndicator currently supplies both the
render prop and JSX children (CheckIcon), causing an API conflict; pick one
pattern — recommended: use render only. Remove the JSX children (the CheckIcon)
from inside SelectPrimitive.ItemIndicator and instead include that icon markup
inside the existing render prop element passed to SelectPrimitive.ItemIndicator;
alternatively, if you prefer the children API, delete the render prop entirely
and keep the CheckIcon as the sole child of SelectPrimitive.ItemIndicator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43483b43-484a-4117-9ef5-a5e5d462962b
📒 Files selected for processing (15)
apps/local-dashboard/src/App.tsxapps/local-dashboard/src/components/ActivityTimeline.tsxapps/local-dashboard/src/components/skill-health-grid.tsxapps/local-dashboard/src/components/theme-provider.tsxapps/local-dashboard/src/components/theme-toggle.tsxapps/local-dashboard/src/components/ui/chart.tsxapps/local-dashboard/src/components/ui/dropdown-menu.tsxapps/local-dashboard/src/components/ui/select.tsxapps/local-dashboard/src/components/ui/skeleton.tsxapps/local-dashboard/src/components/ui/sonner.tsxapps/local-dashboard/src/components/ui/toggle-group.tsxapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/pages/SkillReport.tsxapps/local-dashboard/tsconfig.jsonapps/local-dashboard/vite.config.ts
Comment on lines
+458
to
+461
| const dataIds = React.useMemo<UniqueIdentifier[]>( | ||
| () => data.map((d) => d.name), | ||
| [data] | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/local-dashboard/src/components/skill-health-grid.tsxRepository: selftune-dev/selftune
Length of output: 126
🏁 Script executed:
sed -n '450,500p' apps/local-dashboard/src/components/skill-health-grid.tsxRepository: selftune-dev/selftune
Length of output: 1474
🏁 Script executed:
sed -n '630,650p' apps/local-dashboard/src/components/skill-health-grid.tsxRepository: selftune-dev/selftune
Length of output: 888
🏁 Script executed:
sed -n '1,50p' apps/local-dashboard/src/components/skill-health-grid.tsx | grep -E "import|from"Repository: selftune-dev/selftune
Length of output: 555
🏁 Script executed:
sed -n '700,747p' apps/local-dashboard/src/components/skill-health-grid.tsxRepository: selftune-dev/selftune
Length of output: 1744
Derive DnD ids and drop indices from the current row model, not raw data.
Lines 458-461 build sortable ids from raw data, but lines 637-643 render table.getRowModel().rows, which reflects sorting, filtering, and pagination. After any sort or page change, the DnD model no longer matches the visible row order, causing drops to reorder the wrong records. Additionally, handleDragEnd (lines 488-495) calls arrayMove without guarding against -1 returns from indexOf, which can corrupt the data array. Derive dataIds from the current row model and perform the same lookup there before calling arrayMove.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/components/skill-health-grid.tsx` around lines 458 -
461, dataIds are being derived from the raw data array which diverges from the
visible order in table.getRowModel().rows (which includes
sorting/filtering/pagination), causing DnD mismatches and handleDragEnd calling
arrayMove with -1 indices; change the memo that builds dataIds to map over
table.getRowModel().rows (e.g., table.getRowModel().rows.map(r =>
r.original.name or r.id) so ids match the visible order), then in handleDragEnd
compute source and destination indices by looking up items in the same row model
(not raw data), guard against indexOf returning -1 (skip/return early if either
index is -1) before calling arrayMove to avoid corrupting the array.
| data-orientation={orientation} | ||
| style={{ "--gap": `${spacing * 0.25}rem` } as React.CSSProperties} | ||
| className={cn( | ||
| "group/toggle-group flex w-fit flex-row items-center gap-[var(--gap)] rounded-lg data-[size=sm]:rounded-[min(var(--radius-md),10px)] data-vertical:flex-col data-vertical:items-stretch", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify orientation selectors are aligned with data-orientation usage.
rg -n "data-orientation|data-vertical:|group-data-vertical|group-data-horizontal|group-data-\[orientation=" apps/local-dashboard/src/components/ui/toggle-group.tsxRepository: selftune-dev/selftune
Length of output: 1070
🏁 Script executed:
cat -n apps/local-dashboard/src/components/ui/toggle-group.tsx | head -100Repository: selftune-dev/selftune
Length of output: 3732
Fix Tailwind orientation selectors to match data-orientation attribute.
Line 42 sets data-orientation={orientation}, but lines 45 and 75 use Tailwind variants data-vertical:, group-data-vertical/, and group-data-horizontal/ that match non-existent [data-vertical] and [data-horizontal] attributes. Vertical layout and orientation-specific borders won't apply.
🔧 Suggested fix
- "group/toggle-group flex w-fit flex-row items-center gap-[var(--gap)] rounded-lg data-[size=sm]:rounded-[min(var(--radius-md),10px)] data-vertical:flex-col data-vertical:items-stretch",
+ "group/toggle-group flex w-fit flex-row items-center gap-[var(--gap)] rounded-lg data-[size=sm]:rounded-[min(var(--radius-md),10px)] data-[orientation=vertical]:flex-col data-[orientation=vertical]:items-stretch",- "shrink-0 group-data-[spacing=0]/toggle-group:rounded-none group-data-[spacing=0]/toggle-group:px-2 focus:z-10 focus-visible:z-10 group-data-horizontal/toggle-group:data-[spacing=0]:first:rounded-l-lg group-data-vertical/toggle-group:data-[spacing=0]:first:rounded-t-lg group-data-horizontal/toggle-group:data-[spacing=0]:last:rounded-r-lg group-data-vertical/toggle-group:data-[spacing=0]:last:rounded-b-lg group-data-horizontal/toggle-group:data-[spacing=0]:data-[variant=outline]:border-l-0 group-data-vertical/toggle-group:data-[spacing=0]:data-[variant=outline]:border-t-0 group-data-horizontal/toggle-group:data-[spacing=0]:data-[variant=outline]:first:border-l group-data-vertical/toggle-group:data-[spacing=0]:data-[variant=outline]:first:border-t",
+ "shrink-0 group-data-[spacing=0]/toggle-group:rounded-none group-data-[spacing=0]/toggle-group:px-2 focus:z-10 focus-visible:z-10 group-data-[orientation=horizontal]/toggle-group:data-[spacing=0]:first:rounded-l-lg group-data-[orientation=vertical]/toggle-group:data-[spacing=0]:first:rounded-t-lg group-data-[orientation=horizontal]/toggle-group:data-[spacing=0]:last:rounded-r-lg group-data-[orientation=vertical]/toggle-group:data-[spacing=0]:last:rounded-b-lg group-data-[orientation=horizontal]/toggle-group:data-[spacing=0]:data-[variant=outline]:border-l-0 group-data-[orientation=vertical]/toggle-group:data-[spacing=0]:data-[variant=outline]:border-t-0 group-data-[orientation=horizontal]/toggle-group:data-[spacing=0]:data-[variant=outline]:first:border-l group-data-[orientation=vertical]/toggle-group:data-[spacing=0]:data-[variant=outline]:first:border-t",Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/components/ui/toggle-group.tsx` at line 45, The
Tailwind variants target non-existent data-vertical/data-horizontal attributes;
update them to match the actual prop data-orientation by using arbitrary
attribute variants. Replace occurrences of data-vertical: and data-horizontal:
with data-[orientation=vertical]: and data-[orientation=horizontal]:, and
replace group-data-vertical/ and group-data-horizontal/ with
group-[data-orientation=vertical]/ and group-[data-orientation=horizontal]/
respectively in the class strings used by the ToggleGroup component (where
data-orientation={orientation} is set) so orientation-specific styles apply.
…ailwind v4 migration - Extract sortByPassRateAndChecks to utils.ts, dedupe sorting in App + Overview - Derive DnD dataIds from row model (not raw data), guard against -1 indexOf - Hide pagination when table is empty instead of showing "Page 1 of 0" - Fix ActivityTimeline default tab to prefer non-empty dataset - Import ReactNode directly instead of undeclared React namespace - Quote CSS attribute selector in chart style injection - Use stable composite keys for tooltip and legend items - Remove unnecessary "use client" directive from dropdown-menu (Vite SPA) - Migrate outline-none to outline-hidden for Tailwind v4 accessibility - Fix toggle-group orientation selectors to match data-orientation attribute - Add missing CSSProperties import in sonner.tsx - Add dark mode variant for SkillReport row highlight - Format vite.config.ts with Biome Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/components/app-sidebar.tsx`:
- Around line 54-57: Remove the duplicate local function formatRate and import
the shared utility instead: delete the local function named formatRate in
app-sidebar.tsx, add an import for formatRate from "@/utils", and update any
usages to rely on the imported function (which already handles null and
undefined). Ensure there are no remaining local definitions of formatRate in
this file.
In `@apps/local-dashboard/src/components/skill-health-grid.tsx`:
- Around line 106-136: STATUS_CONFIG is duplicated between this file and
SkillReport.tsx; extract the shared mapping into a single exported constant
(e.g., STATUS_CONFIG or STATUS_CONFIG_MAP) in a new or existing shared module
(such as `@/constants` or `@/utils`) and import it into both components. Locate the
definition in this file (STATUS_CONFIG) and the one in SkillReport.tsx (lines
around the existing mapping), move the JSX icons/variant/label object to the
shared module, export it, then replace the in-file definitions with imports in
both SkillHealthGrid (where STATUS_CONFIG is referenced) and SkillReport so they
use the single source of truth.
In `@apps/local-dashboard/src/types.ts`:
- Around line 56-64: The SkillSummary interface is duplicated and should be
centralized: create a shared contract module exporting the SkillSummary
interface (keeping the exact fields: skill_name, total_checks, triggered_count,
pass_rate, unique_sessions, last_seen, has_evidence with their current types),
replace the local duplicate declarations by importing SkillSummary from that
shared module in both places that currently declare it, and remove the
duplicated interface declarations so both producer and consumer reference the
single source of truth.
In `@cli/selftune/dashboard-server.ts`:
- Around line 1025-1027: The fallback for sessionPlaceholders should not be the
string "''" because that creates a valid SQL literal that can match real rows;
instead set sessionPlaceholders to null (or undefined) when sessionIdList is
empty: change the declaration of sessionPlaceholders (currently using
sessionIdList.map(...).join(",") : "''") to return null when
sessionIdList.length === 0 (e.g., sessionPlaceholders = sessionIdList.length > 0
? sessionIdList.map(() => "?").join(",") : null) and keep the existing ternary
guards that skip queries when sessionIdList is empty so no query interpolates a
placeholder string; this makes the intent explicit and avoids misleading SQL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7e37f6a-447d-4d2b-aadf-186cacb45525
📒 Files selected for processing (9)
apps/local-dashboard/src/App.tsxapps/local-dashboard/src/components/app-sidebar.tsxapps/local-dashboard/src/components/skill-health-grid.tsxapps/local-dashboard/src/components/theme-provider.tsxapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/pages/SkillReport.tsxapps/local-dashboard/src/types.tsapps/local-dashboard/src/utils.tscli/selftune/dashboard-server.ts
Comment on lines
+106
to
+136
| const STATUS_CONFIG: Record<SkillHealthStatus, { | ||
| icon: React.ReactNode | ||
| variant: "default" | "secondary" | "destructive" | "outline" | ||
| label: string | ||
| }> = { | ||
| HEALTHY: { | ||
| icon: <CheckCircleIcon className="fill-emerald-500 dark:fill-emerald-400" />, | ||
| variant: "outline", | ||
| label: "Healthy", | ||
| }, | ||
| WARNING: { | ||
| icon: <AlertTriangleIcon className="fill-amber-500 dark:fill-amber-400" />, | ||
| variant: "secondary", | ||
| label: "Warning", | ||
| }, | ||
| CRITICAL: { | ||
| icon: <XCircleIcon className="fill-red-500 dark:fill-red-400" />, | ||
| variant: "destructive", | ||
| label: "Critical", | ||
| }, | ||
| UNGRADED: { | ||
| icon: <CircleDotIcon className="text-muted-foreground" />, | ||
| variant: "secondary", | ||
| label: "Ungraded", | ||
| }, | ||
| UNKNOWN: { | ||
| icon: <HelpCircleIcon className="text-muted-foreground/60" />, | ||
| variant: "secondary", | ||
| label: "Unknown", | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
STATUS_CONFIG is duplicated across components.
This exact mapping exists in both SkillReport.tsx (lines 67-97) and here. Extract to a shared module (e.g., @/constants or @/utils) to maintain consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/components/skill-health-grid.tsx` around lines 106 -
136, STATUS_CONFIG is duplicated between this file and SkillReport.tsx; extract
the shared mapping into a single exported constant (e.g., STATUS_CONFIG or
STATUS_CONFIG_MAP) in a new or existing shared module (such as `@/constants` or
`@/utils`) and import it into both components. Locate the definition in this file
(STATUS_CONFIG) and the one in SkillReport.tsx (lines around the existing
mapping), move the JSX icons/variant/label object to the shared module, export
it, then replace the in-file definitions with imports in both SkillHealthGrid
(where STATUS_CONFIG is referenced) and SkillReport so they use the single
source of truth.
Comment on lines
+56
to
+64
| export interface SkillSummary { | ||
| skill_name: string; | ||
| total_checks: number; | ||
| triggered_count: number; | ||
| pass_rate: number; | ||
| unique_sessions: number; | ||
| last_seen: string | null; | ||
| has_evidence: boolean; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unify duplicated SkillSummary contract to prevent drift.
Line 56 redefines SkillSummary, while cli/selftune/localdb/queries.ts (Lines 330-338 in provided context) also defines it. Keeping producer/consumer contracts duplicated will eventually drift and silently desync API assumptions. Move this model to a shared contract module and import it on both sides.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/types.ts` around lines 56 - 64, The SkillSummary
interface is duplicated and should be centralized: create a shared contract
module exporting the SkillSummary interface (keeping the exact fields:
skill_name, total_checks, triggered_count, pass_rate, unique_sessions,
last_seen, has_evidence with their current types), replace the local duplicate
declarations by importing SkillSummary from that shared module in both places
that currently declare it, and remove the duplicated interface declarations so
both producer and consumer reference the single source of truth.
…NFIG, cleanup - Remove duplicate formatRate from app-sidebar, import from @/utils - Extract STATUS_CONFIG to shared @/constants module, import in both skill-health-grid and SkillReport - Remove misleading '' fallback from sessionPlaceholders since the ternary guards already skip queries when empty Co-Authored-By: Claude Opus 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 the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/components/app-sidebar.tsx`:
- Around line 38-53: STATUS_ICON and STATUS_FILTERS duplicate icon JSX already
defined in STATUS_CONFIG; replace the hardcoded JSX by deriving icons from
STATUS_CONFIG and adjusting className via React.cloneElement to avoid
duplication: import cloneElement from React, build STATUS_ICON by mapping
STATUS_CONFIG entries and cloning each v.icon with className "size-3.5 ..." for
sidebar use, and build STATUS_FILTERS similarly cloning icons with className
"size-4 ..." (keeping labels and values the same), ensuring you reference
STATUS_ICON, STATUS_FILTERS and STATUS_CONFIG when locating the changes.
In `@apps/local-dashboard/src/components/skill-health-grid.tsx`:
- Around line 477-502: The Select component currently defines options twice (the
items prop and hardcoded <SelectItem> children); remove the redundant items prop
from the Select and rely on the manually rendered SelectItem children (keep
value={activeView} and onValueChange={(v) => v && setActiveView(v)} intact),
ensuring SelectTrigger/SelectValue and SelectContent/SelectItem remain as the
single source of truth for options; alternatively, if you prefer the items array
approach, delete the SelectItem children and map the items array into the
Select’s content—pick one pattern and remove the other to avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: efc74097-541c-46b9-a3f3-d0cc8316554a
📒 Files selected for processing (5)
apps/local-dashboard/src/components/app-sidebar.tsxapps/local-dashboard/src/components/skill-health-grid.tsxapps/local-dashboard/src/constants.tsxapps/local-dashboard/src/pages/SkillReport.tsxcli/selftune/dashboard-server.ts
Comment on lines
+38
to
+53
| const STATUS_ICON: Record<SkillHealthStatus, React.ReactNode> = { | ||
| HEALTHY: <CheckCircleIcon className="size-3.5 text-emerald-600" />, | ||
| WARNING: <AlertTriangleIcon className="size-3.5 text-amber-500" />, | ||
| CRITICAL: <XCircleIcon className="size-3.5 text-red-500" />, | ||
| UNGRADED: <CircleDotIcon className="size-3.5 text-muted-foreground" />, | ||
| UNKNOWN: <HelpCircleIcon className="size-3.5 text-muted-foreground/60" />, | ||
| } | ||
|
|
||
| const STATUS_FILTERS: { label: string; value: SkillHealthStatus | "ALL"; icon: React.ReactNode }[] = [ | ||
| { label: "All Skills", value: "ALL", icon: <LayoutDashboardIcon className="size-4" /> }, | ||
| { label: "Healthy", value: "HEALTHY", icon: <CheckCircleIcon className="size-4 text-emerald-600" /> }, | ||
| { label: "Warning", value: "WARNING", icon: <AlertTriangleIcon className="size-4 text-amber-500" /> }, | ||
| { label: "Critical", value: "CRITICAL", icon: <XCircleIcon className="size-4 text-red-500" /> }, | ||
| { label: "Ungraded", value: "UNGRADED", icon: <CircleDotIcon className="size-4 text-muted-foreground" /> }, | ||
| { label: "Unknown", value: "UNKNOWN", icon: <HelpCircleIcon className="size-4 text-muted-foreground/60" /> }, | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating icon definitions with STATUS_CONFIG.
STATUS_ICON and STATUS_FILTERS duplicate icon JSX that's already in @/constants. While the size difference (size-3.5 vs size-4) justifies some divergence, you could derive these from STATUS_CONFIG and adjust the className:
const STATUS_ICON = Object.fromEntries(
Object.entries(STATUS_CONFIG).map(([k, v]) => [
k,
cloneElement(v.icon as ReactElement, { className: "size-3.5 ..." })
])
)That said, the current approach is explicit and avoids runtime complexity—leaving as-is is also reasonable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/components/app-sidebar.tsx` around lines 38 - 53,
STATUS_ICON and STATUS_FILTERS duplicate icon JSX already defined in
STATUS_CONFIG; replace the hardcoded JSX by deriving icons from STATUS_CONFIG
and adjusting className via React.cloneElement to avoid duplication: import
cloneElement from React, build STATUS_ICON by mapping STATUS_CONFIG entries and
cloning each v.icon with className "size-3.5 ..." for sidebar use, and build
STATUS_FILTERS similarly cloning icons with className "size-4 ..." (keeping
labels and values the same), ensuring you reference STATUS_ICON, STATUS_FILTERS
and STATUS_CONFIG when locating the changes.
The SelectItem children already define the options; the items prop was duplicating them unnecessarily. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/local-dashboard/src/components/skill-health-grid.tsx (1)
633-642: 🧹 Nitpick | 🔵 TrivialRemove duplicate Select option definitions in rows-per-page control.
This
Selectdefines options twice (itemsprop +<SelectItem>children). Keep a single source of truth to avoid drift.Proposed fix
<Select value={`${table.getState().pagination.pageSize}`} onValueChange={(value) => { table.setPageSize(Number(value)) }} - items={[10, 20, 50, 100].map((s) => ({ - label: `${s}`, - value: `${s}`, - }))} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/local-dashboard/src/components/skill-health-grid.tsx` around lines 633 - 642, The rows-per-page Select currently defines options twice (via the items prop and via SelectItem children); remove the duplication by choosing one source of truth—either keep the items array passed to the Select (the mapped [10,20,50,100] labels/values) and delete any <SelectItem> children, or remove the items prop and render the options only as <SelectItem> elements; ensure the control still uses table.getState().pagination.pageSize for value and table.setPageSize(Number(value)) in onValueChange so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/components/skill-health-grid.tsx`:
- Around line 414-418: KeyboardSensor is instantiated without the sortable
keyboard coordinate getter so keyboard reordering jumps by pixels instead of
moving between items; update the useSensor call for KeyboardSensor inside the
sensors declaration to pass the sortable keyboard coordinate getter
(useSensor(KeyboardSensor, { coordinateGetter: sortableKeyboardCoordinates })),
and ensure sortableKeyboardCoordinates is imported from `@dnd-kit/sortable` so
KeyboardSensor uses the correct coordinateGetter for sortable lists.
---
Duplicate comments:
In `@apps/local-dashboard/src/components/skill-health-grid.tsx`:
- Around line 633-642: The rows-per-page Select currently defines options twice
(via the items prop and via SelectItem children); remove the duplication by
choosing one source of truth—either keep the items array passed to the Select
(the mapped [10,20,50,100] labels/values) and delete any <SelectItem> children,
or remove the items prop and render the options only as <SelectItem> elements;
ensure the control still uses table.getState().pagination.pageSize for value and
table.setPageSize(Number(value)) in onValueChange so behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb7d00ef-4b65-4099-a0fc-5485887e1d26
📒 Files selected for processing (1)
apps/local-dashboard/src/components/skill-health-grid.tsx
…board DnD Without this, keyboard navigation moves by pixels instead of jumping between sortable items. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/local-dashboard/src/components/skill-health-grid.tsx (1)
636-657: 🧹 Nitpick | 🔵 TrivialUse a single source of truth for rows-per-page options.
Line 641-644 (
items) and Line 651-655 (<SelectItem>) define the same options twice. Keep one pattern to avoid silent divergence during future edits.♻️ Proposed simplification (remove duplicated
itemsprop)<Select value={`${table.getState().pagination.pageSize}`} onValueChange={(value) => { table.setPageSize(Number(value)) }} - items={[10, 20, 50, 100].map((s) => ({ - label: `${s}`, - value: `${s}`, - }))} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/local-dashboard/src/components/skill-health-grid.tsx` around lines 636 - 657, The rows-per-page options are defined twice (the items prop on the Select and the repeated map producing <SelectItem>), which can drift; extract a single constant/array (e.g., const ROW_OPTIONS = [10,20,50,100]) and use that constant for the Select items prop and for rendering <SelectItem> elements, keeping the Select value/onValueChange logic (table.setPageSize(Number(value))) and SelectTrigger/SelectValue unchanged so behavior stays identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/local-dashboard/src/components/skill-health-grid.tsx`:
- Around line 636-657: The rows-per-page options are defined twice (the items
prop on the Select and the repeated map producing <SelectItem>), which can
drift; extract a single constant/array (e.g., const ROW_OPTIONS =
[10,20,50,100]) and use that constant for the Select items prop and for
rendering <SelectItem> elements, keeping the Select value/onValueChange logic
(table.setPageSize(Number(value))) and SelectTrigger/SelectValue unchanged so
behavior stays identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f87414ac-d1f7-4fa5-97ac-22a2072ada62
📒 Files selected for processing (1)
apps/local-dashboard/src/components/skill-health-grid.tsx
…inks, scope grouping - Simplify sidebar: remove status filters, keep logo + search + skills list - Add collapsible scope groups (Project/Global) using base-ui Collapsible - Surface skill_scope from DB query through API to dashboard types - Replace skill drawer with direct Link navigation to skill report - Add Scope column to skills table with filter dropdown - Slim down site header: remove breadcrumbs, reduce to sidebar trigger + theme toggle - Add side-by-side grid layout: skills table left, activity panel right - Gitignore pnpm-lock.yaml alongside bun.lock Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/selftune/localdb/queries.ts (1)
297-309:⚠️ Potential issue | 🟡 MinorUpdate the
SkillReportPayloadevidence type.This mapper now returns
detailsandeval_set, but the exportedSkillReportPayload["evidence"]shape still omits both fields. The runtime payload and the declared CLI contract are now out of sync, so downstream code loses compile-time coverage for these additions.As per coding guidelines, "Strict typing (no
any, preferunknownwith type guards)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/localdb/queries.ts` around lines 297 - 309, The SkillReportPayload.evidence type is missing the new fields returned by the mapper (details and eval_set), causing a type mismatch; update the exported SkillReportPayload["evidence"] interface/type to include details (string | null/optional to match evidenceRows.details) and eval_set (the typed array returned by safeParseJsonArray, e.g., unknown[] or a concrete element type) so the mapped object in the evidence const aligns with the declared contract; ensure you use strict types (avoid any, prefer unknown with guards or a defined element interface) and adjust any downstream usages or type guards accordingly (refer to SkillReportPayload, the evidence mapping code, safeParseJson, and safeParseJsonArray).
♻️ Duplicate comments (1)
apps/local-dashboard/src/types.ts (1)
56-65:⚠️ Potential issue | 🟠 MajorDeduplicate
SkillSummaryto avoid producer/consumer contract drift.
SkillSummaryis redeclared here whilecli/selftune/localdb/queries.tsalready declares the same contract. Keep one shared source-of-truth type and import it on both sides; otherwise changes can silently desync API assumptions.Based on learnings: Shared interfaces and types must be defined in types.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/local-dashboard/src/types.ts` around lines 56 - 65, The SkillSummary interface is duplicated; remove the local declaration and use a single shared type to prevent drift: extract the SkillSummary interface definition into the central shared types module (the canonical types.ts) and replace the local declaration with an import of SkillSummary wherever it’s currently redeclared (e.g., in the file that declares SkillSummary and in cli/selftune/localdb/queries.ts), ensuring all references (skill_name, skill_scope, total_checks, triggered_count, pass_rate, unique_sessions, last_seen, has_evidence) use the imported symbol so there’s one source-of-truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 3: Update .gitignore so the Bun lockfile is committed: remove the
"bun.lock" entry from the .gitignore (leaving the "pnpm-lock.yaml" ignore entry
if you want to keep ignoring pnpm artifacts) so that "bun.lock" will be tracked
in the repo and provide reproducible builds; locate the "bun.lock" and
"pnpm-lock.yaml" lines in the .gitignore and delete only the "bun.lock" line.
In `@apps/local-dashboard/src/components/app-sidebar.tsx`:
- Around line 177-184: The skill search Input currently relies only on a
placeholder so assistive tech has no stable label; update the Input (the
component rendering the search box) to include an accessible name by adding
aria-label="Filter skills" (or render a visually-hidden/sr-only <label> tied to
the Input) while keeping the existing props (value={search}, onChange={(e) =>
onSearchChange(e.target.value)}, placeholder). Ensure the label targets the same
Input (or the Input receives the aria-label) so screen readers announce the
field correctly.
- Around line 135-147: scopeGroups currently builds groups for every skill but
only returns those in the hardcoded order array, dropping any unexpected or
differently-cased scopes; update the useMemo that defines scopeGroups to include
any remaining keys from groups not listed in the order (e.g., compute remainder
= Object.keys(groups).filter(k => !order.includes(k)).sort() and append
remainder.map(k => ({ scope: k, skills: groups[k] })) to the mapped ordered
list) so all scopes (including unexpected/cased variants) are preserved in the
sidebar; adjust references in the same useMemo block (scopeGroups, groups,
order) accordingly.
In `@apps/local-dashboard/src/components/skill-health-grid.tsx`:
- Around line 463-482: The status list under DropdownMenuContent is rendered
with DropdownMenuCheckboxItem but represents mutually exclusive choices; change
this to the menu's radio-group pattern (replace DropdownMenuCheckboxItem with
the corresponding radio item component and wrap items in the radio-group
container) so keyboard/screen-reader users get single-select semantics; keep the
same value mapping and use statusFilter to set the selected radio and call
onStatusFilterChange (or the radio group's onValueChange handler) to update the
single selected value, ensuring aria/radio semantics replace checkbox semantics
for accessibility.
In `@apps/local-dashboard/src/pages/SkillReport.tsx`:
- Line 75: selectedProposal persists across skill navigations and can point to a
stale ID; add a useEffect in the SkillReport component that watches the route
param name and the proposals list (or proposals.length/IDs) and calls
setSelectedProposal(null) (and reset the Tabs active value if you choose to
control it) whenever those change; alternatively convert the uncontrolled <Tabs>
into a controlled component by adding a value state for the active tab, clamping
that value whenever evolution/proposals update, and updating it in the same
useEffect to ensure EvidenceViewer never receives an invalid proposal id.
---
Outside diff comments:
In `@cli/selftune/localdb/queries.ts`:
- Around line 297-309: The SkillReportPayload.evidence type is missing the new
fields returned by the mapper (details and eval_set), causing a type mismatch;
update the exported SkillReportPayload["evidence"] interface/type to include
details (string | null/optional to match evidenceRows.details) and eval_set (the
typed array returned by safeParseJsonArray, e.g., unknown[] or a concrete
element type) so the mapped object in the evidence const aligns with the
declared contract; ensure you use strict types (avoid any, prefer unknown with
guards or a defined element interface) and adjust any downstream usages or type
guards accordingly (refer to SkillReportPayload, the evidence mapping code,
safeParseJson, and safeParseJsonArray).
---
Duplicate comments:
In `@apps/local-dashboard/src/types.ts`:
- Around line 56-65: The SkillSummary interface is duplicated; remove the local
declaration and use a single shared type to prevent drift: extract the
SkillSummary interface definition into the central shared types module (the
canonical types.ts) and replace the local declaration with an import of
SkillSummary wherever it’s currently redeclared (e.g., in the file that declares
SkillSummary and in cli/selftune/localdb/queries.ts), ensuring all references
(skill_name, skill_scope, total_checks, triggered_count, pass_rate,
unique_sessions, last_seen, has_evidence) use the imported symbol so there’s one
source-of-truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 563f8c6c-7a80-484c-8740-cc489067670c
📒 Files selected for processing (10)
.gitignoreapps/local-dashboard/src/App.tsxapps/local-dashboard/src/components/app-sidebar.tsxapps/local-dashboard/src/components/site-header.tsxapps/local-dashboard/src/components/skill-health-grid.tsxapps/local-dashboard/src/components/ui/collapsible.tsxapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/pages/SkillReport.tsxapps/local-dashboard/src/types.tscli/selftune/localdb/queries.ts
- Remove bun.lock from .gitignore to maintain build reproducibility - Preserve unexpected scope values in sidebar (don't drop unrecognized scopes) - Add aria-label to skill search input for screen reader accessibility - Switch status filter from checkbox to radio-group semantics (mutually exclusive) - Reset selectedProposal when navigating between skills via useEffect on name Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rmance Migrate data fetching from manual polling/dedup hooks to TanStack Query for instant cached navigation, background refetch, and request dedup. Optimize SQL: replace NOT IN subqueries with LEFT JOIN, move JS dedup to GROUP BY, add LIMIT 200 to unbounded evidence queries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/local-dashboard/src/pages/SkillReport.tsx (1)
77-80:⚠️ Potential issue | 🟠 MajorControl tab state and clamp proposal selection on data updates.
selectedProposalis only reset on skill name changes, and<Tabs defaultValue=...>stays uncontrolled. Ifevolutionupdates (live refresh) or a stale proposal id remains selected,EvidenceViewercan receive an invalid proposal and the active tab can drift (e.g., still"evidence"when evidence disappears).Suggested fix
export function SkillReport() { const { name } = useParams<{ name: string }>() const { data, isPending, isError, error, refetch } = useSkillReport(name) const [selectedProposal, setSelectedProposal] = useState<string | null>(null) + const [activeTab, setActiveTab] = useState<"evidence" | "invocations" | "prompts" | "sessions" | "pending">("invocations") // Reset local state when navigating between skills useEffect(() => { setSelectedProposal(null) + setActiveTab("invocations") }, [name]) + + useEffect(() => { + if (!data) return + const validProposalIds = new Set(data.evolution.map((e) => e.proposal_id)) + setSelectedProposal((prev) => (prev && validProposalIds.has(prev) ? prev : null)) + setActiveTab((prev) => (prev === "evidence" && data.evolution.length === 0 ? "invocations" : prev)) + }, [data]) // ... - return ( - <Tabs defaultValue={evolution.length > 0 ? "evidence" : "invocations"}> + return ( + <Tabs + value={activeTab} + onValueChange={(v) => setActiveTab(v as "evidence" | "invocations" | "prompts" | "sessions" | "pending")} + >Also applies to: 162-163, 170-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/local-dashboard/src/pages/SkillReport.tsx` around lines 77 - 80, selectedProposal is only reset on name changes and Tabs uses defaultValue (uncontrolled), so when evolution updates a previously selected proposal may become invalid and the active tab can drift; update the effect that currently watches name to also watch evolution (or specific evolution.ids) and clamp or clear selectedProposal via setSelectedProposal when the selected id no longer exists, and convert the Tabs from defaultValue to a controlled value prop (track activeTab state) so the UI follows state changes; ensure EvidenceViewer receives null/validated proposal when cleared. Use symbols: selectedProposal, setSelectedProposal, useEffect, Tabs (defaultValue -> value), EvidenceViewer, and evolution to locate and implement the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/components/app-sidebar.tsx`:
- Line 78: The Collapsible is currently uncontrolled (uses defaultOpen) so it
won't open when hasActive changes; make it controlled: create a local state like
open with useState(initialized to defaultOpen || hasActive), pass open={open}
and onOpenChange={setOpen} to the Collapsible, and add a useEffect that sets
setOpen(true) whenever hasActive becomes true (and optionally toggles back if
needed) so the UI opens when a new active skill appears; update references:
Collapsible, defaultOpen, hasActive, open, setOpen, onOpenChange, and the
syncing useEffect.
In `@apps/local-dashboard/src/components/skill-health-grid.tsx`:
- Around line 169-175: The cell renderer for the scope column is mutating
display by applying a CSS transform ("capitalize") to free-form scope strings;
locate the cell: ({ row }) => { const scope = row.original.scope ... } in
skill-health-grid.tsx and remove the "capitalize" class from the Badge className
so the raw scope string is rendered unchanged (keep existing null handling that
returns the placeholder and keep other classes like "text-[10px]" if desired).
- Around line 96-109: The DragHandle currently calls useSortable redundantly;
instead call useSortable once in DraggableRow (using id: row.original.name),
keep setNodeRef applied to the TableRow there, and remove useSortable from
DragHandle; pass attributes, listeners and the returned setActivatorNodeRef from
DraggableRow into DragHandle so the button receives attributes, listeners and
setActivatorNodeRef (preserving focus/activation), and ensure DragHandle no
longer invokes useSortable itself.
In `@apps/local-dashboard/src/pages/SkillReport.tsx`:
- Around line 126-130: The current isNotFound heuristic (variable isNotFound)
only checks data.usage.total_checks, data.evidence, data.evolution, and
data.pending_proposals and thus misclassifies skills that have
invocation/session/prompt signals; update the condition to also consider
invocation/session/prompt-related fields (e.g., data.invocations, data.sessions,
data.prompts or data.usage.total_invocations) so isNotFound is true only when
all of those counts/arrays are empty or zero. Modify the isNotFound assignment
to include safe null/undefined checks for these fields and treat non-empty
arrays or positive counts as “found” (i.e., short-circuit the not-found result
if any invocation/session/prompt signal exists).
- Around line 179-235: TabsTrigger currently wraps Tooltip/TooltipTrigger
creating nested interactive buttons; fix each tab by moving the Tooltip wrapper
outside the TabsTrigger so the button (TabsTrigger) is not nested inside another
control. Concretely, for every TabsTrigger (e.g., the "evidence", "invocations",
"prompts", "sessions", "pending" triggers) remove the Tooltip/TooltipTrigger
from inside it and instead place the Tooltip adjacent to the TabsTrigger, or
keep Tooltip outside and render a non-interactive child (like a span) inside
TabsTrigger for the TooltipTrigger target so no two button elements are nested;
ensure TooltipContent remains associated with the same label text.
---
Duplicate comments:
In `@apps/local-dashboard/src/pages/SkillReport.tsx`:
- Around line 77-80: selectedProposal is only reset on name changes and Tabs
uses defaultValue (uncontrolled), so when evolution updates a previously
selected proposal may become invalid and the active tab can drift; update the
effect that currently watches name to also watch evolution (or specific
evolution.ids) and clamp or clear selectedProposal via setSelectedProposal when
the selected id no longer exists, and convert the Tabs from defaultValue to a
controlled value prop (track activeTab state) so the UI follows state changes;
ensure EvidenceViewer receives null/validated proposal when cleared. Use
symbols: selectedProposal, setSelectedProposal, useEffect, Tabs (defaultValue ->
value), EvidenceViewer, and evolution to locate and implement the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cfef67cd-795a-48e1-b58f-09915526056c
📒 Files selected for processing (4)
.gitignoreapps/local-dashboard/src/components/app-sidebar.tsxapps/local-dashboard/src/components/skill-health-grid.tsxapps/local-dashboard/src/pages/SkillReport.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/selftune/localdb/queries.ts (1)
163-181:⚠️ Potential issue | 🟡 MinorSQL-level dedup is cleaner than JS Set.
The
LEFT JOIN+IS NULLpattern correctly identifies proposals without terminal actions. However,GROUP BY ea.proposal_idwithout aggregates onaction,timestamp,detailsrelies on SQLite's non-standard behavior—the returned values for those columns are arbitrary.If you need the most recent
created/validatedentry per proposal, consider:GROUP BY ea.proposal_id HAVING ea.timestamp = MAX(ea.timestamp)Or use a window function to pick the latest row explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/localdb/queries.ts` around lines 163 - 181, The SQL used to populate pending_proposals uses GROUP BY ea.proposal_id but selects ea.action, ea.timestamp and ea.details without aggregates, which yields arbitrary rows in SQLite; update the query in the pending_proposals db.query call to deterministically pick the latest audit row per proposal—either add HAVING ea.timestamp = MAX(ea.timestamp) combined with proper aggregation or rewrite using a window function (ROW_NUMBER() OVER (PARTITION BY ea.proposal_id ORDER BY ea.timestamp DESC) and filter for row_number = 1) so action, timestamp and details correspond to the most recent created/validated entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/package.json`:
- Line 30: The package.json dependency for recharts is hard-pinned as "2.15.4"
causing inconsistency with other caret-ranged dependencies; update the
"recharts" entry to use a caret range (e.g., "^2.15.4") in
apps/local-dashboard's package.json so it matches the project's versioning style
unless there is a deliberate reason to pin it.
In `@CHANGELOG.md`:
- Line 18: Update the changelog entry to clarify that live SSE-backed updates
are the primary mechanism and the 15-second refetchInterval against
SQLite-backed v2 API endpoints is a fallback polling strategy; specifically edit
the line referencing "15-second polling" and mention SSE as primary (with
refetchInterval as fallback/polling for environments where SSE is unavailable)
and include the symbol refetchInterval to tie the note to the implementation.
In `@cli/selftune/dashboard-server.ts`:
- Around line 993-1013: The pending proposals SQL block in dashboard-server.ts
(the pending_proposals query) duplicates the LEFT JOIN + IS NULL + GROUP BY
logic already implemented in getOverviewPayload() in queries.ts; extract that
SQL and its result-mapping into a shared helper (e.g., a new function like
fetchPendingProposals or getPendingProposals) placed in queries.ts or a common
queries helper, then replace the inline query in dashboard-server.ts to call
that helper (preserve the same parameter signature and return type shape used by
pending_proposals). Ensure the helper returns the same
Array<{proposal_id,string; action,string; timestamp,string; details,string;
skill_name,string}> so callers (dashboard-server.ts and getOverviewPayload) can
use it without further changes.
---
Outside diff comments:
In `@cli/selftune/localdb/queries.ts`:
- Around line 163-181: The SQL used to populate pending_proposals uses GROUP BY
ea.proposal_id but selects ea.action, ea.timestamp and ea.details without
aggregates, which yields arbitrary rows in SQLite; update the query in the
pending_proposals db.query call to deterministically pick the latest audit row
per proposal—either add HAVING ea.timestamp = MAX(ea.timestamp) combined with
proper aggregation or rewrite using a window function (ROW_NUMBER() OVER
(PARTITION BY ea.proposal_id ORDER BY ea.timestamp DESC) and filter for
row_number = 1) so action, timestamp and details correspond to the most recent
created/validated entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33f11e8a-ec0b-484c-9663-08834783b13f
⛔ Files ignored due to path filters (2)
apps/local-dashboard/bun.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
CHANGELOG.mdapps/local-dashboard/HANDOFF.mdapps/local-dashboard/package.jsonapps/local-dashboard/src/App.tsxapps/local-dashboard/src/hooks/useOverview.tsapps/local-dashboard/src/hooks/useSkillReport.tsapps/local-dashboard/src/pages/Overview.tsxcli/selftune/dashboard-server.tscli/selftune/localdb/queries.ts
CHANGELOG.md
Outdated
| - Collapsible sidebar navigation listing all skills by health status | ||
| - shadcn/ui component library with dark/light theme toggle and selftune branding | ||
| - TanStack Query for data fetching with smart caching, background refetch, and instant back-navigation | ||
| - 15-second polling against SQLite-backed v2 API endpoints via `refetchInterval` |
There was a problem hiding this comment.
Clarify SSE vs polling behavior in release notes.
Line 18 documents 15-second polling as the update mechanism, but this PR’s stated objective includes live SSE-backed updates. Please update the entry to state whether SSE is primary and polling is fallback, so behavior is accurately documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 18, Update the changelog entry to clarify that live
SSE-backed updates are the primary mechanism and the 15-second refetchInterval
against SQLite-backed v2 API endpoints is a fallback polling strategy;
specifically edit the line referencing "15-second polling" and mention SSE as
primary (with refetchInterval as fallback/polling for environments where SSE is
unavailable) and include the symbol refetchInterval to tie the note to the
implementation.
… a11y, not-found heuristic - Make sidebar Collapsible controlled so it auto-opens when active skill changes (Comment #1) - Consolidate useSortable to single call per row via React context, use setActivatorNodeRef on drag handle button (Comment #2) - Remove capitalize CSS transform on free-form scope values (Comment #3) - Broaden isNotFound heuristic to check invocations, prompts, sessions in addition to evals/evolution/proposals (Comment #4) - Move Tooltip outside TabsTrigger to avoid nested interactive elements, use Base UI render prop for composition (Comment #5) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…, shared query helper - Use caret range for recharts version (^2.15.4) for consistency - Clarify changelog: SSE was removed, polling via refetchInterval is primary - Extract getPendingProposals() shared helper in queries.ts, used by both getOverviewPayload() and dashboard-server skill report endpoint Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/local-dashboard/src/pages/SkillReport.tsx (1)
75-80:⚠️ Potential issue | 🟠 MajorControl/clamp report-local selection state when skill data changes.
selectedProposalis reset onnamechanges, butactiveProposalis not validated against the currentevolutionset, andTabsat Line 175 is still uncontrolled (defaultValue). With live updates or route transitions, this can leave a stale proposal/tab selection and render an empty right pane.Proposed fix
@@ - // Auto-select first proposal if none selected - const activeProposal = selectedProposal ?? (evolution.length > 0 ? evolution[0].proposal_id : null) + // Clamp selected proposal to current evolution entries + const activeProposal = + selectedProposal && evolution.some((e) => e.proposal_id === selectedProposal) + ? selectedProposal + : evolution.length > 0 + ? evolution[0].proposal_id + : null + + // Remount Tabs when route/tab availability changes to avoid stale selected tab + const tabsResetKey = [ + name ?? "", + evolution.length > 0 ? "evidence" : "no-evidence", + (prompt_samples?.length ?? 0) > 0 ? "prompts" : "no-prompts", + (session_metadata?.length ?? 0) > 0 ? "sessions" : "no-sessions", + pending_proposals.length > 0 ? "pending" : "no-pending", + ].join("|") @@ - <Tabs defaultValue={evolution.length > 0 ? "evidence" : "invocations"}> + <Tabs key={tabsResetKey} defaultValue={evolution.length > 0 ? "evidence" : "invocations"}>#!/bin/bash # Verify stale-selection risk is present in current code: # 1) Tabs is uncontrolled (defaultValue) and not keyed by route/availability. # 2) activeProposal does not clamp to current evolution IDs. rg -n -C2 'selectedProposal|activeProposal|<Tabs .*defaultValue|<Tabs .*key=' apps/local-dashboard/src/pages/SkillReport.tsxAlso applies to: 167-168, 175-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/local-dashboard/src/pages/SkillReport.tsx` around lines 75 - 80, selectedProposal is reset on name changes but activeProposal (the selection used by Tabs) isn't validated against the current evolution set and Tabs is using defaultValue (uncontrolled), allowing a stale/invalid tab to remain and render an empty pane; fix by making Tabs controlled (use value prop) and clamp/validate activeProposal whenever evolution or name changes: in the useEffect that currently calls setSelectedProposal(null) also compute a valid id from evolution (e.g., first evolution id or null) and call setSelectedProposal(validId) so activeProposal always matches an existing evolution, and replace Tabs' defaultValue with value={selectedProposal} (ensure handlers call setSelectedProposal) to keep tabs in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/components/app-sidebar.tsx`:
- Around line 253-256: The span rendering "selftune ..." leaves a trailing space
when the variable version is falsy; update the JSX in the span that currently
uses the version variable so the space is only added when version exists (e.g.,
concatenate a leading space inside the conditional) or build the string and call
.trim() before rendering; look for the span with text "selftune" and the version
variable to apply the change (referenced around the ActivityIcon and the span
that uses version).
---
Duplicate comments:
In `@apps/local-dashboard/src/pages/SkillReport.tsx`:
- Around line 75-80: selectedProposal is reset on name changes but
activeProposal (the selection used by Tabs) isn't validated against the current
evolution set and Tabs is using defaultValue (uncontrolled), allowing a
stale/invalid tab to remain and render an empty pane; fix by making Tabs
controlled (use value prop) and clamp/validate activeProposal whenever evolution
or name changes: in the useEffect that currently calls setSelectedProposal(null)
also compute a valid id from evolution (e.g., first evolution id or null) and
call setSelectedProposal(validId) so activeProposal always matches an existing
evolution, and replace Tabs' defaultValue with value={selectedProposal} (ensure
handlers call setSelectedProposal) to keep tabs in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbdd27e2-a2a1-4064-bae5-8005708c60ba
📒 Files selected for processing (3)
apps/local-dashboard/src/components/app-sidebar.tsxapps/local-dashboard/src/components/skill-health-grid.tsxapps/local-dashboard/src/pages/SkillReport.tsx
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/package.json`:
- Line 19: The packages "@tailwindcss/vite", "tailwindcss", and "shadcn" should
be moved out of dependencies into devDependencies because they are build-time
only; update package.json by removing these entries from the "dependencies"
section and adding identical version entries under "devDependencies" (also move
any other listed build-only items referenced in the comment ranges), then run
your package manager to update the lockfile (npm/yarn/pnpm install) so CI and
production installs no longer include them at runtime.
In `@cli/selftune/dashboard-server.ts`:
- Around line 688-709: The handler that checks spaDir and
url.pathname.startsWith("/assets/") must not fall through to the SPA HTML
fallback when an assets file is missing; update the block in dashboard-server.ts
(the GET /assets/* handling) so that if the computed filePath is outside spaDir,
doesn't exist, or isn't a file you immediately return a 404 Response (with
appropriate CORS headers from corsHeaders()) instead of letting execution
continue to the generic SPA index.html fallback; apply the same change to the
second occurrence around the other assets-handling block (the one you noted at
1130-1136).
- Around line 606-608: Change the catch clause that currently reads catch
(error) { console.error("Failed to refresh v2 dashboard data", error); } to type
the caught value as unknown and log only a concise message: use catch (error:
unknown) { const msg = error instanceof Error ? error.message : String(error);
console.error("Failed to refresh v2 dashboard data:", msg); } so you follow the
same typing/logging pattern used elsewhere (see the earlier handling around line
570) and avoid emitting full stack/objects in the console.
- Around line 689-701: The request handler currently uses blocking fs calls
(statSync, readFileSync) when serving assets and index.html (inside the spaDir
branch where url.pathname.startsWith("/assets/") and the GET / and SPA fallback
code paths), which blocks the event loop; replace these with async non-blocking
APIs from fs/promises (e.g., await stat, await readFile) or preload and cache
all static files into memory at server startup and serve from that cache; update
the fetch handler to be async/await-safe and ensure you check file existence
with await stat(filePath) (or cached map lookups) and read file contents with
await readFile(filePath) (or from the preloaded map) before returning the
Response.
In `@cli/selftune/localdb/queries.ts`:
- Around line 284-285: The SkillReportPayload.evidence item type is missing the
details and eval_set fields while the query emits details and eval_set (see
usage around safeParseJsonArray(row.eval_set_json)), so update the
SkillReportPayload interface for the evidence array to include details: string |
null (or the appropriate type emitted by row.details) and eval_set: type[] (or
the specific element type returned by safeParseJsonArray) to match the runtime
shape; adjust any nullable/optional annotations to reflect whether row.details
or row.eval_set_json can be null/undefined so TypeScript stops complaining.
- Around line 382-398: The current getPendingProposals query is nondeterministic
because it SELECTs unaggregated columns while GROUPing by proposal_id; replace
the GROUP BY/LEFT JOIN pattern with a deterministic CTE that ranks rows per
proposal (use ROW_NUMBER() OVER (PARTITION BY proposal_id ORDER BY timestamp
DESC) or a MAX(timestamp) approach) to pick the newest audit row per proposal,
then filter to keep only rows whose action is in ('created','validated') and
exclude any proposal that has a terminal action
('deployed','rejected','rolled_back'); update the SQL inside getPendingProposals
(still using db.query(...).all(...)) to select from that CTE where rn = 1 (or
join to the subquery with max timestamp) so the returned
action/timestamp/details/skill_name are always from the latest audit row for
each proposal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0c7f033-7469-418d-8430-816b13cdadec
📒 Files selected for processing (4)
CHANGELOG.mdapps/local-dashboard/package.jsoncli/selftune/dashboard-server.tscli/selftune/localdb/queries.ts
Comment on lines
+284
to
+285
| details: row.details, | ||
| eval_set: safeParseJsonArray(row.eval_set_json), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declared payload shape vs emitted fields in this file.
rg -n -C3 "export interface SkillReportPayload|validation: safeParseJson|details: row.details|eval_set: safeParseJsonArray" cli/selftune/localdb/queries.tsRepository: selftune-dev/selftune
Length of output: 585
🏁 Script executed:
cat -n cli/selftune/localdb/queries.ts | sed -n '178,220p'Repository: selftune-dev/selftune
Length of output: 1463
Add details and eval_set fields to SkillReportPayload.evidence type.
The evidence array in SkillReportPayload (lines 192–202) does not declare details and eval_set, but lines 284–285 emit them. This breaks strict typing and will cause type errors.
Update the interface:
Type contract fix
evidence: Array<{
proposal_id: string;
target: string;
stage: string;
timestamp: string;
rationale: string | null;
confidence: number | null;
original_text: string | null;
proposed_text: string | null;
validation: Record<string, unknown> | null;
+ details: string | null;
+ eval_set: string[];
}>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/localdb/queries.ts` around lines 284 - 285, The
SkillReportPayload.evidence item type is missing the details and eval_set fields
while the query emits details and eval_set (see usage around
safeParseJsonArray(row.eval_set_json)), so update the SkillReportPayload
interface for the evidence array to include details: string | null (or the
appropriate type emitted by row.details) and eval_set: type[] (or the specific
element type returned by safeParseJsonArray) to match the runtime shape; adjust
any nullable/optional annotations to reflect whether row.details or
row.eval_set_json can be null/undefined so TypeScript stops complaining.
…inistic query - Move @tailwindcss/vite, tailwindcss, shadcn to devDependencies - Fix trailing space in version display when version is empty - Type caught error as unknown in refreshV2Data - Replace sync fs (readFileSync/statSync) with Bun.file() for hot-path asset serving - Return 404 for missing /assets/* files instead of falling through to SPA - Add details and eval_set fields to SkillReportPayload.evidence type - Fix nondeterministic GROUP BY with ROW_NUMBER() CTE in getPendingProposals Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace non-null assertion with type cast in useSkillReport (noNonNullAssertion) - Break long import line in dashboard-server.ts to satisfy Biome formatter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cli/selftune/dashboard-server.ts (2)
611-613:⚠️ Potential issue | 🟡 MinorLog a concise refresh failure message instead of the raw error object.
Line 612 still logs the full caught object, which can dump noisy stack/object details. Keep only the message text.
Suggested fix
- } catch (error: unknown) { - console.error("Failed to refresh v2 dashboard data", error); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error); + console.error(`Failed to refresh v2 dashboard data: ${message}`); // Keep serving the last successful materialization. }As per coding guidelines, "Actionable error messages, not raw stack traces".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/dashboard-server.ts` around lines 611 - 613, The catch block that currently does console.error("Failed to refresh v2 dashboard data", error) should log only a concise message (the error message text) instead of the entire error object/stack; update the catch to extract the message (e.g., use error instanceof Error ? error.message : String(error)) and pass that to console.error (or the module logger) with the "Failed to refresh v2 dashboard data" prefix so only a concise, actionable message is emitted from the refresh logic.
1135-1139:⚠️ Potential issue | 🟡 MinorRestrict SPA fallback to route paths, not file-like URLs.
This fallback currently serves
index.htmlfor any non-/api/GET, including paths like/favicon.icoor/manifest.json. That can return wrong MIME types and hide legitimate 404s.Suggested fix
- if (spaDir && req.method === "GET" && !url.pathname.startsWith("/api/")) { + if ( + spaDir && + req.method === "GET" && + !url.pathname.startsWith("/api/") && + !url.pathname.startsWith("/assets/") && + extname(url.pathname) === "" + ) { const html = await Bun.file(join(spaDir, "index.html")).text(); return new Response(html, { headers: { "Content-Type": "text/html; charset=utf-8", ...corsHeaders() }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/dashboard-server.ts` around lines 1135 - 1139, The SPA fallback currently returns index.html for any non-/api/ GET (spaDir, req.method, url.pathname, Bun.file(join(...))), causing wrong MIME for file-like URLs; change the condition so the fallback only triggers for route-like paths (e.g., pathname is "/" or ends with "/" or the last segment contains no dot/extension) and skip paths that look like files (have a "." after the last "/") so requests such as /favicon.ico or /manifest.json are not served index.html; keep using spaDir, Bun.file and corsHeaders() but add the file-like check (or use path.extname(url.pathname) === "") before returning index.html.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 1001-1006: Replace the current dynamic parameter expansion that
builds sessionPlaceholders from sessionIdList (see sessionIds, sessionIdList,
sessionPlaceholders) with a single reusable CTE or subquery that selects
DISTINCT session_id FROM skill_usage WHERE skill_name = ? (e.g., WITH
skill_sessions AS (...)); then rewrite the four subsequent queries that
currently use IN (?,?,...) to use IN (SELECT session_id FROM skill_sessions) or
join to skill_sessions, passing only the skillName parameter into the CTE so you
no longer expand bind parameters manually across the queries.
In `@cli/selftune/localdb/queries.ts`:
- Around line 329-331: The correlated subquery selecting the latest non-null
skill_scope from skill_usage may cause full-table scans; add a composite index
on (skill_name, skill_scope, timestamp) to speed the lookup used by the subquery
(reference: the correlated subquery that reads FROM skill_usage s2 WHERE
s2.skill_name = su.skill_name AND s2.skill_scope IS NOT NULL ORDER BY
s2.timestamp DESC LIMIT 1). Create the index in your migration or DB setup so
the query planner can efficiently satisfy the WHERE and ORDER BY for skill_scope
lookups.
- Around line 163-164: The returned PendingProposal objects from
getPendingProposals include a skill_name string but
OverviewPayload.pending_proposals' type does not — either update the
OverviewPayload.pending_proposals type to include skill_name or strip skill_name
before assigning; locate getPendingProposals (returns PendingProposal[]) and the
code that constructs the OverviewPayload (OverviewPayload.pending_proposals) and
either add skill_name to that payload type or map the array (e.g., pick/omit) to
remove skill_name so runtime shape matches the declared OverviewPayload type.
---
Duplicate comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 611-613: The catch block that currently does console.error("Failed
to refresh v2 dashboard data", error) should log only a concise message (the
error message text) instead of the entire error object/stack; update the catch
to extract the message (e.g., use error instanceof Error ? error.message :
String(error)) and pass that to console.error (or the module logger) with the
"Failed to refresh v2 dashboard data" prefix so only a concise, actionable
message is emitted from the refresh logic.
- Around line 1135-1139: The SPA fallback currently returns index.html for any
non-/api/ GET (spaDir, req.method, url.pathname, Bun.file(join(...))), causing
wrong MIME for file-like URLs; change the condition so the fallback only
triggers for route-like paths (e.g., pathname is "/" or ends with "/" or the
last segment contains no dot/extension) and skip paths that look like files
(have a "." after the last "/") so requests such as /favicon.ico or
/manifest.json are not served index.html; keep using spaDir, Bun.file and
corsHeaders() but add the file-like check (or use path.extname(url.pathname) ===
"") before returning index.html.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a05c2ec-3bfd-4bbe-8b96-9865737201de
📒 Files selected for processing (5)
apps/local-dashboard/package.jsonapps/local-dashboard/src/components/app-sidebar.tsxapps/local-dashboard/src/hooks/useSkillReport.tscli/selftune/dashboard-server.tscli/selftune/localdb/queries.ts
…pe index - Replace dynamic bind-parameter expansion with CTE subquery for session lookups - Add skill_name to OverviewPayload.pending_proposals type to match runtime shape - Add composite index on skill_usage(skill_name, skill_scope, timestamp) for scope lookups Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/selftune/localdb/queries.ts (1)
48-54:⚠️ Potential issue | 🟡 MinorNullable mismatch:
detailsandskill_nameare nullable inevolution_auditbut typed as required.The schema defines
details TEXTandskill_name TEXT(both nullable), but this type declares them as non-nullable strings. If the database returnsNULL, the runtime shape won't match the declared type.Option: Make types nullable or provide defaults
pending_proposals: Array<{ proposal_id: string; action: string; timestamp: string; - details: string; - skill_name: string; + details: string | null; + skill_name: string | null; }>;Or coalesce to empty string in
getPendingProposalsif non-nullable is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/localdb/queries.ts` around lines 48 - 54, The declared type for pending_proposals incorrectly marks details and skill_name as required strings while the DB columns (evolution_audit.details and .skill_name) are nullable; either change the pending_proposals declaration to make those fields nullable (details: string | null; skill_name: string | null) or, if you want non-nullable types, update the data mapper in getPendingProposals to coalesce NULL to a default (e.g., empty string) before returning results; update the type or the mapper accordingly and run type checks to ensure consistency between the pending_proposals type and getPendingProposals behavior.
♻️ Duplicate comments (2)
cli/selftune/dashboard-server.ts (2)
1122-1127:⚠️ Potential issue | 🟠 MajorRestrict SPA fallback to route paths, not file-like URLs.
Line 1122 currently serves
index.htmlfor any non-/api/*GET, including unresolved/favicon.icoor/manifest.webmanifest, which yields HTML with wrong MIME type. Gate fallback to extensionless client routes.Suggested fix
- if (spaDir && req.method === "GET" && !url.pathname.startsWith("/api/")) { + if ( + spaDir && + req.method === "GET" && + !url.pathname.startsWith("/api/") && + !url.pathname.startsWith("/assets/") && + extname(url.pathname) === "" + ) { const html = await Bun.file(join(spaDir, "index.html")).text(); return new Response(html, { headers: { "Content-Type": "text/html; charset=utf-8", ...corsHeaders() }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/dashboard-server.ts` around lines 1122 - 1127, The SPA fallback currently returns index.html for any non-/api GET (using spaDir and url.pathname), which incorrectly serves HTML for file-like URLs (e.g., /favicon.ico); change the gate so the fallback only triggers for extensionless client routes by checking url.pathname does not look like a file (e.g., no trailing filename with a dot/extension) and/or ends with a slash before reading join(spaDir, "index.html") and returning the response; update the handler around spaDir and req.method === "GET" to skip fallback when url.pathname contains a file extension like ".ico", ".png", ".webmanifest", etc.
611-613:⚠️ Potential issue | 🟡 MinorLog only a concise refresh failure message.
Line 612 logs the raw
errorobject, which can emit full stack/object dumps in CLI output. Keep the catch typed asunknownand log a normalized message string only.Suggested fix
- } catch (error: unknown) { - console.error("Failed to refresh v2 dashboard data", error); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error); + console.error(`Failed to refresh v2 dashboard data: ${message}`); // Keep serving the last successful materialization. }As per coding guidelines, "Actionable error messages, not raw stack traces".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/dashboard-server.ts` around lines 611 - 613, The catch block handling "Failed to refresh v2 dashboard data" currently logs the raw error object (catch (error: unknown) / console.error("Failed to refresh v2 dashboard data", error)); change it so only a concise, normalized string is logged: keep the catch typed as unknown and extract a safe message (e.g. use error instanceof Error ? error.message : String(error)) or otherwise call String(error), then pass only that message to console.error instead of the raw error object to avoid printing full stacks/objects to the CLI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 968-973: The 404 branch in the handler incorrectly returns "Skill
not found" when report.usage.total_checks === 0 and
report.recent_invocations.length === 0 even though getSkillReportPayload may
contain non-empty evidence; update the conditional in the handler that uses
report (the block that calls Response.json with status 404 and corsHeaders()) to
also check that report.evidence is empty (or has no items) before returning 404,
so only truly empty reports (no usage, no invocations, and no evidence) produce
the 404 response; locate the report variable and adjust the conditional logic
accordingly.
- Around line 600-603: Guard the initial v2 materialization so a thrown error
from materializeIncremental(db) doesn't abort startDashboardServer: call
openDb() as before, then wrap materializeIncremental(db) in a try/catch around
that call (reference materializeIncremental and openDb), on error log the
exception (use the module logger or console.error) and continue serving with
degraded v2 data; ensure lastV2MaterializedAt is initialized outside/after the
try (e.g. set to 0 on failure or Date.now() on success) so downstream code can
detect that initial materialization didn’t complete.
In `@cli/selftune/localdb/queries.ts`:
- Around line 385-406: The ROW_NUMBER() in getPendingProposals()’s latest CTE
orders only by timestamp, which can produce non-deterministic results when
timestamps tie; update the CTE (named latest) to include ea.id in its SELECT
list and add ea.id as a deterministic tiebreaker in the ORDER BY clause of
ROW_NUMBER() (e.g., ORDER BY ea.timestamp DESC, ea.id DESC or ASC) so the chosen
row for each proposal_id is stable when timestamps are identical.
---
Outside diff comments:
In `@cli/selftune/localdb/queries.ts`:
- Around line 48-54: The declared type for pending_proposals incorrectly marks
details and skill_name as required strings while the DB columns
(evolution_audit.details and .skill_name) are nullable; either change the
pending_proposals declaration to make those fields nullable (details: string |
null; skill_name: string | null) or, if you want non-nullable types, update the
data mapper in getPendingProposals to coalesce NULL to a default (e.g., empty
string) before returning results; update the type or the mapper accordingly and
run type checks to ensure consistency between the pending_proposals type and
getPendingProposals behavior.
---
Duplicate comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 1122-1127: The SPA fallback currently returns index.html for any
non-/api GET (using spaDir and url.pathname), which incorrectly serves HTML for
file-like URLs (e.g., /favicon.ico); change the gate so the fallback only
triggers for extensionless client routes by checking url.pathname does not look
like a file (e.g., no trailing filename with a dot/extension) and/or ends with a
slash before reading join(spaDir, "index.html") and returning the response;
update the handler around spaDir and req.method === "GET" to skip fallback when
url.pathname contains a file extension like ".ico", ".png", ".webmanifest", etc.
- Around line 611-613: The catch block handling "Failed to refresh v2 dashboard
data" currently logs the raw error object (catch (error: unknown) /
console.error("Failed to refresh v2 dashboard data", error)); change it so only
a concise, normalized string is logged: keep the catch typed as unknown and
extract a safe message (e.g. use error instanceof Error ? error.message :
String(error)) or otherwise call String(error), then pass only that message to
console.error instead of the raw error object to avoid printing full
stacks/objects to the CLI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9927c979-cb56-4361-b585-9dee1957bc94
📒 Files selected for processing (3)
cli/selftune/dashboard-server.tscli/selftune/localdb/queries.tscli/selftune/localdb/schema.ts
Comment on lines
+385
to
+406
| export function getPendingProposals(db: Database, skillName?: string): PendingProposal[] { | ||
| const whereClause = skillName ? "WHERE ea.skill_name = ? AND" : "WHERE"; | ||
| const params = skillName ? [skillName] : []; | ||
| return db | ||
| .query( | ||
| `WITH latest AS ( | ||
| SELECT ea.proposal_id, ea.action, ea.timestamp, ea.details, ea.skill_name, | ||
| ROW_NUMBER() OVER (PARTITION BY ea.proposal_id ORDER BY ea.timestamp DESC) AS rn | ||
| FROM evolution_audit ea | ||
| LEFT JOIN evolution_audit ea2 | ||
| ON ea2.proposal_id = ea.proposal_id | ||
| AND ea2.action IN ('deployed', 'rejected', 'rolled_back') | ||
| ${whereClause} ea.action IN ('created', 'validated') | ||
| AND ea2.id IS NULL | ||
| ) | ||
| SELECT proposal_id, action, timestamp, details, skill_name | ||
| FROM latest | ||
| WHERE rn = 1 | ||
| ORDER BY timestamp DESC`, | ||
| ) | ||
| .all(...params) as PendingProposal[]; | ||
| } |
There was a problem hiding this comment.
Add id tiebreaker to ROW_NUMBER() for determinism.
The ROW_NUMBER() orders only by timestamp DESC. If two audit rows for the same proposal have identical timestamps, the selected row is non-deterministic.
Add tiebreaker
- ROW_NUMBER() OVER (PARTITION BY ea.proposal_id ORDER BY ea.timestamp DESC) AS rn
+ ROW_NUMBER() OVER (PARTITION BY ea.proposal_id ORDER BY ea.timestamp DESC, ea.id DESC) AS rnNote: ea.id isn't selected in the current CTE—you'll need to add it to the CTE's SELECT list for the ORDER BY to reference it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/localdb/queries.ts` around lines 385 - 406, The ROW_NUMBER() in
getPendingProposals()’s latest CTE orders only by timestamp, which can produce
non-deterministic results when timestamps tie; update the CTE (named latest) to
include ea.id in its SELECT list and add ea.id as a deterministic tiebreaker in
the ORDER BY clause of ROW_NUMBER() (e.g., ORDER BY ea.timestamp DESC, ea.id
DESC or ASC) so the chosen row for each proposal_id is stable when timestamps
are identical.
…ministic tiebreaker - Guard initial v2 materialization with try/catch to avoid full server crash - Include evidence in not-found check so evidence-only skills aren't 404'd - Add ea.id DESC tiebreaker to ROW_NUMBER() for deterministic pending proposals Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
cli/selftune/dashboard-server.ts (2)
1132-1138:⚠️ Potential issue | 🟠 MajorRestrict the SPA fallback to extensionless routes.
This fallback still matches any non-API GET, so
/favicon.ico,/manifest.webmanifest, or/missing.jsreceives HTML instead of a 404/static file response. Keep it for client-side routes only, and serve any root-level public assets before this branch if the SPA needs them.Possible fix
- if (spaDir && req.method === "GET" && !url.pathname.startsWith("/api/")) { + if ( + spaDir && + req.method === "GET" && + !url.pathname.startsWith("/api/") && + !url.pathname.startsWith("/assets/") && + extname(url.pathname) === "" + ) { const html = await Bun.file(join(spaDir, "index.html")).text(); return new Response(html, { headers: { "Content-Type": "text/html; charset=utf-8", ...corsHeaders() }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/dashboard-server.ts` around lines 1132 - 1138, The SPA fallback currently triggers for any non-API GET (in the block using spaDir, req.method, and url.pathname and returning Bun.file(join(spaDir, "index.html")).text()), causing requests for static assets like /favicon.ico or /manifest.webmanifest to incorrectly return HTML; change the condition so the fallback only runs for extensionless client routes (e.g., ensure url.pathname does not contain a file extension by checking for the absence of a dot in the final path segment) and still excludes paths starting with "/api/"; also ensure static/public asset handling (serving files from spaDir or a public dir) runs before this fallback so real asset requests return the file or 404 rather than index.html.
618-620:⚠️ Potential issue | 🟡 MinorDo not log the raw refresh error object.
This still prints the caught value directly, which can dump stacks or opaque objects into CLI output. Extract the message the same way as the startup path instead of logging
errorverbatim.Possible fix
} catch (error: unknown) { - console.error("Failed to refresh v2 dashboard data", error); + const message = error instanceof Error ? error.message : String(error); + console.error(`Failed to refresh v2 dashboard data: ${message}`); // Keep serving the last successful materialization. }As per coding guidelines, CLI source code should use actionable error messages, not raw stack traces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/dashboard-server.ts` around lines 618 - 620, Replace the direct logging of the caught value in the catch block that currently does console.error("Failed to refresh v2 dashboard data", error) with a sanitized message extraction (e.g. pull error.message when error is an Error, otherwise stringify the value) and log that string so you don't print raw stacks/opaque objects; update the catch around the "Failed to refresh v2 dashboard data" handling (the catch's error variable and its console.error call) to use this extracted message format consistent with the startup error handling elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 974-979: The current 404 guard runs immediately after calling
getSkillReportPayload() so it only considers skill_usage and evolution_evidence
and can falsely return 404 before enrichment queries run; move the not-found
check to after the enrichment queries that load evolution_audit,
pending_proposals, and skill_invocations (the queries below this block) and
update the hasData calculation to include those datasets (or recompute hasData
after enrichment) so a skill with only audit/proposals/invocations is not
treated as missing; adjust the checks around report, hasData, and the 404
response accordingly in the same function.
- Around line 613-621: The refreshV2Data() function currently only updates
lastV2MaterializedAt on success, causing repeated immediate retries after TTL;
modify it to also maintain a separate lastV2MaterializedAttemptAt timestamp and
use that to throttle retries (compare Date.now() - lastV2MaterializedAttemptAt
against V2_MATERIALIZE_TTL_MS before starting a new materializeIncremental(db)).
Set lastV2MaterializedAttemptAt when an attempt starts (and on error), and only
update lastV2MaterializedAt on successful completion of materializeIncremental;
keep the try/catch around materializeIncremental(db) and log errors as before.
- Around line 600-610: Wrap the openDb() call in the same best-effort guard as
materializeIncremental() so DB bootstrap failures don't prevent
startDashboardServer() from reaching Bun.serve(): try to call openDb() and
assign to a nullable variable (e.g. db or v2Db), catch errors and log them,
leave lastV2MaterializedAt as 0; only call materializeIncremental(db) if the db
handle is non-null; make refreshV2Data and any /api/v2/* handlers check for the
DB handle and short-circuit with a 503 or empty response when the store is
unavailable; on shutdown only call db.close() if the handle exists. Ensure
references include openDb(), materializeIncremental(), lastV2MaterializedAt,
refreshV2Data, startDashboardServer(), and Bun.serve().
In `@cli/selftune/localdb/queries.ts`:
- Around line 330-332: The subquery selecting skill_scope from skill_usage (the
expression starting with "(SELECT s2.skill_scope FROM skill_usage s2 WHERE
s2.skill_name = su.skill_name AND s2.skill_scope IS NOT NULL ORDER BY
s2.timestamp DESC LIMIT 1) as skill_scope") can produce non-deterministic
results when two rows share the same timestamp; modify that subquery's ORDER BY
to add a deterministic tiebreaker "id DESC" after timestamp (i.e., ORDER BY
s2.timestamp DESC, s2.id DESC) so the chosen skill_scope is stable.
---
Duplicate comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 1132-1138: The SPA fallback currently triggers for any non-API GET
(in the block using spaDir, req.method, and url.pathname and returning
Bun.file(join(spaDir, "index.html")).text()), causing requests for static assets
like /favicon.ico or /manifest.webmanifest to incorrectly return HTML; change
the condition so the fallback only runs for extensionless client routes (e.g.,
ensure url.pathname does not contain a file extension by checking for the
absence of a dot in the final path segment) and still excludes paths starting
with "/api/"; also ensure static/public asset handling (serving files from
spaDir or a public dir) runs before this fallback so real asset requests return
the file or 404 rather than index.html.
- Around line 618-620: Replace the direct logging of the caught value in the
catch block that currently does console.error("Failed to refresh v2 dashboard
data", error) with a sanitized message extraction (e.g. pull error.message when
error is an Error, otherwise stringify the value) and log that string so you
don't print raw stacks/opaque objects; update the catch around the "Failed to
refresh v2 dashboard data" handling (the catch's error variable and its
console.error call) to use this extracted message format consistent with the
startup error handling elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef61e07f-3bcf-4482-9012-621511e76f7b
📒 Files selected for processing (2)
cli/selftune/dashboard-server.tscli/selftune/localdb/queries.ts
Comment on lines
+330
to
+332
| (SELECT s2.skill_scope FROM skill_usage s2 | ||
| WHERE s2.skill_name = su.skill_name AND s2.skill_scope IS NOT NULL | ||
| ORDER BY s2.timestamp DESC LIMIT 1) as skill_scope, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find schema definitions and migrations mentioning skill_usage/skill_scope.
fd -e sql -e ts | xargs rg -n -C4 "CREATE TABLE\\s+skill_usage|\\bskill_usage\\b|\\bskill_scope\\b|PRIMARY KEY|\\bid\\s+INTEGER"Repository: selftune-dev/selftune
Length of output: 48806
Add secondary ORDER BY to skill_scope subquery for deterministic tiebreaker.
The subquery at line 332 orders only by timestamp DESC. When two skill_usage rows for the same skill share the same timestamp but different scopes, the selected skill_scope is non-deterministic. Add id DESC as a secondary sort key to ensure consistent results:
SQL correction
- (SELECT s2.skill_scope FROM skill_usage s2
- WHERE s2.skill_name = su.skill_name AND s2.skill_scope IS NOT NULL
- ORDER BY s2.timestamp DESC LIMIT 1) as skill_scope,
+ (SELECT s2.skill_scope FROM skill_usage s2
+ WHERE s2.skill_name = su.skill_name AND s2.skill_scope IS NOT NULL
+ ORDER BY s2.timestamp DESC, s2.id DESC
+ LIMIT 1) as skill_scope,The id column (INTEGER PRIMARY KEY AUTOINCREMENT) is available in the schema and provides a stable monotonic tiebreaker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/localdb/queries.ts` around lines 330 - 332, The subquery
selecting skill_scope from skill_usage (the expression starting with "(SELECT
s2.skill_scope FROM skill_usage s2 WHERE s2.skill_name = su.skill_name AND
s2.skill_scope IS NOT NULL ORDER BY s2.timestamp DESC LIMIT 1) as skill_scope")
can produce non-deterministic results when two rows share the same timestamp;
modify that subquery's ORDER BY to add a deterministic tiebreaker "id DESC"
after timestamp (i.e., ORDER BY s2.timestamp DESC, s2.id DESC) so the chosen
skill_scope is stable.
…d 404 - Guard openDb() in try/catch so DB bootstrap failure doesn't crash server - Make db nullable, return 503 from /api/v2/* when store is unavailable - Throttle failed refresh attempts with separate lastV2RefreshAttemptAt timestamp - Move skill 404 check after enrichment queries (evolution, proposals, invocations) - Use optional chaining for db.close() on shutdown Co-Authored-By: Claude Opus 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 the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 625-628: The catch block that logs "Failed to refresh v2 dashboard
data" currently passes the raw error object to console.error; change it to log
the extracted message string like the earlier logging at line 610—extract the
message using the same logic (e.g., check instanceof Error and use
error.message, otherwise String(error)) and pass that message to console.error
so stack traces are not printed and logs remain consistent; update the
console.error call in the refresh v2 dashboard data catch block accordingly
(refer to the catch block and the earlier extraction logic used at line ~610).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 627d3ca0-6859-44f0-808f-2be6f023d5c0
📒 Files selected for processing (1)
cli/selftune/dashboard-server.ts
Comment on lines
+625
to
+628
| } catch (error: unknown) { | ||
| console.error("Failed to refresh v2 dashboard data", error); | ||
| // Keep serving the last successful materialization. | ||
| } |
There was a problem hiding this comment.
Log the extracted message, not the raw error object.
Line 610 correctly extracts and logs the message, but line 626 logs the raw error object. This can emit full stack traces and is inconsistent with the pattern used elsewhere.
Suggested fix
} catch (error: unknown) {
- console.error("Failed to refresh v2 dashboard data", error);
+ const message = error instanceof Error ? error.message : String(error);
+ console.error(`Failed to refresh v2 dashboard data: ${message}`);
// Keep serving the last successful materialization.
}As per coding guidelines: "Actionable error messages, not raw stack traces".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/dashboard-server.ts` around lines 625 - 628, The catch block
that logs "Failed to refresh v2 dashboard data" currently passes the raw error
object to console.error; change it to log the extracted message string like the
earlier logging at line 610—extract the message using the same logic (e.g.,
check instanceof Error and use error.message, otherwise String(error)) and pass
that message to console.error so stack traces are not printed and logs remain
consistent; update the console.error call in the refresh v2 dashboard data catch
block accordingly (refer to the catch block and the earlier extraction logic
used at line ~610).
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
Introduces a minimal React SPA at
apps/local-dashboard/with two routes: overview (KPIs, skill health grid, evolution feed) and per-skill drilldown (pass rate, invocation breakdown, evaluation records). The app consumes the existing dashboard-server API endpoints with live SSE updates, explicit loading/error/empty states, and design tokens matching the current dashboard. No breaking changes to the existing dashboard server; this is a frontend-only replacement.Changes
apps/local-dashboard/package with Vite + React + TypeScriptapps/local-dashboard/HANDOFF.mdfor integration next stepsTo Run
Start
selftune dashboard --port 7888, thencd apps/local-dashboard && bunx vite→ opens atlocalhost:5199.🤖 Generated with Claude Code