feat(ui): upgrade git-cas to a Bijou v3 repository explorer#24
Open
flyingrobots wants to merge 22 commits intomainfrom
Open
feat(ui): upgrade git-cas to a Bijou v3 repository explorer#24flyingrobots wants to merge 22 commits intomainfrom
flyingrobots wants to merge 22 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDashboard converted into a CAS-aware explorer: added Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (`bin/git-cas.js`)
participant Init as Dashboard Init (`bin/ui/dashboard.js`)
participant Cmds as Dashboard Cmds (`bin/ui/dashboard-cmds.js`)
participant CAS as CAS Backend
participant UI as Surface Renderer / TUI
CLI->>Init: launchDashboard(cas, { source?, cwd? })
Init->>Cmds: readSourceEntries(cas, source)
Cmds->>CAS: resolve (listVault / resolveRef / readBlob / show-ref) per source
CAS-->>Cmds: entries / refs / blobs
Cmds-->>Init: entries / treemap report / refs inventory (include forSource)
Init->>Init: createInitModel(ctx, source)
Init->>UI: runApp(app, model)
UI->>Init: user input events (keys)
Init->>Cmds: dispatch load*Cmd (treemap/refs/stats/doctor/manifest)
Cmds-->>Init: loaded-* events (include forSource)
Init->>UI: render updated Surface (drawers, treemap, toasts)
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/ui/dashboard-cmds.js`:
- Around line 1470-1480: The loader loadStatsCmd must not use the
caller-supplied entries snapshot; instead have it fetch the canonical entries
itself by calling readSourceEntries(cas, source) inside loadStatsCmd before
building the stats (so buildVaultStats runs on the fresh entries), and return
the same result shape; update the analogous loader at the other block (the one
around lines 1491-1500) to do the same. Locate loadStatsCmd and the sibling
loader, replace usage of the passed-in entries array with an awaited
readSourceEntries(cas, source) result, then proceed to map/read manifests and
call buildVaultStats; ensure error handling and returned shape (type:
'loaded-stats' / 'load-error') remain unchanged so caching in
bin/ui/dashboard.js receives the correct source-scoped data.
- Around line 1521-1540: In loadTreemapCmd's catch block, the returned error
object is missing the source context so stale/future-source messages can
collide; update the error return (the object returned in the catch of
loadTreemapCmd after buildRepoTreemapReport) to include forSource using the same
source context as the success path (use report.source if available or the local
source variable) so the returned payload contains forSource alongside
type:'load-error', source:'treemap', scopeId, worktreeMode, drillPath, and
error.
In `@bin/ui/dashboard-view.js`:
- Around line 914-917: The current width math (maxSidebarWidth, sidebarWidth,
listWidth) can exceed the available columns and cause overflow; modify the
layout code that computes sidebarWidth and listWidth so after computing desired
sizes you check if sidebarWidth + listWidth + 1 > options.screen.width and then
either clamp listWidth = Math.max(0, options.screen.width - sidebarWidth - 1)
or, if that would make either pane below a minimum usable width, switch to a
stacked/vertical layout for the view; update the same logic used elsewhere (the
other block around the variables at lines referenced) so both refs and treemap
modes perform the clamp-or-stack fallback instead of relying on composed
minimums.
In `@bin/ui/dashboard.js`:
- Around line 1569-1583: The treemap drill handlers are being invoked globally
from handleOverlayAction causing '+'/'-' to affect the main view; limit those
drill-related handlers to when the treemap drawer is active (e.g., check
model.activeOverlay === 'treemap' or similar state) or delegate to
handleTreemapViewAction which already handles active-treemap input. Update
handleOverlayAction so it only registers/calls 'treemap-drill-in' and
'treemap-drill-out' (and any treemap-specific key handlers) when the treemap
overlay is open, otherwise return no-op.
In `@bin/ui/repo-treemap.js`:
- Around line 450-456: In renderLegendLines, clip is being called on the
already-colored string so ANSI escape codes count toward width; instead, first
call clip on the plain label `${fill} ${TILE_LABEL[kind]}` to trim to width,
then pass the clipped plain string into ctx.style.rgb(...) to apply color;
update the return mapping in renderLegendLines to compute fill, rawLabel =
`${fill} ${TILE_LABEL[kind]}`, clipped = clip(rawLabel, width) and then style
with ctx.style.rgb using TILE_COLOR[kind] so clip measures the unstyled text
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab9f3303-da40-466a-a637-bf2b9280d484
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
bin/git-cas.jsbin/ui/dashboard-cmds.jsbin/ui/dashboard-view.jsbin/ui/dashboard.jsbin/ui/encryption-card.jsbin/ui/manifest-view.jsbin/ui/repo-treemap.jsbin/ui/theme.jsbin/ui/vault-report.jspackage.jsontest/unit/cli/dashboard-cmds.test.jstest/unit/cli/dashboard.launch.test.jstest/unit/cli/dashboard.test.jstest/unit/cli/manifest-view.test.jstest/unit/cli/repo-treemap.test.jstest/unit/cli/vault-report.test.js
Comment on lines
+1470
to
+1480
| export function loadStatsCmd(cas, entries, source) { | ||
| return async () => { | ||
| try { | ||
| const records = await Promise.all(entries.map(async (entry) => ({ | ||
| ...entry, | ||
| manifest: await cas.readManifest({ treeOid: entry.treeOid }), | ||
| }))); | ||
| return /** @type {const} */ ({ type: 'loaded-stats', stats: buildVaultStats(records), source }); | ||
| } catch (/** @type {any} */ err) { | ||
| return /** @type {const} */ ({ type: 'load-error', source: 'stats', forSource: source, error: /** @type {Error} */ (err).message }); | ||
| } |
There was a problem hiding this comment.
Don't build source-scoped reports from the caller's current entries snapshot.
These loaders depend on whatever entries array the dashboard happens to have at dispatch time. If the user opens Stats — or the non-vault Doctor view — while loaded-entries is still in flight for the active source, that snapshot is still empty. The resulting zero-entry report is then cached as ready in bin/ui/dashboard.js and won't refresh on reopen. These commands should resolve readSourceEntries(cas, source) themselves, or the caller should defer dispatch until the source entries are ready.
Also applies to: 1491-1500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ui/dashboard-cmds.js` around lines 1470 - 1480, The loader loadStatsCmd
must not use the caller-supplied entries snapshot; instead have it fetch the
canonical entries itself by calling readSourceEntries(cas, source) inside
loadStatsCmd before building the stats (so buildVaultStats runs on the fresh
entries), and return the same result shape; update the analogous loader at the
other block (the one around lines 1491-1500) to do the same. Locate loadStatsCmd
and the sibling loader, replace usage of the passed-in entries array with an
awaited readSourceEntries(cas, source) result, then proceed to map/read
manifests and call buildVaultStats; ensure error handling and returned shape
(type: 'loaded-stats' / 'load-error') remain unchanged so caching in
bin/ui/dashboard.js receives the correct source-scoped data.
Comment on lines
+1521
to
+1540
| export function loadTreemapCmd(cas, options = {}) { | ||
| const { | ||
| source = { type: 'vault' }, | ||
| scope = 'repository', | ||
| worktreeMode = 'tracked', | ||
| drillPath = [], | ||
| } = options; | ||
| return async () => { | ||
| try { | ||
| const manifest = await cas.readManifest({ treeOid }); | ||
| return /** @type {const} */ ({ type: 'loaded-manifest', slug, manifest }); | ||
| const report = await buildRepoTreemapReport(cas, { source, scope, worktreeMode, drillPath }); | ||
| return /** @type {const} */ ({ type: 'loaded-treemap', report }); | ||
| } catch (/** @type {any} */ err) { | ||
| return /** @type {const} */ ({ type: 'load-error', source: 'manifest', slug, error: /** @type {Error} */ (err).message }); | ||
| return /** @type {const} */ ({ | ||
| type: 'load-error', | ||
| source: 'treemap', | ||
| scopeId: scope, | ||
| worktreeMode, | ||
| drillPath, | ||
| error: /** @type {Error} */ (err).message, | ||
| }); |
There was a problem hiding this comment.
Include forSource on treemap load failures.
bin/ui/dashboard.js can ignore stale source loads only when the message carries forSource. The success path already includes report.source, but this error path drops source context entirely, so a late failure for source A can still mark source B's treemap as failed when the scope/path happen to match.
Possible fix
return async () => {
try {
const report = await buildRepoTreemapReport(cas, { source, scope, worktreeMode, drillPath });
return /** `@type` {const} */ ({ type: 'loaded-treemap', report });
} catch (/** `@type` {any} */ err) {
return /** `@type` {const} */ ({
type: 'load-error',
source: 'treemap',
+ forSource: source,
scopeId: scope,
worktreeMode,
drillPath,
error: /** `@type` {Error} */ (err).message,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ui/dashboard-cmds.js` around lines 1521 - 1540, In loadTreemapCmd's catch
block, the returned error object is missing the source context so
stale/future-source messages can collide; update the error return (the object
returned in the catch of loadTreemapCmd after buildRepoTreemapReport) to include
forSource using the same source context as the success path (use report.source
if available or the local source variable) so the returned payload contains
forSource alongside type:'load-error', source:'treemap', scopeId, worktreeMode,
drillPath, and error.
Comment on lines
+914
to
+917
| const maxSidebarWidth = Math.max(22, options.screen.width - 25); | ||
| const sidebarWidth = Math.min(maxSidebarWidth, Math.max(30, Math.min(46, Math.floor(options.screen.width * 0.35)))); | ||
| const listWidth = Math.max(18, options.screen.width - sidebarWidth - 1); | ||
| const viewHeight = options.height; |
There was a problem hiding this comment.
These full-screen drawer width calculations still overflow narrow terminals.
The minimums do not compose. In refs mode, a 40-column terminal yields sidebarWidth = 22 and listWidth = 18, so the layout needs 41 columns with the divider. Treemap mode has the same problem below 35 columns. That guarantees truncated or out-of-bounds blits instead of a graceful fallback. Please clamp one pane to the remaining width, or switch to a stacked layout once both preferred minimums no longer fit.
Also applies to: 1009-1013
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ui/dashboard-view.js` around lines 914 - 917, The current width math
(maxSidebarWidth, sidebarWidth, listWidth) can exceed the available columns and
cause overflow; modify the layout code that computes sidebarWidth and listWidth
so after computing desired sizes you check if sidebarWidth + listWidth + 1 >
options.screen.width and then either clamp listWidth = Math.max(0,
options.screen.width - sidebarWidth - 1) or, if that would make either pane
below a minimum usable width, switch to a stacked/vertical layout for the view;
update the same logic used elsewhere (the other block around the variables at
lines referenced) so both refs and treemap modes perform the clamp-or-stack
fallback instead of relying on composed minimums.
Comment on lines
+1569
to
+1583
| function handleOverlayAction(action, model, deps) { | ||
| const handlers = { | ||
| 'open-palette': () => setPalette(model, createPalette(model.rows)), | ||
| 'open-stats': () => openStatsDrawer(model, deps), | ||
| 'open-doctor': () => openDoctorDrawer(model, deps), | ||
| 'open-refs': () => openRefsDrawer(model, deps), | ||
| 'open-treemap': () => openTreemapDrawer(model, deps), | ||
| 'toggle-treemap-scope': () => toggleTreemapScope(model, deps), | ||
| 'toggle-treemap-worktree': () => toggleTreemapWorktreeMode(model, deps), | ||
| 'treemap-drill-in': () => handleTreemapDrillIn(model, deps), | ||
| 'treemap-drill-out': () => handleTreemapDrillOut(model, deps), | ||
| 'overlay-close': () => closeOverlay(model), | ||
| }; | ||
| return action.type in handlers ? handlers[action.type]() : null; | ||
| } |
There was a problem hiding this comment.
Keep raw +/- drill keys scoped to the treemap drawer.
These handlers still run from the explorer and refs screens. After closing a drilled treemap, pressing - in the main view can reopen treemap and queue a load from the remembered treemapPath, which is very surprising. handleTreemapViewAction() already covers the active-treemap case, so the global overlay router should no-op outside that drawer.
Possible fix
function handleOverlayAction(action, model, deps) {
const handlers = {
'open-palette': () => setPalette(model, createPalette(model.rows)),
'open-stats': () => openStatsDrawer(model, deps),
'open-doctor': () => openDoctorDrawer(model, deps),
'open-refs': () => openRefsDrawer(model, deps),
'open-treemap': () => openTreemapDrawer(model, deps),
'toggle-treemap-scope': () => toggleTreemapScope(model, deps),
'toggle-treemap-worktree': () => toggleTreemapWorktreeMode(model, deps),
- 'treemap-drill-in': () => handleTreemapDrillIn(model, deps),
- 'treemap-drill-out': () => handleTreemapDrillOut(model, deps),
+ 'treemap-drill-in': () => model.activeDrawer === 'treemap'
+ ? handleTreemapDrillIn(model, deps)
+ : [model, []],
+ 'treemap-drill-out': () => model.activeDrawer === 'treemap'
+ ? handleTreemapDrillOut(model, deps)
+ : [model, []],
'overlay-close': () => closeOverlay(model),
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ui/dashboard.js` around lines 1569 - 1583, The treemap drill handlers are
being invoked globally from handleOverlayAction causing '+'/'-' to affect the
main view; limit those drill-related handlers to when the treemap drawer is
active (e.g., check model.activeOverlay === 'treemap' or similar state) or
delegate to handleTreemapViewAction which already handles active-treemap input.
Update handleOverlayAction so it only registers/calls 'treemap-drill-in' and
'treemap-drill-out' (and any treemap-specific key handlers) when the treemap
overlay is open, otherwise return no-op.
Comment on lines
+450
to
+456
| function renderLegendLines(ctx, width) { | ||
| return /** @type {Array<keyof typeof TILE_FILL>} */ (['worktree', 'git', 'ref', 'vault', 'cas', 'meta']) | ||
| .map((kind) => { | ||
| const fill = TILE_FILL[kind]; | ||
| const color = TILE_COLOR[kind]; | ||
| return clip(ctx.style.rgb(color[0], color[1], color[2], `${fill} ${TILE_LABEL[kind]}`), width); | ||
| }); |
There was a problem hiding this comment.
Clip the legend label before applying ANSI color.
clip() is measuring the raw styled string here, so with a real color-capable ctx.style.rgb() the escape sequences count toward width and the legend lines get truncated/broken almost immediately. Clip the plain ${fill} ${TILE_LABEL[kind]} text first, then style it.
Possible fix
function renderLegendLines(ctx, width) {
return /** `@type` {Array<keyof typeof TILE_FILL>} */ (['worktree', 'git', 'ref', 'vault', 'cas', 'meta'])
.map((kind) => {
const fill = TILE_FILL[kind];
const color = TILE_COLOR[kind];
- return clip(ctx.style.rgb(color[0], color[1], color[2], `${fill} ${TILE_LABEL[kind]}`), width);
+ const text = clip(`${fill} ${TILE_LABEL[kind]}`, width);
+ return ctx.style.rgb(color[0], color[1], color[2], text);
});
}📝 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
| function renderLegendLines(ctx, width) { | |
| return /** @type {Array<keyof typeof TILE_FILL>} */ (['worktree', 'git', 'ref', 'vault', 'cas', 'meta']) | |
| .map((kind) => { | |
| const fill = TILE_FILL[kind]; | |
| const color = TILE_COLOR[kind]; | |
| return clip(ctx.style.rgb(color[0], color[1], color[2], `${fill} ${TILE_LABEL[kind]}`), width); | |
| }); | |
| function renderLegendLines(ctx, width) { | |
| return /** `@type` {Array<keyof typeof TILE_FILL>} */ (['worktree', 'git', 'ref', 'vault', 'cas', 'meta']) | |
| .map((kind) => { | |
| const fill = TILE_FILL[kind]; | |
| const color = TILE_COLOR[kind]; | |
| const text = clip(`${fill} ${TILE_LABEL[kind]}`, width); | |
| return ctx.style.rgb(color[0], color[1], color[2], text); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ui/repo-treemap.js` around lines 450 - 456, In renderLegendLines, clip is
being called on the already-colored string so ANSI escape codes count toward
width; instead, first call clip on the plain label `${fill} ${TILE_LABEL[kind]}`
to trim to width, then pass the clipped plain string into ctx.style.rgb(...) to
apply color; update the return mapping in renderLegendLines to compute fill,
rawLabel = `${fill} ${TILE_LABEL[kind]}`, clipped = clip(rawLabel, width) and
then style with ctx.style.rgb using TILE_COLOR[kind] so clip measures the
unstyled text correctly.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
bin/ui/dashboard-view.js (1)
1019-1045:⚠️ Potential issue | 🟠 MajorThe full-screen split still overflows narrow terminals.
This is the same sizing problem called out earlier. Line 1020-Line 1022 still require 41 columns in refs mode (
22 + 18 + 1), and Line 1115-Line 1117 still require 35 columns in treemap mode (18 + 16 + 1). Clamp one pane to the remaining width or switch these drawers to a stacked/min-size fallback before blitting the divider and both panels.Also applies to: 1114-1145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/dashboard-view.js` around lines 1019 - 1045, renderRefsView (and the similar treemap view code) can still overflow narrow terminals because listWidth + sidebarWidth + divider assume fixed minima; compute availableWidth = options.screen.width - 1 (for divider) and clamp sidebarWidth and listWidth so sidebarWidth + listWidth <= availableWidth (e.g., ensure sidebarWidth = Math.min(sidebarWidth, availableWidth - minListWidth) and listWidth = Math.min(listWidth, availableWidth - minSidebarWidth)); if availableWidth is smaller than minListWidth + minSidebarWidth, switch to a stacked fallback (render the list panel above the detail panel) instead of side-by-side blitting; update the code paths in renderRefsView (and the treemap equivalent) to enforce these clamps before calling renderDividerSurface and options.screen.blit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/ui/dashboard-view.js`:
- Around line 863-892: The compact refs schema in refTableSchema currently
reserves two columns (Ref 16 + CAS 10) that overflow the smallest pane; update
refTableSchema so that when width is below the second threshold it renders a
single-column schema (only 'Ref' using width Math.max(16, width - 4) or similar)
or else compute the second column's width from the remaining budget (e.g., set
Ref width = Math.max(12, width - 12) and CAS width = Math.max(6, width -
RefWidth) dynamically) so the total never exceeds the available width; modify
the final returned object in refTableSchema (the fallback branch) to implement
one of these two behaviors and adjust the indexes accordingly.
- Around line 1249-1261: When terminal rows are very small (<9) renderDashboard
should use a compact layout instead of blitting the full 4-line header/footer
that overwrites the lone body line; modify renderDashboard to detect small
height (model.rows < 9) and either reduce header/footer heights (collapse
chrome) or render a single minimal surface (e.g., show only essential title and
footer) so bodyHeight remains >=1 and footer is blitted below the body without
overlapping it. Update the logic around bodyTop, bodyHeight, and the footer blit
so footer position is computed from the actual compact footer.height or by using
Math.max(0, height - footer.height) after adjusting header/footer sizes, and
ensure renderBody and renderOverlays are called with the adjusted top/height
values.
- Around line 130-155: appendSelectionBadges indexes model.filtered with the raw
model.table.focusRow which can be out-of-range after filtering; replace that
usage by computing a clamped focus index (use the project's shared clamped-index
helper used elsewhere for the table visible selection) and use
filtered[clampedIndex] instead of filtered[model.table.focusRow]; update the
initial selected assignment in appendSelectionBadges and any other places in
this file that index filtered/refsItems with model.table.focusRow (same pattern
as in the table code) so badges and inspectors use the same clamped selection as
the table.
---
Duplicate comments:
In `@bin/ui/dashboard-view.js`:
- Around line 1019-1045: renderRefsView (and the similar treemap view code) can
still overflow narrow terminals because listWidth + sidebarWidth + divider
assume fixed minima; compute availableWidth = options.screen.width - 1 (for
divider) and clamp sidebarWidth and listWidth so sidebarWidth + listWidth <=
availableWidth (e.g., ensure sidebarWidth = Math.min(sidebarWidth,
availableWidth - minListWidth) and listWidth = Math.min(listWidth,
availableWidth - minSidebarWidth)); if availableWidth is smaller than
minListWidth + minSidebarWidth, switch to a stacked fallback (render the list
panel above the detail panel) instead of side-by-side blitting; update the code
paths in renderRefsView (and the treemap equivalent) to enforce these clamps
before calling renderDividerSurface and options.screen.blit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 320a7521-ba66-47c6-a709-4aa9dbae7103
📒 Files selected for processing (3)
bin/ui/dashboard-view.jsbin/ui/dashboard.jstest/unit/cli/dashboard.test.js
✅ Files skipped from review due to trivial changes (1)
- test/unit/cli/dashboard.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/ui/dashboard.js
Comment on lines
+130
to
+155
| function appendSelectionBadges(parts, model, ctx) { | ||
| const selected = model.filtered[model.table.focusRow]; | ||
| if (selected && model.activeDrawer !== 'treemap') { | ||
| parts.push(chipSurface(ctx, `selected ${selected.slug}`, 'accent')); | ||
| } | ||
| if (model.toasts.length > 0) { | ||
| parts.push(chipSurface(ctx, `alerts ${model.toasts.length}`, 'warning')); | ||
| } | ||
| if (model.activeDrawer === 'treemap') { | ||
| parts.push(chipSurface(ctx, `scope ${model.treemapScope}`, 'brand')); | ||
| if (model.treemapScope === 'repository') { | ||
| parts.push(chipSurface(ctx, `files ${model.treemapWorktreeMode}`, 'accent')); | ||
| } | ||
| parts.push(chipSurface(ctx, `level ${treemapLevelLabel(model)}`, 'info')); | ||
| const tile = selectedTreemapTile(model); | ||
| if (tile) { | ||
| parts.push(chipSurface(ctx, `focus ${tile.label}`, 'warning')); | ||
| } | ||
| } | ||
| if (model.activeDrawer && model.activeDrawer !== 'treemap') { | ||
| parts.push(chipSurface(ctx, `${model.activeDrawer} drawer`, 'info')); | ||
| } | ||
| if (model.palette) { | ||
| parts.push(chipSurface(ctx, 'command deck', 'warning')); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the clamped focus row for selection-dependent UI.
Line 738-Line 751 and Line 905-Line 918 already clamp the visible table selection, but Line 131, Line 687, and Line 929 still index filtered/refsItems with the raw focusRow. After filtering or source changes, the table can highlight the last valid row while the badge/detail pane sees undefined. Reuse a shared clamped-index helper here so the table, badges, and inspectors stay in sync.
Also applies to: 686-687, 928-929
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ui/dashboard-view.js` around lines 130 - 155, appendSelectionBadges
indexes model.filtered with the raw model.table.focusRow which can be
out-of-range after filtering; replace that usage by computing a clamped focus
index (use the project's shared clamped-index helper used elsewhere for the
table visible selection) and use filtered[clampedIndex] instead of
filtered[model.table.focusRow]; update the initial selected assignment in
appendSelectionBadges and any other places in this file that index
filtered/refsItems with model.table.focusRow (same pattern as in the table code)
so badges and inspectors use the same clamped selection as the table.
Comment on lines
1249
to
+1261
| export function renderDashboard(model, deps) { | ||
| return flex( | ||
| { direction: 'column', width: model.columns, height: model.rows }, | ||
| { content: renderHeader(model, deps.ctx), basis: 1 }, | ||
| { content: (/** @type {number} */ w, /** @type {number} */ _h) => '\u2500'.repeat(w), basis: 1 }, | ||
| { content: (/** @type {number} */ w, /** @type {number} */ h) => renderBody(model, deps, { width: w, height: h }), flex: 1 }, | ||
| { content: (/** @type {number} */ w, /** @type {number} */ _h) => '\u2500'.repeat(w), basis: 1 }, | ||
| { content: 'j/k Navigate enter Load / Filter J/K Scroll q Quit', basis: 1 }, | ||
| ); | ||
| const width = Math.max(1, model.columns); | ||
| const height = Math.max(1, model.rows); | ||
| const screen = createSurface(width, height); | ||
| const header = renderHeaderSurface(model, deps); | ||
| const footer = renderFooterSurface(model, deps.ctx, width); | ||
| const bodyTop = header.height; | ||
| const bodyHeight = Math.max(1, height - header.height - footer.height); | ||
|
|
||
| screen.blit(header, 0, 0); | ||
| renderBody(model, deps, { top: bodyTop, height: bodyHeight, screen }); | ||
| renderOverlays(model, deps, { top: bodyTop, height: bodyHeight, screen }); | ||
| screen.blit(footer, 0, Math.max(0, height - footer.height)); |
There was a problem hiding this comment.
The dashboard needs a short-terminal fallback.
At 8 rows, Line 1255-Line 1256 set bodyTop = 4 and force bodyHeight = 1, but Line 1261 still blits the 4-line footer at row 4. That overwrites the only body row instead of degrading cleanly. Please show a min-size screen or collapse some chrome when rows < 9.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ui/dashboard-view.js` around lines 1249 - 1261, When terminal rows are
very small (<9) renderDashboard should use a compact layout instead of blitting
the full 4-line header/footer that overwrites the lone body line; modify
renderDashboard to detect small height (model.rows < 9) and either reduce
header/footer heights (collapse chrome) or render a single minimal surface
(e.g., show only essential title and footer) so bodyHeight remains >=1 and
footer is blitted below the body without overlapping it. Update the logic around
bodyTop, bodyHeight, and the footer blit so footer position is computed from the
actual compact footer.height or by using Math.max(0, height - footer.height)
after adjusting header/footer sizes, and ensure renderBody and renderOverlays
are called with the adjusted top/height values.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
bin/ui/dashboard-view.js (2)
745-753:selectedEntryshould use the same clamped index as the table.This helper indexes into
model.filteredwith rawfocusRow, which can diverge from what the table visually highlights after filtering. Consider extracting a shared clamped-index helper for consistency.♻️ Suggested approach
+/** + * Compute a clamped focus index for the current filtered entries. + * + * `@param` {DashModel} model + * `@returns` {number} + */ +function clampedFocusRow(model) { + return Math.max(0, Math.min(model.table.focusRow, model.filtered.length - 1)); +} + /** * Select the current vault entry from table focus. * * `@param` {DashModel} model * `@returns` {{ slug: string, treeOid: string } | undefined} */ function selectedEntry(model) { - return model.filtered[model.table.focusRow]; + if (model.filtered.length === 0) return undefined; + return model.filtered[clampedFocusRow(model)]; }Then reuse
clampedFocusRowinappendSelectionBadges(Line 131) and create a similar helper forselectedRef.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/dashboard-view.js` around lines 745 - 753, selectedEntry currently returns model.filtered[model.table.focusRow] using the raw focusRow which can be out-of-range after filtering; extract or reuse a clamped-focus-index helper (e.g. clampedFocusRow(model)) that computes Math.max(0, Math.min(model.filtered.length - 1, model.table.focusRow)) and update selectedEntry to return model.filtered[clampedFocusRow(model)]; also replace usages in appendSelectionBadges and implement a matching clamped helper for selectedRef so all selection accessors use the same bounded index logic (refer to selectedEntry, clampedFocusRow, appendSelectionBadges, selectedRef).
59-69: JSDoc comment is stale copy-paste from a removed function.The docstring describes "Compute visible window for cursor scrolling" but the function is
textSurfacewhich parses ANSI text into a surface.📝 Suggested fix
/** - * Compute visible window for cursor scrolling. + * Parse ANSI text into a surface with the given dimensions. * - * `@param` {number} cursor - * `@param` {number} total - * `@param` {number} height - * `@returns` {{ start: number, end: number }} + * `@param` {string} text + * `@param` {number} width + * `@param` {number} height + * `@returns` {Surface} */ function textSurface(text, width, height) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/dashboard-view.js` around lines 59 - 69, The JSDoc above function textSurface is stale; replace it with a concise docstring that accurately describes textSurface (it parses ANSI text into a drawable surface) and documents the actual parameters (text: string, width: number, height: number) and return value (surface/object) used by parseAnsiToSurface; update the description and param/returns tags to reference text, width, height and the correct return type and remove the "Compute visible window for cursor scrolling" wording so the doc matches the function name textSurface and the call to parseAnsiToSurface.test/unit/cli/dashboard.test.js (1)
191-210: Consider adding a test for non-drillable tile selection.
makeTreemapTilesetspath: nullwhendrillable: false, but there's no test asserting that drill commands are no-ops on non-drillable tiles. This would strengthen coverage for the+key behavior when focused onmeta:othertiles.Would you like me to draft a test case that verifies drilling into a non-drillable tile is safely ignored?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/cli/dashboard.test.js` around lines 191 - 210, Add a unit test that verifies pressing the drill key on a non-drillable treemap tile is a no-op: use makeTreemapTile with drillable: false (so path is null) and a kind like "meta:other", focus/select that tile in the same test harness used by existing treemap tests, simulate the '+' (drill) key or invoke the same drill handler used elsewhere (e.g., the dashboard's key handler/drill command), and assert that no navigation/drill action occurs and the selection/state remains unchanged; ensure the test also asserts the created tile's path is null to make the intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/ui/dashboard-view.js`:
- Around line 745-753: selectedEntry currently returns
model.filtered[model.table.focusRow] using the raw focusRow which can be
out-of-range after filtering; extract or reuse a clamped-focus-index helper
(e.g. clampedFocusRow(model)) that computes Math.max(0,
Math.min(model.filtered.length - 1, model.table.focusRow)) and update
selectedEntry to return model.filtered[clampedFocusRow(model)]; also replace
usages in appendSelectionBadges and implement a matching clamped helper for
selectedRef so all selection accessors use the same bounded index logic (refer
to selectedEntry, clampedFocusRow, appendSelectionBadges, selectedRef).
- Around line 59-69: The JSDoc above function textSurface is stale; replace it
with a concise docstring that accurately describes textSurface (it parses ANSI
text into a drawable surface) and documents the actual parameters (text: string,
width: number, height: number) and return value (surface/object) used by
parseAnsiToSurface; update the description and param/returns tags to reference
text, width, height and the correct return type and remove the "Compute visible
window for cursor scrolling" wording so the doc matches the function name
textSurface and the call to parseAnsiToSurface.
In `@test/unit/cli/dashboard.test.js`:
- Around line 191-210: Add a unit test that verifies pressing the drill key on a
non-drillable treemap tile is a no-op: use makeTreemapTile with drillable: false
(so path is null) and a kind like "meta:other", focus/select that tile in the
same test harness used by existing treemap tests, simulate the '+' (drill) key
or invoke the same drill handler used elsewhere (e.g., the dashboard's key
handler/drill command), and assert that no navigation/drill action occurs and
the selection/state remains unchanged; ensure the test also asserts the created
tile's path is null to make the intent explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 810f868c-8364-4451-8b0a-44e116b47b45
📒 Files selected for processing (2)
bin/ui/dashboard-view.jstest/unit/cli/dashboard.test.js
There was a problem hiding this comment.
♻️ Duplicate comments (3)
bin/ui/dashboard-view.js (3)
130-132:⚠️ Potential issue | 🟡 MinorClamp selection lookups to the rendered focus row.
Line 131, Line 766, and Line 943 still index
model.filtered/model.refsItemswith the rawfocusRow. After filtering or source changes, the table can render a clamped selection while the badges or inspector resolveundefined. Reuse the same clamp logic as the table/ref viewport code here so every selection-dependent panel reads the same row.Also applies to: 765-767, 942-944
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/dashboard-view.js` around lines 130 - 132, The selection lookup uses raw model.table.focusRow and can be out-of-range after filtering; clamp the index before indexing model.filtered or model.refsItems. For appendSelectionBadges and the other occurrences, compute a safe index like: let idx = Math.max(0, Math.min(model.table.focusRow, model.filtered.length - 1)) and use model.filtered[idx]; for refs use model.refsItems and its length similarly. Replace direct uses of model.table.focusRow when indexing filtered/refs arrays so badges/inspector always read the same clamped row as the table viewport.
1418-1426:⚠️ Potential issue | 🟠 MajorFooter still overwrites the body on short terminals.
At 8 rows,
bodyTopis 4,bodyHeightis forced to 1, and Line 1426 blits the 4-line footer at row 4, so the footer replaces the only body row. DerivefooterTopfirst and compute the body from the remaining space, or switch to a compact/min-size layout below this height.🧭 Minimal non-overlapping fix
const header = renderHeaderSurface(model, deps); const footer = renderFooterSurface(model, deps.ctx, width); const bodyTop = header.height; - const bodyHeight = Math.max(1, height - header.height - footer.height); + const footerTop = Math.max(bodyTop, height - footer.height); + const bodyHeight = Math.max(0, footerTop - bodyTop); screen.blit(header, 0, 0); - renderBody(model, deps, { top: bodyTop, height: bodyHeight, screen }); - renderOverlays(model, deps, { top: bodyTop, height: bodyHeight, screen }); - screen.blit(footer, 0, Math.max(0, height - footer.height)); + if (bodyHeight > 0) { + renderBody(model, deps, { top: bodyTop, height: bodyHeight, screen }); + renderOverlays(model, deps, { top: bodyTop, height: bodyHeight, screen }); + } + screen.blit(footer, 0, footerTop);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/dashboard-view.js` around lines 1418 - 1426, Footer currently blits at Math.max(0, height - footer.height) after body is sized, which lets the footer overlap the single forced body row; compute footerTop first (e.g., footerTop = Math.max(0, height - footer.height)), then derive bodyTop = header.height and bodyHeight = Math.max(0, footerTop - bodyTop) so the body occupies only the space between header and footer, and finally blit footer at footerTop and renderBody/renderOverlays only for bodyHeight > 0 (or switch to a compact/min-size layout when bodyHeight is zero). Use the existing symbols header, footer, bodyTop, bodyHeight, footerTop, screen.blit, renderBody and renderOverlays to implement the change.
1185-1187:⚠️ Potential issue | 🟠 MajorThese split widths still overflow narrow terminals.
The minimums still do not compose. A 40-column refs view yields
22 + 18 + 1 = 41, and a 34-column treemap view yields16 + 18 + 1 = 35, so both paths still guarantee clipped/out-of-bounds blits instead of degrading cleanly. Clamp one pane to the remaining width, or switch to a stacked layout once both preferred minimums no longer fit.Also applies to: 1280-1282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/dashboard-view.js` around lines 1185 - 1187, The current width calculations for maxSidebarWidth, sidebarWidth and listWidth can sum to more than options.screen.width and overflow narrow terminals; update the layout logic in dashboard-view.js so you first compute desired minimums (e.g., sidebarMin = 22 or 30 depending on mode and listMin = 18) and then if sidebarMin + listMin + 1 > options.screen.width either (a) clamp listWidth = Math.max(0, options.screen.width - sidebarWidth - 1) so the list takes the remaining space, or (b) switch to a stacked layout when options.screen.width is below the combined minimum threshold; modify the code around maxSidebarWidth, sidebarWidth and listWidth to enforce this fallback and ensure no computed width exceeds options.screen.width or becomes negative.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bin/ui/dashboard-view.js`:
- Around line 130-132: The selection lookup uses raw model.table.focusRow and
can be out-of-range after filtering; clamp the index before indexing
model.filtered or model.refsItems. For appendSelectionBadges and the other
occurrences, compute a safe index like: let idx = Math.max(0,
Math.min(model.table.focusRow, model.filtered.length - 1)) and use
model.filtered[idx]; for refs use model.refsItems and its length similarly.
Replace direct uses of model.table.focusRow when indexing filtered/refs arrays
so badges/inspector always read the same clamped row as the table viewport.
- Around line 1418-1426: Footer currently blits at Math.max(0, height -
footer.height) after body is sized, which lets the footer overlap the single
forced body row; compute footerTop first (e.g., footerTop = Math.max(0, height -
footer.height)), then derive bodyTop = header.height and bodyHeight =
Math.max(0, footerTop - bodyTop) so the body occupies only the space between
header and footer, and finally blit footer at footerTop and
renderBody/renderOverlays only for bodyHeight > 0 (or switch to a
compact/min-size layout when bodyHeight is zero). Use the existing symbols
header, footer, bodyTop, bodyHeight, footerTop, screen.blit, renderBody and
renderOverlays to implement the change.
- Around line 1185-1187: The current width calculations for maxSidebarWidth,
sidebarWidth and listWidth can sum to more than options.screen.width and
overflow narrow terminals; update the layout logic in dashboard-view.js so you
first compute desired minimums (e.g., sidebarMin = 22 or 30 depending on mode
and listMin = 18) and then if sidebarMin + listMin + 1 > options.screen.width
either (a) clamp listWidth = Math.max(0, options.screen.width - sidebarWidth -
1) so the list takes the remaining space, or (b) switch to a stacked layout when
options.screen.width is below the combined minimum threshold; modify the code
around maxSidebarWidth, sidebarWidth and listWidth to enforce this fallback and
ensure no computed width exceeds options.screen.width or becomes negative.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 574aae4d-4e9a-4edb-a730-a0ef74b38c0f
📒 Files selected for processing (2)
bin/ui/dashboard-view.jstest/unit/cli/dashboard.test.js
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
Highlights
Verification
Summary by CodeRabbit