-
-
Notifications
You must be signed in to change notification settings - Fork 320
chore: update gunshi and byethrow dependencies #791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- gunshi: ^0.26.3 → ^0.27.5 - @praha/byethrow: ^0.6.3 → ^0.9.0 These updates bring latest features and bug fixes from both libraries.
Update all Result.try usages to match the new API signature in byethrow 0.9.0. The `try` field now requires a function instead of a direct Promise, and `immediate: true` must be set for immediate execution. Affected files: - packages/internal/src/pricing.ts - apps/ccusage/src/_utils.ts - apps/ccusage/scripts/generate-json-schema.ts - apps/codex/src/data-loader.ts - apps/amp/src/data-loader.ts
📝 WalkthroughWalkthroughThis pull request updates async error handling patterns across multiple TypeScript modules and bumps dependency versions. Changes include wrapping async operations (readFile, stat, fetch) in async lambdas within Result.try blocks with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
ccusage-guide | 645e7c6 | Jan 09 2026, 11:39 PM |
@ccusage/amp
ccusage
@ccusage/codex
@ccusage/mcp
@ccusage/opencode
@ccusage/pi
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/codex/src/data-loader.ts (1)
202-218: Remove invalidimmediate: trueparameter from Result.try callsThe
immediateparameter does not exist in@praha/byethrow0.9.0. The documented options aretry,catch, andsafe.Lines 204-208 and 229-233 use
immediate: truewhich is invalid and will cause runtime errors. The correct pattern is shown in line 251-255, whereResult.try({...})returns a thunk that must be invoked separately viaparseLine(). Either apply the correct thunk pattern or use thesafeboolean parameter if immediate invocation is intended.Code example (lines 204-208)
const statResult = await Result.try({ immediate: true, // ❌ Invalid parameter try: async () => stat(directoryPath), catch: (error: unknown) => error, });Should be one of:
// Option 1: Thunk pattern (invoke the result) const statResult = Result.try({ try: async () => stat(directoryPath), catch: (error: unknown) => error, })(); // Option 2: Use safe parameter if immediate execution is needed const statResult = await Result.try({ safe: true, try: async () => stat(directoryPath), catch: (error: unknown) => error, });Also applies to: 229-233
apps/ccusage/src/_utils.ts (1)
14-23: Removeasynckeyword from function signature.The function returns a
Promise<number>fromResult.unwrap(0). Declaring the function asasynccreates an unintended double-wrappedPromise<Promise<number>>sinceasyncfunctions automatically wrap their return values in a Promise. Change to:export function getFileModifiedTime(filePath: string): Promise<number>. TheResult.try({ immediate: true })pattern itself is correct and matches@praha/byethrowsemantics.pnpm-workspace.yaml (1)
39-54: @praha/byethrow v0.9.0 does not exist on npm (latest: v0.6.3). Correct the version or clarify if it should be sourced from JSR. For gunshi v0.27.5, verify peer/engine requirements and breaking changes against the target Node.js version in this repo.
🧹 Nitpick comments (2)
packages/internal/src/pricing.ts (1)
145-199: Fetch/JSON parsing migration is correct; consider adding a fetch timeout to avoid hangs.The
Error(..., { cause })wrapping improves error diagnostics. The implementation correctly follows the Result.try() pattern withimmediate: trueand proper async error handling.Node.js 18+ provides
fetchas a global API, confirming compatibility. However, there's no timeout mechanism on the fetch call, which could cause indefinite hangs if LiteLLM is unresponsive. Adding an AbortController with a reasonable timeout (e.g., 10s) is recommended for robustness.Proposed hardening: AbortController timeout around fetch
- try: async () => fetch(this.url), + try: async () => { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 10_000); + try { + return await fetch(this.url, { signal: controller.signal }); + } finally { + clearTimeout(timeout); + } + },apps/ccusage/scripts/generate-json-schema.ts (1)
199-215: Consider aligning Result.try patterns for consistency.The
writeFileandreadFilefunctions use the lazy evaluation pattern (withoutimmediate: true) whilerunFormatuses immediate execution. Both approaches work, but consistency would improve maintainability.Additionally, consider explicitly typing the error parameter as
unknownto matchrunFormat's pattern and improve type safety.♻️ Suggested refactor for consistency
async function writeFile(path: string, content: string) { - const attempt = Result.try({ + return Result.try({ + immediate: true, try: async () => Bun.write(path, content), - catch: (error) => error, + catch: (error: unknown) => error, }); - return attempt(); } async function readFile(path: string): Promise<Result.Result<string, any>> { return Result.try({ + immediate: true, try: async () => { const file = Bun.file(path); return file.text(); }, - catch: (error) => error, - })(); + catch: (error: unknown) => error, + }); }
📜 Review details
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 (6)
apps/amp/src/data-loader.tsapps/ccusage/scripts/generate-json-schema.tsapps/ccusage/src/_utils.tsapps/codex/src/data-loader.tspackages/internal/src/pricing.tspnpm-workspace.yaml
🧰 Additional context used
📓 Path-based instructions (5)
apps/ccusage/src/**/*.ts
📄 CodeRabbit inference engine (apps/ccusage/CLAUDE.md)
apps/ccusage/src/**/*.ts: Write tests in-source usingif (import.meta.vitest != null)blocks instead of separate test files
Use Vitest globals (describe,it,expect) without imports in test blocks
In tests, use current Claude 4 models (sonnet-4, opus-4)
Usefs-fixturewithcreateFixture()to simulate Claude data in tests
Only export symbols that are actually used by other modules
Do not use console.log; use the logger utilities fromsrc/logger.tsinstead
Files:
apps/ccusage/src/_utils.ts
apps/ccusage/**/*.ts
📄 CodeRabbit inference engine (apps/ccusage/CLAUDE.md)
apps/ccusage/**/*.ts: NEVER useawait import()dynamic imports anywhere (especially in tests)
Prefer@praha/byethrowResult type for error handling instead of try-catch
Use.tsextensions for local imports (e.g.,import { foo } from './utils.ts')
Files:
apps/ccusage/src/_utils.tsapps/ccusage/scripts/generate-json-schema.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Import conventions: Use.tsextensions for local file imports (e.g.,import { foo } from './utils.ts')
Use ESLint for linting and formatting with tab indentation and double quotes
No console.log allowed except where explicitly disabled with eslint-disable
Use camelCase for variable names (e.g.,usageDataSchema,modelBreakdownSchema)
Use PascalCase for type names (e.g.,UsageData,ModelBreakdown)
Use UPPER_SNAKE_CASE for constants (e.g.,DEFAULT_CLAUDE_CODE_PATH)
Only export constants, functions, and types that are actually used by other modules - do not export internal/private constants used only within the same file
Prefer @praha/byethrow Result type over traditional try-catch for functional error handling
UseResult.try()for wrapping operations that may throw (JSON parsing, etc.)
UseResult.isFailure()for checking errors (more readable than!Result.isSuccess())
Use early return pattern (if (Result.isFailure(result)) continue;) instead of ternary operators with Result types
NEVER useawait import()dynamic imports anywhere in the codebase - this causes tree-shaking issues
Do not use console.log - use logger.ts instead
Vitest globals (describe,it,expect) are available automatically without imports in test blocks
NEVER useawait import()dynamic imports in vitest test blocks - this is particularly problematic for test execution
All test files must use current Claude 4 models (claude-sonnet-4-20250514, claude-opus-4-20250514), not outdated Claude 3 models
Model names in tests must exactly match LiteLLM's pricing database entries
Files:
apps/ccusage/src/_utils.tsapps/codex/src/data-loader.tsapps/amp/src/data-loader.tspackages/internal/src/pricing.tsapps/ccusage/scripts/generate-json-schema.ts
**/_*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use underscore prefix for internal files (e.g.,
_types.ts,_utils.ts,_consts.ts)
Files:
apps/ccusage/src/_utils.ts
apps/amp/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (apps/amp/CLAUDE.md)
NEVER use await import() dynamic imports anywhere, especially in test blocks in Amp CLI
Files:
apps/amp/src/data-loader.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: apps/ccusage/CLAUDE.md:0-0
Timestamp: 2025-09-18T16:06:37.474Z
Learning: Applies to apps/ccusage/**/*.ts : Prefer `praha/byethrow` Result type for error handling instead of try-catch
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: apps/pi/CLAUDE.md:0-0
Timestamp: 2026-01-09T12:07:00.515Z
Learning: Applies to apps/pi/src/**/*.{ts,tsx} : Prefer `praha/byethrow Result` type over try-catch for error handling
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T12:22:39.520Z
Learning: Applies to **/*.ts : Prefer praha/byethrow Result type over traditional try-catch for functional error handling
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: apps/mcp/CLAUDE.md:0-0
Timestamp: 2025-09-17T18:29:15.764Z
Learning: Applies to apps/mcp/**/*.ts : Prefer `praha/byethrow` Result type over try-catch for error handling
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T12:22:39.520Z
Learning: Applies to **/*.ts : Use `Result.try()` for wrapping operations that may throw (JSON parsing, etc.)
📚 Learning: 2025-09-18T16:06:37.474Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: apps/ccusage/CLAUDE.md:0-0
Timestamp: 2025-09-18T16:06:37.474Z
Learning: Applies to apps/ccusage/**/*.ts : Prefer `praha/byethrow` Result type for error handling instead of try-catch
Applied to files:
apps/ccusage/src/_utils.tsapps/codex/src/data-loader.tspackages/internal/src/pricing.tsapps/ccusage/scripts/generate-json-schema.tspnpm-workspace.yaml
📚 Learning: 2026-01-09T12:22:39.520Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T12:22:39.520Z
Learning: Applies to **/*.ts : Use `Result.try()` for wrapping operations that may throw (JSON parsing, etc.)
Applied to files:
apps/ccusage/src/_utils.tsapps/codex/src/data-loader.tspackages/internal/src/pricing.tsapps/ccusage/scripts/generate-json-schema.ts
📚 Learning: 2026-01-09T12:07:00.515Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: apps/pi/CLAUDE.md:0-0
Timestamp: 2026-01-09T12:07:00.515Z
Learning: Applies to apps/pi/src/**/*.{ts,tsx} : Prefer `praha/byethrow Result` type over try-catch for error handling
Applied to files:
apps/ccusage/src/_utils.tsapps/codex/src/data-loader.tspackages/internal/src/pricing.tsapps/ccusage/scripts/generate-json-schema.ts
📚 Learning: 2026-01-09T12:22:39.520Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T12:22:39.520Z
Learning: The data loading architecture follows a clear separation of concerns: Data Loading (data-loader.ts), Token Aggregation (calculate-cost.ts), Command Execution (commands/), and CLI Entry (index.ts)
Applied to files:
apps/codex/src/data-loader.ts
📚 Learning: 2026-01-09T12:22:39.520Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T12:22:39.520Z
Learning: Applies to **/*.ts : Prefer praha/byethrow Result type over traditional try-catch for functional error handling
Applied to files:
packages/internal/src/pricing.tsapps/ccusage/scripts/generate-json-schema.ts
📚 Learning: 2025-09-18T16:07:16.293Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: apps/codex/CLAUDE.md:0-0
Timestamp: 2025-09-18T16:07:16.293Z
Learning: Fetch per-model pricing from LiteLLM model_prices_and_context_window.json via LiteLLMPricingFetcher using an offline cache scoped to Codex-prefixed models; handle aliases (e.g., gpt-5-codex → gpt-5) in CodexPricingSource
Applied to files:
packages/internal/src/pricing.ts
📚 Learning: 2026-01-09T12:22:39.520Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T12:22:39.520Z
Learning: Applies to **/*.ts : Use camelCase for variable names (e.g., `usageDataSchema`, `modelBreakdownSchema`)
Applied to files:
apps/ccusage/scripts/generate-json-schema.ts
📚 Learning: 2025-09-17T18:29:15.764Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: apps/mcp/CLAUDE.md:0-0
Timestamp: 2025-09-17T18:29:15.764Z
Learning: Applies to apps/mcp/**/*.ts : Prefer `praha/byethrow` Result type over try-catch for error handling
Applied to files:
apps/ccusage/scripts/generate-json-schema.tspnpm-workspace.yaml
📚 Learning: 2026-01-09T12:07:00.515Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: apps/pi/CLAUDE.md:0-0
Timestamp: 2026-01-09T12:07:00.515Z
Learning: Run `pnpm run format`, `pnpm typecheck`, and `pnpm run test` in parallel after code changes
Applied to files:
apps/ccusage/scripts/generate-json-schema.ts
📚 Learning: 2025-09-18T16:06:37.474Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: apps/ccusage/CLAUDE.md:0-0
Timestamp: 2025-09-18T16:06:37.474Z
Learning: After any code change, run format, typecheck, and tests in parallel (`pnpm run format`, `pnpm typecheck`, `pnpm run test`)
Applied to files:
apps/ccusage/scripts/generate-json-schema.ts
📚 Learning: 2025-09-17T18:29:15.764Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: apps/mcp/CLAUDE.md:0-0
Timestamp: 2025-09-17T18:29:15.764Z
Learning: After code changes, always run `pnpm run format`, `pnpm typecheck`, and `pnpm run test` in parallel
Applied to files:
apps/ccusage/scripts/generate-json-schema.ts
📚 Learning: 2026-01-09T12:22:39.520Z
Learnt from: CR
Repo: ryoppippi/ccusage PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T12:22:39.520Z
Learning: After making code changes, ALWAYS run `pnpm run format`, `pnpm typecheck`, and `pnpm run test` in parallel
Applied to files:
apps/ccusage/scripts/generate-json-schema.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: ccusage-guide
🔇 Additional comments (3)
apps/amp/src/data-loader.ts (1)
167-177:loadThreadFile: Result.try wrapper withimmediate: trueis correct and failure handling is properly preserved.The async file read operation correctly uses
await Result.try({ immediate: true, ... })to immediately return a Result, which is then properly checked withResult.isFailure()before accessing the value.apps/ccusage/scripts/generate-json-schema.ts (2)
191-197: Implementation correctly adapted to byethrow 0.9.0 API.The
runFormatfunction properly usesimmediate: trueand wraps the async operation in a function as required by the new API. The explicitunknownerror type annotation is also good practice.
260-260: LGTM!The direct calls to
writeFileandrunFormatare correct. Both functions returnPromise<Result>which integrate properly withResult.pipe.Also applies to: 273-273
Summary
gunshifrom ^0.26.3 to ^0.27.5@praha/byethrowfrom ^0.6.3 to ^0.9.0What Changed
The byethrow 0.9.0 release introduced breaking changes to the
Result.tryAPI:tryfield now requires a function instead of a direct Promiseimmediate: truemust be set for immediate executionUpdated all affected files to use the new API signature:
packages/internal/src/pricing.tsapps/ccusage/src/_utils.tsapps/ccusage/scripts/generate-json-schema.tsapps/codex/src/data-loader.tsapps/amp/src/data-loader.tsTesting
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.