fix(cli): address M13 dashboard code review findings#9
Open
flyingrobots wants to merge 6 commits intomainfrom
Open
fix(cli): address M13 dashboard code review findings#9flyingrobots wants to merge 6 commits intomainfrom
flyingrobots wants to merge 6 commits intomainfrom
Conversation
Route CLI restore through cas.readManifest() to preserve v2 Merkle sub-manifest reconstitution. Add integration regression coverage for --oid restore on forced Merkle manifests.
TEA-based full-screen dashboard for browsing vault entries and inspecting manifests, powered by @flyingrobots/bijou-tui. Progressively loads all manifests, supports j/k navigation, / filtering, J/K detail scrolling, and non-TTY static fallback.
Progress bars, encryption card, history timeline, manifest view, heatmap, and shared CLI context. Includes tests, README rewrite, roadmap updates, and bijou dependency additions.
Fix filter-start not resetting filtered list, handleLoadedEntries ignoring active filter, list pane ignoring width, and formatSize closing brace style. Add 3 missing tests and update ROADMAP labels.
|
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. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds a full CLI TUI system for git-cas: new dashboard, manifest/heatmap/history/encryption renderers, progress UIs, new CLI commands/options (inspect, dashboard, quiet, history --pretty, info --encryption), test coverage, README/ROADMAP docs updates, and new Bijou UI dependencies. Changes are documentation, CLI wiring, UI modules, tests, and package deps — no public API surface changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (bin/git-cas.js)
participant DashApp as Dashboard App (TEA)
participant CAS as Content-Addressable Store
participant View as Render Pipeline
User->>CLI: run `git-cas vault dashboard`
CLI->>DashApp: launchDashboard(cas)
DashApp->>CAS: loadEntriesCmd() (async)
CAS-->>DashApp: loaded-entries (entries, metadata)
DashApp->>DashApp: handleAppMsg -> update model
DashApp->>View: renderDashboard(model, deps)
User->>DashApp: key event (j/k/enter/filter)
DashApp->>CAS: loadManifestCmd(slug, treeOid) (async) on select
CAS-->>DashApp: loaded-manifest (slug, manifest)
DashApp->>DashApp: cache manifest, update detail pane
DashApp->>View: re-render split panes (list + manifest)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
test/unit/cli/encryption-card.test.js (1)
19-31: Potential locale sensitivity in iteration format assertion.The test expects
'600,000'(line 28), which assumestoLocaleString()produces comma-separated thousands. This format depends on the Node.js environment's default locale. Consider using a regex match or ensuring the test environment has a consistent locale (e.g.,en-US).🔧 Suggested alternative assertion
- expect(output).toContain('600,000'); + // Match the numeric value regardless of locale formatting + expect(output).toMatch(/600[,.]?000/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/cli/encryption-card.test.js` around lines 19 - 31, The test "renders pbkdf2 details" is asserting a locale-specific formatted string '600,000' which can break on non‑en locales; update the assertion in test/unit/cli/encryption-card.test.js that checks the iterations from renderEncryptionCard to be locale-independent—either assert the raw digits (e.g., '600000'), use a regex that accepts common thousands separators, or convert the number with a consistent locale before asserting—so replace the expect(output).toContain('600,000') with a locale-agnostic check against renderEncryptionCard's iterations output.bin/ui/heatmap.js (1)
60-70: Legend assumes uniform chunk sizes.
buildLegenduseschunks[0].sizeto display the per-chunk size, which assumes all chunks are the same size. If the last chunk could be smaller (common in file chunking), this display might be slightly misleading.This is a minor cosmetic concern—the current approach is acceptable for a summary view.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/heatmap.js` around lines 60 - 70, buildLegend currently assumes uniform chunk sizes by using chunks[0].size; change it to detect non-uniform sizes and show a more accurate per-chunk label: inside buildLegend compute whether all chunk.size values equal the first size (or compute min and max and/or average), and if uniform keep the existing `${formatBytes(size)}/chunk` output but if not uniform emit a clear alternate string such as a range `${formatBytes(min)}/${formatBytes(max)}/chunk` or an average `${formatBytes(avg)}/chunk`; keep the rest of the function (chunks count and subManifests handling) unchanged and use the existing formatBytes helper for formatting.bin/ui/dashboard-view.js (1)
12-17: Consider consolidatingformatSizewithformatBytesfrom heatmap.js.This file uses compact suffixes (
B,K,M,G) whilebin/ui/heatmap.jsusesformatByteswith full binary unit suffixes (KiB,MiB,GiB). If the compact format is intentional for the dashboard's constrained space, this is fine. Otherwise, consider extracting a shared utility to ensure consistency.🤖 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 12 - 17, The dashboard's formatSize function uses compact suffixes while heatmap.js uses formatBytes with binary unit suffixes; create a shared utility (e.g., export a single formatBytes util) that supports an option for compact vs. full binary suffixes, replace the local formatSize in bin/ui/dashboard-view.js and the existing formatBytes in bin/ui/heatmap.js to import and use this shared formatter (or call it with compact:true for the dashboard to preserve current UI), and ensure the exported function name (e.g., formatBytes) is used consistently across both files.test/unit/cli/dashboard.test.js (1)
10-16: Consider adding return values forreadManifestmock.The
readManifestmock is set up without a default return value, which could cause issues if tests inadvertently call it. Consider adding a default mock implementation for consistency withlistVaultandgetVaultMetadata.♻️ Optional: Add default mock return
function mockCas() { return { listVault: vi.fn().mockResolvedValue([]), getVaultMetadata: vi.fn().mockResolvedValue(null), - readManifest: vi.fn(), + readManifest: vi.fn().mockResolvedValue({ slug: 'test', chunks: [] }), }; }🤖 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 10 - 16, The mockCas helper defines readManifest with no default return which can cause unexpected test failures; update mockCas so readManifest is a vi.fn() with a sensible mockResolvedValue (e.g., null or an empty object/manifest) or a mockImplementation that mirrors expected real behavior, keeping listVault and getVaultMetadata patterns intact; target the mockCas function and its readManifest property to add the default return value.bin/git-cas.js (1)
168-175: Duplicated validation logic - consider reusingvalidateRestoreFlags.The flag validation for
--slugand--oidin theinspectcommand duplicates the logic invalidateRestoreFlags(lines 82-91). Consider extracting or reusing the existing function for consistency and maintainability.♻️ Reuse validateRestoreFlags
.action(async (opts) => { try { - if (opts.slug && opts.oid) { - process.stderr.write('error: Provide --slug or --oid, not both\n'); - process.exit(1); - } - if (!opts.slug && !opts.oid) { - process.stderr.write('error: Provide --slug <slug> or --oid <tree-oid>\n'); - process.exit(1); - } + validateRestoreFlags(opts); const cas = createCas(opts.cwd);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/git-cas.js` around lines 168 - 175, The inspect command duplicates flag validation for opts.slug and opts.oid; replace the inline checks with a call to the existing validateRestoreFlags function so there’s a single source of truth. Locate the inspect command block where opts.slug and opts.oid are inspected and remove the duplicated if statements, then call validateRestoreFlags(opts) (or pass the same args that function expects) before proceeding; ensure any error handling/exit behavior relies on validateRestoreFlags so behavior remains identical.bin/ui/dashboard.js (1)
81-85: Cursor clamping has an edge case whenfilteredis empty.When
filteredis empty,maxbecomes-1, andMath.max(0, Math.min(-1, ...))will always return0. This is technically safe but could be clearer. Consider documenting this behavior or adding an explicit empty check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/dashboard.js` around lines 81 - 85, handleMove's clamping logic mishandles the empty list edge-case implicitly; update handleMove to explicitly check if model.filtered is empty (e.g., model.filtered.length === 0) and in that case set cursor to 0 (and reset detailScroll to 0) or otherwise return the model with cursor 0, otherwise perform the existing clamping calculation (using model.filtered.length - 1) so the intent is clear; reference the handleMove function and the model.filtered, cursor, and detailScroll fields when making the change.bin/ui/manifest-view.js (1)
11-22:formatByteshandles common sizes well but lacks edge case handling.The function correctly formats B, KiB, MiB, and GiB. However, it doesn't handle edge cases like negative values or non-numeric input, which could produce unexpected output if malformed data is passed.
♻️ Optional: Add defensive check
function formatBytes(bytes) { + if (typeof bytes !== 'number' || bytes < 0) { + return '0 B'; + } if (bytes < 1024) { return `${bytes} B`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/manifest-view.js` around lines 11 - 22, The formatBytes function lacks validation for negative or non-numeric input; add a defensive guard at the start of formatBytes that coerces the input to a Number (e.g., const n = Number(bytes)), then check Number.isFinite(n) and n >= 0; if the check fails return a safe default like "0 B" (or another agreed sentinel), otherwise use n in the existing size calculations; reference function name formatBytes when locating where to insert this early validation.
🤖 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/git-cas.js`:
- Around line 168-175: The inspect command duplicates flag validation for
opts.slug and opts.oid; replace the inline checks with a call to the existing
validateRestoreFlags function so there’s a single source of truth. Locate the
inspect command block where opts.slug and opts.oid are inspected and remove the
duplicated if statements, then call validateRestoreFlags(opts) (or pass the same
args that function expects) before proceeding; ensure any error handling/exit
behavior relies on validateRestoreFlags so behavior remains identical.
In `@bin/ui/dashboard-view.js`:
- Around line 12-17: The dashboard's formatSize function uses compact suffixes
while heatmap.js uses formatBytes with binary unit suffixes; create a shared
utility (e.g., export a single formatBytes util) that supports an option for
compact vs. full binary suffixes, replace the local formatSize in
bin/ui/dashboard-view.js and the existing formatBytes in bin/ui/heatmap.js to
import and use this shared formatter (or call it with compact:true for the
dashboard to preserve current UI), and ensure the exported function name (e.g.,
formatBytes) is used consistently across both files.
In `@bin/ui/dashboard.js`:
- Around line 81-85: handleMove's clamping logic mishandles the empty list
edge-case implicitly; update handleMove to explicitly check if model.filtered is
empty (e.g., model.filtered.length === 0) and in that case set cursor to 0 (and
reset detailScroll to 0) or otherwise return the model with cursor 0, otherwise
perform the existing clamping calculation (using model.filtered.length - 1) so
the intent is clear; reference the handleMove function and the model.filtered,
cursor, and detailScroll fields when making the change.
In `@bin/ui/heatmap.js`:
- Around line 60-70: buildLegend currently assumes uniform chunk sizes by using
chunks[0].size; change it to detect non-uniform sizes and show a more accurate
per-chunk label: inside buildLegend compute whether all chunk.size values equal
the first size (or compute min and max and/or average), and if uniform keep the
existing `${formatBytes(size)}/chunk` output but if not uniform emit a clear
alternate string such as a range `${formatBytes(min)}/${formatBytes(max)}/chunk`
or an average `${formatBytes(avg)}/chunk`; keep the rest of the function (chunks
count and subManifests handling) unchanged and use the existing formatBytes
helper for formatting.
In `@bin/ui/manifest-view.js`:
- Around line 11-22: The formatBytes function lacks validation for negative or
non-numeric input; add a defensive guard at the start of formatBytes that
coerces the input to a Number (e.g., const n = Number(bytes)), then check
Number.isFinite(n) and n >= 0; if the check fails return a safe default like "0
B" (or another agreed sentinel), otherwise use n in the existing size
calculations; reference function name formatBytes when locating where to insert
this early validation.
In `@test/unit/cli/dashboard.test.js`:
- Around line 10-16: The mockCas helper defines readManifest with no default
return which can cause unexpected test failures; update mockCas so readManifest
is a vi.fn() with a sensible mockResolvedValue (e.g., null or an empty
object/manifest) or a mockImplementation that mirrors expected real behavior,
keeping listVault and getVaultMetadata patterns intact; target the mockCas
function and its readManifest property to add the default return value.
In `@test/unit/cli/encryption-card.test.js`:
- Around line 19-31: The test "renders pbkdf2 details" is asserting a
locale-specific formatted string '600,000' which can break on non‑en locales;
update the assertion in test/unit/cli/encryption-card.test.js that checks the
iterations from renderEncryptionCard to be locale-independent—either assert the
raw digits (e.g., '600000'), use a regex that accepts common thousands
separators, or convert the number with a consistent locale before asserting—so
replace the expect(output).toContain('600,000') with a locale-agnostic check
against renderEncryptionCard's iterations output.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
README.mdROADMAP.mdbin/git-cas.jsbin/ui/context.jsbin/ui/dashboard-cmds.jsbin/ui/dashboard-view.jsbin/ui/dashboard.jsbin/ui/encryption-card.jsbin/ui/heatmap.jsbin/ui/history-timeline.jsbin/ui/manifest-view.jsbin/ui/progress.jspackage.jsontest/integration/vault-cli.test.jstest/unit/cli/_testContext.jstest/unit/cli/dashboard.test.jstest/unit/cli/encryption-card.test.jstest/unit/cli/heatmap.test.jstest/unit/cli/history-timeline.test.jstest/unit/cli/manifest-view.test.jstest/unit/cli/progress.test.js
Reset cursor on filter-start, use flex-allocated width in list pane, and add regression tests for filter-start reset and active-filter application on loaded-entries.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/ui/dashboard.js (1)
139-144: Consider surfacing error state in the UI.When
load-erroroccurs,model.erroris set but the view doesn't display it. If entry loading fails, the status remains'loading'and users see "Loading..." indefinitely. Consider either:
- Updating status to an
'error'state that the view can render- Adding error display to the loading/empty state logic in
renderListPane🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/dashboard.js` around lines 139 - 144, The handler handleAppMsg sets model.error on 'load-error' but never updates the loading status or view; modify handleAppMsg so that on msg.type === 'load-error' it returns an updated model with both error: msg.error and status: 'error' (or another explicit error flag) so the view can react, and then update renderListPane to check model.status === 'error' (or model.error) and render an error message instead of the "Loading..." state; reference handleLoadedEntries/handleLoadedManifest for pattern of returning [model, []] and ensure renderListPane reads model.error or model.status to display the error UI.
🤖 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.js`:
- Around line 139-144: The handler handleAppMsg sets model.error on 'load-error'
but never updates the loading status or view; modify handleAppMsg so that on
msg.type === 'load-error' it returns an updated model with both error: msg.error
and status: 'error' (or another explicit error flag) so the view can react, and
then update renderListPane to check model.status === 'error' (or model.error)
and render an error message instead of the "Loading..." state; reference
handleLoadedEntries/handleLoadedManifest for pattern of returning [model, []]
and ensure renderListPane reads model.error or model.status to display the error
UI.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/ui/dashboard-view.jsbin/ui/dashboard.jstest/unit/cli/dashboard.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/cli/dashboard.test.js
- Use locale-agnostic regex for iterations assertion - Add default return for readManifest mock - Reuse validateRestoreFlags in inspect command - Set status to 'error' on load-error and render it in the view
Member
Author
CodeRabbit Review ResponseAddressed in 5a97571
Declined (false positives / over-engineering)
@coderabbitai review please 🥕 |
|
✅ Addressed:
📋 Declined with rationale:
I'll trigger a fresh review now to verify the fixes and check the latest state of the PR! 🥕 ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/git-cas.js`:
- Around line 114-121: The progress tracker created by
createStoreProgress(...)/createRestoreProgress(...) is attached to the service
(progress.attach(service)) but detached only after cas.storeFile(...) /
cas.restoreFile(...), so if those throw the progress.detach() is skipped and
listeners leak; wrap the attach/store/detach sequence in a try/finally: call
progress.attach(service) before the try, perform await cas.storeFile(storeOpts)
or await cas.restoreFile(restoreOpts) inside the try, and always call
progress.detach() in the finally block (same change needed for the other block
using createRestoreProgress and cas.restoreFile); reference functions:
cas.getService, createStoreProgress, createRestoreProgress, progress.attach,
progress.detach, cas.storeFile, cas.restoreFile.
In `@bin/ui/dashboard-view.js`:
- Around line 63-69: filterLine and the empty-state msg can exceed the pane
width causing wrapping; before calling padToHeight, truncate filterLine and msg
to size.width (or size.width minus 1 if you need room for the cursor block) so
they never exceed the pane; update the code that builds filterLine and msg
(symbols: filterLine, msg, model.filtering, model.error) to slice/substring them
to the pane width (or add a small helper like truncate(text, width)) then pass
the truncated strings into padToHeight to prevent wrapping artifacts.
In `@bin/ui/dashboard.js`:
- Around line 139-143: The current handleAppMsg treats every 'load-error' as a
global failure; update handleAppMsg to inspect a source field on the incoming
msg (e.g. msg.source === 'entries' | 'manifest' and optional slug) and only set
the global status to 'error' when msg.source === 'entries' (bootstrap entries
failure); for manifest errors, route the message to per-entry handling (e.g.
call or adapt handleLoadedManifest/handleLoadedEntries to mark that specific
entry's state/error by slug) so a single manifest preload failure does not flip
the whole app to error.
- Around line 58-66: In handleLoadedEntries, after computing filtered via
applyFilter, clamp or adjust model.cursor so it remains within the new filtered
bounds (e.g., if filtered is empty set cursor to -1, otherwise ensure cursor ≤
filtered.length-1 and ≥ 0) so selection remains valid; update the returned
model.cursor together with status/entries/filtered/metadata (refer to
handleLoadedEntries, model.cursor and filtered).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bin/git-cas.jsbin/ui/dashboard-view.jsbin/ui/dashboard.jstest/unit/cli/dashboard.test.jstest/unit/cli/encryption-card.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/cli/encryption-card.test.js
Comment on lines
+114
to
121
| const service = await cas.getService(); | ||
| const progress = createStoreProgress({ | ||
| filePath: file, chunkSize: cas.chunkSize, quiet: program.opts().quiet, | ||
| }); | ||
| progress.attach(service); | ||
| const manifest = await cas.storeFile(storeOpts); | ||
| progress.detach(); | ||
|
|
There was a problem hiding this comment.
Always detach progress trackers in a finally block.
If cas.storeFile(...) or cas.restoreFile(...) throws, detach() is skipped, which can leak event listeners and leave stale progress state.
💡 Suggested patch
const service = await cas.getService();
const progress = createStoreProgress({
filePath: file, chunkSize: cas.chunkSize, quiet: program.opts().quiet,
});
progress.attach(service);
- const manifest = await cas.storeFile(storeOpts);
- progress.detach();
+ let manifest;
+ try {
+ manifest = await cas.storeFile(storeOpts);
+ } finally {
+ progress.detach();
+ }
@@
const service = await cas.getService();
const progress = createRestoreProgress({
totalChunks: manifest.chunks.length, quiet: program.opts().quiet,
});
progress.attach(service);
- const { bytesWritten } = await cas.restoreFile({
- ...restoreOpts,
- outputPath: opts.out,
- });
- progress.detach();
+ let bytesWritten;
+ try {
+ ({ bytesWritten } = await cas.restoreFile({
+ ...restoreOpts,
+ outputPath: opts.out,
+ }));
+ } finally {
+ progress.detach();
+ }
process.stdout.write(`${bytesWritten}\n`);Also applies to: 211-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/git-cas.js` around lines 114 - 121, The progress tracker created by
createStoreProgress(...)/createRestoreProgress(...) is attached to the service
(progress.attach(service)) but detached only after cas.storeFile(...) /
cas.restoreFile(...), so if those throw the progress.detach() is skipped and
listeners leak; wrap the attach/store/detach sequence in a try/finally: call
progress.attach(service) before the try, perform await cas.storeFile(storeOpts)
or await cas.restoreFile(restoreOpts) inside the try, and always call
progress.detach() in the finally block (same change needed for the other block
using createRestoreProgress and cas.restoreFile); reference functions:
cas.getService, createStoreProgress, createRestoreProgress, progress.attach,
progress.detach, cas.storeFile, cas.restoreFile.
Comment on lines
+63
to
+69
| const filterLine = model.filtering ? `/${model.filterText}\u2588` : ''; | ||
| const listHeight = model.filtering ? size.height - 1 : size.height; | ||
| const items = model.filtered; | ||
|
|
||
| if (items.length === 0) { | ||
| const msg = model.status === 'loading' ? 'Loading...' : model.error ? `Error: ${model.error}` : 'No entries'; | ||
| return padToHeight(msg, listHeight, filterLine); |
There was a problem hiding this comment.
Clamp filter/error lines to pane width to avoid wrapping artifacts.
On Line 63 and Line 68, filterLine and empty-state msg can exceed pane width, causing visual wrap/misalignment in narrow terminals.
💡 Suggested patch
function renderListPane(model, size) {
- const filterLine = model.filtering ? `/${model.filterText}\u2588` : '';
+ const clamp = (s) => (size.width ? s.slice(0, size.width) : s);
+ const filterLine = model.filtering ? clamp(`/${model.filterText}\u2588`) : '';
const listHeight = model.filtering ? size.height - 1 : size.height;
const items = model.filtered;
if (items.length === 0) {
- const msg = model.status === 'loading' ? 'Loading...' : model.error ? `Error: ${model.error}` : 'No entries';
+ const msg = clamp(
+ model.status === 'loading'
+ ? 'Loading...'
+ : model.error
+ ? `Error: ${model.error}`
+ : 'No entries'
+ );
return padToHeight(msg, listHeight, filterLine);
}📝 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
| const filterLine = model.filtering ? `/${model.filterText}\u2588` : ''; | |
| const listHeight = model.filtering ? size.height - 1 : size.height; | |
| const items = model.filtered; | |
| if (items.length === 0) { | |
| const msg = model.status === 'loading' ? 'Loading...' : model.error ? `Error: ${model.error}` : 'No entries'; | |
| return padToHeight(msg, listHeight, filterLine); | |
| const clamp = (s) => (size.width ? s.slice(0, size.width) : s); | |
| const filterLine = model.filtering ? clamp(`/${model.filterText}\u2588`) : ''; | |
| const listHeight = model.filtering ? size.height - 1 : size.height; | |
| const items = model.filtered; | |
| if (items.length === 0) { | |
| const msg = clamp( | |
| model.status === 'loading' | |
| ? 'Loading...' | |
| : model.error | |
| ? `Error: ${model.error}` | |
| : 'No entries' | |
| ); | |
| return padToHeight(msg, listHeight, filterLine); |
🤖 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 63 - 69, filterLine and the
empty-state msg can exceed the pane width causing wrapping; before calling
padToHeight, truncate filterLine and msg to size.width (or size.width minus 1 if
you need room for the cursor block) so they never exceed the pane; update the
code that builds filterLine and msg (symbols: filterLine, msg, model.filtering,
model.error) to slice/substring them to the pane width (or add a small helper
like truncate(text, width)) then pass the truncated strings into padToHeight to
prevent wrapping artifacts.
Comment on lines
+58
to
+66
| function handleLoadedEntries(msg, model, cas) { | ||
| const cmds = msg.entries.map(e => loadManifestCmd(cas, e.slug, e.treeOid)); | ||
| return [{ | ||
| ...model, | ||
| status: 'ready', | ||
| entries: msg.entries, | ||
| filtered: applyFilter(msg.entries, model.filterText), | ||
| metadata: msg.metadata, | ||
| }, cmds]; |
There was a problem hiding this comment.
Clamp cursor after applying filtered entries.
After Line 64 recomputes filtered, the existing cursor may point past the new list. That leaves no selected row and makes Enter a no-op until the user moves.
💡 Suggested patch
function handleLoadedEntries(msg, model, cas) {
+ const filtered = applyFilter(msg.entries, model.filterText);
+ const cursor = Math.max(0, Math.min(model.cursor, filtered.length - 1));
const cmds = msg.entries.map(e => loadManifestCmd(cas, e.slug, e.treeOid));
return [{
...model,
status: 'ready',
entries: msg.entries,
- filtered: applyFilter(msg.entries, model.filterText),
+ filtered,
+ cursor,
metadata: msg.metadata,
}, cmds];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ui/dashboard.js` around lines 58 - 66, In handleLoadedEntries, after
computing filtered via applyFilter, clamp or adjust model.cursor so it remains
within the new filtered bounds (e.g., if filtered is empty set cursor to -1,
otherwise ensure cursor ≤ filtered.length-1 and ≥ 0) so selection remains valid;
update the returned model.cursor together with status/entries/filtered/metadata
(refer to handleLoadedEntries, model.cursor and filtered).
Comment on lines
+139
to
+143
| function handleAppMsg(msg, model, cas) { | ||
| if (msg.type === 'loaded-entries') { return handleLoadedEntries(msg, model, cas); } | ||
| if (msg.type === 'loaded-manifest') { return handleLoadedManifest(msg, model); } | ||
| if (msg.type === 'load-error') { return [{ ...model, status: 'error', error: msg.error }, []]; } | ||
| return [model, []]; |
There was a problem hiding this comment.
Differentiate entry-manifest load failures from full dashboard load failures.
Line 142 sets global status: 'error' for all load-error messages. With background manifest preloads, one entry failure can incorrectly mark the whole app as errored.
A safer pattern is to include source context in load-error (e.g., { source: 'entries' | 'manifest', slug }) and only set global status: 'error' for entries bootstrap failures; handle manifest failures as per-entry state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ui/dashboard.js` around lines 139 - 143, The current handleAppMsg treats
every 'load-error' as a global failure; update handleAppMsg to inspect a source
field on the incoming msg (e.g. msg.source === 'entries' | 'manifest' and
optional slug) and only set the global status to 'error' when msg.source ===
'entries' (bootstrap entries failure); for manifest errors, route the message to
per-entry handling (e.g. call or adapt handleLoadedManifest/handleLoadedEntries
to mark that specific entry's state/error by slug) so a single manifest preload
failure does not flip the whole app to error.
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
filter-startnow resetsfilteredto full entries list, preventing stale filter resultsrenderListItem, truncating lines to fit the paneformatSizeclosing brace style (moved to its own line)handleLoadedEntriesapplies active filter viaapplyFilter()instead of ignoring itfilter-backspace,load-error,selecton uncached entry*(planned — Task 13.x)*to*(implemented)*Test plan
npm test)npx eslint .)Summary by CodeRabbit