fix(ccusage): ccusage session id= issue on Windows with rel paths#921
fix(ccusage): ccusage session id= issue on Windows with rel paths#921Andrej730 wants to merge 1 commit intoryoppippi:mainfrom
ccusage session id= issue on Windows with rel paths#921Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChanged glob invocation in loadSessionUsageById to return absolute paths and added a Vitest test that verifies constructed session glob patterns normalize Windows drive-letter base paths to use forward slashes (no backslashes). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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 |
Andrej730
force-pushed
the
fix-windows-session-by-id
branch
from
1dca33d to
f01bffa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/ccusage/src/data-loader.ts (1)
1135-1139: Add a Windows-specific regression test for this glob path fix.Line 1139 is the right functional fix, but this path bug is platform-specific and easy to regress. Please add an in-source test that simulates Windows-style paths (including a drive-letter-like base path) and asserts
loadSessionUsageByIdresolves and reads the file successfully.As per coding guidelines, "Write tests in-source using
if (import.meta.vitest != null)blocks instead of separate test files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccusage/src/data-loader.ts` around lines 1135 - 1139, Add an in-source Vitest block to prevent regression of the Windows-path glob fix: inside apps/ccusage/src/data-loader.ts add an if (import.meta.vitest != null) test that stubs claudePaths to contain a Windows-style base (e.g., "C:\\some\\base") and a mocked filesystem where a projects/**/<sessionId>.jsonl file exists; call loadSessionUsageById with that sessionId and assert it resolves and returns the expected content. Ensure the test exercises the path.join(...).replace(/\\/g,'/') behavior (so it uses the same code path that builds patterns) and uses a temporary/mock glob or fs stub to avoid real IO; reference the functions/variables loadSessionUsageById and claudePaths/patterns to locate where to hook the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/ccusage/src/data-loader.ts`:
- Around line 1135-1139: Add an in-source Vitest block to prevent regression of
the Windows-path glob fix: inside apps/ccusage/src/data-loader.ts add an if
(import.meta.vitest != null) test that stubs claudePaths to contain a
Windows-style base (e.g., "C:\\some\\base") and a mocked filesystem where a
projects/**/<sessionId>.jsonl file exists; call loadSessionUsageById with that
sessionId and assert it resolves and returns the expected content. Ensure the
test exercises the path.join(...).replace(/\\/g,'/') behavior (so it uses the
same code path that builds patterns) and uses a temporary/mock glob or fs stub
to avoid real IO; reference the functions/variables loadSessionUsageById and
claudePaths/patterns to locate where to hook the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 441b61a4-932a-472d-aadd-41ec305ca90d
📒 Files selected for processing (1)
apps/ccusage/src/data-loader.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/ccusage/src/data-loader.ts (1)
1621-1629: Good regression test for pattern normalization, but consider adding a note about theabsolute: truefix.This test validates the backslash-to-forward-slash normalization, which is essential for
tinyglobbycompatibility on Windows. However, it doesn't directly test the primary fix ({absolute: true}option) that prevents malformed relative paths when session files are on a different disk.Consider adding a brief comment to document that this test covers the pattern normalization aspect, while the
{absolute: true}option (tested implicitly by the integration tests above) handles the cross-disk absolute path requirement.📝 Suggested documentation enhancement
- it('builds glob patterns with forward slashes from Windows-style drive-letter paths', () => { + // Regression test: Pattern normalization for tinyglobby on Windows. + // The `{absolute: true}` option in loadSessionUsageById handles cross-disk path resolution. + it('builds glob patterns with forward slashes from Windows-style drive-letter paths', () => { const windowsBase = 'C:\\Users\\test\\.claude';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccusage/src/data-loader.ts` around lines 1621 - 1629, Add a short inline comment in the test "builds glob patterns with forward slashes from Windows-style drive-letter paths" (the spec that reproduces the pattern-building logic used by loadSessionUsageById) clarifying that this unit test verifies path separator normalization for tinyglobby, and that the separate fix using the {absolute: true} option (applied where loadSessionUsageById builds the glob) is responsible for ensuring absolute paths across drives and is covered by the integration tests; reference the test name and the loadSessionUsageById pattern construction so future readers know which code handles normalization vs. cross-disk absolute path behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/ccusage/src/data-loader.ts`:
- Around line 1621-1629: Add a short inline comment in the test "builds glob
patterns with forward slashes from Windows-style drive-letter paths" (the spec
that reproduces the pattern-building logic used by loadSessionUsageById)
clarifying that this unit test verifies path separator normalization for
tinyglobby, and that the separate fix using the {absolute: true} option (applied
where loadSessionUsageById builds the glob) is responsible for ensuring absolute
paths across drives and is covered by the integration tests; reference the test
name and the loadSessionUsageById pattern construction so future readers know
which code handles normalization vs. cross-disk absolute path behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 755838b8-e92d-4b9f-93b8-48b9ac31c0db
📒 Files selected for processing (1)
apps/ccusage/src/data-loader.ts
Andrej730
force-pushed
the
fix-windows-session-by-id
branch
from
0902962 to
ca7eef5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/ccusage/src/data-loader.ts (1)
1139-1140: Good fix for Windows cross-drive path handling.Adding
{ absolute: true }aligns this glob call with the other two usages in this file (lines 719-722 and 1364-1367) and correctly fixes the Windows issue where relative paths would produce malformed combined paths.Minor formatting nit: Consider adding spaces inside the object literal for consistency with the rest of the codebase (e.g., line 721 uses
{ cwd: claudeDir, absolute: true }).♻️ Optional formatting fix
- const jsonlFiles = await glob(patterns, {absolute: true}); + const jsonlFiles = await glob(patterns, { absolute: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccusage/src/data-loader.ts` around lines 1139 - 1140, The glob call creating jsonlFiles uses an object literal without spaces; update the call to glob(patterns, { absolute: true }) for consistency with other usages (see other calls around glob(..., { cwd: claudeDir, absolute: true }) ) so the option remains absolute: true while matching project formatting; ensure you only change the spacing inside the object literal for the symbol jsonlFiles/glob.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/ccusage/src/data-loader.ts`:
- Around line 1139-1140: The glob call creating jsonlFiles uses an object
literal without spaces; update the call to glob(patterns, { absolute: true })
for consistency with other usages (see other calls around glob(..., { cwd:
claudeDir, absolute: true }) ) so the option remains absolute: true while
matching project formatting; ensure you only change the spacing inside the
object literal for the symbol jsonlFiles/glob.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c45ca477-d604-4360-8c9c-4815481d4621
📒 Files selected for processing (1)
apps/ccusage/src/data-loader.ts
Andrej730
force-pushed
the
fix-windows-session-by-id
branch
from
ca7eef5 to
cdd6840
Compare
Otherwise `glob` resulted in odd relative paths like `"../../../../../../C:/Users/xxxx/.claude/projects/L--test/xxxxxxxxx.json"` leading to the following error on Windows:
```
ccusage session --id=xxxxx
node:internal/modules/run_main:107
triggerUncaughtException(
^
[Error: ENOENT: no such file or directory, open 'L:\C:\Users\xxxxx\.claude\projects\L--test\xxxxx.jsonl'] {
errno: -4058,
code: 'ENOENT',
syscall: 'open',
path: 'L:\\C:\\Users\\xxxxx\\.claude\\projects\\L--test\\xxxxx.jsonl'
}
Node.js v24.14.0
NativeCommandExitException: Program "node.exe" ended with non-zero exit code: 1 (0x00000001).
```
|
I tried to put some regression test together, but I'm not sure if it's feasible without some hacky way - since we would need to refactor Btw, just noticed existing |
1 participant
Otherwise
globresulted in odd relative paths like"../../../../../../C:/Users/xxxx/.claude/projects/L--test/xxxxxxxxx.json"leading to the following error on Windows:Summary by CodeRabbit