feat: TUI polish — performance, copywriting, tab fix#11
Merged
flyingrobots merged 7 commits intomainfrom Feb 22, 2026
Merged
Conversation
… v1.2.0 Triaged all 31 inbox items: 20 promoted to campaign:CLITOOL (Milestone 10), 10 promoted to campaign:DASHBOARD, 1 rejected (SUB-ENTITY-001, fixed in PR #10). Created intent:CLI-FOUNDATION and campaign:CLITOOL. Upgraded VISION_NORTH_STAR from v1.1.0 to v1.2.0 (authoritative). Updated README milestones table.
Replace all hardcoded color values (91 chalk calls, 40+ Ink color props, 2 duplicate STATUS_COLOR maps) with a configurable theme token system. New module: src/tui/theme/ (7 files) — typed tokens, two presets (cyan-magenta default + teal-orange-pink candidate), N-stop gradient interpolation, NO_COLOR support, chalk adapter, and Ink ThemeProvider. Env vars: XYPH_THEME selects preset, NO_COLOR disables all coloring. Default output matches prior behavior exactly. 44 new tests (338 total). Build + lint clean.
Performance: batch-fetch node props and neighbors via Promise.all instead of sequential awaits (142 nodes). Loading UI: activity log on landing screen during graph initialization. Copywriting: WARP GRAPH → XYPH/Project Graph. Tab cycling: explicit key.tab guards in all view handlers to prevent interference with Dashboard view-switching.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR adds a centralized theme/token system replacing hard-coded colors across TUI and CLI, instruments UI with theme-aware Ink context, and implements snapshot fetch performance improvements (batch node/neighbor fetching) plus an onProgress callback for streaming load logs to the Dashboard. Changes
Sequence Diagram(s)sequenceDiagram
participant DashboardUI as Dashboard (Ink)
participant DashboardSvc as DashboardService
participant DashboardPort as DashboardPort (interface)
participant Adapter as WarpDashboardAdapter
participant Graph as Graph/Repo
DashboardUI->>DashboardSvc: getSnapshot(onProgress)
DashboardSvc->>DashboardPort: fetchSnapshot(onProgress)
DashboardPort->>Adapter: fetchSnapshot(onProgress)
Adapter->>Graph: open graph
Adapter->>DashboardUI: onProgress("opening graph")
Adapter->>Graph: batch fetch node props (parallel)
Adapter->>DashboardUI: onProgress("syncing coverage / materializing")
Adapter->>Graph: batch fetch outgoing neighbors (parallel)
Adapter->>DashboardUI: onProgress("resolving edges / building models")
Adapter-->>DashboardSvc: return GraphSnapshot (with caches applied)
DashboardSvc-->>DashboardUI: resolved snapshot (clears loadLog)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (15)
docs/canonical/VISION_NORTH_STAR.md (1)
24-24: Parenthetical in a canonical section heading reads as a working note, not a title.
## 3. The Planning Compiler (and its Expansion)— for anAUTHORITATIVEdocument, section headings should name the concept, not narrate the author's intent to expand it. Consider a cleaner title that captures the broader scope now described in the section.✏️ Suggested alternatives
-## 3. The Planning Compiler (and its Expansion) +## 3. The Planning Compiler & Execution Engineor simply:
-## 3. The Planning Compiler (and its Expansion) +## 3. The Planning Compiler🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/canonical/VISION_NORTH_STAR.md` at line 24, The section heading "## 3. The Planning Compiler (and its Expansion)" reads like a working note; change it to a canonical title that names the concept rather than the author's intent. Replace the heading text (search for the exact string "## 3. The Planning Compiler (and its Expansion)") with a cleaner title such as "## 3. The Planning Compiler and Expansion" or "## 3. The Planning Compiler" so the section header is authoritative and concept-focused.src/tui/theme/__tests__/presets.test.ts (1)
5-9: Consider derivingALL_STATUS_KEYSfrom theStatusKeytype to keep it in sync.This array duplicates the
StatusKeyunion. If a new key is added toStatusKey, the compiler won't flag its absence here because this is just a plainstring[]-compatible array. TheThemeinterface'sRecord<StatusKey, TokenValue>enforces completeness at compile time, so this is more of a test-hygiene concern than a correctness gap.A
satisfies StatusKey[]assertion would make the compiler flag any missing key:const ALL_STATUS_KEYS = [ 'DONE', 'IN_PROGRESS', 'BACKLOG', 'BLOCKED', 'PLANNED', 'INBOX', 'GRAVEYARD', 'PENDING', 'APPROVED', 'REJECTED', 'UNKNOWN', 'OPEN', 'CHANGES_REQUESTED', 'MERGED', 'CLOSED', ] as const satisfies readonly StatusKey[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/__tests__/presets.test.ts` around lines 5 - 9, Update the ALL_STATUS_KEYS declaration so the compiler verifies it matches the StatusKey union: make the array a const tuple and use the satisfies operator (e.g., "as const satisfies readonly StatusKey[]") so any missing or extra keys are flagged; keep the identifier ALL_STATUS_KEYS and ensure it still can be iterated where Theme (Record<StatusKey, TokenValue>) or tests use it.src/tui/theme/gradient.ts (1)
18-19: PotentialNaNif two adjacent stops share the samepos.If
a.pos === b.pos, line 19 divides by zero, producingNaNRGB values that propagate to the return. The precondition comment says "sorted ascending" but doesn't guarantee strictly ascending. Since stops are currently only defined internally, this is low-risk, but a one-line guard would make the function robust against degenerate input.🛡️ Suggested guard
if (t >= a.pos && t <= b.pos) { + if (a.pos === b.pos) return a.color; const local = (t - a.pos) / (b.pos - a.pos);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/gradient.ts` around lines 18 - 19, The division (const local = (t - a.pos) / (b.pos - a.pos)) can produce NaN when two adjacent stops have the same pos; update the block in the gradient interpolation function to guard against b.pos === a.pos (compare a.pos and b.pos) and handle it by treating local as 0 (or another defined fallback) or by skipping interpolation for that pair, ensuring subsequent RGB math uses a valid numeric local value; refer to the variables a, b, t and the local interpolation calculation to locate and fix the issue.xyph-actuator.ts (1)
6-6: CLI entry point coupled tosrc/tui/module
xyph-actuator.tsis a CLI adapter, so this is less severe than the domain-service coupling flagged inTriageService.ts, but it shares the same root cause:styled/getThemelive undersrc/tui/rather than a neutral shared location. Resolving the TriageService finding (moving theme utilities tosrc/shared/orsrc/utils/) would fix this import automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xyph-actuator.ts` at line 6, xyph-actuator.ts imports UI-specific utilities getTheme and styled from src/tui, coupling the CLI entrypoint to the TUI module; move these utilities to a neutral shared location (e.g., src/shared or src/utils) and update references. Specifically, create a new module exporting getTheme and styled, update xyph-actuator.ts to import them from that shared module, and also update TriageService.ts to import the same shared exports so both files reference the neutral location instead of src/tui.src/domain/services/TriageService.ts (1)
3-3: Domain service should not import from the TUI layer
getThemeandstyledlive insrc/tui/theme/— the TUI adapter — butTriageServiceis a pure domain service. This inverts the dependency direction that hexagonal architecture requires (domain is the core; TUI adapts to it, not the other way around). It also makes the domain layer indirectly sensitive toXYPH_THEME/NO_COLORenv vars and to any future restructuring ofsrc/tui/.The same violation appears in
SovereigntyService.ts(line 2).Consider moving the chalk-based theme utilities to a shared location (e.g.,
src/shared/theme/orsrc/utils/theme.ts) so both the CLI, TUI, and domain services can import from a neutral layer, or inject a logger abstraction into services that need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/TriageService.ts` at line 3, TriageService (and SovereigntyService) currently import getTheme and styled from the TUI layer which breaks hexagonal boundaries; remove the direct imports from '../../tui/theme/index.js' and instead either (A) move the chalk-based utilities (getTheme, styled) into a neutral shared module (e.g., src/shared/theme or src/utils/theme) and update TriageService and SovereigntyService to import getTheme/styled from that shared module, or (B) refactor the services to accept a theme/logger abstraction via constructor or method injection and have the TUI adapter supply the concrete getTheme/styled implementation; update references to getTheme and styled in TriageService and SovereigntyService accordingly.src/tui/theme/__tests__/resolve.test.ts (2)
34-34: Moveviimport to the top of the file.
viis first used at line 34 but only imported at line 97. While ES module imports are statically hoisted and this works at runtime, placing an import after its use site is non-idiomatic and misleading. Additionally, if Vitest is configured withglobals: true, this import is redundant.♻️ Proposed fix
-import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { getTheme, isNoColor, _resetThemeForTesting } from '../resolve.js';Then remove the trailing declaration:
-// Import vi from vitest for spying -import { vi } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/__tests__/resolve.test.ts` at line 34, Move the vi import to the top of the test file and remove the redundant trailing declaration: relocate the existing vi import so it appears before any use (it’s currently used in const warnSpy = vi.spyOn(...) in resolve.test.ts) and delete the later import/declaration at the bottom of the file so there’s a single, idiomatic import for vi.
76-80: Hardcoded hex'#00ff00'ties test to a specific token value.If the default theme's
successtoken changes, this assertion silently becomes wrong. Consider deriving the expected value from the token itself.♻️ Proposed refactor
it('ink() returns hex when NO_COLOR is unset', () => { delete process.env['NO_COLOR']; const t = getTheme(); - expect(t.ink(t.theme.semantic.success)).toBe('#00ff00'); + expect(t.ink(t.theme.semantic.success)).toBe(t.hex(t.theme.semantic.success)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/__tests__/resolve.test.ts` around lines 76 - 80, The test hardcodes '#00ff00' making it brittle; change the assertion to derive the expected value from the theme token instead of a literal. In the test that calls getTheme() and stores it in t, compute the expected hex by using the same token under test (t.theme.semantic.success) — e.g., pass that token into the ink conversion helper used in production or otherwise reference its resolved value — and assert t.ink(t.theme.semantic.success) equals that computed expected value; this keeps the test robust to token changes while still validating ink().src/tui/theme/ink-adapter.tsx (1)
15-22:resolveTheme(themeName)creates a new object on every render, causing unnecessary downstream re-renders.When
themeNameis supplied,resolveThemereturns a fresh object each timeThemeProviderrenders. React's context comparison is by reference (Object.is), so every re-render ofThemeProviderwill cascade to alluseTheme()consumers — even whenthemeNamehasn't changed. ThegetTheme()path avoids this because it returns the cached singleton.♻️ Proposed fix
+import { createContext, useContext, useMemo } from 'react'; -import { createContext, useContext } from 'react'; export function ThemeProvider({ children, themeName }: ThemeProviderProps): ReactElement { - const resolved = themeName !== undefined ? resolveTheme(themeName) : getTheme(); + const resolved = useMemo( + () => themeName !== undefined ? resolveTheme(themeName) : getTheme(), + [themeName], + ); return ( <ThemeContext.Provider value={resolved}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/ink-adapter.tsx` around lines 15 - 22, The ThemeProvider currently calls resolveTheme(themeName) on every render which returns a new object reference and forces all ThemeContext consumers to re-render; change ThemeProvider to memoize the resolved value (e.g., useMemo or an internal cache) so that when themeName is unchanged you return the same object reference as getTheme() provides; specifically, wrap the resolveTheme(themeName) call in a memoization keyed by themeName (keeping the getTheme() branch as-is) so ThemeContext.Provider receives a stable value and consumers (useTheme()) won't re-render unnecessarily.src/tui/theme/chalk-adapter.ts (1)
15-24: Missing exhaustive check forTextModifierin switch statement.If a new modifier is added to the
TextModifiertype (e.g.,'underline','italic'), this switch will silently ignore it. Consider adding a default case with a compile-time exhaustiveness check:♻️ Add exhaustive check
case 'inverse': c = c.inverse; break; + default: { + const _exhaustive: never = mod; + void _exhaustive; + } }This turns forgotten modifier cases into a compile-time error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/chalk-adapter.ts` around lines 15 - 24, The switch over token modifiers (token.modifiers and the switch handling cases 'bold','dim','strikethrough','inverse') is non-exhaustive for the TextModifier union; add a default branch that performs a compile-time exhaustiveness check by calling a helper like assertUnreachable(x: never): never with the unknown modifier so new variants cause a TypeScript error, and implement/assertUnreachable near this module (or import it) to throw at runtime and accept only never at compile time.src/tui/theme/__tests__/chalk-adapter.test.ts (2)
57-67: Status keys list may drift from theStatusKeytype.The hardcoded list of status keys could become stale if new statuses are added to
StatusKey. Consider importing the type or deriving the keys programmatically from the theme'sstatusmap to keep this in sync.#!/bin/bash # Check what StatusKey is defined as rg -n 'StatusKey' --type=ts -g 'src/tui/theme/tokens.ts' -A 10🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/__tests__/chalk-adapter.test.ts` around lines 57 - 67, The test hardcodes status keys which can drift from the StatusKey type; update the test to derive keys programmatically (instead of the hardcoded array) by importing the theme status map or the StatusKey type and generating the key list (e.g. Object.keys(status) or using keyof typeof StatusKey) and then iterating those keys when calling styledStatus to assert each key is rendered; locate the test referencing styledStatus and replace the static keys array with the programmatically derived keys from the theme's status map or StatusKey type.
15-18:process.envreassignment is fragile — consider restoring individual keys instead.Replacing
process.envwith a spread copy (process.env = { ...origEnv }) detaches it from the native binding. While this works in vitest's isolated workers, it can cause subtle issues if any code later caches a reference to the originalprocess.envobject. A safer approach is to restore/delete individual keys:♻️ Safer env restoration
afterEach(() => { - process.env = { ...origEnv }; + // Restore changed keys + for (const key of Object.keys(process.env)) { + if (!(key in origEnv)) delete process.env[key]; + } + Object.assign(process.env, origEnv); _resetThemeForTesting(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/__tests__/chalk-adapter.test.ts` around lines 15 - 18, In afterEach in the chalk-adapter.test (the block using origEnv and _resetThemeForTesting), avoid reassigning process.env; instead restore individual environment keys by iterating over Object.keys(process.env) and deleting any keys not present in origEnv, then iterate over Object.entries(origEnv) to set/backfill each key to its original value; keep calling _resetThemeForTesting() as before. This preserves the original process.env object reference while restoring state and prevents breaking code that cached the original env reference.src/coordinator-daemon.ts (1)
34-35: Manually constructing aTokenValuedrops existing modifiers from the success token.Line 35 creates
{ hex: t.semantic.success.hex, modifiers: ['bold'] }, which discards any modifiers already defined ont.semantic.success(e.g., if the preset defines success as dim or with other modifiers). Spread the existing token and merge modifiers:♻️ Preserve existing modifiers
- console.log(styled({ hex: t.semantic.success.hex, modifiers: ['bold'] }, 'XYPH Coordinator Daemon starting...')); + console.log(styled({ ...t.semantic.success, modifiers: [...(t.semantic.success.modifiers ?? []), 'bold'] }, 'XYPH Coordinator Daemon starting...'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coordinator-daemon.ts` around lines 34 - 35, The code creates a new TokenValue manually and loses existing modifiers on the success token; instead of constructing { hex: t.semantic.success.hex, modifiers: ['bold'] }, spread the existing token from t.semantic.success and merge its modifiers with ['bold'] so existing modifiers are preserved (e.g., use getTheme() -> t, and update the TokenValue passed to styled for the success token while combining existing modifiers and deduplicating if needed).src/tui/Dashboard.tsx (1)
45-71:loadLogis never cleared between refreshes.When the user presses 'r' to refresh (Line 100-102),
refresh()is called withoutshowLoading, soonProgressis undefined andloadLogis neither cleared nor updated. However, if the user navigates back to the landing screen (which showsloadLog), stale log entries from the initial load would still be visible. This is likely fine sinceshowLandingis one-way (set tofalseon any key and never set back totrue), but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/Dashboard.tsx` around lines 45 - 71, The refresh callback leaves previous loadLog entries intact when called without showLoading; update the refresh function (the useCallback named refresh) to clear the load log at the start of the operation by calling setLoadLog([]) (or conditionally when !showLoading) before invoking service.getSnapshot so stale progress messages are not shown on subsequent navigations; keep the existing requestCounter, onProgress, setLoading, setSnapshot, setError, and finally logic intact.src/infrastructure/adapters/WarpDashboardAdapter.ts (1)
91-103: Batch prop fetch lacks per-node error isolation.If any single
graph.getNodeProps(id)rejects, the entirePromise.allfails andfetchSnapshotthrows — no partial results, no indication which node caused it. The sequential version had the same behavior, so this isn't a regression, but now that you're explicitly designing for batch I/O, consider whetherPromise.allSettledwould be more resilient (skip failed nodes, log warnings).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/adapters/WarpDashboardAdapter.ts` around lines 91 - 103, The batch fetch using Promise.all over nodeIds can fail completely if any graph.getNodeProps(id) rejects; change the implementation in WarpDashboardAdapter (within the fetchSnapshot flow that builds propsCache) to use Promise.allSettled for nodeIds.map(...), then iterate results: for fulfilled results add the [id, props] to propsCache, for rejected results log a warning including the node id and error (so you can continue with partial results) and skip setting that id; keep the rest of the logic unchanged so consumers get available node props instead of an all-or-nothing failure.src/tui/theme/resolve.ts (1)
34-38: Consider narrowing theinkStatusparameter type.The
statusparameter is typed asstringin the interface (Line 19) and cast toStatusKeyat Line 35. This is flexible for callers, but the silentas StatusKeycast means a typo like"DOEN"would silently fall through to theUNKNOWNfallback with no indication of a problem. This is probably fine given the fallback is safe and theThemeinterface guaranteesUNKNOWNexists, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/resolve.ts` around lines 34 - 38, The inkStatus function accepts a generic string and casts it to StatusKey, which can silently mask typos; update the function signature (and the interface declaration where it's defined) to accept StatusKey instead of string so callers must pass a valid key, or alternatively add a runtime validation against theme.status keys before casting and log or throw if the value is invalid; refer to inkStatus, StatusKey, theme.status and the UNKNOWN fallback when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 5: Add a missing comparison link entry for the tag "[1.0.0-alpha.6]" in
the comparison links block and update the "[Unreleased]" comparison range from
"v1.0.0-alpha.5...HEAD" to "v1.0.0-alpha.6...HEAD"; specifically, locate the
links section that defines "[Unreleased]" and the per-release comparison links,
add a new link target for "[1.0.0-alpha.6]" pointing to the GitHub compare URL
for "v1.0.0-alpha.5...v1.0.0-alpha.6" (or the appropriate previous tag range),
and change the "[Unreleased]" target to "v1.0.0-alpha.6...HEAD" so all
comparison links are present and consistent.
In `@docs/canonical/STYLE_GUIDE.md`:
- Around line 242-247: Update the CLI header example that still reads "WARP
Status — Roadmap" to the new brand by replacing that literal string with "XYPH
Status — Roadmap" in the example block (and search for any other occurrences of
the "WARP" header in the same section 7.3 example to update them consistently).
- Line 50: The docs contain a stale reference to the removed status-colors.ts
file; find the sentence mentioning "status-colors.ts" (the claim that colors are
defined there and used by TUI and CLI) and replace it with a brief statement
that status colors are now provided by the shared theme tokens (used by both the
TUI's color prop and the CLI's chalk usage), removing the removed-file claim and
updating wording to reference the theme token names instead of the deleted file.
In `@docs/canonical/VISION_NORTH_STAR.md`:
- Line 38: The document uses the unexpanded acronym "JIT" in the runtime line
("**Runtime:** JIT (SWS, promotion, collapse, receipts)"); expand and define it
inline and, if it has XYPH-specific semantics, add a brief parenthetical
definition or a one-sentence sentence after that line describing the intended
meaning in the causal/graph model (e.g., "JIT (Just‑In‑Time — [XYPH-specific
semantics summary])"), so readers and agents see both the conventional expansion
and the project-specific interpretation.
In `@src/infrastructure/adapters/WarpDashboardAdapter.ts`:
- Around line 63-67: Update the progress message string used in
WarpDashboardAdapter where log('Opening WARP graph…') is called (around
graphHolder.getGraph and graph.syncCoverage) to match the PR's copy change —
replace it with "Loading project graph snapshot…" so the message aligns with the
renamed branding; keep the graph.syncCoverage() message unless other copy
changes require it.
In `@src/tui/render-status.ts`:
- Line 188: Update the user-facing header string in render-status.ts where
snapshotHeader is called with 'All WARP Nodes' to use the new naming convention
(e.g., 'All XYPH Nodes' or the agreed project label) so it matches other headers
like 'XYPH Roadmap'; locate the call to snapshotHeader (the line constructing
lines.push(snapshotHeader('All WARP Nodes', `${total} node(s) total`,
'success'))), replace the literal, and run a quick grep over src/tui/** to
ensure no other "WARP" strings remain in UI text.
---
Duplicate comments:
In `@src/domain/services/SovereigntyService.ts`:
- Line 2: The SovereigntyService currently imports getTheme and styled from the
TUI package (symbols: getTheme, styled), which violates the same-domain → TUI
import rule; change the code to use the domain-level theme abstraction instead
of importing TUI directly by either (a) consuming an injected
ThemeProvider/ThemeFacade passed into SovereigntyService (add a constructor
parameter or setter) and call its getTheme/styled equivalents, or (b) import the
theme helpers from the sanctioned domain-facing module referenced in the
TriageService comment (replace the direct '../../tui/theme/index.js' import with
the domain facade module), ensuring all references to getTheme and styled in
SovereigntyService use the new indirection.
---
Nitpick comments:
In `@docs/canonical/VISION_NORTH_STAR.md`:
- Line 24: The section heading "## 3. The Planning Compiler (and its Expansion)"
reads like a working note; change it to a canonical title that names the concept
rather than the author's intent. Replace the heading text (search for the exact
string "## 3. The Planning Compiler (and its Expansion)") with a cleaner title
such as "## 3. The Planning Compiler and Expansion" or "## 3. The Planning
Compiler" so the section header is authoritative and concept-focused.
In `@src/coordinator-daemon.ts`:
- Around line 34-35: The code creates a new TokenValue manually and loses
existing modifiers on the success token; instead of constructing { hex:
t.semantic.success.hex, modifiers: ['bold'] }, spread the existing token from
t.semantic.success and merge its modifiers with ['bold'] so existing modifiers
are preserved (e.g., use getTheme() -> t, and update the TokenValue passed to
styled for the success token while combining existing modifiers and
deduplicating if needed).
In `@src/domain/services/TriageService.ts`:
- Line 3: TriageService (and SovereigntyService) currently import getTheme and
styled from the TUI layer which breaks hexagonal boundaries; remove the direct
imports from '../../tui/theme/index.js' and instead either (A) move the
chalk-based utilities (getTheme, styled) into a neutral shared module (e.g.,
src/shared/theme or src/utils/theme) and update TriageService and
SovereigntyService to import getTheme/styled from that shared module, or (B)
refactor the services to accept a theme/logger abstraction via constructor or
method injection and have the TUI adapter supply the concrete getTheme/styled
implementation; update references to getTheme and styled in TriageService and
SovereigntyService accordingly.
In `@src/infrastructure/adapters/WarpDashboardAdapter.ts`:
- Around line 91-103: The batch fetch using Promise.all over nodeIds can fail
completely if any graph.getNodeProps(id) rejects; change the implementation in
WarpDashboardAdapter (within the fetchSnapshot flow that builds propsCache) to
use Promise.allSettled for nodeIds.map(...), then iterate results: for fulfilled
results add the [id, props] to propsCache, for rejected results log a warning
including the node id and error (so you can continue with partial results) and
skip setting that id; keep the rest of the logic unchanged so consumers get
available node props instead of an all-or-nothing failure.
In `@src/tui/Dashboard.tsx`:
- Around line 45-71: The refresh callback leaves previous loadLog entries intact
when called without showLoading; update the refresh function (the useCallback
named refresh) to clear the load log at the start of the operation by calling
setLoadLog([]) (or conditionally when !showLoading) before invoking
service.getSnapshot so stale progress messages are not shown on subsequent
navigations; keep the existing requestCounter, onProgress, setLoading,
setSnapshot, setError, and finally logic intact.
In `@src/tui/theme/__tests__/chalk-adapter.test.ts`:
- Around line 57-67: The test hardcodes status keys which can drift from the
StatusKey type; update the test to derive keys programmatically (instead of the
hardcoded array) by importing the theme status map or the StatusKey type and
generating the key list (e.g. Object.keys(status) or using keyof typeof
StatusKey) and then iterating those keys when calling styledStatus to assert
each key is rendered; locate the test referencing styledStatus and replace the
static keys array with the programmatically derived keys from the theme's status
map or StatusKey type.
- Around line 15-18: In afterEach in the chalk-adapter.test (the block using
origEnv and _resetThemeForTesting), avoid reassigning process.env; instead
restore individual environment keys by iterating over Object.keys(process.env)
and deleting any keys not present in origEnv, then iterate over
Object.entries(origEnv) to set/backfill each key to its original value; keep
calling _resetThemeForTesting() as before. This preserves the original
process.env object reference while restoring state and prevents breaking code
that cached the original env reference.
In `@src/tui/theme/__tests__/presets.test.ts`:
- Around line 5-9: Update the ALL_STATUS_KEYS declaration so the compiler
verifies it matches the StatusKey union: make the array a const tuple and use
the satisfies operator (e.g., "as const satisfies readonly StatusKey[]") so any
missing or extra keys are flagged; keep the identifier ALL_STATUS_KEYS and
ensure it still can be iterated where Theme (Record<StatusKey, TokenValue>) or
tests use it.
In `@src/tui/theme/__tests__/resolve.test.ts`:
- Line 34: Move the vi import to the top of the test file and remove the
redundant trailing declaration: relocate the existing vi import so it appears
before any use (it’s currently used in const warnSpy = vi.spyOn(...) in
resolve.test.ts) and delete the later import/declaration at the bottom of the
file so there’s a single, idiomatic import for vi.
- Around line 76-80: The test hardcodes '#00ff00' making it brittle; change the
assertion to derive the expected value from the theme token instead of a
literal. In the test that calls getTheme() and stores it in t, compute the
expected hex by using the same token under test (t.theme.semantic.success) —
e.g., pass that token into the ink conversion helper used in production or
otherwise reference its resolved value — and assert
t.ink(t.theme.semantic.success) equals that computed expected value; this keeps
the test robust to token changes while still validating ink().
In `@src/tui/theme/chalk-adapter.ts`:
- Around line 15-24: The switch over token modifiers (token.modifiers and the
switch handling cases 'bold','dim','strikethrough','inverse') is non-exhaustive
for the TextModifier union; add a default branch that performs a compile-time
exhaustiveness check by calling a helper like assertUnreachable(x: never): never
with the unknown modifier so new variants cause a TypeScript error, and
implement/assertUnreachable near this module (or import it) to throw at runtime
and accept only never at compile time.
In `@src/tui/theme/gradient.ts`:
- Around line 18-19: The division (const local = (t - a.pos) / (b.pos - a.pos))
can produce NaN when two adjacent stops have the same pos; update the block in
the gradient interpolation function to guard against b.pos === a.pos (compare
a.pos and b.pos) and handle it by treating local as 0 (or another defined
fallback) or by skipping interpolation for that pair, ensuring subsequent RGB
math uses a valid numeric local value; refer to the variables a, b, t and the
local interpolation calculation to locate and fix the issue.
In `@src/tui/theme/ink-adapter.tsx`:
- Around line 15-22: The ThemeProvider currently calls resolveTheme(themeName)
on every render which returns a new object reference and forces all ThemeContext
consumers to re-render; change ThemeProvider to memoize the resolved value
(e.g., useMemo or an internal cache) so that when themeName is unchanged you
return the same object reference as getTheme() provides; specifically, wrap the
resolveTheme(themeName) call in a memoization keyed by themeName (keeping the
getTheme() branch as-is) so ThemeContext.Provider receives a stable value and
consumers (useTheme()) won't re-render unnecessarily.
In `@src/tui/theme/resolve.ts`:
- Around line 34-38: The inkStatus function accepts a generic string and casts
it to StatusKey, which can silently mask typos; update the function signature
(and the interface declaration where it's defined) to accept StatusKey instead
of string so callers must pass a valid key, or alternatively add a runtime
validation against theme.status keys before casting and log or throw if the
value is invalid; refer to inkStatus, StatusKey, theme.status and the UNKNOWN
fallback when making the change.
In `@xyph-actuator.ts`:
- Line 6: xyph-actuator.ts imports UI-specific utilities getTheme and styled
from src/tui, coupling the CLI entrypoint to the TUI module; move these
utilities to a neutral shared location (e.g., src/shared or src/utils) and
update references. Specifically, create a new module exporting getTheme and
styled, update xyph-actuator.ts to import them from that shared module, and also
update TriageService.ts to import the same shared exports so both files
reference the neutral location instead of src/tui.
Comment on lines
63
to
67
| log('Opening WARP graph…'); | ||
| const graph = await this.graphHolder.getGraph(); | ||
|
|
||
| log('Syncing coverage…'); | ||
| await graph.syncCoverage(); |
There was a problem hiding this comment.
Progress messages reference "WARP" while the PR renames to "XYPH" elsewhere.
Line 63 logs 'Opening WARP graph…' and Line 66 logs 'Syncing coverage…'. The PR objectives explicitly rename "Loading WARP graph snapshot…" → "Loading project graph snapshot…" for brand consistency. The internal log('Opening WARP graph…') still references "WARP" — this may be intentional (it's a technical internal name for the graph engine), but confirm it aligns with the copywriting goals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/infrastructure/adapters/WarpDashboardAdapter.ts` around lines 63 - 67,
Update the progress message string used in WarpDashboardAdapter where
log('Opening WARP graph…') is called (around graphHolder.getGraph and
graph.syncCoverage) to match the PR's copy change — replace it with "Loading
project graph snapshot…" so the message aligns with the renamed branding; keep
the graph.syncCoverage() message unless other copy changes require it.
- Remove TUI dependency from domain services (TriageService, SovereigntyService) - Fix WARP branding in progress log, styled() spacing in actuator - Remove redundant syncCoverage/materialize from WarpGraphHolder.initGraph() - Clear loadLog on snapshot load, fix inline token in coordinator-daemon - Re-indent bar-demo.ts, consolidate CHANGELOG under alpha.6
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/services/TriageService.ts (1)
19-25:⚠️ Potential issue | 🟡 MinorLog fires before the guard — emits a misleading "Linking" entry for non-existent quests.
Line 20 is emitted unconditionally, so a caller supplying an invalid
taskIdwill produce a[Triage] Linking quest … to intent …log line immediately followed by a thrown error. Moving the log after the null check gives accurate signal.🛠️ Proposed fix
public async linkIntent(taskId: string, contextHash: string): Promise<void> { - console.log(`[Triage] Linking quest ${taskId} to intent ${contextHash}`); - const quest = await this.roadmap.getQuest(taskId); if (!quest) { throw new Error(`Quest ${taskId} not found for triage`); } + console.log(`[Triage] Linking quest ${taskId} to intent ${contextHash}`); + const enrichedQuest = new Quest({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/TriageService.ts` around lines 19 - 25, The log in linkIntent currently runs before verifying the quest exists, producing misleading "[Triage] Linking..." entries for invalid taskIds; move the console.log from the start of TriageService.linkIntent to after the await this.roadmap.getQuest(taskId) null check so it only logs when quest is found (use the existing quest variable and keep the same message content), leaving the subsequent throw for the missing quest intact.
🧹 Nitpick comments (3)
src/infrastructure/adapters/WarpDashboardAdapter.ts (1)
167-181: Consider using theNeighborEntrytype forneighborsCache.Line 170 uses an inline
Array<{ label: string; nodeId: string }>which duplicates the shape ofNeighborEntryfromisNeighborEntry.ts. Using the named type would improve readability and keep the two in sync if the shape ever changes.♻️ Suggested change
+import { toNeighborEntries, type NeighborEntry } from '../helpers/isNeighborEntry.js'; -import { toNeighborEntries } from '../helpers/isNeighborEntry.js'; - const neighborsCache = new Map<string, Array<{ label: string; nodeId: string }>>(); + const neighborsCache = new Map<string, NeighborEntry[]>();And update the
buildSubmissionDataparameter type at Line 332 similarly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/adapters/WarpDashboardAdapter.ts` around lines 167 - 181, Replace the inline type Array<{ label: string; nodeId: string }> used for neighborsCache with the shared NeighborEntry type from isNeighborEntry.ts (i.e., declare neighborsCache as Map<string, NeighborEntry[]>), and likewise update the parameter type for buildSubmissionData to use NeighborEntry[] instead of the inline shape so both places reference the canonical NeighborEntry type (ensure you import NeighborEntry where needed).xyph-actuator.ts (1)
62-62: Consider destructuring the theme once per handler to reduce verbosity.Every handler repeats the full
styled(getTheme().theme.semantic.error, ...)/.success/.mutedchain. Destructuring at the top of each action reduces noise and makes future token renames a single-line change.♻️ Example (apply to each action handler)
.action(async (id: string, opts: { title: string; campaign: string; hours?: number; intent?: string }) => { try { + const { error, success, muted, warning } = getTheme().theme.semantic; // Validate all inputs before any async graph I/O if (!id.startsWith('task:')) { - console.error(styled(getTheme().theme.semantic.error, `[ERROR] Quest ID must start with 'task:' prefix, got: '${id}'`)); + console.error(styled(error, `[ERROR] Quest ID must start with 'task:' prefix, got: '${id}'`)); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xyph-actuator.ts` at line 62, Handlers repeatedly call styled(getTheme().theme.semantic.error / .success / .muted) — destructure the theme once at the top of each action handler to reduce repetition; e.g., retrieve the semantic tokens (or specific tokens like error, success, muted) from getTheme().theme into local constants inside the handler, then replace occurrences of styled(getTheme().theme.semantic.* , ...) with styled(error, ...), styled(success, ...), etc., so future renames are a single-line change and the handler body is less verbose.src/tui/theme/__tests__/resolve.test.ts (1)
11-13: Prefervi.stubEnv/vi.unstubAllEnvs()over replacingprocess.env.
process.env = { ...origEnv }replaces the object reference rather than patching the live object, so any module or Node.js binding that already holds a reference to the originalprocess.envwon't observe the restoration. Vitest shipsvi.stubEnv/vi.unstubAllEnvs()specifically for this purpose.♻️ Proposed refactor using Vitest-native env helpers
- const origEnv = { ...process.env }; - beforeEach(() => { _resetThemeForTesting(); }); afterEach(() => { - process.env = { ...origEnv }; + vi.unstubAllEnvs(); _resetThemeForTesting(); });Then replace each
process.env['KEY'] = 'value'/delete process.env['KEY']call withvi.stubEnv('KEY', 'value')/vi.stubEnv('KEY', undefined)(or simply omit thedeletecalls sinceunstubAllEnvshandles the teardown).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/__tests__/resolve.test.ts` around lines 11 - 13, The test teardown currently restores env by reassigning process.env using origEnv in the afterEach block (see afterEach, origEnv, and _resetThemeForTesting), which should be replaced by Vitest native env stubs: instead of saving and reassigning process.env, use vi.stubEnv(...) whenever tests set or delete env keys, and call vi.unstubAllEnvs() in the afterEach along with _resetThemeForTesting to restore environment state; update any tests that do process.env['KEY']='value' or delete process.env['KEY'] to use vi.stubEnv('KEY','value') or vi.stubEnv('KEY', undefined)/omit deletion so unstubAllEnvs cleans up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tui/theme/__tests__/resolve.test.ts`:
- Around line 34-38: The test creates a console.warn spy (warnSpy) and restores
it at the end of the test, but if an assertion fails the restore is never
called; update the test to always restore the spy by moving
warnSpy.mockRestore() into a shared afterEach hook or by wrapping the assertions
in a try/finally that calls warnSpy.mockRestore() (identify the spy as warnSpy
and the function under test as getTheme() to locate the test block).
- Around line 76-80: The test hardcodes '#00ff00' which couples it to the token
value; update the assertion in the test that calls ink() so it derives the
expected hex from the theme object instead of a literal string — for example,
call getTheme() (as you already do), then
expect(t.ink(t.theme.semantic.success)).toBe(t.theme.semantic.success.hex) so
the test uses the token value from the theme; update the assertion in the it
block that references ink() and t.theme.semantic.success accordingly.
---
Outside diff comments:
In `@src/domain/services/TriageService.ts`:
- Around line 19-25: The log in linkIntent currently runs before verifying the
quest exists, producing misleading "[Triage] Linking..." entries for invalid
taskIds; move the console.log from the start of TriageService.linkIntent to
after the await this.roadmap.getQuest(taskId) null check so it only logs when
quest is found (use the existing quest variable and keep the same message
content), leaving the subsequent throw for the missing quest intact.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 426-427: Add an empty release section header for the existing
[Unreleased] comparison link to suppress markdownlint MD053 and prepare for
future changes: insert a top-level markdown header "## [Unreleased]" (an empty
section with no entries) above the existing "## [1.0.0-alpha.6]" section so the
reference label [Unreleased] used in the link block at the bottom matches a
defined section; ensure the header text exactly matches the bracketed reference
"[Unreleased]" and leave it empty for now.
---
Nitpick comments:
In `@src/infrastructure/adapters/WarpDashboardAdapter.ts`:
- Around line 167-181: Replace the inline type Array<{ label: string; nodeId:
string }> used for neighborsCache with the shared NeighborEntry type from
isNeighborEntry.ts (i.e., declare neighborsCache as Map<string,
NeighborEntry[]>), and likewise update the parameter type for
buildSubmissionData to use NeighborEntry[] instead of the inline shape so both
places reference the canonical NeighborEntry type (ensure you import
NeighborEntry where needed).
In `@src/tui/theme/__tests__/resolve.test.ts`:
- Around line 11-13: The test teardown currently restores env by reassigning
process.env using origEnv in the afterEach block (see afterEach, origEnv, and
_resetThemeForTesting), which should be replaced by Vitest native env stubs:
instead of saving and reassigning process.env, use vi.stubEnv(...) whenever
tests set or delete env keys, and call vi.unstubAllEnvs() in the afterEach along
with _resetThemeForTesting to restore environment state; update any tests that
do process.env['KEY']='value' or delete process.env['KEY'] to use
vi.stubEnv('KEY','value') or vi.stubEnv('KEY', undefined)/omit deletion so
unstubAllEnvs cleans up.
In `@xyph-actuator.ts`:
- Line 62: Handlers repeatedly call styled(getTheme().theme.semantic.error /
.success / .muted) — destructure the theme once at the top of each action
handler to reduce repetition; e.g., retrieve the semantic tokens (or specific
tokens like error, success, muted) from getTheme().theme into local constants
inside the handler, then replace occurrences of
styled(getTheme().theme.semantic.* , ...) with styled(error, ...),
styled(success, ...), etc., so future renames are a single-line change and the
handler body is less verbose.
Branding: - Fix stale status-colors.ts reference in STYLE_GUIDE - Update CLI header example WARP → XYPH in STYLE_GUIDE - Rename "All WARP Nodes" → "All XYPH Nodes" in render-status - Expand JIT acronym in VISION_NORTH_STAR - Clean up section heading in VISION_NORTH_STAR Code quality: - Move TriageService log after null guard - Add NaN guard for duplicate gradient stops - Add useMemo to ThemeProvider for stable context value - Add exhaustive switch in chalk-adapter TextModifier - Use Promise.allSettled for resilient batch prop fetch - Use NeighborEntry type instead of inline shape Test hygiene: - Wrap warnSpy in try/finally for safe cleanup - Derive ink() expected hex from theme token, not hardcoded value - Use vi.unstubAllEnvs() instead of process.env reassignment - Derive status keys from theme in chalk-adapter tests - Add satisfies assertion to presets test ALL_STATUS_KEYS
Owner
Author
|
✅ Round 2 feedback addressed in commit 85cd4c2 — 16 issues resolved across 11 files. Inline comments resolved:
Nit comments resolved:
Intentionally deferred:
Build ✅ · 338 tests ✅ · Lint clean ✅ |
Owner
Author
|
@coderabbitai review please 🥕 |
|
Build ✅ · 338/338 tests ✅ · Lint clean ✅ — excellent hygiene. I'll review the changes now! 🥕 ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
docs/canonical/VISION_NORTH_STAR.md (1)
56-59: "Temporary names" label conflicts with bold-term formatting.Labelling items as "temporary names" while bolding them as stable vocabulary creates a documentation tension. If these names are genuinely provisional, consider an admonition or a parenthetical "(subject to rename)" per item; if they are stable enough for this version, drop the "temporary names" qualifier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/canonical/VISION_NORTH_STAR.md` around lines 56 - 59, The heading "Early scaffolding (temporary names):" conflicts with the bolded vocabulary; either remove the "(temporary names)" qualifier to treat **Submission / Patchset / Review**, **Merge**, and **Automation** as stable terms, or keep the qualifier but mark each bullet with a parenthetical note like "(subject to rename)" or add an admonition above the list explaining these are provisional names; update the text around the three items (Submission / Patchset / Review, Merge, Automation) accordingly so the intent is unambiguous.src/tui/render-status.ts (1)
6-8:colorStatusis now a no-op wrapper — consider inliningstyledStatus.After the theme migration,
colorStatussimply delegates tostyledStatuswith no added logic. Removing it and callingstyledStatus(...)directly at the ~9 call sites eliminates a level of indirection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/render-status.ts` around lines 6 - 8, colorStatus is now a no-op wrapper around styledStatus; remove the indirection by deleting the colorStatus function and replacing all calls to colorStatus(...) with styledStatus(...). Update any exports or imports that reference colorStatus (remove or re-export styledStatus if needed) and run a quick search for colorStatus to ensure no call sites remain; keep styledStatus name and behavior unchanged.src/tui/theme/__tests__/resolve.test.ts (1)
14-94: Prefervi.stubEnvover directprocess.envmutations for propervi.unstubAllEnvs()cleanup.Every test in the suite mutates
process.envdirectly (via assignment ordelete).vi.unstubAllEnvsonly restores values that were changed withvi.stubEnv; directprocess.envassignments bypass this mechanism. The current tests are safe because each one explicitly resets whatever it needs at its own start and they run sequentially — but this is fragile. Addingconcurrent, reordering, or inserting a new test that omits the per-test setup would silently pollute downstream tests.
vi.stubEnvsupportsundefinedas the value, covering thedelete process.env[...]cases too.♻️ Proposed refactor (illustrative for one test; apply the same pattern throughout)
- it('selects teal-orange-pink via XYPH_THEME', () => { - process.env['XYPH_THEME'] = 'teal-orange-pink'; - delete process.env['NO_COLOR']; + it('selects teal-orange-pink via XYPH_THEME', () => { + vi.stubEnv('XYPH_THEME', 'teal-orange-pink'); + vi.stubEnv('NO_COLOR', undefined); const t = getTheme(); expect(t.theme.name).toBe('teal-orange-pink'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/theme/__tests__/resolve.test.ts` around lines 14 - 94, Replace all direct process.env assignments/deletions in these tests with vi.stubEnv calls (use vi.stubEnv('NO_COLOR', undefined) for deletes) and add a cleanup afterEach hook calling vi.unstubAllEnvs() so environment changes are tracked and reset; update tests referencing getTheme(), isNoColor(), _resetThemeForTesting(), and ink/inkStatus expectations to use vi.stubEnv instead of process.env[...] = ... or delete process.env[...] throughout the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/canonical/STYLE_GUIDE.md`:
- Around line 25-335: Add language identifiers "text" to each of the nine fenced
code blocks that currently lack one (the blocks around the Cyan→Magenta
gradient, Progress Bar anatomy, examples, CLI boxen snippet, truncation
examples, and other ASCII/terminal examples) so they read ```text; and on the
line containing the inline code spans showing indentation (`│ ` and ` `) add
an inline markdownlint suppression (// markdownlint-disable-line MD038 or <!--
markdownlint-disable-line MD038 -->) to silence MD038 while preserving the
intentional spaces.
- Around line 325-327: The status-line example uses the outdated prefix "WARP"
in the string starting with "/// WARP [tick: 47 (a1b2c3d) ← 46 (e4f5g6h) | me:
12 | writers: 3]"; update that literal so the prefix label is "XYPH" instead of
"WARP" to match the branding rename (edit the example string in the status-line
section 9.4 where the "/// WARP [...]" token appears).
- Line 11: Update the stale phrase "WARP graph" in the philosophy sentence to
use the new branding: replace "the WARP graph is always the single source of
truth" with either "the XYPH graph is always the single source of truth" or "the
project graph is always the single source of truth" to match the PR's WARP →
XYPH rename (search for the exact token "WARP graph" in STYLE_GUIDE.md and
update it).
In `@docs/canonical/VISION_NORTH_STAR.md`:
- Around line 66-67: The End State lines use the bold term "seek" and the
informal phrase "not vibes": define or cross-reference "seek" (e.g., add a short
parenthetical or link to the SWS/time-travel model that explains "seek back to
any prior tick" and its semantics) and replace the colloquial "not vibes" with a
formal alternative such as "— not probabilistic attestation" or remove the
qualifier; update the two lines so they read something like: "You can seek (see
SWS/time-travel model) the system to any prior tick, fork reality, and replay
forward." and "Trust, tests, and deployment safety are cryptographically
provable — not probabilistic attestation."
In `@src/infrastructure/adapters/WarpDashboardAdapter.ts`:
- Line 105: The warning logs in WarpDashboardAdapter currently interpolate
PromiseRejectedResult.reason (typed unknown) directly, which can produce
"[object Object]"; update the rejection handling where console.warn is called to
safely stringify the reason: detect if result.reason is an Error (use its
message and/or stack) or otherwise coerce with String(...) or
JSON.stringify(...) and include that in the log; locate the console.warn line in
WarpDashboardAdapter (the method handling Promise.allSettled results) and
replace the direct interpolation with the safe stringified reason.
- Around line 177-186: The neighbor fetch uses Promise.all which will reject the
whole batch on a single graph.neighbors error; change it to Promise.allSettled
and only write fulfilled results to neighborsCache (mirroring the props batch
pattern). Specifically, replace the Promise.all(neighborsNeeded.map(...)) call
with Promise.allSettled(neighborsNeeded.map(...)), then iterate the settled
results in order, and for each result with status === 'fulfilled' extract the
returned [id, neighbors] tuple (produced by the inner async that calls
graph.neighbors and toNeighborEntries) and call neighborsCache.set(id,
neighbors); skip entries with status === 'rejected' so transient neighbor errors
do not abort fetchSnapshot or overwrite cachedSnapshot.
---
Duplicate comments:
In `@docs/canonical/VISION_NORTH_STAR.md`:
- Line 38: This review is a duplicate noting that line "JIT — Just-In-Time graph
execution" has already been updated; no code change is required—mark the comment
as resolved and leave the existing text ("JIT — Just-In-Time graph execution")
as-is in VISION_NORTH_STAR.md.
---
Nitpick comments:
In `@docs/canonical/VISION_NORTH_STAR.md`:
- Around line 56-59: The heading "Early scaffolding (temporary names):"
conflicts with the bolded vocabulary; either remove the "(temporary names)"
qualifier to treat **Submission / Patchset / Review**, **Merge**, and
**Automation** as stable terms, or keep the qualifier but mark each bullet with
a parenthetical note like "(subject to rename)" or add an admonition above the
list explaining these are provisional names; update the text around the three
items (Submission / Patchset / Review, Merge, Automation) accordingly so the
intent is unambiguous.
In `@src/tui/render-status.ts`:
- Around line 6-8: colorStatus is now a no-op wrapper around styledStatus;
remove the indirection by deleting the colorStatus function and replacing all
calls to colorStatus(...) with styledStatus(...). Update any exports or imports
that reference colorStatus (remove or re-export styledStatus if needed) and run
a quick search for colorStatus to ensure no call sites remain; keep styledStatus
name and behavior unchanged.
In `@src/tui/theme/__tests__/resolve.test.ts`:
- Around line 14-94: Replace all direct process.env assignments/deletions in
these tests with vi.stubEnv calls (use vi.stubEnv('NO_COLOR', undefined) for
deletes) and add a cleanup afterEach hook calling vi.unstubAllEnvs() so
environment changes are tracked and reset; update tests referencing getTheme(),
isNoColor(), _resetThemeForTesting(), and ink/inkStatus expectations to use
vi.stubEnv instead of process.env[...] = ... or delete process.env[...]
throughout the file.
Comment on lines
+25
to
+335
| ``` | ||
| Cyan ████████████████████████████████████████ Magenta | ||
| (0,255,255) ↔ (255,0,255) | ||
| ``` | ||
|
|
||
| **Interpolation:** Linear RGB. For a bar of length `N`, position `i` maps to: | ||
| ``` | ||
| t = i / (N - 1) | ||
| r = round(0 + 255 * t) // 0 → 255 | ||
| g = round(255 - 255 * t) // 255 → 0 | ||
| b = 255 // constant | ||
| ``` | ||
|
|
||
| **Positional mapping:** Color is determined by the slot's position in the bar, not by fill amount. A 10% bar shows cyan fills on the left; a 90% bar reveals magenta fills on the right. The gradient is the bar's identity, not a heat indicator. | ||
|
|
||
| **Application:** | ||
| - Progress bars (primary use) | ||
| - Potential future use: header accents, wordmark treatment | ||
|
|
||
| --- | ||
|
|
||
| ## 3. Color Palette | ||
|
|
||
| ### 3.1 Status Colors | ||
|
|
||
| Defined in `src/tui/theme/tokens.ts` (status field). Consumed via `t.inkStatus(status)` in TUI and `styled(token, text)` / chalk in CLI. | ||
|
|
||
| | Status | Color | Hex/Note | | ||
| |--------------------|-----------|-----------------------------| | ||
| | `DONE` | green | Completed, sealed | | ||
| | `IN_PROGRESS` | cyan | Active work | | ||
| | `BACKLOG` | gray | Waiting, unstarted | | ||
| | `BLOCKED` | red | Cannot proceed | | ||
| | `PLANNED` | yellow | Scheduled but not started | | ||
| | `INBOX` | magenta | Untriaged suggestion | | ||
| | `GRAVEYARD` | gray | Rejected (+ strikethrough in CLI) | | ||
| | `PENDING` | yellow | Awaiting decision | | ||
| | `APPROVED` | green | Review passed | | ||
| | `REJECTED` | red | Review failed | | ||
| | `UNKNOWN` | white | Fallback | | ||
|
|
||
| **Submission-specific statuses** (CLI only, in `render-status.ts`): | ||
|
|
||
| | Status | Color | | ||
| |----------------------|-----------| | ||
| | `OPEN` | cyan | | ||
| | `CHANGES_REQUESTED` | yellow | | ||
| | `MERGED` | green | | ||
| | `CLOSED` | gray/dim | | ||
|
|
||
| ### 3.2 Semantic Colors | ||
|
|
||
| | Role | Color | Usage | | ||
| |-----------|---------|------------------------------------| | ||
| | Primary | cyan | Selection, active, IDs, accents | | ||
| | Success | green | Sealed, approved, `[OK]` | | ||
| | Error | red | Failures, blocked, `[ERROR]` | | ||
| | Warning | yellow | Pending, caution, `[*]`, `[WARN]` | | ||
| | Accent | magenta | Intents, inbox, lineage | | ||
| | Campaign | blue | Campaign headers | | ||
| | Secondary | dim | Metadata, IDs, detail lines | | ||
|
|
||
| ### 3.3 Track / Empty State | ||
|
|
||
| - **Track color:** dark gray `rgb(80,80,80)` — used for unfilled progress bar slots | ||
| - **Empty text:** dim gray — used for "no items" states, inactive panels | ||
|
|
||
| --- | ||
|
|
||
| ## 4. Progress Bar | ||
|
|
||
| > **Status: LOCKED** — this specification is final. | ||
|
|
||
| ### 4.1 Anatomy | ||
|
|
||
| ``` | ||
| 42% ████████████████████▒⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐ | ||
| ╰── fill (█) ──╯╰le╯╰───── track (⠐) ─────╯ | ||
| ``` | ||
|
|
||
| - **Fill character:** `█` (U+2588 FULL BLOCK) — colored with positional gradient | ||
| - **Leading edge:** fractional shading at the boundary between fill and track | ||
| - **Track character:** `⠐` (U+2810 BRAILLE PATTERN DOTS-5) — dark gray `rgb(80,80,80)` | ||
| - **Label:** percentage right-justified in a 4-character column, then a space, then the bar | ||
|
|
||
| ### 4.2 Leading Edge Thresholds | ||
|
|
||
| The fractional remainder at the fill boundary determines the edge character: | ||
|
|
||
| | Remainder | Character | Name | | ||
| |--------------|-----------|---------------| | ||
| | `< 0.25` | (track) | No partial | | ||
| | `< 0.50` | `░` | Light shade | | ||
| | `< 0.75` | `▒` | Medium shade | | ||
| | `≥ 0.75` | `▓` | Dark shade | | ||
|
|
||
| The edge character inherits the gradient color at that position. | ||
|
|
||
| ### 4.3 Examples | ||
|
|
||
| ``` | ||
| 3% █░⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐ | ||
| 42% █████████████████████⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐⠐ | ||
| 100% ██████████████████████████████████████████████████ | ||
| ``` | ||
|
|
||
| (In terminal: fills render in cyan→magenta gradient; track renders in dark gray.) | ||
|
|
||
| ### 4.4 Reference Implementation | ||
|
|
||
| See `scripts/bar-demo.ts` — a living demo that renders all track character variants with animation. Run with: | ||
|
|
||
| ```bash | ||
| npx tsx scripts/bar-demo.ts | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 5. Icons & Symbols | ||
|
|
||
| ### 5.1 Navigation | ||
|
|
||
| | Glyph | Usage | Color | | ||
| |-------|--------------------------------|---------| | ||
| | `▶` | Selected item / collapsed node | cyan | | ||
| | `▼` | Expanded node | cyan | | ||
|
|
||
| ### 5.2 Status Indicators | ||
|
|
||
| | Glyph | Meaning | Color | | ||
| |-------|----------------------------|---------| | ||
| | `✓` | Sealed / completed | green | | ||
| | `⊕` | Guild-sealed (signed) | green | | ||
| | `○` | Unsigned / pending | yellow | | ||
| | `⚠` | Warning / orphan | yellow or red | | ||
| | `↩` | Previously rejected | yellow | | ||
|
|
||
| ### 5.3 Tree Structure | ||
|
|
||
| | Glyph | Usage | | ||
| |--------|--------------------------------| | ||
| | `├─` | Branch (non-terminal child) | | ||
| | `└─` | Branch (terminal child) | | ||
| | `│` | Continuation line | | ||
| | `◆` | Intent node marker (magenta) | | ||
|
|
||
| Indentation: `│ ` (pipe + 2 spaces) for continuation, ` ` (3 spaces) after terminal `└─`. | ||
|
|
||
| ### 5.4 Miscellaneous | ||
|
|
||
| | Glyph | Usage | | ||
| |-------|--------------------------------| | ||
| | `—` | Empty / N/A | | ||
| | `…` | Truncation indicator | | ||
| | `←` | Tick progression (status line) | | ||
| | `─` | Horizontal rule / separator | | ||
|
|
||
| --- | ||
|
|
||
| ## 6. Borders & Containers | ||
|
|
||
| ### 6.1 CLI (boxen) | ||
|
|
||
| ```typescript | ||
| boxen(content, { | ||
| padding: { top: 0, bottom: 0, left: 1, right: 1 }, | ||
| borderStyle: 'single', | ||
| borderColor: 'cyan', | ||
| }) | ||
| ``` | ||
|
|
||
| Standard padding is `0` vertical, `1` horizontal. Border style is always `single`. | ||
|
|
||
| ### 6.2 TUI (Ink) | ||
|
|
||
| All Ink `<Box>` borders use `borderStyle="round"` (rounded corners). | ||
|
|
||
| ### 6.3 Border Color Assignments | ||
|
|
||
| | Color | Context | | ||
| |---------|------------------------------------------| | ||
| | cyan | Primary panels, detail views, help modal | | ||
| | magenta | Lineage view, inbox modals | | ||
| | yellow | Submissions view, warning modals | | ||
| | green | All-nodes view | | ||
| | gray | Empty states, inactive panels | | ||
| | red | Error modals | | ||
|
|
||
| --- | ||
|
|
||
| ## 7. CLI Output Patterns | ||
|
|
||
| ### 7.1 Message Prefixes | ||
|
|
||
| | Prefix | Color | Usage | | ||
| |-----------|--------|----------------------------------------| | ||
| | `[OK]` | green | Successful operation | | ||
| | `[ERROR]` | red | Operation failed | | ||
| | `[WARN]` | yellow | Non-fatal issue, advisory | | ||
| | `[FAIL]` | red | Expected operation that didn't succeed | | ||
| | `[*]` | yellow | In-progress activity | | ||
|
|
||
| ### 7.2 Detail Lines | ||
|
|
||
| Secondary information is rendered dim and indented 2 spaces: | ||
|
|
||
| ``` | ||
| [OK] Submission sub:abc123 created. | ||
| Patchset: patchset:abc123-1 | ||
| Branch: feat/my-feature | ||
| Quest: task:do-the-thing | ||
| ``` | ||
|
|
||
| ### 7.3 Headers | ||
|
|
||
| CLI headers use boxen with a bold label and dim detail line: | ||
|
|
||
| ``` | ||
| ┌──────────────────────────────────────────┐ | ||
| │ XYPH Status — Roadmap │ | ||
| │ 12 quests · 3 campaigns · tick 47 │ | ||
| └──────────────────────────────────────────┘ | ||
| ``` | ||
|
|
||
| ### 7.4 Tables | ||
|
|
||
| Tables use `cli-table3` with: | ||
| - Headers: `chalk.white()` (plain white, no background) | ||
| - Empty `style.head` and `style.border` arrays (disable default styling) | ||
| - Explicit `colWidths` for alignment | ||
|
|
||
| --- | ||
|
|
||
| ## 8. Text Handling | ||
|
|
||
| ### 8.1 Truncation | ||
|
|
||
| Long strings are truncated with an ellipsis character (`…`): | ||
|
|
||
| ```typescript | ||
| const trunc = (s: string, n: number): string => | ||
| s.length > n ? s.slice(0, n - 1) + '…' : s; | ||
| ``` | ||
|
|
||
| Common truncation widths: | ||
| - Quest IDs: 16–20 characters | ||
| - Titles: 30–40 characters | ||
| - IDs in tables: 24–26 characters | ||
|
|
||
| ### 8.2 Padding | ||
|
|
||
| Numbers right-align with `padStart`; IDs left-align with `padEnd`: | ||
|
|
||
| ```typescript | ||
| String(hours).padStart(3) // " 8" | ||
| quest.id.slice(0, 16).padEnd(18) // "task:do-thing " | ||
| ``` | ||
|
|
||
| ### 8.3 Text Modifiers | ||
|
|
||
| | Modifier | Usage | | ||
| |-----------------|--------------------------------| | ||
| | `bold` | Headers, emphasis | | ||
| | `dim` / `dimColor` | Secondary text, metadata | | ||
| | `strikethrough` | GRAVEYARD status only | | ||
|
|
||
| --- | ||
|
|
||
| ## 9. Layout | ||
|
|
||
| ### 9.1 TUI Chrome | ||
|
|
||
| ```typescript | ||
| const DEFAULT_CHROME_LINES = 4; // status line + headers + footer | ||
| ``` | ||
|
|
||
| Available content height = terminal rows - chrome. | ||
|
|
||
| ### 9.2 Split Panels | ||
|
|
||
| Inbox view uses a 40/60 split: | ||
|
|
||
| ```typescript | ||
| const listHeight = Math.max(3, Math.floor(availableRows * 0.40)); | ||
| const detailHeight = Math.max(0, availableRows - listHeight); | ||
| ``` | ||
|
|
||
| ### 9.3 Scrollbar | ||
|
|
||
| Vertical scrollbar rendered as a single column in the right margin: | ||
|
|
||
| | Element | Character | Color | | ||
| |---------|-----------|-------| | ||
| | Thumb | `█` | cyan | | ||
| | Track | `░` | gray | | ||
|
|
||
| ### 9.4 Status Line | ||
|
|
||
| Footer bar spanning full terminal width: | ||
|
|
||
| ``` | ||
| /// WARP [tick: 47 (a1b2c3d) ← 46 (e4f5g6h) | me: 12 | writers: 3] ///////////// | ||
| ``` | ||
|
|
||
| Always dim. Padded with `/` to fill the terminal width. | ||
|
|
||
| ### 9.5 Modal Input | ||
|
|
||
| Text input uses underscore as cursor: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Markdownlint MD040 / MD038 warnings from static analysis.
Nine fenced code blocks are missing a language identifier (MD040 — lines 25, 31, 100, 125, 231, 242, 325, 335). Most of these are terminal/ASCII-art examples; adding text satisfies the linter without implying syntax highlighting. Additionally, line 171 has spaces inside two inline code spans (\│ `and` ``), which triggers MD038. Since the spaces are intentional (they illustrate indentation characters), the simplest fix is an inline suppression comment on that line.
🧰 Tools
🪛 LanguageTool
[grammar] ~240-~240: Ensure spelling is correct
Context: ...g ``` ### 7.3 Headers CLI headers use boxen with a bold label and dim detail line: ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.21.0)
[warning] 25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 100-100: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 171-171: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 231-231: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 242-242: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 325-325: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 335-335: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/canonical/STYLE_GUIDE.md` around lines 25 - 335, Add language
identifiers "text" to each of the nine fenced code blocks that currently lack
one (the blocks around the Cyan→Magenta gradient, Progress Bar anatomy,
examples, CLI boxen snippet, truncation examples, and other ASCII/terminal
examples) so they read ```text; and on the line containing the inline code spans
showing indentation (`│ ` and ` `) add an inline markdownlint suppression (//
markdownlint-disable-line MD038 or <!-- markdownlint-disable-line MD038 -->) to
silence MD038 while preserving the intentional spaces.
Comment on lines
+66
to
+67
| - You can **seek** the system to any prior tick, fork reality, and replay forward. | ||
| - Trust, tests, and deployment safety are cryptographically provable — not vibes. |
There was a problem hiding this comment.
Two minor copy concerns in the End State section.
-
seekis bolded (implying a domain term) but never defined. Since this document is taggedAUTHORITATIVE, either define it inline or add a cross-reference to wherever the seek semantics are specified (e.g., "seek back to any prior tick" in the SWS/time-travel model). -
"not vibes" is informal for an authoritative spec. Consider tightening to something like
— not probabilistic attestationor simply dropping the qualifier if the cryptographic provability claim stands on its own.
✏️ Suggested fix
-- You can **seek** the system to any prior tick, fork reality, and replay forward.
+- You can **seek** (rewind the graph to an arbitrary prior tick), fork reality, and replay forward.
-- Trust, tests, and deployment safety are cryptographically provable — not vibes.
+- Trust, tests, and deployment safety are cryptographically provable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/canonical/VISION_NORTH_STAR.md` around lines 66 - 67, The End State
lines use the bold term "seek" and the informal phrase "not vibes": define or
cross-reference "seek" (e.g., add a short parenthetical or link to the
SWS/time-travel model that explains "seek back to any prior tick" and its
semantics) and replace the colloquial "not vibes" with a formal alternative such
as "— not probabilistic attestation" or remove the qualifier; update the two
lines so they read something like: "You can seek (see SWS/time-travel model) the
system to any prior tick, fork reality, and replay forward." and "Trust, tests,
and deployment safety are cryptographically provable — not probabilistic
attestation."
- Use Promise.allSettled for neighbor batch (mirrors props pattern) - Fix result.reason toString for Error instances in batch warnings - Remove stale WARP branding in STYLE_GUIDE philosophy + status line - Update StatusLine prefix: /// WARP → /// XYPH
Owner
Author
|
✅ Round 3 feedback addressed in commit 24df040 — 5 issues resolved.
Deferred (out of scope for this PR):
Build ✅ · 338 tests ✅ · Lint clean ✅ |
Owner
Author
|
@coderabbitai review please 🥕 |
|
Build ✅ · 338/338 tests ✅ · Lint clean ✅ — excellent work across all three feedback rounds! I'll review the changes now! 🥕 ✅ Actions performedReview triggered.
|
This was referenced Feb 25, 2026
6 tasks
flyingrobots
added a commit
that referenced
this pull request
Mar 8, 2026
Cover assertNodeExists, assertPrefix, assertMinLength, assertPrefixOneOf, and parseHours with 16 unit tests.
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
Promise.allinstead of sequential awaits across 142 nodes and 4 passes.buildSubmissionDataconverted from async to sync using pre-fetched neighbors cache.key.tabearly-return guards in all 4 viewuseInputhandlers +returnafter Dashboard's tab handler, ensuring Tab is cleanly handled only by the view-switching logic.Test plan
npx tsc --noEmit— cleannpm run test:local— 338/338 passSummary by CodeRabbit
New Features
Performance
Bug Fixes
Documentation
Style
Chores
Tests