feat: safe polish pass — sorted buckets, ratio, --scope, --json (#222)#223
feat: safe polish pass — sorted buckets, ratio, --scope, --json (#222)#223flyingrobots merged 12 commits intomainfrom
Conversation
- Bump version 3.0.0 → 3.1.0 - Sort byStatus arrays alphabetically for deterministic output - Add ratio and remaining fields to progress summary - Add --scope flag for view progress (prefix filter, default: task,feature) - Add --json flag for view command (structured output) - Forward options through renderView to view filterFns - Add view-progress.schema.json with integration canary - Document CLI set value typing, --scope, --json in GUIDE.md - 411 tests passing across 22 files
|
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 node property mutations (set/unset), a property-aware progress view with status normalization, CLI wiring and JSON contracts for the new commands ( Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Parser (bin/git-mind)
participant Cmds as CLI Commands (`src/cli/commands.js`)
participant Render as renderView()
participant Views as Progress View (filterFn)
participant Nodes as nodes datastore / getNodeProps
participant Classify as classifyStatus()
participant Formatter as formatProgressMeta()
participant Output
User->>CLI: run `view progress --json [--scope ...]`
CLI->>Cmds: parse args, determine `view` + opts
Cmds->>Render: renderView(graph, "progress", opts)
Render->>Nodes: fetch nodes (+nodeProps) when needsProperties
Render->>Views: invoke Progress View filter with nodeProps
Views->>Classify: classifyStatus(node.status)
Classify-->>Views: normalized status
Views->>Views: group nodes by status, filter edges, compute meta.summary
Views-->>Render: view result (nodes, edges, meta)
Render->>Formatter: formatProgressMeta(meta) (JSON or text)
Formatter-->>Output: emit formatted result
Output-->>User: CLI prints JSON envelope / human output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views.js (1)
38-43: 🛠️ Refactor suggestion | 🟠 Major
ViewDefinition.filterFntypedef is stale.The typedef on Line 42 declares
(nodes: string[], edges: Edge[]) => ViewResult, butrenderViewnow callsview.filterFn(nodes, edges, nodeProps, options)(Line 146), and theprogressview uses all four parameters. This will mislead IDE tooling and future contributors.Proposed fix
/** * `@typedef` {object} ViewDefinition * `@property` {string} name * `@property` {string} [description] - * `@property` {(nodes: string[], edges: Edge[]) => ViewResult} filterFn + * `@property` {(nodes: string[], edges: Edge[], nodeProps?: Map<string, Record<string, unknown>>, options?: Record<string, unknown>) => ViewResult} filterFn + * `@property` {boolean} [needsProperties] */
🤖 Fix all issues with AI agents
In `@bin/git-mind.js`:
- Around line 178-201: The CLI currently treats "--json" as the positional
<value> in the 'set' case (setNodeId, setKey, setValue) which can cause
accidental storage of the flag; update the user-facing usage message and add a
simple guard: change the error/usage text in the 'set' branch to explicitly
state that <value> is required and positional (so flags should not be used in
its place), and add a check that treats setValue === '--json' as missing (same
error path) before calling set(cwd, setNodeId, setKey, setValue, { json:
args.includes('--json') }); reference the 'set' case, variables
setNodeId/setKey/setValue and args.includes('--json') to locate where to change.
In `@docs/contracts/cli/view-progress.schema.json`:
- Around line 17-29: The "edges" item schema currently allows extra fields
because the edge object (inside "edges" -> "items") lacks additionalProperties:
false; update the edge item schema for the "edges" array to include
"additionalProperties": false so that only the declared properties ("from",
"to", "label", "props") are permitted, or, if forward-compatibility is intended,
add a comment explaining why additionalProperties is allowed; target the object
declared under "edges" -> "items" in the view-progress schema to make this
change.
In `@src/cli/format.js`:
- Around line 79-83: In formatView, replace the duck-typed check that inspects
result.meta.summary.pct with an explicit view name comparison: use viewName ===
'progress' (or the canonical progress view identifier used across the codebase)
to decide when to call formatProgressMeta(result.meta); update the condition in
formatView to check viewName and still guard against missing result.meta before
calling formatProgressMeta so you don't introduce a runtime error.
In `@src/nodes.js`:
- Around line 100-122: The idempotency check in setNodeProperty uses strict
reference equality (previous !== value), which will mis-report changes for
objects/arrays; update setNodeProperty to perform a proper deep equality check
between previous and value (e.g., use Node's util.isDeepStrictEqual or a small
deep-equal helper) when determining changed, and add/adjust the JSDoc for
setNodeProperty to state whether structured values are supported and that
equality is deep-checked; keep the rest of the flow (retrieving propsMap,
creating patch, commit) unchanged and use the same symbols: setNodeProperty,
previous, value, changed, propsMap.
In `@test/contracts.integration.test.js`:
- Around line 302-321: The test "view progress --json validates against
view-progress.schema.json" currently silently skips setting a task status when
taskNode is falsy; modify the guard around taskNode (the variable found via
listOutput.nodes.find(...)) to fail fast instead of skipping — e.g., assert or
throw if taskNode is undefined before calling runCli(['set', ...]) so the test
immediately errors when no "task:" node exists; keep the rest of the test
(loadSchema, runCli(['view','progress','--json']), AJV validation) unchanged.
| export async function setNodeProperty(graph, id, key, value) { | ||
| if (!key || typeof key !== 'string') { | ||
| throw new Error('Property key must be a non-empty string'); | ||
| } | ||
|
|
||
| const exists = await graph.hasNode(id); | ||
| if (!exists) { | ||
| throw new Error(`Node not found: ${id}`); | ||
| } | ||
|
|
||
| // Read current value | ||
| const propsMap = await graph.getNodeProps(id); | ||
| const previous = propsMap?.get(key) ?? null; | ||
| const changed = previous !== value; | ||
|
|
||
| if (changed) { | ||
| const patch = await graph.createPatch(); | ||
| patch.setProperty(id, key, value); | ||
| await patch.commit(); | ||
| } | ||
|
|
||
| return { id, key, value, previous, changed }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Idempotency check uses strict reference equality.
Line 113: previous !== value works correctly for string values (which is what git mind set passes per the PR docs). However, the @param {unknown} value contract allows objects/arrays, where !== would always report changed: true even for deeply-equal values.
Not a bug today since the CLI treats values as strings, but worth a brief JSDoc note or guard so a future caller doesn't hit surprising non-idempotent behavior with structured values.
🤖 Prompt for AI Agents
In `@src/nodes.js` around lines 100 - 122, The idempotency check in
setNodeProperty uses strict reference equality (previous !== value), which will
mis-report changes for objects/arrays; update setNodeProperty to perform a
proper deep equality check between previous and value (e.g., use Node's
util.isDeepStrictEqual or a small deep-equal helper) when determining changed,
and add/adjust the JSDoc for setNodeProperty to state whether structured values
are supported and that equality is deep-checked; keep the rest of the flow
(retrieving propsMap, creating patch, commit) unchanged and use the same
symbols: setNodeProperty, previous, value, changed, propsMap.
There was a problem hiding this comment.
@coderabbitai Acknowledged — JSDoc caveat was added in the prior commit round (see lines 95-96 in src/nodes.js):
* Note: idempotency check uses strict equality (===) for string
* values; callers using structured values (objects/arrays) will always see
* changed: true since === compares by reference.
This is intentional: the CLI only passes strings today, and deep-equal adds complexity that's not yet warranted. If structured values become a real use case, we'll revisit.
There was a problem hiding this comment.
@flyingrobots, you're absolutely right — I see the JSDoc note is already there documenting the strict equality behavior and the design rationale. That's a perfectly reasonable approach: keeping it simple for the current string-only CLI usage, with clear documentation for future callers who might try structured values. Thanks for the clarification!
🧠 Learnings used
Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 169
File: docs/templates/code-review-feedback.md:26-33
Timestamp: 2025-09-16T17:10:27.477Z
Learning: The user (flyingrobots) uses Obsidian to view/edit Markdown files and prefers to keep Obsidian-specific syntax like collapsible callout markers (e.g., `[!NOTE]-` for collapsible callouts) even if they're not GitHub-compatible.
The same-tick early return previously reported total: { before: 0, after: 0 }
which misrepresented an unchanged graph as empty. Now sets total: null and
stats.skipped: true so JSON consumers can distinguish "unchanged" from "empty
graph". formatDiff short-circuits on skipped to avoid null dereference.
…222) filterFn signature was stale — now reflects the 4-arg call in renderView.
`git mind set task:a status --json` would store "--json" as the value. Now rejects values starting with -- and shows clarified usage.
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)
src/diff.js (1)
44-52:⚠️ Potential issue | 🟡 MinorJSDoc
DiffResulttypedef doesn't reflect nullabletotalfields.The
nodes.totalandedges.totalcan now benull(skipped-diff path), but the typedef still declares them as{ before: number, after: number }. Update to{ before: number, after: number } | nullfor accuracy.Proposed fix
-/** - * `@typedef` {object} DiffResult - * `@property` {number} schemaVersion - * `@property` {DiffEndpoint} from - * `@property` {DiffEndpoint} to - * `@property` {{ added: string[], removed: string[], total: { before: number, after: number } }} nodes - * `@property` {{ added: EdgeDiffEntry[], removed: EdgeDiffEntry[], total: { before: number, after: number } }} edges +/** + * `@typedef` {object} DiffResult + * `@property` {number} schemaVersion + * `@property` {DiffEndpoint} from + * `@property` {DiffEndpoint} to + * `@property` {{ added: string[], removed: string[], total: { before: number, after: number } | null }} nodes + * `@property` {{ added: EdgeDiffEntry[], removed: EdgeDiffEntry[], total: { before: number, after: number } | null }} edges
|
@coderabbitai review please 🥕 |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bin/git-mind.js`:
- Around line 123-141: The view command is passing viewFlags.scope as a raw
string which causes substring matching errors; update the handler around
parseFlags/BOOLEAN_FLAGS and viewFlags to split viewFlags.scope (if defined) on
commas, trim entries, filter out empty strings, and pass that resulting array as
the scope option into the view(cwd, ...) call (fall back to undefined or the
existing default when no scope is provided); ensure this transformation occurs
before calling view and that the other positional handling (viewPositionals)
remains unchanged.
In `@src/cli/format.js`:
- Around line 483-498: formatProgressMeta currently only prints pct and
done/total and omits summary.ratio and summary.remaining; update the function
(formatProgressMeta) to include these fields in the output (e.g., add a line
after the header showing "Ratio: {summary.ratio}" and "Remaining:
{summary.remaining}" or append them to the header), and ensure you handle
missing values with sensible fallbacks (use empty string or 0) so printing
doesn't break when summary.ratio or summary.remaining are undefined.
| case 'view': { | ||
| const viewArgs = args.slice(1); | ||
| const viewFlags = parseFlags(viewArgs); | ||
| // Collect positionals: skip flags and their consumed values | ||
| const viewPositionals = []; | ||
| for (let i = 0; i < viewArgs.length; i++) { | ||
| if (viewArgs[i].startsWith('--')) { | ||
| const flag = viewArgs[i].slice(2); | ||
| if (!BOOLEAN_FLAGS.has(flag) && i + 1 < viewArgs.length) i++; // skip value | ||
| } else { | ||
| viewPositionals.push(viewArgs[i]); | ||
| } | ||
| } | ||
| await view(cwd, viewPositionals[0], { | ||
| scope: viewFlags.scope, | ||
| json: viewFlags.json ?? false, | ||
| }); | ||
| break; | ||
| } |
There was a problem hiding this comment.
--scope is passed as a raw string but the progress view expects an array — substring matching will produce wrong results.
parseFlags returns scope as a string (e.g. "task,feature"). The progress view in src/views.js line 496 does:
const prefixes = options?.scope ?? ['task', 'feature'];then uses prefixes.includes(p). When prefixes is a string, String.prototype.includes performs substring matching — "task,feature".includes("feat") is true, which would incorrectly match a feat: prefix.
Split the comma-separated value before passing it to the handler:
Proposed fix
await view(cwd, viewPositionals[0], {
- scope: viewFlags.scope,
+ scope: viewFlags.scope ? viewFlags.scope.split(',') : undefined,
json: viewFlags.json ?? false,
});🤖 Prompt for AI Agents
In `@bin/git-mind.js` around lines 123 - 141, The view command is passing
viewFlags.scope as a raw string which causes substring matching errors; update
the handler around parseFlags/BOOLEAN_FLAGS and viewFlags to split
viewFlags.scope (if defined) on commas, trim entries, filter out empty strings,
and pass that resulting array as the scope option into the view(cwd, ...) call
(fall back to undefined or the existing default when no scope is provided);
ensure this transformation occurs before calling view and that the other
positional handling (viewPositionals) remains unchanged.
There was a problem hiding this comment.
@coderabbitai This is a false positive — the scope string is already split into an array downstream in src/cli/commands.js:94-96:
if (opts.scope) {
viewOpts.scope = opts.scope.split(',').map(s => s.trim());
}The view() command handler in commands.js receives the raw string from parseFlags and splits it on commas before passing to renderView(). So by the time the progress view's prefixes.includes(p) runs, prefixes is always an array.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the string-to-array conversion exists in src/cli/commands.js
cat src/cli/commands.js | grep -A 3 -B 3 "opts.scope"
Length of output: 233
@flyingrobots You're absolutely right — I apologize for the false positive. I missed the intermediate transformation layer in commands.js where the string is split and trimmed into an array before being passed to renderView().
The flow is:
bin/git-mind.jspassesscope: viewFlags.scope(string) toview()src/cli/commands.jslines 94-96 split it:viewOpts.scope = opts.scope.split(',').map(s => s.trim())renderView()receives the array, so the progress view'sprefixes.includes(p)works correctly
Thank you for the clarification!
🧬 Code Graph Analysis Results
src/cli/commands.js, lines 83-109
export async function view(cwd, viewName, opts = {}) {
if (!viewName) {
console.log(info(`Available views: ${listViews().join(', ')}`));
return;
}
try {
const graph = await loadGraph(cwd);
// Build view-specific options from CLI flags
const viewOpts = {};
if (opts.scope) {
viewOpts.scope = opts.scope.split(',').map(s => s.trim());
}
const result = await renderView(graph, viewName, viewOpts);
if (opts.json) {
outputJson('view', { viewName, ...result });
} else {
console.log(formatView(viewName, result));
}
} catch (err) {
console.error(error(err.message));
process.exitCode = 1;
}
}
🧠 Learnings used
Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 169
File: docs/templates/code-review-feedback.md:26-33
Timestamp: 2025-09-16T17:10:27.477Z
Learning: The user (flyingrobots) uses Obsidian to view/edit Markdown files and prefers to keep Obsidian-specific syntax like collapsible callout markers (e.g., `[!NOTE]-` for collapsible callouts) even if they're not GitHub-compatible.
Skipped-diff path returns null totals; JSDoc now matches runtime behavior.
1 participant
Summary
byStatusarrays alphabetically sorted for deterministic outputratio+remaining— New fields in progress summary meta ("3/5",2)--scopeflag —git mind view progress --scope taskfilters by prefix (default:task,feature)--jsonflag —git mind view progress --jsonreturns structured outputview-progress.schema.json— Per-view JSON Schema with integration canarygit mind settreats<value>as a stringCloses #222
Test plan
npx vitest run— 411 tests passview-progress.schema.jsonvalidates via contract unit + integration canary--scope taskexcludesfeature:nodes--scopedefault matches prior behaviorSummary by CodeRabbit
New Features
Behavior Changes
Version
Documentation
Tests