feat: extract @selftune/ui shared component package#61
Merged
WellDunDun merged 8 commits intomainfrom Mar 16, 2026
Merged
Conversation
Move 11 shadcn/ui primitives and 7 domain components from apps/local-dashboard into packages/ui/ as a source-only workspace package. Rewire dashboard imports to consume from @selftune/ui. Add packages/ui/README.md and update system-overview design doc. - SkillHealthGrid now accepts renderSkillName prop (decouples react-router) - Root workspaces expanded to include apps/* for workspace resolution - Dashboard types.ts, utils.ts, constants.tsx re-export from shared package - Build, tests (1398/1398), and lint all pass Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR extracts UI components, primitives, utilities, and types from the local dashboard into a new shared package ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR involves substantial structural reorganization across 30+ files with heterogeneous changes: new package creation with dependency declarations, systematic import-path refactoring across multiple dashboard files (repetitive but requiring verification), type consolidation with split sources (apps-local vs. shared), one notable component signature change (SkillHealthGrid renderSkillName prop), and compile-time type parity enforcement. While many changes follow a consistent pattern, the diversity of concerns (package structure, import updates, type sourcing decisions, component API extension) and the need to verify type correctness across boundaries elevates review complexity. 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: 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/pages/Overview.tsx`:
- Around line 3-6: Multiple imports from the same module should be consolidated:
replace the four separate imports of ActivityPanel, OrchestrateRunsPanel,
SectionCards, and SkillHealthGrid from "@selftune/ui/components" with a single
named-import statement that imports all four symbols together to reduce
repetition and improve readability.
In `@docs/design-docs/index.md`:
- Line 9: The document-level verification date at the top of
docs/design-docs/index.md is out of sync with the table entry for
system-overview.md (table shows 2026-03-16); update the file-level stamp text at
the top of the file (the document-level date/header) to 2026-03-16 so both dates
match, and save the file; verify the top-of-file stamp string and the table row
for "system-overview.md" now both read 2026-03-16.
In `@packages/ui/src/components/skill-health-grid.tsx`:
- Around line 119-158: The column renderer createColumns currently types
renderSkillName as (name: string) => React.ReactNode which only passes
row.original.name; change the renderSkillName parameter to accept the full
SkillCard (e.g., renderSkillName?: (skill: SkillCard) => React.ReactNode) and
update usages inside createColumns to call renderSkillName(row.original) instead
of renderSkillName(row.original.name); make the same change in the other
name-column block referenced (the similar column around the 299–311 area) so
consumers can access scope/status via the SkillCard object.
In `@packages/ui/src/lib/format.ts`:
- Around line 26-34: The timeAgo function can produce NaN results for invalid
timestamps (new Date(timestamp).getTime() -> NaN) leading to outputs like "NaNd
ago"; modify timeAgo to validate the parsed time (e.g., const t = new
Date(timestamp).getTime(); if (isNaN(t)) return a safe fallback such as "just
now" or an empty string), then compute diff/mins/hours/days using that validated
value; update references to diff/mins/hours in the function (timeAgo) to use the
validated time so invalid inputs don't produce "NaNd ago".
In `@packages/ui/src/types.ts`:
- Around line 16-87: The duplicated dashboard contract types (EvalSnapshot,
EvolutionEntry, UnmatchedQuery, PendingProposal, EvidenceEntry,
OrchestrateRunReport, OrchestrateRunSkillAction) risk drifting; add a
compile-time parity guard that asserts these local types are assignable to the
canonical dashboard contract types and vice‑versa (e.g., an
AssertAssignable/AssertEqual type check) by importing the canonical types and
the local types in a small TS-only file or test that runs at build time, failing
the build if any type diverges; alternatively export and re-use the canonical
types directly from the UI package to eliminate duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b72927c2-a1b2-4a41-9d5e-89ab6c59a2ec
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
apps/local-dashboard/package.jsonapps/local-dashboard/src/App.tsxapps/local-dashboard/src/components/app-sidebar.tsxapps/local-dashboard/src/components/theme-toggle.tsxapps/local-dashboard/src/components/ui/sheet.tsxapps/local-dashboard/src/components/ui/sidebar.tsxapps/local-dashboard/src/constants.tsxapps/local-dashboard/src/lib/utils.tsapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/pages/SkillReport.tsxapps/local-dashboard/src/pages/Status.tsxapps/local-dashboard/src/types.tsapps/local-dashboard/src/utils.tsdocs/design-docs/index.mddocs/design-docs/system-overview.mdpackage.jsonpackages/ui/README.mdpackages/ui/index.tspackages/ui/package.jsonpackages/ui/src/components/ActivityTimeline.tsxpackages/ui/src/components/EvidenceViewer.tsxpackages/ui/src/components/EvolutionTimeline.tsxpackages/ui/src/components/InfoTip.tsxpackages/ui/src/components/OrchestrateRunsPanel.tsxpackages/ui/src/components/index.tspackages/ui/src/components/section-cards.tsxpackages/ui/src/components/skill-health-grid.tsxpackages/ui/src/lib/constants.tsxpackages/ui/src/lib/format.tspackages/ui/src/lib/index.tspackages/ui/src/lib/utils.tspackages/ui/src/primitives/badge.tsxpackages/ui/src/primitives/button.tsxpackages/ui/src/primitives/card.tsxpackages/ui/src/primitives/checkbox.tsxpackages/ui/src/primitives/collapsible.tsxpackages/ui/src/primitives/dropdown-menu.tsxpackages/ui/src/primitives/index.tspackages/ui/src/primitives/label.tsxpackages/ui/src/primitives/select.tsxpackages/ui/src/primitives/table.tsxpackages/ui/src/primitives/tabs.tsxpackages/ui/src/primitives/tooltip.tsxpackages/ui/src/types.tspackages/ui/tsconfig.json
Comment on lines
+16
to
+87
| // -- Dashboard contract types (re-declared for package independence) ---------- | ||
|
|
||
| export interface EvalSnapshot { | ||
| before_pass_rate?: number; | ||
| after_pass_rate?: number; | ||
| net_change?: number; | ||
| improved?: boolean; | ||
| regressions?: Array<Record<string, unknown>>; | ||
| new_passes?: Array<Record<string, unknown>>; | ||
| } | ||
|
|
||
| export interface EvolutionEntry { | ||
| timestamp: string; | ||
| proposal_id: string; | ||
| action: string; | ||
| details: string; | ||
| eval_snapshot?: EvalSnapshot | null; | ||
| } | ||
|
|
||
| export interface UnmatchedQuery { | ||
| timestamp: string; | ||
| session_id: string; | ||
| query: string; | ||
| } | ||
|
|
||
| export interface PendingProposal { | ||
| proposal_id: string; | ||
| action: string; | ||
| timestamp: string; | ||
| details: string; | ||
| skill_name?: string; | ||
| } | ||
|
|
||
| export interface EvidenceEntry { | ||
| 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: Array<Record<string, unknown>>; | ||
| } | ||
|
|
||
| export interface OrchestrateRunSkillAction { | ||
| skill: string; | ||
| action: "evolve" | "watch" | "skip"; | ||
| reason: string; | ||
| deployed?: boolean; | ||
| rolledBack?: boolean; | ||
| alert?: string | null; | ||
| elapsed_ms?: number; | ||
| llm_calls?: number; | ||
| } | ||
|
|
||
| export interface OrchestrateRunReport { | ||
| run_id: string; | ||
| timestamp: string; | ||
| elapsed_ms: number; | ||
| dry_run: boolean; | ||
| approval_mode: "auto" | "review"; | ||
| total_skills: number; | ||
| evaluated: number; | ||
| evolved: number; | ||
| deployed: number; | ||
| watched: number; | ||
| skipped: number; | ||
| skill_actions: OrchestrateRunSkillAction[]; | ||
| } |
There was a problem hiding this comment.
Contract re-declaration introduces a drift risk across packages.
These dashboard contract types are duplicated in @selftune/ui without an enforced parity mechanism. A contract update in cli/selftune/dashboard-contract.ts can silently desync compile-time assumptions in shared UI consumers.
Add an automated parity guard (generated shared contract types, or compile-time compatibility assertions in a package that can import both sides) before release.
Based on learnings: Dashboard contract changes in dashboard-contract.ts must be synchronized with apps/local-dashboard/src/types.ts and dashboard components that consume the changed fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/types.ts` around lines 16 - 87, The duplicated dashboard
contract types (EvalSnapshot, EvolutionEntry, UnmatchedQuery, PendingProposal,
EvidenceEntry, OrchestrateRunReport, OrchestrateRunSkillAction) risk drifting;
add a compile-time parity guard that asserts these local types are assignable to
the canonical dashboard contract types and vice‑versa (e.g., an
AssertAssignable/AssertEqual type check) by importing the canonical types and
the local types in a small TS-only file or test that runs at build time, failing
the build if any type diverges; alternatively export and re-use the canonical
types directly from the UI package to eliminate duplication.
- Consolidate four separate @selftune/ui/components imports into one (Overview.tsx) - Align index.md file-level verification date to 2026-03-16 - Guard timeAgo() against invalid timestamps returning NaN - Broaden renderSkillName prop to accept full SkillCard instead of just name - Add type-parity-check.ts compile-time guard against contract drift Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/skill-health-grid.tsx (1)
267-270:⚠️ Potential issue | 🟠 MajorUse a stable composite row ID;
namealone is not unique.Line 370 and Line 397 derive identity from
nameonly. If two rows share a name across scopes, table row identity and DnD reorder can target the wrong row.Proposed fix
+function skillRowId(skill: Pick<SkillCard, "name" | "scope">): string { + return `${skill.scope ?? ""}::${skill.name}`; +} + function DraggableRow({ row }: { row: Row<SkillCard> }) { const { transform, transition, setNodeRef, setActivatorNodeRef, isDragging, attributes, listeners } = useSortable({ - id: row.original.name, + id: skillRowId(row.original), }) @@ const table = useReactTable({ @@ - getRowId: (row) => row.name, + getRowId: (row) => skillRowId(row), @@ function handleDragEnd(event: DragEndEvent) { @@ setData((prev) => { - const ids = prev.map((d) => d.name) + const ids = prev.map((d) => skillRowId(d)) const oldIndex = ids.indexOf(active.id as string) const newIndex = ids.indexOf(over.id as string)Also applies to: 299-311, 370-370, 397-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/skill-health-grid.tsx` around lines 267 - 270, The row identity used for drag-and-drop is currently derived from row.original.name which is not globally unique; update DraggableRow (and any other useSortable/id usages that pass row.original.name) to use a stable composite ID (for example combining scope + name or using row.id if present) so each Row<SkillCard> has a unique identifier for useSortable; change the id argument passed to useSortable from row.original.name to a deterministic composite (e.g., `${row.original.scope}:${row.original.name}`) in DraggableRow and mirror the same change wherever row.original.name is used as the sortable id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/skill-health-grid.tsx`:
- Around line 244-248: The comparator in sortingFn uses new Date(...).getTime(),
which can produce NaN for malformed lastSeen values and break sorting; update
sortingFn to parse lastSeen for each row (e.g., Date.parse or new
Date(...).getTime()), then normalize any non-finite result to a numeric fallback
(0 or -Infinity as desired) using Number.isFinite (or isNaN) before returning a
- b so the comparator always returns a stable number; check
rowA.original.lastSeen and rowB.original.lastSeen and apply the same
normalization for both.
In `@packages/ui/src/lib/format.ts`:
- Around line 10-13: formatRate currently only checks for null/undefined and can
output "NaN%" or "Infinity%"; guard against non-finite numbers by checking
Number.isFinite(rate) in the function (alongside the existing null/undefined
check) and return the fallback "--" for non-finite values before performing
Math.round(rate * 100) and returning the formatted string.
In `@packages/ui/src/type-parity-check.ts`:
- Around line 29-31: Formatter drift detected around the AssertAssignable type
definition; run the project's formatter (e.g., Prettier/tsfmt) on
packages/ui/src/type-parity-check.ts and commit the resulting changes so CI no
longer flags formatting differences. Ensure the file is saved after formatting
and that the type AssertAssignable<T, U> and surrounding whitespace/line breaks
match the project's formatting rules before pushing.
- Around line 9-17: The parity check in packages/ui (type-parity-check.ts)
imports canonical types like Canonical_EvalSnapshot, Canonical_EvidenceEntry,
Canonical_EvolutionEntry, Canonical_OrchestrateRunReport,
Canonical_OrchestrateRunSkillAction, Canonical_PendingProposal, and
Canonical_UnmatchedQuery directly from the CLI codebase; move this parity test
out of the UI package into a repo-level/internal verification module or a new
shared contract package so `@selftune/ui` no longer depends on CLI internals.
Create a new module (e.g., internal-verification or packages/contract) and
relocate type-parity-check.ts there, update its imports to point to the CLI
contract path only within that repo-level tool or change the import to the new
shared contract package, then update package.json/tsconfig references for the
moved module and remove the relative CLI import from `@selftune/ui` so the UI
package remains a leaf package.
---
Outside diff comments:
In `@packages/ui/src/components/skill-health-grid.tsx`:
- Around line 267-270: The row identity used for drag-and-drop is currently
derived from row.original.name which is not globally unique; update DraggableRow
(and any other useSortable/id usages that pass row.original.name) to use a
stable composite ID (for example combining scope + name or using row.id if
present) so each Row<SkillCard> has a unique identifier for useSortable; change
the id argument passed to useSortable from row.original.name to a deterministic
composite (e.g., `${row.original.scope}:${row.original.name}`) in DraggableRow
and mirror the same change wherever row.original.name is used as the sortable
id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a342a179-aa88-4f4a-a1d0-b18eab3c879d
📒 Files selected for processing (5)
apps/local-dashboard/src/pages/Overview.tsxdocs/design-docs/index.mdpackages/ui/src/components/skill-health-grid.tsxpackages/ui/src/lib/format.tspackages/ui/src/type-parity-check.ts
packages/ui/src/type-parity-check.ts
Outdated
Comment on lines
+9
to
+17
| import type { | ||
| EvalSnapshot as Canonical_EvalSnapshot, | ||
| EvidenceEntry as Canonical_EvidenceEntry, | ||
| EvolutionEntry as Canonical_EvolutionEntry, | ||
| OrchestrateRunReport as Canonical_OrchestrateRunReport, | ||
| OrchestrateRunSkillAction as Canonical_OrchestrateRunSkillAction, | ||
| PendingProposal as Canonical_PendingProposal, | ||
| UnmatchedQuery as Canonical_UnmatchedQuery, | ||
| } from "../../../cli/selftune/dashboard-contract"; |
There was a problem hiding this comment.
Break @selftune/ui → cli source coupling to preserve package boundaries
Importing canonical types from ../../../cli/selftune/dashboard-contract makes the UI package depend on CLI internals, which weakens isolated builds/reuse of @selftune/ui. Move this parity check to a repo-level/internal verification module (or depend on a dedicated shared contract package) so packages/ui stays leaf-like.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/type-parity-check.ts` around lines 9 - 17, The parity check
in packages/ui (type-parity-check.ts) imports canonical types like
Canonical_EvalSnapshot, Canonical_EvidenceEntry, Canonical_EvolutionEntry,
Canonical_OrchestrateRunReport, Canonical_OrchestrateRunSkillAction,
Canonical_PendingProposal, and Canonical_UnmatchedQuery directly from the CLI
codebase; move this parity test out of the UI package into a repo-level/internal
verification module or a new shared contract package so `@selftune/ui` no longer
depends on CLI internals. Create a new module (e.g., internal-verification or
packages/contract) and relocate type-parity-check.ts there, update its imports
to point to the CLI contract path only within that repo-level tool or change the
import to the new shared contract package, then update package.json/tsconfig
references for the moved module and remove the relative CLI import from
`@selftune/ui` so the UI package remains a leaf package.
- Guard lastSeen sort comparator against NaN from invalid timestamps - Guard formatRate against NaN/Infinity inputs - Move type-parity-check from packages/ui to tests/types/ (preserves UI package as a leaf with no CLI dependency) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lftune-dev/selftune into WellDunDun/extract-ui-package
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.
Extract 11 shadcn/ui primitives and 7 domain components into
packages/ui/as a source-only workspace package. Rewire the dashboard to consume from@selftune/ui. All tests pass (1398/1398), build succeeds, and lint is clean.Key changes:
packages/ui/with presentational components, shadcn primitives, utilities, and typesSkillHealthGridnow accepts optionalrenderSkillNameprop (decouples react-router)@selftune/uiinstead of local filespackage.jsonworkspaces expanded to includeapps/*Positions the codebase for future cloud dashboard reuse of shared components.