feat(tui): XYPH wordmark, centralized logo loader, layout stability#9
Merged
flyingrobots merged 18 commits intomainfrom Feb 21, 2026
Merged
feat(tui): XYPH wordmark, centralized logo loader, layout stability#9flyingrobots merged 18 commits intomainfrom
flyingrobots merged 18 commits intomainfrom
Conversation
…bility fixes
- New logo-loader.ts: dimension-aware logo selection with size cascade
(large → medium → small) and terminal constraint filtering
- XYPH wordmark rendered upper-right on all dashboard views (hidden < 50 cols)
- Views accept dynamic chromeLines prop instead of hardcoded constant
- Landing page centers logo as block (not per-line), fixing multi-width art
- Dashboard header pinned with flex-start, split into tab row + hint row
- LineageView truncates long text to prevent wrapping-induced layout shift
- Detail modal (RoadmapView, AllNodesView) uses fixed height wrapper
- Logos reorganized into logos/{family}/{size}/ directory structure
…bility fixes
- New logo-loader.ts: dimension-aware logo selection with size cascade
(large → medium → small) and terminal constraint filtering
- XYPH wordmark rendered upper-right on all dashboard views (hidden < 50 cols)
- Views accept dynamic chromeLines prop instead of hardcoded constant
- Landing page centers logo as block (not per-line), fixing multi-width art
- Dashboard header pinned with flex-start, split into tab row + hint row
- LineageView truncates long text to prevent wrapping-induced layout shift
- Detail modal (RoadmapView, AllNodesView) uses fixed height wrapper
- Logos reorganized into logos/{family}/{size}/ directory structure
…ts/xyph into feat/tui-wordmark-logo-loader
Owner
Author
|
@coderabbitai review please 🥕 |
|
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. 📝 WalkthroughWalkthroughAdds a dimension-aware logo loader and many ASCII logo assets, a StatusLine component and GraphMeta model, dashboard layout driven by chromeLines, adapter snapshot caching with frontier short-circuiting and periodic checkpoints, TUI view adjustments, docs/mockups updates, and integration tests for cross-adapter visibility and cache coherence. Changes
Sequence DiagramsequenceDiagram
participant User as Terminal User
participant Entry as xyph-dashboard.tsx
participant Loader as logo-loader
participant Dashboard as Dashboard (TUI)
participant Adapter as WarpDashboardAdapter
participant Graph as WarpGraph
User->>Entry: start (cols, rows)
Entry->>Loader: selectLogoSize(cols, rows)
Loader-->>Entry: size bucket
Entry->>Loader: loadRandomLogo(logosDir, family, size, constraints)
Loader-->>Entry: LogoResult {text, lines, width}
Entry->>Dashboard: render({ logoText, wordmarkText, wordmarkLines })
Dashboard->>Adapter: fetchSnapshot()
Adapter->>Graph: hasFrontierChanged()
Graph-->>Adapter: boolean
alt no frontier change
Adapter-->>Dashboard: return cachedSnapshot
else frontier changed
Adapter->>Graph: materialize()/syncCoverage
Graph-->>Adapter: materialized snapshot
Adapter->>Adapter: compute graphMeta {maxTick,myTick,writerCount,tipSha}
Adapter-->>Dashboard: snapshot + graphMeta (cached)
end
Dashboard->>Dashboard: compute chromeLines from header/wordmark
Dashboard->>Dashboard: render views with chromeLines
Dashboard->>Dashboard: render StatusLine(graphMeta, prevGraphMeta)
Dashboard-->>User: updated TUI display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
|
✅ Actions performedReview triggered.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34023c5ef2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+140
to
+142
| const tabColumnHeight = 2; // tab labels row + hints row | ||
| const headerHeight = showWordmark ? Math.max(tabColumnHeight, wordmarkLines) : tabColumnHeight; | ||
| const chromeLines = headerHeight + 1 + 2; // +1 marginBottom, +1 scroll indicator, +1 status line |
There was a problem hiding this comment.
chromeLines assumes the left header column is always exactly two lines (tabColumnHeight = 2), but once the wordmark is shown (cols >= 50) that column gets much narrower and the hint text can wrap (especially in inbox where the hint string is longest). In those terminal widths, the actual header height exceeds the computed budget, so child views overestimate listHeight and the layout starts clipping/jumping because the screen consumes more rows than expected.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
README.md (1)
218-218: Inconsistent emphasis style — use underscores consistently (MD049).Static analysis flagged multiple instances across lines 218, 224, and 273 where
*asterisks*are used instead of the file-level convention of_underscores_for italics. Minor, but worth a global fix to silence the linter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 218, Replace the inline italics written with asterisks to use underscores to match the file-level convention (MD049); for example change "is a *planning compiler*" to "is a _planning compiler_" and do the same for the other italicized phrases referenced (the occurrences using *...* around words like planning compiler and any other *asterisk* usages at the flagged locations), ensuring all italics in the README use _underscores_ consistently.src/tui/StatusLine.tsx (1)
14-16: Replace the magic number14withprefix.lengthto avoid a silent drift bug.The hardcoded
14is the length of'/// WARP [--] '. If the prefix is ever edited, the padding arithmetic breaks silently.♻️ Proposed fix
- if (graphMeta === undefined) { - const empty = '/// WARP [--] ' + '/'.repeat(Math.max(0, cols - 14)); - return <Text dimColor>{empty}</Text>; - } + if (graphMeta === undefined) { + const prefix = '/// WARP [--] '; + const empty = prefix + '/'.repeat(Math.max(0, cols - prefix.length)); + return <Text dimColor>{empty}</Text>; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/StatusLine.tsx` around lines 14 - 16, In StatusLine (around the graphMeta check) replace the hardcoded 14 with the actual prefix length: construct the empty line using the existing prefix variable (e.g., prefix + '/'.repeat(Math.max(0, cols - prefix.length))) so the padding uses prefix.length instead of the magic number; update the expression that sets empty (and any related uses) to reference prefix.length, keeping symbols graphMeta, prefix, cols, and empty intact.src/tui/views/LineageView.tsx (1)
160-161: Truncated text has no visual indicator that content was clipped.All the
.slice(0, N)truncations silently clip IDs and titles without appending an ellipsis or similar marker. Users see "intent:some-long-" but have no way to know the text was cut. Consider a small helper like:const trunc = (s: string, n: number) => s.length > n ? s.slice(0, n - 1) + '…' : s;This would also DRY up the repeated
.slice()calls scattered across multiple view files.Also applies to: 188-189, 202-202, 215-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/views/LineageView.tsx` around lines 160 - 161, In LineageView.tsx replace the silent .slice(0, N) truncations with a reusable truncation helper (e.g., trunc(s: string, n: number)) and use it where Text renders IDs and titles (references: the Text components rendering row.id and row.title in LineageView, and the other locations you noted around lines 188-189, 202, 215-216) so that when a string is longer than n it appends an ellipsis (e.g., '…'); add the helper near the top of the file or a shared UI/utils module and swap all .slice usages to trunc(row.id, 30) and trunc(row.title, 38) to DRY the logic and provide a visible clipping indicator.src/tui/views/AllNodesView.tsx (1)
141-162: Detail modal: fixed-height wrapper is a good layout-stability improvement.The
height={listHeight + 1}plusflexGrow={1}on the inner box prevents the layout shift that would occur when toggling between list and detail views. Note that the detail modal JSX (border, QuestDetailPanel wiring, campaign/intent title lookup via.find(), "Esc to close" hint) is nearly identical to RoadmapView lines 153-176. If both continue to evolve together, extracting a sharedQuestDetailModalwrapper would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/views/AllNodesView.tsx` around lines 141 - 162, The detail-modal JSX is duplicated between AllNodesView and RoadmapView; extract a shared QuestDetailModal component that accepts the selected quest (selectedQuest), listHeight, clampedQuestIdx usage can be moved upstream, and snapshot (for campaign/intent title lookups using snapshot.campaigns and snapshot.intents) and renders the border, QuestDetailPanel and the "Esc to close" hint; then replace the duplicated blocks in AllNodesView and RoadmapView with <QuestDetailModal ... /> (or equivalent) to reduce duplication and keep behavior (height={listHeight + 1} and inner flexGrow={1}) identical.src/tui/logo-loader.ts (1)
51-66: A single bad file poisons the entire directory's candidates.The
try/catchwraps bothreaddirSyncand theparseLogoFileloop. If one.txtfile in a directory is unreadable (e.g., permissions),parseLogoFilethrows, and the catch discards all already-parsed candidates from that directory — potentially cascading down to a smaller size or theFALLBACKunnecessarily.Moving error handling inside the loop would be more resilient:
🛡️ Proposed fix: per-file error handling
function loadCandidates(dir: string, constraints?: LogoConstraints): LogoResult[] { try { const fileNames = readdirSync(dir).filter((f) => f.endsWith('.txt')); const results: LogoResult[] = []; for (const name of fileNames) { - const logo = parseLogoFile(join(dir, name)); - if (constraints && (logo.width > constraints.maxWidth || logo.lines > constraints.maxHeight)) { - continue; + try { + const logo = parseLogoFile(join(dir, name)); + if (constraints && (logo.width > constraints.maxWidth || logo.lines > constraints.maxHeight)) { + continue; + } + results.push(logo); + } catch { + // skip unreadable file } - results.push(logo); } return results; } catch { return []; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/logo-loader.ts` around lines 51 - 66, The current loadCandidates function wraps both readdirSync and the file parsing loop in a single try/catch so a thrown error from parseLogoFile discards all parsed results; change it to only protect the directory read with one try (using readdirSync) and move per-file error handling inside the for loop around parseLogoFile(join(dir, name)) so a failure on one .txt file is caught/logged and the loop continues; keep returning the accumulated results (LogoResult[]) and still apply the LogoConstraints check (logo.width/logo.lines) after a successful parse.src/infrastructure/adapters/WarpDashboardAdapter.ts (1)
227-242: GraphMeta construction looks correct; minor note ongetStateSnapshot()nullability.The
state ?guards handle the case wheregetStateSnapshot()returns null/undefined. TheMath.max(0, ...state.observedFrontier.values())spread on aMapiterator is valid ES6+.One observation:
tipSha(line 237) is derived fromfrontier(notstate), so even whenstateis null, you'll still get a tipSha value. This seems intentional but worth noting — ifstateis null, the meta will havemaxTick: 0, myTick: 0, writerCount: 0but a potentially non-emptytipSha, which could look inconsistent in the UI.🤖 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 227 - 242, The GraphMeta can show inconsistent data when getStateSnapshot() returns null because tipSha is derived from frontier regardless; update the GraphMeta construction in WarpDashboardAdapter so that tipSha is computed only when state is present (or explicitly set to an empty string when state is null) to keep { maxTick, myTick, writerCount, tipSha } consistent; change the tipSha logic around the existing getStateSnapshot()/getFrontier() usage and ensure GraphMeta and cachedSnapshot assignment use the new tipSha rule (refer to getStateSnapshot, getFrontier, GraphMeta, tipSha, and cachedSnapshot).src/tui/Dashboard.tsx (1)
136-142: Chrome height calculation: the comment is helpful but the expression could be clearer.The arithmetic
headerHeight + 1 + 2is correct (marginBottom + scrollIndicator + statusLine = 3), but splitting3as1 + 2doesn't map obviously to three distinct items. A named breakdown would be more self-documenting:const chromeLines = headerHeight + 1 /* marginBottom */ + 1 /* scroll indicator */ + 1 /* status line */;This is a minor readability nit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/Dashboard.tsx` around lines 136 - 142, Update the chrome height calculation to make the three added lines explicit and self-documenting: in the block around cols/showWordmark/wordmarkLinesArray/tabColumnHeight/headerHeight replace the terse expression setting chromeLines (currently "headerHeight + 1 + 2") with an explicit sum that names each item (e.g., "headerHeight + 1 /* marginBottom */ + 1 /* scroll indicator */ + 1 /* status line */" or by using small constants for marginBottom, scrollIndicator, and statusLine) so the three contributions are clear when reading the code.xyph-dashboard.tsx (1)
32-40: Splash loading with constraints looks good; wordmark has no constraints.The splash logo correctly applies
maxWidth/maxHeightconstraints to avoid overflow. However, the wordmark on line 40 is loaded without constraints — if a "small" logo happens to be wider than the available header space (roughlycols - tabRowWidth), it could push the layout. Since the Dashboard hides the wordmark below 50 columns, the practical risk is low, but you could add amaxWidthconstraint here for robustness:-const wordmark = loadRandomLogo(logosDir, 'xyph', 'small'); +const wordmark = loadRandomLogo(logosDir, 'xyph', 'small', { maxWidth: 30, maxHeight: 4 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xyph-dashboard.tsx` around lines 32 - 40, The wordmark is loaded without sizing constraints (see the wordmark variable and loadRandomLogo call); update the wordmark instantiation to pass a constraints object (e.g., include maxWidth and optionally maxHeight) so the selected "small" logo cannot exceed available header space — use cols and the existing tabRowWidth to compute the limit (for example maxWidth: cols - tabRowWidth - 4) and pass that object as the third/fourth argument to loadRandomLogo so the wordmark is clamped like the splash.
🤖 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/mockups/tui_mockups_index.html`:
- Line 2: The HTML root element is missing a lang attribute which breaks
accessibility; update the <html> tag in tui_mockups_index.html (the HTML root
element) to include an appropriate language identifier (e.g., lang="en" or the
document's actual language) so screen readers and WCAG 3.1.1 requirements are
satisfied.
- Line 24: Replace the bare ampersand in the heading text so the HTML is valid:
update the <h2>Trust & Identity</h2> string to use an escaped ampersand (e.g.,
"Trust & Identity"); also check nearby attributes such as the <img
alt="Trust & Identity"/> and link text "tui_7_trust.svg" display text and escape
any bare "&" there if present.
In `@README.md`:
- Around line 20-24: The Markdown jumps from the top-level title (#) to
third-level headings (### **What**, ### **is**, ### **XYPH, actually?**) which
violates MD001; change those three `###` headings to `##` (or convert them into
plain paragraph/blockquote text if you intended a stylistic split) so headings
progress one level at a time and restore correct semantics for ToC and parsers.
- Around line 57-214: The README contains an unedited AI-assistant reply in the
"Direct competitors (closest match)" section (starts with "If your current pitch
is…") including second-person phrasing and an offer ("If you want, I can also
give you a competitor matrix…"); replace that block with product-facing prose
written in third-person (or neutral voice), remove any assistant/offering
sentences, and ensure headings like "Direct competitors (closest match)",
"Neighbor families", and the 1–6 family breakdowns preserve their intent but use
neutral language that describes competitors and XYPH differences without
second-person prompts or offers of further help.
- Line 222: Fix the typo in the README where "interacative" is misspelled;
replace "interacative" with "interactive" in the sentence describing XYPH ("part
interacative documentation library" → "part interactive documentation library")
so the project description reads correctly.
In `@src/tui/logos/byFlyingRobots/1.txt`:
- Around line 1-8: The two files byFlyingRobots/1.txt and byFlyingRobots/2.txt
are placed at the family root but loadRandomLogo builds paths as join(logosDir,
family, size) and only searches size subdirectories, so move these files into an
appropriate size folder (e.g., byFlyingRobots/large/1.txt and
byFlyingRobots/large/2.txt to match existing byFlyingRobots/large/1.txt) or
delete them if unused so they can be discovered by loadRandomLogo using the
family/size layout; ensure the filenames remain N.txt format to match the other
logo files.
In `@src/tui/logos/byFlyingRobots/2.txt`:
- Around line 1-7: The byFlyingRobots flat logo files are never picked up
because logo-loader.ts only iterates SIZE_CASCADE (large/medium/small)
subfolders; either move the root files (byFlyingRobots/1.txt,
byFlyingRobots/2.txt) into the correct size folder (e.g., byFlyingRobots/large/)
or update the loader (in logo-loader.ts, the code that uses SIZE_CASCADE—likely
the loadLogos or equivalent function around line 85) to treat a family root as a
fallback: after checking each size path, also check the family root directory
and include any logos found there so flat families are handled.
In `@src/tui/logos/flyingRobotsWide/large/2.txt`:
- Line 1: Remove the unintended leading blank line in the flyingRobotsWide large
variant logo file (the first line of the "2.txt" logo) so the logo height
measures 9 instead of 10; alternatively (or additionally) update parseLogoFile
to strip leading blank lines before measuring height by trimming empty lines at
the start of the parsed content so parseLogoFile reports correct height and
respects maxHeight constraints. Reference: the logo file "2.txt" and the
parseLogoFile function.
In `@src/tui/logos/xyph/large/11.txt`:
- Line 1: The file src/tui/logos/xyph/large/11.txt has an unintended leading
blank line making the visible logo start at line 2; remove that empty first line
so the logo body occupies lines 1–15 (no extra blank at the top), matching the
expected raw/visible line counts and avoiding the height-measurement discrepancy
noted for flyingRobotsWide/large/2.txt.
In `@src/tui/logos/xyph/medium/5.txt`:
- Around line 1-10: selectLogoSize() currently maps terminals to a "medium"
bucket but xyph-dashboard.tsx enforces maxHeight = rows - 12 which causes
10-line medium logos to be downscaled to small on 20–21 row terminals; update
the sizing logic so medium logos are permitted to be up to 10 lines (either by
changing the maxHeight calculation from rows - 12 to rows - 10, by expanding the
medium bucket range in selectLogoSize(), or by adding an explicit per-size
minHeight check that allows a 10-line logo when selectLogoSize() returns
"medium"); locate and change the code referencing maxHeight and selectLogoSize
in xyph-dashboard.tsx so medium logos no longer silently cascade to small when a
10-line logo actually fits.
In `@src/tui/logos/xyph/small/7.txt`:
- Around line 1-3: logo-loader.ts currently measures logo widths with l.length
(see the width-check logic and the constraint validation that rejects oversized
logos), which undercounts block-drawing glyphs in CJK/wide modes; replace the
length-based measurement with a column-aware width call (e.g., import and use
wcwidth or string-width to compute display columns for each line) wherever the
code computes line/display width and enforces the max-width constraint, ensure
you update the validation that uses that width (the same check that previously
compared l.length against the max) to use the new wcwidth/string-width result,
and add a small fallback or note in the loader initialization if the native
module is unavailable or a test to assert correct width for a sample line like
"▄▄ ▄▄ ▄▄ ▄▄ ▄▄▄▄ ▄▄ ▄▄".
In `@src/tui/logos/xyph/small/8.txt`:
- Around line 1-4: This file uses ambiguous-width block/quadrant glyphs (▚
U+259A, ▘ U+2598, ▛ U+259B, ▞ U+259E, ▖ U+2596, ▙ U+2599 and also ▄ U+2584, ▌
U+258C) which can render at different widths in CJK locales; replace these
characters with monospace-safe alternatives (e.g., ASCII characters like '#' or
'X', simple box-drawing characters known to be narrow, or plain spaces) so the
small logo layout remains stable across terminals, updating the lines that
contain those specific glyphs to use the chosen safe characters.
---
Nitpick comments:
In `@README.md`:
- Line 218: Replace the inline italics written with asterisks to use underscores
to match the file-level convention (MD049); for example change "is a *planning
compiler*" to "is a _planning compiler_" and do the same for the other
italicized phrases referenced (the occurrences using *...* around words like
planning compiler and any other *asterisk* usages at the flagged locations),
ensuring all italics in the README use _underscores_ consistently.
In `@src/infrastructure/adapters/WarpDashboardAdapter.ts`:
- Around line 227-242: The GraphMeta can show inconsistent data when
getStateSnapshot() returns null because tipSha is derived from frontier
regardless; update the GraphMeta construction in WarpDashboardAdapter so that
tipSha is computed only when state is present (or explicitly set to an empty
string when state is null) to keep { maxTick, myTick, writerCount, tipSha }
consistent; change the tipSha logic around the existing
getStateSnapshot()/getFrontier() usage and ensure GraphMeta and cachedSnapshot
assignment use the new tipSha rule (refer to getStateSnapshot, getFrontier,
GraphMeta, tipSha, and cachedSnapshot).
In `@src/tui/Dashboard.tsx`:
- Around line 136-142: Update the chrome height calculation to make the three
added lines explicit and self-documenting: in the block around
cols/showWordmark/wordmarkLinesArray/tabColumnHeight/headerHeight replace the
terse expression setting chromeLines (currently "headerHeight + 1 + 2") with an
explicit sum that names each item (e.g., "headerHeight + 1 /* marginBottom */ +
1 /* scroll indicator */ + 1 /* status line */" or by using small constants for
marginBottom, scrollIndicator, and statusLine) so the three contributions are
clear when reading the code.
In `@src/tui/logo-loader.ts`:
- Around line 51-66: The current loadCandidates function wraps both readdirSync
and the file parsing loop in a single try/catch so a thrown error from
parseLogoFile discards all parsed results; change it to only protect the
directory read with one try (using readdirSync) and move per-file error handling
inside the for loop around parseLogoFile(join(dir, name)) so a failure on one
.txt file is caught/logged and the loop continues; keep returning the
accumulated results (LogoResult[]) and still apply the LogoConstraints check
(logo.width/logo.lines) after a successful parse.
In `@src/tui/StatusLine.tsx`:
- Around line 14-16: In StatusLine (around the graphMeta check) replace the
hardcoded 14 with the actual prefix length: construct the empty line using the
existing prefix variable (e.g., prefix + '/'.repeat(Math.max(0, cols -
prefix.length))) so the padding uses prefix.length instead of the magic number;
update the expression that sets empty (and any related uses) to reference
prefix.length, keeping symbols graphMeta, prefix, cols, and empty intact.
In `@src/tui/views/AllNodesView.tsx`:
- Around line 141-162: The detail-modal JSX is duplicated between AllNodesView
and RoadmapView; extract a shared QuestDetailModal component that accepts the
selected quest (selectedQuest), listHeight, clampedQuestIdx usage can be moved
upstream, and snapshot (for campaign/intent title lookups using
snapshot.campaigns and snapshot.intents) and renders the border,
QuestDetailPanel and the "Esc to close" hint; then replace the duplicated blocks
in AllNodesView and RoadmapView with <QuestDetailModal ... /> (or equivalent) to
reduce duplication and keep behavior (height={listHeight + 1} and inner
flexGrow={1}) identical.
In `@src/tui/views/LineageView.tsx`:
- Around line 160-161: In LineageView.tsx replace the silent .slice(0, N)
truncations with a reusable truncation helper (e.g., trunc(s: string, n:
number)) and use it where Text renders IDs and titles (references: the Text
components rendering row.id and row.title in LineageView, and the other
locations you noted around lines 188-189, 202, 215-216) so that when a string is
longer than n it appends an ellipsis (e.g., '…'); add the helper near the top of
the file or a shared UI/utils module and swap all .slice usages to trunc(row.id,
30) and trunc(row.title, 38) to DRY the logic and provide a visible clipping
indicator.
In `@xyph-dashboard.tsx`:
- Around line 32-40: The wordmark is loaded without sizing constraints (see the
wordmark variable and loadRandomLogo call); update the wordmark instantiation to
pass a constraints object (e.g., include maxWidth and optionally maxHeight) so
the selected "small" logo cannot exceed available header space — use cols and
the existing tabRowWidth to compute the limit (for example maxWidth: cols -
tabRowWidth - 4) and pass that object as the third/fourth argument to
loadRandomLogo so the wordmark is clamped like the splash.
Comment on lines
+1
to
+10
| d8b | ||
| ?88 | ||
| 88b | ||
| ?88, 88P?88 d8P ?88,.d88b, 888888b | ||
| `?8bd8P'd88 88 `?88' ?88 88P `?8b | ||
| d8P?8b, ?8( d88 88b d8P d88 88P | ||
| d8P' `?8b`?88P'?8b 888888P'd88' 88b | ||
| )88 88P' | ||
| ,d8P d88 | ||
| `?888P' ?8P |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find src -name "logo-loader.ts" -o -name "*loader*.ts" | head -20Repository: flyingrobots/xyph
Length of output: 83
🏁 Script executed:
cat -n src/tui/logo-loader.ts | head -100Repository: flyingrobots/xyph
Length of output: 3466
🏁 Script executed:
rg -n "loadRandomLogo" --type ts -B 2 -A 2 src/Repository: flyingrobots/xyph
Length of output: 295
🏁 Script executed:
rg -n "loadRandomLogo\(" --type ts -B 3 -A 3 src/ | head -50Repository: flyingrobots/xyph
Length of output: 434
🏁 Script executed:
rg -n "loadRandomLogo" --type ts src/ | grep -v "export function"Repository: flyingrobots/xyph
Length of output: 43
🏁 Script executed:
rg "loadRandomLogo" --type ts -A 5 src/ | grep -A 5 "constraints"Repository: flyingrobots/xyph
Length of output: 155
🏁 Script executed:
rg "loadRandomLogo\(" --type ts -B 2 -A 5 src/Repository: flyingrobots/xyph
Length of output: 416
🏁 Script executed:
rg "loadRandomLogo" --type ts src/ -B 2 -A 2 | grep -v "^export function" | grep -v "^--$"Repository: flyingrobots/xyph
Length of output: 280
🏁 Script executed:
rg "loadRandomLogo\s*\(" --type ts src/ -B 2 -A 8Repository: flyingrobots/xyph
Length of output: 555
🏁 Script executed:
rg "loadRandomLogo" --type ts src/ -B 1 -A 1 | grep -v "export function"Repository: flyingrobots/xyph
Length of output: 130
🏁 Script executed:
rg "loadRandomLogo" --type ts src/ | grep -v "export function loadRandomLogo"Repository: flyingrobots/xyph
Length of output: 43
🏁 Script executed:
grep -r "loadRandomLogo" src/ --include="*.ts" | grep -v "export function"Repository: flyingrobots/xyph
Length of output: 43
🏁 Script executed:
grep -rn "loadRandomLogo" src/ --include="*.ts"Repository: flyingrobots/xyph
Length of output: 118
🏁 Script executed:
rg "loadRandomLogo" --type tsRepository: flyingrobots/xyph
Length of output: 385
🏁 Script executed:
grep -n "loadRandomLogo\|selectLogoSize\|maxHeight\|maxWidth" xyph-dashboard.tsx -B 2 -A 2Repository: flyingrobots/xyph
Length of output: 768
🏁 Script executed:
wc -l src/tui/logos/xyph/medium/5.txtRepository: flyingrobots/xyph
Length of output: 95
Verify the medium height constraint accommodates 10 lines.
This logo is 10 lines tall. The constraints in xyph-dashboard.tsx set maxHeight = rows - 12. When selectLogoSize() returns medium (rows 20–29), the maximum allowed height ranges from 8 to 17 lines. A 10-line logo will fit when the terminal has ≥22 rows but will silently cascade to small on terminals with 20–21 rows even though medium was selected by selectLogoSize(). Confirm this trade-off is acceptable for the intended use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui/logos/xyph/medium/5.txt` around lines 1 - 10, selectLogoSize()
currently maps terminals to a "medium" bucket but xyph-dashboard.tsx enforces
maxHeight = rows - 12 which causes 10-line medium logos to be downscaled to
small on 20–21 row terminals; update the sizing logic so medium logos are
permitted to be up to 10 lines (either by changing the maxHeight calculation
from rows - 12 to rows - 10, by expanding the medium bucket range in
selectLogoSize(), or by adding an explicit per-size minHeight check that allows
a 10-line logo when selectLogoSize() returns "medium"); locate and change the
code referencing maxHeight and selectLogoSize in xyph-dashboard.tsx so medium
logos no longer silently cascade to small when a 10-line logo actually fits.
Comment on lines
+1
to
+3
| ▄▄ ▄▄ ▄▄ ▄▄ ▄▄▄▄ ▄▄ ▄▄ | ||
| ▀█▄█▀ ▀███▀ ██▄█▀ ██▄██ | ||
| ██ ██ █ ██ ██ ██ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*logo-loader*" -type f | head -20Repository: flyingrobots/xyph
Length of output: 85
🏁 Script executed:
cat -n ./src/tui/logo-loader.tsRepository: flyingrobots/xyph
Length of output: 3466
Width measurement uses string.length without wcwidth support — block-drawing characters will be under-counted in CJK terminals.
Line 43 of logo-loader.ts measures width via l.length, which counts characters rather than display columns. This does not account for the Ambiguous East Asian Width of ▄ (U+2584), ▀ (U+2580), and █ (U+2588). In CJK locales where "wide ambiguous characters" mode is active, these characters render at double width, but the constraint check (line 57) will under-count them, allowing logos like src/tui/logos/xyph/small/7.txt to exceed the actual maximum displayable width and render misaligned.
Migrate width measurement to use wcwidth (or equivalent column-aware library) to account for display width, or document this as a known limitation for non-CJK terminals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui/logos/xyph/small/7.txt` around lines 1 - 3, logo-loader.ts currently
measures logo widths with l.length (see the width-check logic and the constraint
validation that rejects oversized logos), which undercounts block-drawing glyphs
in CJK/wide modes; replace the length-based measurement with a column-aware
width call (e.g., import and use wcwidth or string-width to compute display
columns for each line) wherever the code computes line/display width and
enforces the max-width constraint, ensure you update the validation that uses
that width (the same check that previously compared l.length against the max) to
use the new wcwidth/string-width result, and add a small fallback or note in the
loader initialization if the native module is unavailable or a test to assert
correct width for a sample line like "▄▄ ▄▄ ▄▄ ▄▄ ▄▄▄▄ ▄▄ ▄▄".
Comment on lines
+1
to
+4
| ▌ | ||
| ▚▘▌▌▛▌▛▌ | ||
| ▞▖▙▌▙▌▌▌ | ||
| ▄▌▌ |
There was a problem hiding this comment.
Same Unicode Ambiguous Width concern as xyph/small/7.txt.
This file uses additional quadrant block characters (▚ U+259A, ▘ U+2598, ▛ U+259B, ▞ U+259E, ▖ U+2596, ▙ U+2599) alongside ▄/▌. All block elements in the U+2580–U+259F range carry "Ambiguous" East Asian Width per UAX #11 — same rendering risk in CJK-locale terminals as flagged for xyph/small/7.txt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui/logos/xyph/small/8.txt` around lines 1 - 4, This file uses
ambiguous-width block/quadrant glyphs (▚ U+259A, ▘ U+2598, ▛ U+259B, ▞ U+259E, ▖
U+2596, ▙ U+2599 and also ▄ U+2584, ▌ U+258C) which can render at different
widths in CJK locales; replace these characters with monospace-safe alternatives
(e.g., ASCII characters like '#' or 'X', simple box-drawing characters known to
be narrow, or plain spaces) so the small logo layout remains stable across
terminals, updating the lines that contain those specific glyphs to use the
chosen safe characters.
📝 WalkthroughWalkthroughThis PR introduces a logo loading system (TUI-008) with dynamic, dimension-aware logo selection and a new status line displaying graph metadata. It adds graphMeta tracking to snapshots, implements caching in the dashboard adapter, updates all TUI views to support configurable chrome line heights, and includes comprehensive README documentation rewrites alongside changelog and asset additions. Changes
Sequence Diagram(s)sequenceDiagram
participant xyph as xyph-dashboard<br/>(main entry)
participant loader as logo-loader
participant dash as Dashboard
participant adapter as WarpDashboardAdapter
participant graph as WarpGraph
participant status as StatusLine
participant views as TUI Views
xyph->>loader: selectLogoSize(cols, rows)
loader-->>xyph: LogoSize
xyph->>loader: loadRandomLogo(logosDir,<br/>family, size, constraints)
loader-->>xyph: splash & wordmark
xyph->>dash: Dashboard({..., wordmarkText,<br/>wordmarkLines})
dash->>adapter: fetchSnapshot()
adapter->>graph: materialize()
graph-->>adapter: materialized state
adapter->>adapter: Build graphMeta<br/>(maxTick, myTick, tipSha)
adapter->>adapter: Cache snapshot +<br/>graphMeta
adapter-->>dash: GraphSnapshot{graphMeta}
dash->>status: StatusLine({graphMeta,<br/>prevGraphMeta})
status-->>dash: rendered status line
dash->>views: RoadmapView({snapshot,<br/>chromeLines})
dash->>views: LineageView({snapshot,<br/>chromeLines})
dash->>views: AllNodesView({snapshot,<br/>chromeLines})
views->>views: Compute listHeight =<br/>stdout.rows - chromeLines
views-->>dash: rendered views
dash-->>xyph: complete TUI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/domain/models/dashboard.ts (1)
65-70: Consider whether implementation-detail comments belong in a pure domain interface.The file header declares this module has "no external dependencies — only TypeScript shapes," yet the inline comments on
GraphMetafields directly reference infrastructure symbols (observedFrontier.values(),getFrontier()). This leaks the adapter's internals into the domain model definition. Replacing them with intent-focused descriptions (e.g.,// highest observed tick across all writers) would keep the domain layer clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/models/dashboard.ts` around lines 65 - 70, Replace the implementation-detail inline comments on the GraphMeta interface (fields maxTick, myTick, writerCount, tipSha) with domain-focused descriptions: describe intent rather than referencing infrastructure functions like observedFrontier.values() or getFrontier(); e.g., explain maxTick as the highest observed tick across writers, myTick as the tick observed for the current writer or 0 if none, writerCount as the number of writers observed, and tipSha as the short SHA of the writer's current tip—keep the interface free of adapter/internal-symbol references.xyph-dashboard.tsx (1)
40-40: Consider addingmaxWidth/maxHeightconstraints to the wordmark loader.Without constraints, any logo in
xyph/small/qualifies regardless of its actual dimensions. A wider-than-expected small logo can push content in the header, and a taller one silently inflateschromeLines(and thus squeezes the list panes in every view). The splash loader already guards against this — applying the same pattern here would be consistent.♻️ Suggested change
-const wordmark = loadRandomLogo(logosDir, 'xyph', 'small'); +const WORDMARK_MAX_WIDTH = 30; // fits comfortably beside tab row in ≥80-col terminals +const WORDMARK_MAX_HEIGHT = 6; // keeps chrome overhead bounded +const wordmark = loadRandomLogo(logosDir, 'xyph', 'small', { + maxWidth: WORDMARK_MAX_WIDTH, + maxHeight: WORDMARK_MAX_HEIGHT, +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xyph-dashboard.tsx` at line 40, The wordmark loader call currently allows any image from loadRandomLogo(logosDir, 'xyph', 'small') so oversized assets can distort layout; update the call (or the loadRandomLogo invocation site) to accept and enforce maxWidth and maxHeight constraints (e.g., pass maxWidth/maxHeight options or use the same guard logic as the splash loader) so only images within the expected small dimensions are returned, and ensure the returned wordmark respects those bounds before assigning to the wordmark variable.src/tui/logo-loader.ts (1)
35-45: Synchronous I/O is acceptable at startup but worth documenting.
readFileSyncis fine here since logo loading runs once at startup before the Ink render loop. A brief doc comment noting this is a startup-only path would help future maintainers avoid callingparseLogoFilein a render cycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/logo-loader.ts` around lines 35 - 45, parseLogoFile uses readFileSync for startup-only synchronous I/O but lacks documentation; add a concise doc comment above the parseLogoFile function (referencing parseLogoFile and its use of readFileSync) stating that this function performs synchronous file I/O and is intended to be called only once at application startup (not from the Ink render loop or per-frame code), so future maintainers won't call it during render cycles.src/tui/views/RoadmapView.tsx (1)
153-175: Consider extracting the shared detail modal into a reusable wrapper.The detail modal block (fixed-height container → bordered box →
QuestDetailPanelwith campaign/intent title lookup → "Esc to close") is nearly identical betweenRoadmapViewandAllNodesView. Extracting a smallQuestDetailModalcomponent that acceptsquest,snapshot, andlistHeightwould reduce the duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/views/RoadmapView.tsx` around lines 153 - 175, The detail-modal JSX block duplicated in RoadmapView and AllNodesView should be extracted into a new reusable component (e.g., QuestDetailModal) that accepts props { quest, snapshot, listHeight, onClose } and encapsulates the fixed-height container, bordered Box, the quest → campaign/intent title lookups, QuestDetailPanel usage, and the "Esc to close" footer; replace the inline modal in RoadmapView (the code that reads showDetail, const row = vrows[clampedVIdx], selectedQuest, and the returned Box tree containing QuestDetailPanel) with a call to QuestDetailModal, passing the selected quest and snapshot so the lookup logic (snapshot.campaigns.find(...), snapshot.intents.find(...)) is moved into the new component to avoid duplication with AllNodesView.src/tui/views/LineageView.tsx (1)
160-161: Truncation applied without visual indicator.All
.slice()calls (lines 160, 161, 188, 189, 202, 215, 216) silently truncate text without appending an ellipsis (…). This is consistent with howRoadmapViewandAllNodesViewalso truncate, so it's clearly intentional — but worth noting that users with long IDs or titles won't know content was cut off.Also applies to: 188-189, 202-202, 215-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/views/LineageView.tsx` around lines 160 - 161, Update the truncation to append an ellipsis when content is cut off: locate the Text nodes in LineageView (the lines using row.id.slice(...) and row.title.slice(...), and the other occurrences around the component where row.*.slice is used) and change the logic so you check the original string length against the maximum (e.g., 30 or 38) and render the sliced value plus "…" only if original.length > limit; keep the full value otherwise (use the same pattern for all occurrences at the mentioned slices: id, title, and the other row fields on lines ~188/189, ~202 and ~215/216).src/tui/Dashboard.tsx (1)
49-52: Side effect insidesetSnapshotupdater.Mutating
prevMeta.currentinside the state updater function (line 50) is a known React anti-pattern — updaters can be invoked multiple times in StrictMode. In practice this works here because the double-invocation will set the same value (theprevargument is the same both times). Still, extracting the ref mutation to a separate step beforesetSnapshotwould be more conventional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/Dashboard.tsx` around lines 49 - 52, Move the ref mutation out of the state updater: read the current snapshot's graphMeta (the existing snapshot state variable) and assign it to prevMeta.current before calling setSnapshot, then call setSnapshot(s) with the new snapshot value instead of mutating prevMeta.current inside the updater; remove the prev => { prevMeta.current = prev?.graphMeta; return s; } updater and replace it with a separate assignment using prev?.graphMeta and a plain setSnapshot(s) call (references: setSnapshot, prevMeta.current, prev?.graphMeta, s).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 267: Replace the transposed product name "XPHY" with the correct "XYPH"
in the README sentence "Now you're all set. Let's see how we might use XPHY in
our everyday workflows." so the product name reads "XYPH"; locate occurrences of
the exact token "XPHY" in the README and update them to "XYPH" to ensure
consistency.
- Line 232: Fix the typographical error in the README sentence that begins
"Although XYPH lives in a Git repo, its invisible to normal Git workflows..." by
replacing "its" with the contraction "it's" (i.e., "it's invisible"); update the
sentence text so it reads "it's invisible to normal Git workflows and tools..."
in the README content where that sentence appears.
- Around line 493-495: Replace the Markdown link inside the HTML paragraph block
so it renders as a clickable link: locate the HTML element starting with <p
align="center"> that contains the text "Built with Ω¹ by [FLYING
ROBOTS](https://github.com/flyingrobots)" and swap the Markdown link for an HTML
anchor (e.g., use an <a href="https://github.com/flyingrobots">FLYING
ROBOTS</a>) so the link functions correctly within the HTML block.
In `@src/tui/logos/xyph/medium/4.txt`:
- Line 1: parseLogoFile in logo-loader.ts currently only strips trailing blank
lines so a leading empty line in files like xyph/medium/4.txt inflates the
reported height; update parseLogoFile to trim leading blank lines as well (e.g.,
remove empty entries from the start of the lines array before computing height)
so the measured height reflects actual content and maxHeight checks behave
correctly.
---
Duplicate comments:
In `@docs/mockups/tui_mockups_index.html`:
- Line 24: Replace the bare ampersand in the heading text "Trust & Identity" and
in the img alt attribute so they are HTML-escaped; update the <h2> content and
the img alt value (both inside the div with class "section" containing "Trust &
Identity") to use "Trust & Identity" and alt="Trust & Identity"
respectively.
- Line 2: The <html> root element is missing a lang attribute required by WCAG
3.1.1; update the <html> tag to include an appropriate language code (e.g.,
lang="en" or the correct locale) so the document begins with <html lang="...">
and assistive technologies can detect the page language.
In `@README.md`:
- Line 222: Fix the typo in the README phrase "part interacative documentation
library" by changing "interacative" to "interactive" so the line reads "part
interactive documentation library"; update the exact string in the README.md
entry where the project description contains "part interacative documentation
library".
- Around line 20-24: The README currently jumps from an h1 to three h3 lines
("### **What**", "### **is**", "### **XYPH, actually?**") causing MD001; update
these heading levels so they form a proper sequence (e.g., change them to h2 "##
**What**", "## **is**", "## **XYPH, actually?**" or insert a single h2 before
the h3s) so the headings follow H1 → H2 → H3 order and the linter error is
resolved.
- Around line 57-214: Summary: Remove the leftover unedited AI-assistant copy
and second‑person phrasing near the end of the competitors section. Locate the
stray assistant text (the sentence starting "If you want, I can also give you a
**competitor matrix**…" and any surrounding second‑person phrasing flagged as
"Unedited AI-assistant response") and delete or rewrite it into neutral,
product‑facing language; ensure the README uses third‑person or declarative tone
(no "If you want..." offers) and keep the content concise and authoritative
without conversational assistant remnants.
In `@src/tui/logos/byFlyingRobots/1.txt`:
- Around line 1-8: The file byFlyingRobots/1.txt is placed at the family root so
loadRandomLogo (which builds its search path with join(logosDir, family, size)
and only looks inside the size subdirectory) cannot discover it; move
byFlyingRobots/1.txt into the intended size folder (for example
byFlyingRobots/large/1.txt) so loadRandomLogo can find it when it iterates size
directories under join(logosDir, family, size).
In `@src/tui/logos/byFlyingRobots/2.txt`:
- Around line 1-7: The art in this logo file has its root glyph shifted (leading
spaces/rows) so the project's logo loader can't detect the root; adjust the
ASCII so the root marker (the top/root character sequence starting with "┓" in
this file) is placed at the expected origin (no leading spaces and on the first
row the loader expects), i.e., remove leading whitespace and/or shift rows
up/left so the root "┓" sits at column 0 and the rest of the glyphs maintain
their relative positions; ensure the transformed file matches the same
root-alignment pattern used by the working logo (e.g., 1.txt) so the logo loader
can find it.
In `@src/tui/logos/flyingRobotsWide/large/2.txt`:
- Line 1: The leading blank line in the logo file is inflating height; either
remove the leading empty line from the "2.txt" logo file or update parseLogoFile
to trim leading blank lines (or trim all empty lines at the start) before
computing height/rows so parseLogoFile returns the correct height (9) and avoids
the size cascade.
In `@src/tui/logos/xyph/large/11.txt`:
- Line 1: The first line of the ASCII-art file "11.txt" is a stray blank line
causing off-by-one height issues; open the asset
(src/tui/logos/xyph/large/11.txt), delete the empty first line so the art begins
at the very first line with no leading whitespace, save the file, and re-run the
provided audit script (or the awk check) to confirm no logo files have leading
blank lines.
In `@src/tui/logos/xyph/medium/5.txt`:
- Around line 1-10: selectLogoSize() currently sets maxHeight = rows − 12 which
makes 10-line logos in the "medium" bucket (rows 20–29) overflow on 20–21-row
terminals; change the maxHeight calculation to rows − 10 (or otherwise align the
medium bucket boundary) so a 10-line logo fits when selectLogoSize classifies
the terminal as medium, and update any related comment/tests referencing
maxHeight or the medium bucket to reflect the corrected threshold.
In `@src/tui/logos/xyph/small/7.txt`:
- Around line 1-3: The logo-width check uses l.length which undercounts
block-drawing characters (▄ ▀ █) in CJK locales; update the code in
logo-loader.ts to measure display width with a column-aware function (e.g.,
import and use string-width or wcwidth) instead of using l.length wherever the
code checks or enforces max width (look for usages like l.length, lineWidth, or
similar in functions that validate/trim/pad logo lines); replace those length
checks with stringWidth(line) and use that value for constraints and
trimming/padding so wide-ambiguous characters are counted correctly.
In `@src/tui/logos/xyph/small/8.txt`:
- Around line 1-4: The logo uses block elements from the U+2580–U+259F range
(e.g., characters like ▌ ▛ ▞ ▙ ▚ ▘) which are "Ambiguous" width and can render
double-width in CJK locales; update src/tui/logos/xyph/small/8.txt by replacing
all U+2580–U+259F characters with single-width alternatives (either ASCII
characters or single-width Unicode such as Braille Patterns U+2800–U+28FF or
box-drawing characters) so the logo layout remains stable in CJK terminals;
ensure characters visible in this file (the repeated ▌ ▛ ▞ ▙ ▚ ▘ glyphs) are all
swapped to their single-width equivalents and verify the visual shape is
preserved.
In `@src/tui/views/AllNodesView.tsx`:
- Around line 141-162: The detail-modal UI in AllNodesView.tsx duplicates the
fixed-height wrapper pattern used in RoadmapView; extract the repeated wrapper
into a reusable component (e.g., DetailModalWrapper or FixedHeightDetailModal)
and replace the inline JSX in AllNodesView (the block guarded by showDetail that
uses flatQuests[clampedQuestIdx], QuestDetailPanel, snapshot.campaigns/intents,
and listHeight) with that shared component; ensure the new component accepts
props for the selected item (quest), computed campaignTitle and intentTitle,
listHeight, and a close hint/handler so both AllNodesView and RoadmapView can
import and render the same modal wrapper without duplicating the JSX.
---
Nitpick comments:
In `@src/domain/models/dashboard.ts`:
- Around line 65-70: Replace the implementation-detail inline comments on the
GraphMeta interface (fields maxTick, myTick, writerCount, tipSha) with
domain-focused descriptions: describe intent rather than referencing
infrastructure functions like observedFrontier.values() or getFrontier(); e.g.,
explain maxTick as the highest observed tick across writers, myTick as the tick
observed for the current writer or 0 if none, writerCount as the number of
writers observed, and tipSha as the short SHA of the writer's current tip—keep
the interface free of adapter/internal-symbol references.
In `@src/tui/Dashboard.tsx`:
- Around line 49-52: Move the ref mutation out of the state updater: read the
current snapshot's graphMeta (the existing snapshot state variable) and assign
it to prevMeta.current before calling setSnapshot, then call setSnapshot(s) with
the new snapshot value instead of mutating prevMeta.current inside the updater;
remove the prev => { prevMeta.current = prev?.graphMeta; return s; } updater and
replace it with a separate assignment using prev?.graphMeta and a plain
setSnapshot(s) call (references: setSnapshot, prevMeta.current, prev?.graphMeta,
s).
In `@src/tui/logo-loader.ts`:
- Around line 35-45: parseLogoFile uses readFileSync for startup-only
synchronous I/O but lacks documentation; add a concise doc comment above the
parseLogoFile function (referencing parseLogoFile and its use of readFileSync)
stating that this function performs synchronous file I/O and is intended to be
called only once at application startup (not from the Ink render loop or
per-frame code), so future maintainers won't call it during render cycles.
In `@src/tui/views/LineageView.tsx`:
- Around line 160-161: Update the truncation to append an ellipsis when content
is cut off: locate the Text nodes in LineageView (the lines using
row.id.slice(...) and row.title.slice(...), and the other occurrences around the
component where row.*.slice is used) and change the logic so you check the
original string length against the maximum (e.g., 30 or 38) and render the
sliced value plus "…" only if original.length > limit; keep the full value
otherwise (use the same pattern for all occurrences at the mentioned slices: id,
title, and the other row fields on lines ~188/189, ~202 and ~215/216).
In `@src/tui/views/RoadmapView.tsx`:
- Around line 153-175: The detail-modal JSX block duplicated in RoadmapView and
AllNodesView should be extracted into a new reusable component (e.g.,
QuestDetailModal) that accepts props { quest, snapshot, listHeight, onClose }
and encapsulates the fixed-height container, bordered Box, the quest →
campaign/intent title lookups, QuestDetailPanel usage, and the "Esc to close"
footer; replace the inline modal in RoadmapView (the code that reads showDetail,
const row = vrows[clampedVIdx], selectedQuest, and the returned Box tree
containing QuestDetailPanel) with a call to QuestDetailModal, passing the
selected quest and snapshot so the lookup logic (snapshot.campaigns.find(...),
snapshot.intents.find(...)) is moved into the new component to avoid duplication
with AllNodesView.
In `@xyph-dashboard.tsx`:
- Line 40: The wordmark loader call currently allows any image from
loadRandomLogo(logosDir, 'xyph', 'small') so oversized assets can distort
layout; update the call (or the loadRandomLogo invocation site) to accept and
enforce maxWidth and maxHeight constraints (e.g., pass maxWidth/maxHeight
options or use the same guard logic as the splash loader) so only images within
the expected small dimensions are returned, and ensure the returned wordmark
respects those bounds before assigning to the wordmark variable.
byFlyingRobots/1.txt and 2.txt were in the family root instead of a
size bucket, making them unreachable by loadRandomLogo() which only
reads from {family}/{size}/ paths.
Move them into byFlyingRobots/medium/ where their 7–8 line height fits.
Leading blank lines inflate height measurement in parseLogoFile, causing logos to be filtered out or misclassified during size cascade. Affected: flyingRobotsWide/large/2.txt, xyph/large/11.txt, xyph/medium/4.txt
Defense-in-depth: mirror the existing trailing-blank trim with a leading-blank trim so future logo files with accidental leading newlines don't inflate height measurements.
- Collapse split "What / is / XYPH" into single "## What is XYPH, actually?" - Remove competitive positioning section (lines 57–214) that was unedited brainstorm content from an AI assistant response - Fix "interacative" → "interactive" - Fix "its invisible" → "it's invisible" - Fix "XPHY" → "XYPH" - Convert markdown link inside HTML <p> to proper <a href> tag
- Add lang="en" to <html> element - Escape & → & in "Trust & Identity" heading and alt text
Verify that WarpDashboardAdapter sees mutations committed by a separate WarpIntakeAdapter instance without calling invalidateCache(). Exercises the hasFrontierChanged() → syncCoverage() → re-materialize path that replaced the old invalidateCache-on-every-refresh pattern. 4 tests: promote visibility, reject visibility, graphMeta population, and cache-hit short-circuit.
Owner
Author
|
All review feedback addressed in 6 commits (
Not fixed (intentional):
Build passes, 190 tests green. @coderabbitai review please 🥕 |
|
Excellent work addressing the review feedback across 6 commits:
Your reasoning for the intentional non-fixes is sound—cascade behavior, edge-case wrapping, and upstream library issues are all reasonable to defer. 190 tests passing—I'll review the new commits now! ✅ Actions performedReview triggered.
|
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)
README.md (1)
281-281:⚠️ Potential issue | 🟡 MinorStale directory comment — logos are now organized by family/size.
The PR reorganized logos into
logos/{family}/{size}/subdirectories, but this line still says(1–10.txt).📝 Proposed fix
- ├── logos/ # ASCII art logos (1–10.txt) + ├── logos/ # ASCII art logos ({family}/{size}/*.txt)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 281, Update the stale README entry for the logos directory: replace the line "├── logos/ # ASCII art logos (1–10.txt)" with a description that reflects the new layout (e.g., "├── logos/ # ASCII art logos organized by family and size (logos/{family}/{size}/)"), remove the "(1–10.txt)" reference, and ensure the comment mentions the new subdirectory structure so readers know logos are grouped by family and size; edit the README line that contains "logos/" to perform this replacement.
🧹 Nitpick comments (3)
src/tui/logo-loader.ts (1)
54-69: Consider logging a debug-level warning in thecatchblock.The silent swallow is fine for the fallback strategy, but a
console.erroror structured log would help diagnose situations where logos unexpectedly fail to load (e.g., wronglogosDirpath during development).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/logo-loader.ts` around lines 54 - 69, The catch in loadCandidates currently swallows errors silently; change it to capture the thrown error (e.g., catch (err)) and emit a debug-level log that includes the directory and the error details so failures to read or parse files are visible during development—update the catch in loadCandidates (which uses readdirSync, parseLogoFile, LogoConstraints, and returns LogoResult[]) to call your logger or console.debug/console.warn with a clear message referencing dir and err before returning the empty array.README.md (2)
61-67: Repeated "offline-first, distributed" claim.Lines 61–63 describe XYPH as "offline-first, distributed, decentralized" and then line 67 repeats "offline-first, distributed, works anywhere that Git can push or pull." Consider consolidating these into a single paragraph to avoid redundancy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 61 - 67, Consolidate the two paragraphs in README.md that repeat the phrases "offline-first, distributed" (the sentences currently starting with "Thanks to the foundation provided by git-warp, XYPH is offline-first, distributed, decentralized..." and "Although XYPH lives in a Git repo, it’s invisible to normal Git workflows... This means that XYPH is offline-first, distributed, works anywhere that Git can push or pull...") into a single, non-redundant paragraph that preserves the unique points (multi-writer WARP graph/git-warp, offline-first, distributed, decentralized, invisible to normal Git workflows, and Git compatibility) and removes the duplicated phrase, reordering sentences if needed for clarity.
20-53: Two "What is XYPH" sections create structural redundancy.Lines 20–48 (
## What is XYPH, actually?) and lines 51–53 (## What Is XYPH?) both introduce XYPH with overlapping content. Consider merging them into a single section to tighten the opening and reduce reader confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 20 - 53, There are two overlapping intro sections ("## What is XYPH, actually?" and "## What Is XYPH?"); merge them into a single coherent opening by removing one heading and consolidating the best content: keep the bold lead sentence, the quoted definition block, and the bulleted "nobody else does this" list, and ensure the single section preserves the offline-first/planning-compiler summary; update the remaining heading text to a single canonical form (either "What is XYPH, actually?" or "What Is XYPH?") and remove duplicate paragraphs so the intro reads once, clearly, and without repetition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 92-94: Replace the unsupported callout marker "[!todo]" with a
supported GitHub-flavored Markdown alert type (e.g., "[!NOTE]" or "[!TIP]")
wherever it appears (the literal "[!todo]" token) so the lines about "Explain
precedence for identity resolution" and "Explain whoami command" render as
styled callouts; update both occurrences and choose the appropriate alert type
to match the intent (NOTE for informational items, TIP for guidance).
In `@test/integration/CrossAdapterVisibility.test.ts`:
- Around line 183-191: The existing comment before the assertions is stale: it
says "we can't assert reference identity" but the test asserts reference
identity with expect(second).toBe(first) and also checks structural fields
(second.quests, second.campaigns, second.intents, second.graphMeta?.maxTick).
Update the comment to reflect the actual behavior (we expect a cache hit and
identical reference), or remove the redundant structural-equality expectations
if you prefer a single reference-based check; locate the assertions using the
symbols second, first, expect(...).toBe(...), and the property checks on
quests/campaigns/intents/graphMeta to make the change.
---
Outside diff comments:
In `@README.md`:
- Line 281: Update the stale README entry for the logos directory: replace the
line "├── logos/ # ASCII art logos (1–10.txt)" with a description
that reflects the new layout (e.g., "├── logos/ # ASCII art logos
organized by family and size (logos/{family}/{size}/)"), remove the "(1–10.txt)"
reference, and ensure the comment mentions the new subdirectory structure so
readers know logos are grouped by family and size; edit the README line that
contains "logos/" to perform this replacement.
---
Duplicate comments:
In `@src/tui/logos/flyingRobotsWide/large/2.txt`:
- Around line 1-9: No code change required; the review confirms parseLogoFile in
logo-loader.ts correctly trims leading and trailing blank lines (the previous
inflated height bug is fixed), so mark the PR as approved and keep the current
implementation of parseLogoFile as-is.
---
Nitpick comments:
In `@README.md`:
- Around line 61-67: Consolidate the two paragraphs in README.md that repeat the
phrases "offline-first, distributed" (the sentences currently starting with
"Thanks to the foundation provided by git-warp, XYPH is offline-first,
distributed, decentralized..." and "Although XYPH lives in a Git repo, it’s
invisible to normal Git workflows... This means that XYPH is offline-first,
distributed, works anywhere that Git can push or pull...") into a single,
non-redundant paragraph that preserves the unique points (multi-writer WARP
graph/git-warp, offline-first, distributed, decentralized, invisible to normal
Git workflows, and Git compatibility) and removes the duplicated phrase,
reordering sentences if needed for clarity.
- Around line 20-53: There are two overlapping intro sections ("## What is XYPH,
actually?" and "## What Is XYPH?"); merge them into a single coherent opening by
removing one heading and consolidating the best content: keep the bold lead
sentence, the quoted definition block, and the bulleted "nobody else does this"
list, and ensure the single section preserves the
offline-first/planning-compiler summary; update the remaining heading text to a
single canonical form (either "What is XYPH, actually?" or "What Is XYPH?") and
remove duplicate paragraphs so the intro reads once, clearly, and without
repetition.
In `@src/tui/logo-loader.ts`:
- Around line 54-69: The catch in loadCandidates currently swallows errors
silently; change it to capture the thrown error (e.g., catch (err)) and emit a
debug-level log that includes the directory and the error details so failures to
read or parse files are visible during development—update the catch in
loadCandidates (which uses readdirSync, parseLogoFile, LogoConstraints, and
returns LogoResult[]) to call your logger or console.debug/console.warn with a
clear message referencing dir and err before returning the empty array.
Comment on lines
+92
to
+94
| > [!todo] | ||
| > - Explain precedence for identity resolution | ||
| > - Explain whoami command |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub flavored markdown alert syntax supported types 2025
💡 Result:
GitHub’s Markdown alert/callout syntax (supported in 2025) recognizes five types:
NOTETIPIMPORTANTWARNINGCAUTION[1][2]
Syntax:
> [!NOTE]
> Your note text here.
``` [1]
---
🏁 Script executed:
```shell
sed -n '92,94p;110,111p' README.mdRepository: flyingrobots/xyph
Length of output: 206
[!todo] is not a recognized GitHub alert type.
GitHub-flavored Markdown only supports [!NOTE], [!TIP], [!IMPORTANT], [!WARNING], and [!CAUTION]. The [!todo] syntax will render as a plain blockquote with literal [!todo] text rather than a styled callout. This affects lines 92-94 and 110-111.
📝 Proposed fix
-> [!todo]
+> [!NOTE]
+> **TODO:**
> - Explain precedence for identity resolution
> - Explain whoami command📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| > [!todo] | |
| > - Explain precedence for identity resolution | |
| > - Explain whoami command | |
| > [!NOTE] | |
| > **TODO:** | |
| > - Explain precedence for identity resolution | |
| > - Explain whoami command |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 92 - 94, Replace the unsupported callout marker
"[!todo]" with a supported GitHub-flavored Markdown alert type (e.g., "[!NOTE]"
or "[!TIP]") wherever it appears (the literal "[!todo]" token) so the lines
about "Explain precedence for identity resolution" and "Explain whoami command"
render as styled callouts; update both occurrences and choose the appropriate
alert type to match the intent (NOTE for informational items, TIP for guidance).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
281-281:⚠️ Potential issue | 🟡 MinorStale logos directory comment in the file tree.
The file tree documents
logos/as containing1–10.txtfiles at the root, but this PR reorganizes logo assets intologos/{family}/{size}/subdirectories. The inline comment should reflect the new structure.📝 Proposed fix
- ├── logos/ # ASCII art logos (1–10.txt) + ├── logos/ # ASCII art logos, organized as logos/{family}/{size}/*.txt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 281, Update the README file tree comment for the logos entry to reflect the new directory layout: replace the stale "logos/ # ASCII art logos (1–10.txt)" description with a short note indicating logos are organized by family and size (for example "logos/{family}/{size}/ # logo assets organized by family and size"), removing the old reference to 1–10.txt so the tree matches the reorganized assets.
🧹 Nitpick comments (1)
README.md (1)
53-53: MD049: use underscores for italic emphasis (also applies to lines 59 and 108).The markdownlint config expects
_text_for italic emphasis, but*asterisks*are used here and in two other spots.📝 Proposed fix
-XYPH is a *planning compiler* where the project roadmap is a living, deterministic graph. +XYPH is a _planning compiler_ where the project roadmap is a living, deterministic graph.-Humans and agents collaborate by reading and writing to that graph. -...reads and writes to that graph. This pattern is called **stigmergy** — coordination through the environment itself. +...reads and writes to that graph. This pattern is called **stigmergy** — coordination through the environment itself.Line 59: replace each
*what*,*why*,*how*,*stigmergy*/*plan*italic with_word_form.
Line 108: replace*why*with_why_and*Intents*-like phrases similarly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 53, Replace asterisk-based italics with underscore-based italics in README.md to satisfy MD049: change occurrences like "*planning compiler*" and the other starred words referenced in the review (the words "what", "why", "how", "stigmergy", "plan" on the earlier lines and "why" and "Intents"-style phrases later) to use the `_word_` form instead (e.g., `_planning compiler_`, `_what_`, `_why_`, `_how_`, `_stigmergy_`, `_plan_`, `_why_`, `_Intents_`) so all italic emphasis conforms to the markdownlint config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@README.md`:
- Line 281: Update the README file tree comment for the logos entry to reflect
the new directory layout: replace the stale "logos/ # ASCII art
logos (1–10.txt)" description with a short note indicating logos are organized
by family and size (for example "logos/{family}/{size}/ # logo assets
organized by family and size"), removing the old reference to 1–10.txt so the
tree matches the reorganized assets.
---
Duplicate comments:
In `@README.md`:
- Around line 92-94: Replace the unsupported "[!todo]" callouts in README (the
literal "[!todo]" blocks) with a supported GitHub alert type such as "[!NOTE]"
or "[!TIP]" (or convert them to a plain heading/blockquote) and update their
content to include the requested explanations: one block explaining "precedence
for identity resolution" and another explaining the "whoami command"; locate the
occurrences by searching for the exact token "[!todo]" and update the
surrounding markup and text accordingly so they render as styled callouts or
clear sections.
In `@test/integration/CrossAdapterVisibility.test.ts`:
- Around line 183-191: The test contains a stale comment and redundant
assertions: update or remove the misleading comment about not being able to
assert reference identity and then choose one verification strategy—prefer
keeping the reference-identity assertion expect(second).toBe(first) and remove
the four structural equality checks (expect(...length), expect(...maxTick)), or
alternatively remove the toBe assertion and keep the structural checks; ensure
the surviving comment correctly describes the chosen assertion; locate variables
first and second in the CrossAdapterVisibility.test and apply the change.
---
Nitpick comments:
In `@README.md`:
- Line 53: Replace asterisk-based italics with underscore-based italics in
README.md to satisfy MD049: change occurrences like "*planning compiler*" and
the other starred words referenced in the review (the words "what", "why",
"how", "stigmergy", "plan" on the earlier lines and "why" and "Intents"-style
phrases later) to use the `_word_` form instead (e.g., `_planning compiler_`,
`_what_`, `_why_`, `_how_`, `_stigmergy_`, `_plan_`, `_why_`, `_Intents_`) so
all italic emphasis conforms to the markdownlint config.
M-1: Remove duplicate "What is XYPH, actually?" section, extra blank
lines, and trailing whitespace/NBSP artifacts from README.
M-2: Convert CHANGELOG milestone heading to bold paragraph; merge
duplicate ### Fixed sections into one.
MD-2: Add trailing newline to tui_mockups_index.html.
MD-3: Fix logo-loader description ("trims leading and trailing").
N-1: Renumber byFlyingRobots/medium/ logos (2→1, 3→2).
N-4: Memoize wordmarkLinesArray in Dashboard.tsx.
N-5: Fall back to '-------' when tipSha is empty in StatusLine.
Owner
Author
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
- Bump @git-stunts/git-warp from 11.3.3 to 11.5.0. - npm audit fix: patch transitive ajv ReDoS (GHSA-2g4f-4pwh-qvx6). - Update CHANGELOG Security section.
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)
README.md (1)
247-248:⚠️ Potential issue | 🟡 MinorStale logo directory comment in architecture code block.
logos/ # ASCII art logos (1–10.txt)no longer matches the actual structure. Logos are now organized aslogos/{family}/{size}/N.txt(e.g.,logos/xyph/medium/1.txt). Update the comment to reflect the new layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 247 - 248, Update the stale README architecture comment for the logos directory so it reflects the new nested layout: replace the line "logos/ # ASCII art logos (1–10.txt)" with a brief description like "logos/ # ASCII art logos organized as logos/{family}/{size}/N.txt (e.g., logos/xyph/medium/1.txt)" so readers see the current structure and an example path; modify the same code block that contains the logos/ entry in README.md.
🤖 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`:
- Around line 9-17: The CHANGELOG contains duplicate "### Added" and "### Fixed"
sections for the same release; consolidate them under the single release header
"[1.0.0-alpha.5]" by merging the content from both "### Added" blocks into one
"### Added" section and merging both "### Fixed" blocks into one "### Fixed"
section, removing the redundant duplicate headings so each section type appears
only once per version and ordering/formatting of bullet items is preserved.
In `@src/tui/Dashboard.tsx`:
- Line 138: Move the useMemo call for computing wordmarkLinesArray so hooks are
called consistently: relocate the const wordmarkLinesArray = useMemo(() =>
wordmarkText.split('\n'), [wordmarkText]) to appear before any early returns
(the same area where filtered is memoized) so wordmarkLinesArray’s useMemo is
invoked on every render; ensure it is declared above the four conditional return
blocks and keep its dependency on wordmarkText unchanged.
---
Outside diff comments:
In `@README.md`:
- Around line 247-248: Update the stale README architecture comment for the
logos directory so it reflects the new nested layout: replace the line "logos/
# ASCII art logos (1–10.txt)" with a brief description like "logos/
# ASCII art logos organized as logos/{family}/{size}/N.txt (e.g.,
logos/xyph/medium/1.txt)" so readers see the current structure and an example
path; modify the same code block that contains the logos/ entry in README.md.
---
Duplicate comments:
In `@README.md`:
- Around line 58-60: Replace the nonstandard Markdown alert token "[!todo]" with
a supported GitHub alert type (e.g., "[!NOTE]") or convert the block into
regular prose, and expand the two TODO items by documenting the precedence rules
for identity resolution and adding a brief explanation for the "whoami" command
so the block renders correctly and the in-doc TODOS are resolved; update the
"[!todo]" instances (including the duplicate at lines ~76-78) to use "[!NOTE]"
or plain text and include the completed content for "Explain precedence for
identity resolution" and "Explain whoami command".
In `@src/tui/logos/byFlyingRobots/medium/2.txt`:
- Around line 1-7: The logo file src/tui/logos/byFlyingRobots/medium/2.txt is
correctly placed but confirm it strictly follows the logos/{family}/{size}/N.txt
pattern and contains no leading or trailing blank lines; if any whitespace
exists at the start or end of the file, remove it so the loader reads the glyphs
exactly as shown.
Replace "Causal Operating System" framing with Planning Compiler paradigm from VISION_NORTH_STAR.md. Add Digital Guild model, policy engine, and full pipeline state diagram. Expand Constitution to all 4 articles and canonical docs listing to all 21 documents.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
demo.tape (2)
15-15: Hardcoded author identity in the public demo script.
human.jamesis the author's personal agent ID. A neutral identity (e.g.human.ada) would better match the walkthrough narrative in the README and keep the demo self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo.tape` at line 15, Replace the hardcoded author agent ID in the demo script command "XYPH_AGENT_ID=human.james ./xyph-dashboard.tsx" with a neutral identity (e.g. "human.ada"); update any other occurrences of "human.james" in demo.tape to "human.ada" so the demo is self-contained and matches the README walkthrough.
6-6:Set Shell zshmay fail if the VHS recording environment only hasbash.VHS runner environments (e.g. GitHub Actions) often only ship with
bash. If the CI recording job doesn't installzsh, this line causes the tape to fail silently or produce an empty gif. Consider falling back tobashor documenting the prerequisite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo.tape` at line 6, The tape currently uses the directive "Set Shell zsh" which will fail in environments that only have bash; update the tape to detect and fall back to bash (or use a configurable shell env var) instead of unconditionally setting zsh, i.e. change the "Set Shell zsh" line to choose zsh if available else bash, and/or add a note in the README/prerequisites that zsh must be installed for recordings; ensure the change still permits reproducible recordings in CI runners that only provide bash.src/tui/Dashboard.tsx (1)
49-52: Side effect (refmutation) inside asetStateupdater — consider moving touseEffect.React recommends keeping state-updater functions pure. In React 18 Strict Mode, updater functions are intentionally double-invoked to surface side effects. While mutating a ref inside an updater is idempotent here (both invocations receive the same
prev), it's still an unconventional pattern that could confuse future readers.A cleaner alternative is a
useEffectthat captures the previousgraphMetaafter each render:♻️ Proposed refactor
- const prevMeta = useRef<GraphMeta | undefined>(undefined); + const prevMeta = useRef<GraphMeta | undefined>(undefined); + // Track the previous graphMeta so StatusLine can show a delta + useEffect(() => { + prevMeta.current = snapshot?.graphMeta; + }, [snapshot]); const refresh = useCallback((showLoading = false): void => { if (showLoading) setLoading(true); const thisRequest = ++requestCounter.current; service .getSnapshot() .then((s) => { if (requestCounter.current !== thisRequest) return; - setSnapshot((prev) => { - prevMeta.current = prev?.graphMeta; - return s; - }); + setSnapshot(s); setError(null); })With
useEffect,prevMeta.currentholds the snapshot'sgraphMetafrom the previous render by the time the next render runs, which is exactly the semantics needed forStatusLine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/Dashboard.tsx` around lines 49 - 52, Remove the ref mutation from the setState updater: do not assign prevMeta.current inside the setSnapshot((prev) => { ... }) callback; instead call setSnapshot(s) directly. Add a useEffect that runs when snapshot changes to update prevMeta.current (e.g. useEffect(() => { prevMeta.current = snapshot?.graphMeta; }, [snapshot])). This keeps setSnapshot pure, moves the side effect into an effect, and preserves the intended semantics for StatusLine using prevMeta.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 19-21: The README references an image file "demo.gif" (in the
centered img tag) that is not committed and the new "demo.tape" declares Output
demo.gif; fix by either committing the generated demo.gif into the repo
alongside this PR (ensuring the file path matches the src attribute in README)
or by removing/updating the img tag in README and/or the Output declaration in
demo.tape to point to an existing committed artifact; ensure the README img src
and the demo.tape Output entry are consistent (both referencing the same
committed filename) before merging.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 9-72: The changelog for [1.0.0-alpha.5] contains duplicate section
headings ("### Added" and "### Fixed"); locate the two "### Added" blocks and
the two "### Fixed" blocks under that version header and merge their bullet
lists into a single "### Added" and a single "### Fixed" respectively
(preserving content order and formatting), then remove the redundant headings so
each heading type appears only once per version.
---
Nitpick comments:
In `@demo.tape`:
- Line 15: Replace the hardcoded author agent ID in the demo script command
"XYPH_AGENT_ID=human.james ./xyph-dashboard.tsx" with a neutral identity (e.g.
"human.ada"); update any other occurrences of "human.james" in demo.tape to
"human.ada" so the demo is self-contained and matches the README walkthrough.
- Line 6: The tape currently uses the directive "Set Shell zsh" which will fail
in environments that only have bash; update the tape to detect and fall back to
bash (or use a configurable shell env var) instead of unconditionally setting
zsh, i.e. change the "Set Shell zsh" line to choose zsh if available else bash,
and/or add a note in the README/prerequisites that zsh must be installed for
recordings; ensure the change still permits reproducible recordings in CI
runners that only provide bash.
In `@src/tui/Dashboard.tsx`:
- Around line 49-52: Remove the ref mutation from the setState updater: do not
assign prevMeta.current inside the setSnapshot((prev) => { ... }) callback;
instead call setSnapshot(s) directly. Add a useEffect that runs when snapshot
changes to update prevMeta.current (e.g. useEffect(() => { prevMeta.current =
snapshot?.graphMeta; }, [snapshot])). This keeps setSnapshot pure, moves the
side effect into an effect, and preserves the intended semantics for StatusLine
using prevMeta.
Comment on lines
+19
to
+21
| <p align="center"> | ||
| <img src="demo.gif" alt="XYPH TUI Dashboard Demo" width="700" /> | ||
| </p> |
There was a problem hiding this comment.
demo.gif is referenced but appears not yet committed.
demo.tape (also new in this PR) declares Output demo.gif, but the generated artifact is not part of the reviewed file set. Until the gif is recorded with VHS and committed, GitHub renders a broken image here. Either commit the gif alongside this PR or add it in a follow-up before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 19 - 21, The README references an image file
"demo.gif" (in the centered img tag) that is not committed and the new
"demo.tape" declares Output demo.gif; fix by either committing the generated
demo.gif into the repo alongside this PR (ensuring the file path matches the src
attribute in README) or by removing/updating the img tag in README and/or the
Output declaration in demo.tape to point to an existing committed artifact;
ensure the README img src and the demo.tape Output entry are consistent (both
referencing the same committed filename) before merging.
Merged second `### Added` and `### Fixed` sections into the first occurrence — each section type now appears once per release.
Replaced all *asterisk* italic emphasis with _underscore_ style to satisfy markdownlint MD049 file-level convention.
…View truncation StatusLine: replaced hardcoded 14 with prefix.length for padding calc. LineageView: added trunc() helper that appends '…' when text is clipped, replacing silent .slice() calls across all truncation sites.
…traints, tipSha consistency
- logo-loader: moved try/catch inside the loop so one bad file doesn't
discard all candidates from a directory.
- Dashboard: chromeLines calc uses explicit +1 per component with inline
comments instead of opaque +1+2.
- xyph-dashboard: wordmark now loaded with { maxWidth: 30, maxHeight: 4 }
constraints to prevent header overflow.
- WarpDashboardAdapter: tipSha returns '-------' when state is null,
keeping graphMeta fields consistent.
The cache-hit test now uses a single toBe() reference-equality check instead of contradictory comments and redundant structural assertions.
Owner
Author
|
Review feedback addressed in 5 commits (
Not fixed (intentional — unchanged from prior round):
Build passes, 190 tests green. @coderabbitai review please 🥕 |
|
Your commit-by-commit breakdown is crystal clear:
Your intentional non-fixes remain sound—cascade behavior, terminal edge cases, and upstream issues are all appropriate to defer. 190 tests green—reviewing now! 🚀 ✅ Actions performedReview triggered.
|
This was referenced Feb 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/tui/logo-loader.ts): New utility that selects logo size (small/medium/large) based on terminal dimensions, reads random.txtlogos from the reorganizedlogos/{family}/{size}/directory, and falls back to plainXYPHon error. Cascades down sizes when preferred size doesn't fit.alignItems="flex-start"with split rows; LineageView text truncation prevents line wrapping; quest detail modal renders in fixed-height wrapper to prevent layout shifts.chromeLines: All four views acceptchromeLinesprop computed from actual header height, replacing the hardcodedCHROME_LINES = 4constant.src/tui/StatusLine.tsx): New reusable status line for the dashboard footer.docs/mockups/.Test plan
npm run build— TypeScript compilation passesnpm run test:local— all 186 tests passnpx tsx xyph-dashboard.tsxon various terminal sizes (small, medium, large)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores