Add Black Box TUI graph#136
Open
ya-nsh wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughAdds BlackBox traces: core types and builder, renderer live HTML/server, CLI blackbox subcommand, TUI state/view integration, panel UI with keyboard navigation, and supporting tests. ChangesBlackBox Trace Visualization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 4
🧹 Nitpick comments (2)
packages/core/src/aggregation/blackbox.test.ts (1)
106-116: ⚡ Quick winAdd regression tests for offset timestamps and Windows paths.
Please add cases proving: (1) session/event ordering with mixed ISO offsets, and (2)
C:\...prompt path redaction.Suggested additions
describe('redactPromptSnippet', () => { test('redacts paths, emails, secrets, and truncates long prompts', () => { @@ }); + + test('redacts Windows paths', () => { + const redacted = redactPromptSnippet('open C:\\Users\\alice\\repo\\src\\file.ts'); + expect(redacted).toContain('[path]'); + }); }); + +describe('buildBlackBoxTrace ordering', () => { + test('orders sessions chronologically when timestamps include offsets', () => { + const trace = buildBlackBoxTrace( + [provider([ + event({ sessionId: 'a', timestamp: '2026-04-25T10:00:00+02:00', date: '2026-04-25' }), + event({ sessionId: 'b', timestamp: '2026-04-25T09:30:00Z', date: '2026-04-25' }), + ])], + { since: '2026-04-01', until: '2026-04-30' }, + ); + expect(trace.target?.sessionId).toBe('b'); + }); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/aggregation/blackbox.test.ts` around lines 106 - 116, Add two regression tests in packages/core/src/aggregation/blackbox.test.ts: one that verifies session/event ordering when timestamps use mixed ISO offsets by creating events/sessions with differing timezone offsets and asserting the ordering logic (the same code path used by your session/event ordering functions) sorts them chronologically, and another test that ensures redactPromptSnippet correctly redacts Windows-style paths (e.g., a prompt containing "C:\\Users\\alice\\secret\\file.txt") by asserting the output contains "[path]". Place both tests alongside the existing redactPromptSnippet test, reusing redactPromptSnippet for the Windows path case and invoking the same session/event ordering code path used in production for the timestamp-offset case so the regression is covered.packages/tui/src/index.ts (1)
1106-1119: ⚡ Quick winUnify the Black Box reset path.
handleViewSwitch()manually clears onlyblackBoxSelectedNodeIndexandblackBoxExpanded, whileresetBlackBoxInteraction()also resetsblackBoxTargetIndexandblackBoxFocusMode. Either reuse the helper here or split “view switch” vs “window/data reset” helpers so the differing semantics are explicit; right now the two reset paths can drift.[recommendation]
♻️ If a full view-switch reset is intended
- currentState.blackBoxSelectedNodeIndex = 0; - currentState.blackBoxExpanded = false; + resetBlackBoxInteraction(currentState);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tui/src/index.ts` around lines 1106 - 1119, handleViewSwitch currently resets only blackBoxSelectedNodeIndex and blackBoxExpanded while resetBlackBoxInteraction also clears blackBoxTargetIndex and blackBoxFocusMode, causing divergent reset paths; update handleViewSwitch to either call resetBlackBoxInteraction() or replicate its full reset (blackBoxSelectedNodeIndex, blackBoxExpanded, blackBoxTargetIndex, blackBoxFocusMode) so both view-switch and explicit black-box resets remain consistent, or refactor by splitting into two clear helpers (e.g., resetBlackBoxForViewSwitch vs resetBlackBoxForDataClear) and use the appropriate one from handleViewSwitch and wherever resetBlackBoxInteraction is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/aggregation/blackbox.ts`:
- Around line 79-84: The code currently orders events using lexical timestamp
comparison (localeCompare) which misorders ISO timestamps with offsets; update
collectEvents and any other places using string comparison (e.g., the sort
comparator at the other occurrences mentioned) to parse timestamps to numeric
time and compare those numbers instead (use Date.parse(...) or new
Date(...).getTime() for a and b) so sorting and chronology use actual
chronological order; ensure the comparator handles NaN safely (fall back to 0)
and preserve stable ordering for equal timestamps.
- Around line 210-213: The matching logic builds repoRoots by trimming
event.repoRoot but then compares raw signal.repoRoot, causing mismatches; update
the check in the loop that iterates over signals to normalize signal.repoRoot
the same way (trim and guard for falsy) before comparing against the repoRoots
Set (i.e., normalize the value used in the has() call so
repoRoots.has(trimmedSignalRepoRoot) is used), and ensure the repoRoots Set
creation and the comparison treat undefined/null consistently (events.map(...)
and the for (const signal of signals) { ... } block referencing
signal.repoRoot).
- Around line 173-177: The compactization regex for paths (used when building
the compact variable in blackbox.ts) only matches POSIX-style paths and misses
Windows paths like C:\foo\bar; update the path-replacement regex to also detect
Windows drive-letter paths and backslash separators (and consider UNC paths) so
that .replace(/(?:[A-Za-z]:)?(?:\/[\w.-]+){2,}/g, '[path]') covers patterns like
C:\... and \\server\share\...; modify the path-matching expression used in the
chain that constructs compact to include a branch for Windows-style paths
(escaping backslashes properly in the JS string) while preserving the existing
POSIX match and the surrounding replacement logic.
In `@packages/tui/src/index.ts`:
- Around line 509-521: The block that calls
loadNutritionOutcomeSignalsForWindow(state) must snapshot the relevant state
before awaiting to avoid applying stale results: capture the current state.data
reference (e.g., const dataBefore = state.data), the selectedWindowIndex (const
idxBefore = state.selectedWindowIndex) and the signalKey (const signalKey =
nutritionWindowKey(state)) before the await; after the await, re-check that
dataBefore === state.data, idxBefore === state.selectedWindowIndex and
getSelectedViewTaskKey(state, 'blackbox') still equals key (and signalKey still
truthy) and only then assign window.nutritionOutcomeSignals and add to
state.nutritionSignalsLoadedKeys and call ensureBlackBoxTrace; if any of those
changed, discard the loaded signals.
---
Nitpick comments:
In `@packages/core/src/aggregation/blackbox.test.ts`:
- Around line 106-116: Add two regression tests in
packages/core/src/aggregation/blackbox.test.ts: one that verifies session/event
ordering when timestamps use mixed ISO offsets by creating events/sessions with
differing timezone offsets and asserting the ordering logic (the same code path
used by your session/event ordering functions) sorts them chronologically, and
another test that ensures redactPromptSnippet correctly redacts Windows-style
paths (e.g., a prompt containing "C:\\Users\\alice\\secret\\file.txt") by
asserting the output contains "[path]". Place both tests alongside the existing
redactPromptSnippet test, reusing redactPromptSnippet for the Windows path case
and invoking the same session/event ordering code path used in production for
the timestamp-offset case so the regression is covered.
In `@packages/tui/src/index.ts`:
- Around line 1106-1119: handleViewSwitch currently resets only
blackBoxSelectedNodeIndex and blackBoxExpanded while resetBlackBoxInteraction
also clears blackBoxTargetIndex and blackBoxFocusMode, causing divergent reset
paths; update handleViewSwitch to either call resetBlackBoxInteraction() or
replicate its full reset (blackBoxSelectedNodeIndex, blackBoxExpanded,
blackBoxTargetIndex, blackBoxFocusMode) so both view-switch and explicit
black-box resets remain consistent, or refactor by splitting into two clear
helpers (e.g., resetBlackBoxForViewSwitch vs resetBlackBoxForDataClear) and use
the appropriate one from handleViewSwitch and wherever resetBlackBoxInteraction
is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b699b729-a9fc-4d9f-a630-7fe700cfd279
📒 Files selected for processing (16)
packages/cli/src/cli.test.tspackages/core/src/aggregation/blackbox.test.tspackages/core/src/aggregation/blackbox.tspackages/core/src/aggregation/index.tspackages/core/src/index.tspackages/core/src/types.tspackages/tui/src/index.tspackages/tui/src/lib/data.tspackages/tui/src/lib/state.tspackages/tui/src/panels/blackbox.test.tspackages/tui/src/panels/blackbox.tspackages/tui/src/panels/header.test.tspackages/tui/src/panels/header.tspackages/tui/src/panels/help.tspackages/tui/src/panels/status-bar.test.tspackages/tui/src/panels/status-bar.ts
Comment on lines
+79
to
+84
| function collectEvents(providers: ProviderData[], range: DateRange): UsageEvent[] { | ||
| return providers | ||
| .flatMap((provider) => provider.events ?? []) | ||
| .filter((event) => inRange(event, range)) | ||
| .sort((a, b) => a.timestamp.localeCompare(b.timestamp)); | ||
| } |
There was a problem hiding this comment.
Use parsed-time ordering for chronology and target selection.
Line 83 / Line 97 / Line 113 / Line 306 rely on lexical timestamp comparison. That can misorder valid ISO timestamps with offsets, which skews session ordering, flow chronology, and hot-path selection.
Suggested fix
+function compareEventTimeAsc(a: UsageEvent, b: UsageEvent): number {
+ return eventTime(a) - eventTime(b) || a.timestamp.localeCompare(b.timestamp);
+}
+
+function compareIsoDesc(a: string, b: string): number {
+ const diff = Date.parse(b) - Date.parse(a);
+ return Number.isFinite(diff) && diff !== 0 ? diff : b.localeCompare(a);
+}
+
function collectEvents(providers: ProviderData[], range: DateRange): UsageEvent[] {
return providers
.flatMap((provider) => provider.events ?? [])
.filter((event) => inRange(event, range))
- .sort((a, b) => a.timestamp.localeCompare(b.timestamp));
+ .sort(compareEventTimeAsc);
}
@@
- const ordered = sessionEvents.slice().sort((a, b) => a.timestamp.localeCompare(b.timestamp));
+ const ordered = sessionEvents.slice().sort(compareEventTimeAsc);
@@
- .sort((a, b) => b.end.localeCompare(a.end) || b.cost - a.cost || b.tokens - a.tokens);
+ .sort((a, b) => compareIsoDesc(a.end, b.end) || b.cost - a.cost || b.tokens - a.tokens);
@@
- .sort((a, b) => a.event.timestamp.localeCompare(b.event.timestamp));
+ .sort((a, b) => compareEventTimeAsc(a.event, b.event));Also applies to: 95-114, 302-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/aggregation/blackbox.ts` around lines 79 - 84, The code
currently orders events using lexical timestamp comparison (localeCompare) which
misorders ISO timestamps with offsets; update collectEvents and any other places
using string comparison (e.g., the sort comparator at the other occurrences
mentioned) to parse timestamps to numeric time and compare those numbers instead
(use Date.parse(...) or new Date(...).getTime() for a and b) so sorting and
chronology use actual chronological order; ensure the comparator handles NaN
safely (fall back to 0) and preserve stable ordering for equal timestamps.
Comment on lines
+173
to
+177
| const compact = prompt | ||
| .replace(/[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}/gi, '[email]') | ||
| .replace(/(?:[A-Za-z]:)?(?:\/[\w.-]+){2,}/g, '[path]') | ||
| .replace(/\b(?:sk|pk|ghp|gho|ghu|ghs|xox[baprs])_[A-Za-z0-9_=-]{12,}\b/g, '[secret]') | ||
| .replace(/\s+/g, ' ') |
There was a problem hiding this comment.
Redaction currently misses Windows-style paths.
Line 175 only matches /... paths. Prompts containing C:\... won’t be redacted, which can leak local path details in snippets.
Suggested fix
- .replace(/(?:[A-Za-z]:)?(?:\/[\w.-]+){2,}/g, '[path]')
+ .replace(/(?:[A-Za-z]:)?(?:[\\/][^\\/\s]+){2,}/g, '[path]')📝 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 compact = prompt | |
| .replace(/[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}/gi, '[email]') | |
| .replace(/(?:[A-Za-z]:)?(?:\/[\w.-]+){2,}/g, '[path]') | |
| .replace(/\b(?:sk|pk|ghp|gho|ghu|ghs|xox[baprs])_[A-Za-z0-9_=-]{12,}\b/g, '[secret]') | |
| .replace(/\s+/g, ' ') | |
| const compact = prompt | |
| .replace(/[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}/gi, '[email]') | |
| .replace(/(?:[A-Za-z]:)?(?:[\\/][^\\/\s]+){2,}/g, '[path]') | |
| .replace(/\b(?:sk|pk|ghp|gho|ghu|ghs|xox[baprs])_[A-Za-z0-9_=-]{12,}\b/g, '[secret]') | |
| .replace(/\s+/g, ' ') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/aggregation/blackbox.ts` around lines 173 - 177, The
compactization regex for paths (used when building the compact variable in
blackbox.ts) only matches POSIX-style paths and misses Windows paths like
C:\foo\bar; update the path-replacement regex to also detect Windows
drive-letter paths and backslash separators (and consider UNC paths) so that
.replace(/(?:[A-Za-z]:)?(?:\/[\w.-]+){2,}/g, '[path]') covers patterns like
C:\... and \\server\share\...; modify the path-matching expression used in the
chain that constructs compact to include a branch for Windows-style paths
(escaping backslashes properly in the JS string) while preserving the existing
POSIX match and the surrounding replacement logic.
Comment on lines
+210
to
+213
| const repoRoots = new Set(events.map((event) => event.repoRoot?.trim()).filter(Boolean) as string[]); | ||
| for (const signal of signals) { | ||
| if (repoRoots.has(signal.repoRoot)) return signal; | ||
| } |
There was a problem hiding this comment.
Normalize outcome signal repo roots before matching.
Line 210 trims event repo roots, but Line 212 compares against raw signal.repoRoot. This can miss valid matches due to whitespace differences.
Suggested fix
const repoRoots = new Set(events.map((event) => event.repoRoot?.trim()).filter(Boolean) as string[]);
for (const signal of signals) {
- if (repoRoots.has(signal.repoRoot)) return signal;
+ const repoRoot = signal.repoRoot.trim();
+ if (repoRoot && repoRoots.has(repoRoot)) return signal;
}📝 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 repoRoots = new Set(events.map((event) => event.repoRoot?.trim()).filter(Boolean) as string[]); | |
| for (const signal of signals) { | |
| if (repoRoots.has(signal.repoRoot)) return signal; | |
| } | |
| const repoRoots = new Set(events.map((event) => event.repoRoot?.trim()).filter(Boolean) as string[]); | |
| for (const signal of signals) { | |
| const repoRoot = signal.repoRoot.trim(); | |
| if (repoRoot && repoRoots.has(repoRoot)) return signal; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/aggregation/blackbox.ts` around lines 210 - 213, The
matching logic builds repoRoots by trimming event.repoRoot but then compares raw
signal.repoRoot, causing mismatches; update the check in the loop that iterates
over signals to normalize signal.repoRoot the same way (trim and guard for
falsy) before comparing against the repoRoots Set (i.e., normalize the value
used in the has() call so repoRoots.has(trimmedSignalRepoRoot) is used), and
ensure the repoRoots Set creation and the comparison treat undefined/null
consistently (events.map(...) and the for (const signal of signals) { ... }
block referencing signal.repoRoot).
Comment on lines
+509
to
+521
| const signalKey = nutritionWindowKey(state); | ||
| if (signalKey && !state.nutritionSignalsLoadedKeys.has(signalKey)) { | ||
| const signals = await loadNutritionOutcomeSignalsForWindow(state); | ||
| if (getSelectedViewTaskKey(state, 'blackbox') !== key) { | ||
| return; | ||
| } | ||
| const window = state.data?.windows[state.selectedWindowIndex]; | ||
| if (window) { | ||
| window.nutritionOutcomeSignals = signals; | ||
| } | ||
| state.nutritionSignalsLoadedKeys.add(signalKey); | ||
| } | ||
| ensureBlackBoxTrace(state); |
There was a problem hiding this comment.
Drop stale async Black Box results when state.data changes.
loadNutritionOutcomeSignalsForWindow(state) reads the current dataset before the await, but after it resolves this block only re-checks the view key. applyLoadedData() can replace state.data during boot refresh or an r refresh without changing that key, so these lines can write outcome signals computed from the old dataset into the new window and then build a trace against mismatched inputs. Capture the current state.data and selectedWindowIndex before awaiting, and ignore the result if either changed before assignment/build.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/tui/src/index.ts` around lines 509 - 521, The block that calls
loadNutritionOutcomeSignalsForWindow(state) must snapshot the relevant state
before awaiting to avoid applying stale results: capture the current state.data
reference (e.g., const dataBefore = state.data), the selectedWindowIndex (const
idxBefore = state.selectedWindowIndex) and the signalKey (const signalKey =
nutritionWindowKey(state)) before the await; after the await, re-check that
dataBefore === state.data, idxBefore === state.selectedWindowIndex and
getSelectedViewTaskKey(state, 'blackbox') still equals key (and signalKey still
truthy) and only then assign window.nutritionOutcomeSignals and add to
state.nutritionSignalsLoadedKeys and call ensureBlackBoxTrace; if any of those
changed, discard the loaded signals.
ya-nsh
force-pushed
the
feat/blackbox-tui
branch
from
c7ceada to
778346b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/src/cli.test.ts (1)
122-153: ⚡ Quick winAdd a regression test for out-of-range positive
--targetbehavior.Current tests cover negative-target rejection only. Add one case that verifies runtime behavior when
--targetexceeds available targets (clamp or explicit error), so the CLI/live contract stays stable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli.test.ts` around lines 122 - 153, Add a regression test "handles out-of-range positive target" for parseBlackBoxArgs that exercises a large --target (e.g. ['--target','999']) and asserts the current, intended runtime contract: if parseBlackBoxArgs currently throws for out-of-range positives, assert it throws with the expected error message; otherwise assert it returns a numeric targetIndex (and if your design clamps, assert the clamped value). Name the test clearly and place it alongside the existing parseBlackBoxArgs tests so future changes to parseBlackBoxArgs will be caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/cli.ts`:
- Around line 3076-3147: The CLI currently uses the raw targetIndex when
creating initialTrace and serverArg.initialTargetIndex which can be out-of-range
for initialTrace.targets; clamp or validate the index before building the trace:
compute the trace (via buildTrace) and then set initialTargetIndex = Math.max(0,
Math.min(rawTargetIndex, initialTrace.targets.length - 1)) (or throw if you
prefer rejection) and use that clamped value when generating initialTrace/html
and when populating serverArg.initialTargetIndex and getTrace defaults so the
initial selection is always valid; update references to targetIndex/initialTrace
accordingly (symbols: targetIndex, buildTrace, initialTrace,
serverArg.initialTargetIndex, getTrace).
---
Nitpick comments:
In `@packages/cli/src/cli.test.ts`:
- Around line 122-153: Add a regression test "handles out-of-range positive
target" for parseBlackBoxArgs that exercises a large --target (e.g.
['--target','999']) and asserts the current, intended runtime contract: if
parseBlackBoxArgs currently throws for out-of-range positives, assert it throws
with the expected error message; otherwise assert it returns a numeric
targetIndex (and if your design clamps, assert the clamped value). Name the test
clearly and place it alongside the existing parseBlackBoxArgs tests so future
changes to parseBlackBoxArgs will be caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e83f1c89-7351-4912-9d5f-1b8c653f2af4
📒 Files selected for processing (21)
packages/cli/src/cli.test.tspackages/cli/src/cli.tspackages/core/src/aggregation/blackbox.test.tspackages/core/src/aggregation/blackbox.tspackages/core/src/aggregation/index.tspackages/core/src/index.tspackages/core/src/types.tspackages/renderers/src/index.tspackages/renderers/src/live/__tests__/blackbox-live-template.test.tspackages/renderers/src/live/blackbox-live-server.tspackages/renderers/src/live/blackbox-live-template.tspackages/tui/src/index.tspackages/tui/src/lib/data.tspackages/tui/src/lib/state.tspackages/tui/src/panels/blackbox.test.tspackages/tui/src/panels/blackbox.tspackages/tui/src/panels/header.test.tspackages/tui/src/panels/header.tspackages/tui/src/panels/help.tspackages/tui/src/panels/status-bar.test.tspackages/tui/src/panels/status-bar.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/tui/src/panels/header.ts
- packages/core/src/aggregation/index.ts
- packages/tui/src/panels/status-bar.ts
- packages/tui/src/panels/header.test.ts
- packages/tui/src/panels/status-bar.test.ts
- packages/tui/src/panels/blackbox.test.ts
- packages/core/src/aggregation/blackbox.test.ts
- packages/tui/src/panels/help.ts
- packages/tui/src/lib/data.ts
- packages/core/src/types.ts
- packages/tui/src/lib/state.ts
- packages/core/src/aggregation/blackbox.ts
- packages/tui/src/panels/blackbox.ts
- packages/tui/src/index.ts
- packages/core/src/index.ts
Comment on lines
+3076
to
+3147
| const targetIndex = typeof cliArgs['targetIndex'] === 'number' ? cliArgs['targetIndex'] : 0; | ||
|
|
||
| if ( | ||
| config.allProviders && | ||
| (config.provider || | ||
| config.claude || | ||
| config.codex || | ||
| config.cursor || | ||
| config.pi || | ||
| config.openCode) | ||
| ) { | ||
| throw new TokenleakError('--all-providers cannot be combined with provider filters'); | ||
| } | ||
|
|
||
| if (config.listProviders) { | ||
| const registry = createRegistry(); | ||
| const providers = registry.getAll(); | ||
| const availabilityResults = await Promise.all( | ||
| providers.map(async (provider) => [provider.name, await provider.isAvailable()] as const), | ||
| ); | ||
| process.stdout.write(buildProviderList(providers, new Map(availabilityResults))); | ||
| return; | ||
| } | ||
|
|
||
| const dateRange = computeDateRange({ | ||
| since: config.since, | ||
| until: config.until, | ||
| days: config.days, | ||
| }); | ||
| const available = await selectAvailableProviders(config); | ||
|
|
||
| if (available.length === 0) { | ||
| throw new TokenleakError('No provider data found'); | ||
| } | ||
|
|
||
| const output = await loadTokenleakData(available, dateRange); | ||
| emitProviderWarnings(output.providers, 'Warning'); | ||
| const events = output.providers.flatMap((provider) => provider.events ?? []); | ||
| let outcomeSignals: Awaited<ReturnType<typeof collectGitOutcomeSignals>> = []; | ||
| try { | ||
| outcomeSignals = await collectGitOutcomeSignals(events, dateRange); | ||
| } catch (err) { | ||
| const detail = err instanceof Error ? err.message : String(err); | ||
| process.stderr.write(`Warning: could not load Git outcome signals: ${detail}\n`); | ||
| } | ||
|
|
||
| const buildTrace = (index: number): BlackBoxTrace => | ||
| buildBlackBoxTrace(output.providers, dateRange, { | ||
| targetIndex: index, | ||
| outcomeSignals, | ||
| }); | ||
| const initialTrace = buildTrace(targetIndex); | ||
|
|
||
| if (config.output) { | ||
| const html = generateBlackBoxLiveHtml(initialTrace, { targetIndex }); | ||
| writeFileSync(config.output, html); | ||
| process.stderr.write(`Black Box graph written to ${config.output}\n`); | ||
| if (config.open) { | ||
| await openFile(config.output); | ||
| process.stderr.write(`Opened ${config.output} in default application.\n`); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const serverArg: BlackBoxLiveDataProvider = { | ||
| initialTargetIndex: targetIndex, | ||
| initialTrace, | ||
| getTrace: (index: number) => { | ||
| if (index < 0 || index >= initialTrace.targets.length) return null; | ||
| return buildTrace(index); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Clamp or reject out-of-range --target before building live/output state.
--target is validated as non-negative, but not bounded against trace.targets.length. In runBlackBox, the raw value is used as initialTargetIndex, which can yield an invalid initial selection state even when valid targets exist. TUI already clamps this index after trace construction (packages/tui/src/lib/data.ts, Lines 524-540), so CLI should align.
💡 Suggested fix
- const targetIndex = typeof cliArgs['targetIndex'] === 'number' ? cliArgs['targetIndex'] : 0;
+ const requestedTargetIndex = typeof cliArgs['targetIndex'] === 'number' ? cliArgs['targetIndex'] : 0;
...
- const initialTrace = buildTrace(targetIndex);
+ const probeTrace = buildTrace(requestedTargetIndex);
+ const maxIndex = Math.max(0, probeTrace.targets.length - 1);
+ const targetIndex = Math.min(requestedTargetIndex, maxIndex);
+ const initialTrace = targetIndex === requestedTargetIndex ? probeTrace : buildTrace(targetIndex);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/cli.ts` around lines 3076 - 3147, The CLI currently uses the
raw targetIndex when creating initialTrace and serverArg.initialTargetIndex
which can be out-of-range for initialTrace.targets; clamp or validate the index
before building the trace: compute the trace (via buildTrace) and then set
initialTargetIndex = Math.max(0, Math.min(rawTargetIndex,
initialTrace.targets.length - 1)) (or throw if you prefer rejection) and use
that clamped value when generating initialTrace/html and when populating
serverArg.initialTargetIndex and getTrace defaults so the initial selection is
always valid; update references to targetIndex/initialTrace accordingly
(symbols: targetIndex, buildTrace, initialTrace, serverArg.initialTargetIndex,
getTrace).
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
tokenleak blackboxfor an Obsidian-style browser graph with pan/zoom, click-to-expand nodes, focus lanes, inspector details, session target navigation, and standalone HTML exportTests
Manual verification
Summary by CodeRabbit
New Features
Tests