feat: add codex and claude quota mac menubar#106
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a macOS menubar feature: new CLI subcommand and installer, menubar state/config and snapshot handling, Codex/Claude quota extractors, launchd wrapper/plist generation, SwiftUI menubar app sources, and Bun build/typecheck/package scripts. Also exported new provider extractor functions/types and small test adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (bun)
participant Installer as Menubar Installer
participant LaunchD as launchd
participant App as Menubar .app
participant FS as Snapshot FS
User->>CLI: `tokenleak menubar install`
CLI->>Installer: installMenubar(home, entry)
Installer->>Installer: build .app (swiftc) & write wrappers/plist
Installer->>LaunchD: bootstrap LaunchAgent (plist)
LaunchD->>App: start menubar app
App->>FS: write/read snapshots (claude/codex)
User->>CLI: `tokenleak menubar refresh`
CLI->>FS: trigger refreshMenubarSnapshot()
FS-->>App: updated snapshot written
App->>App: reload snapshot -> UI update
sequenceDiagram
participant Timer as App Timer
participant VM as UsageViewModel
participant CLIwrap as CLI wrapper
participant Codex as Codex extractor
participant Claude as Claude bridge file
participant UI as Popover UI
Timer->>VM: tick -> reloadSnapshot()
VM->>CLIwrap: spawn `menubar refresh` (detached)
CLIwrap->>Codex: extractCodexQuotaSnapshot()
CLIwrap->>Claude: read claude snapshot file
Codex-->>CLIwrap: codex quota snapshot
Claude-->>CLIwrap: claude quota snapshot
CLIwrap->>VM: write combined snapshot
VM->>UI: publish updated UsageCardState
UI-->>User: refreshed usage cards
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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📝 Generate docstrings
🧪 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
🧹 Nitpick comments (4)
packages/cli/src/menubar/command.ts (1)
118-120: Consider adding error handling for daemon tick failures.If
tick()throws during the interval, the error becomes an unhandled promise rejection. This could crash the daemon or leave it in an undefined state.♻️ Proposed fix to catch tick errors
const interval = setInterval(() => { - void tick(); + tick().catch((err) => { + process.stderr.write(`[menubar] tick error: ${err instanceof Error ? err.message : String(err)}\n`); + }); }, config.pollIntervalSeconds * 1000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/menubar/command.ts` around lines 118 - 120, The interval currently calls tick() without handling promise rejections, so failures become unhandled; update the setInterval callback that creates "interval" to invoke tick() with error handling (e.g., call void tick().catch(err => processLogger.error(...) or wrap in an async IIFE with try/catch) so any thrown/rejected errors are logged and do not trigger unhandled promise rejections; reference the existing interval, tick(), and config.pollIntervalSeconds when making the change.packages/cli/src/menubar/state.ts (1)
120-120: Verify: Both windows must be stale for "stale" state.The condition
fiveHour.isStale && sevenDay.isStalemeans the provider only shows "stale" when both windows are stale. This seems intentional (conservative approach), but if one window is fresh and the other stale, the UI might be misleading.Consider whether
fiveHour.isStale || sevenDay.isStalebetter represents partial staleness, or add distinct handling for mixed states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/menubar/state.ts` at line 120, The current assignment to state uses strict conjunction (const state = fiveHour.isStale && sevenDay.isStale ? 'stale' : 'ready'), which only marks the overall state as 'stale' when both fiveHour.isStale and sevenDay.isStale are true; update the logic in the state calculation to reflect partial staleness (e.g., use logical OR: fiveHour.isStale || sevenDay.isStale to mark 'stale' if either window is stale) or implement explicit mixed-state handling (add a third branch such as 'partial' when fiveHour.isStale !== sevenDay.isStale) so the UI accurately represents mixed freshness; change the expression that sets state (referencing state, fiveHour.isStale, sevenDay.isStale) accordingly.packages/cli/src/menubar/claude-statusline.ts (1)
48-54: Redundant conditional branches.Both the
ifandelsebranches perform identical operations (Buffer.from(chunk)), making the conditional unnecessary.♻️ Simplify by removing redundant conditional
for await (const chunk of process.stdin) { - if (typeof chunk === 'string') { - chunks.push(Buffer.from(chunk)); - } else { - chunks.push(Buffer.from(chunk)); - } + chunks.push(Buffer.from(chunk)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/menubar/claude-statusline.ts` around lines 48 - 54, The for-await loop in claude-statusline.ts contains a redundant if/else that both call Buffer.from(chunk); simplify by removing the conditional and always pushing Buffer.from(chunk) into chunks (i.e., replace the if/else in the for await (const chunk of process.stdin) loop with a single chunks.push(Buffer.from(chunk))). Keep the surrounding loop and variable names (process.stdin, chunk, chunks) intact.packages/menubar/App/TokenleakUsage.swift (1)
252-278: Consider potential race in daemon log file handling.The daemon process is started with stdout/stderr directed to a log file. If
createFilesucceeds butFileHandle(forWritingTo:)throws, the daemon runs but logs are lost. This is a minor reliability concern.♻️ Improved error handling for daemon logging
- FileManager.default.createFile(atPath: daemonLogPath, contents: nil) - if let handle = try? FileHandle(forWritingTo: URL(fileURLWithPath: daemonLogPath)) { + let logURL = URL(fileURLWithPath: daemonLogPath) + FileManager.default.createFile(atPath: daemonLogPath, contents: nil) + if let handle = try? FileHandle(forWritingTo: logURL) { _ = try? handle.seekToEnd() process.standardOutput = handle process.standardError = handle + } else { + // Fall back to /dev/null to avoid pipe buffer deadlock + process.standardOutput = FileHandle.nullDevice + process.standardError = FileHandle.nullDevice }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/menubar/App/TokenleakUsage.swift` around lines 252 - 278, In startDaemonIfNeeded the code creates daemonLogPath then attempts FileHandle(forWritingTo:) which can throw after file creation, letting the daemon run without any log redirection; fix by opening/creating the log file and obtaining a FileHandle before calling process.run(): attempt to open the file atomically (e.g., using FileManager.createFile or open with FileHandle(forUpdatingAtPath:)/FileHandle(forWritingTo:) inside a do/catch), if that fails fall back to a safe sink (e.g., /dev/null) and only assign process.standardOutput/standardError when you have a valid handle; ensure the handle is soughtToEnd() and retained/closed appropriately (references: startDaemonIfNeeded, daemonLogPath, daemonProcess, FileHandle(forWritingTo:)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/menubar/claude-statusline.ts`:
- Line 11: The code that initializes usedPercent from record currently checks
record['used_percentage'] and record['usedPercent'] but omits the canonical name
used_percent (as set in codex-rate-limits.ts); update the assignment for
usedPercent (the const usedPercent = ...) to also read record['used_percent']
(and keep the existing fallbacks) so it will correctly parse the canonical field
name from the record.
In `@packages/cli/src/menubar/format.ts`:
- Around line 22-29: The percent formatting functions currently treat
NaN/Infinity as valid numbers; update toRemainingPercent to only process finite
numbers (use Number.isFinite(value)) before calling clampPercent and returning
100 - value, and ensure formatPercentLeft continues to rely on
toRemainingPercent (or additionally check Number.isFinite on the result) so that
non-finite inputs yield null and the display falls back to "--"; reference the
functions toRemainingPercent and formatPercentLeft (and clampPercent) when
making the change.
In `@packages/cli/src/menubar/install.ts`:
- Line 4: Remove the unused mkdirSync import from the top-level imports in the
menubar install module; the file uses ensureMenubarDir from state.ts instead, so
delete mkdirSync from the import list (leave any other imports intact) to
eliminate the dead import.
In `@packages/registry/src/providers/codex-rate-limits.ts`:
- Around line 50-56: toResetAtIso can throw RangeError from toISOString and
parseQuotaWindow doesn't validate numeric inputs; update toResetAtIso to guard
against non-finite and out-of-range timestamps by wrapping new Date(value *
1000).toISOString() in a try/catch and return null on any error (so it never
throws), and update parseQuotaWindow to validate usedPercent and windowMinutes
using Number.isFinite (and typeof === 'number') before using them—if either is
invalid, treat the parsed result as null/invalid (consistent with existing null
returns) to avoid propagating NaN/Infinity or throwing later.
- Around line 31-45: collectJsonlFiles currently uses statSync which follows
symlinks and can cause infinite recursion if a symlinked directory cycles;
update the function to use lstatSync for each fullPath and skip entries where
lstatSync(fullPath).isSymbolicLink() is true (or otherwise avoid descending into
symlinked directories) before recursing, while still using readdirSync/join to
enumerate and checking isDirectory/isFile on the non-symlink lstat result so
.jsonl files are collected but directory symlinks are not followed.
---
Nitpick comments:
In `@packages/cli/src/menubar/claude-statusline.ts`:
- Around line 48-54: The for-await loop in claude-statusline.ts contains a
redundant if/else that both call Buffer.from(chunk); simplify by removing the
conditional and always pushing Buffer.from(chunk) into chunks (i.e., replace the
if/else in the for await (const chunk of process.stdin) loop with a single
chunks.push(Buffer.from(chunk))). Keep the surrounding loop and variable names
(process.stdin, chunk, chunks) intact.
In `@packages/cli/src/menubar/command.ts`:
- Around line 118-120: The interval currently calls tick() without handling
promise rejections, so failures become unhandled; update the setInterval
callback that creates "interval" to invoke tick() with error handling (e.g.,
call void tick().catch(err => processLogger.error(...) or wrap in an async IIFE
with try/catch) so any thrown/rejected errors are logged and do not trigger
unhandled promise rejections; reference the existing interval, tick(), and
config.pollIntervalSeconds when making the change.
In `@packages/cli/src/menubar/state.ts`:
- Line 120: The current assignment to state uses strict conjunction (const state
= fiveHour.isStale && sevenDay.isStale ? 'stale' : 'ready'), which only marks
the overall state as 'stale' when both fiveHour.isStale and sevenDay.isStale are
true; update the logic in the state calculation to reflect partial staleness
(e.g., use logical OR: fiveHour.isStale || sevenDay.isStale to mark 'stale' if
either window is stale) or implement explicit mixed-state handling (add a third
branch such as 'partial' when fiveHour.isStale !== sevenDay.isStale) so the UI
accurately represents mixed freshness; change the expression that sets state
(referencing state, fiveHour.isStale, sevenDay.isStale) accordingly.
In `@packages/menubar/App/TokenleakUsage.swift`:
- Around line 252-278: In startDaemonIfNeeded the code creates daemonLogPath
then attempts FileHandle(forWritingTo:) which can throw after file creation,
letting the daemon run without any log redirection; fix by opening/creating the
log file and obtaining a FileHandle before calling process.run(): attempt to
open the file atomically (e.g., using FileManager.createFile or open with
FileHandle(forUpdatingAtPath:)/FileHandle(forWritingTo:) inside a do/catch), if
that fails fall back to a safe sink (e.g., /dev/null) and only assign
process.standardOutput/standardError when you have a valid handle; ensure the
handle is soughtToEnd() and retained/closed appropriately (references:
startDaemonIfNeeded, daemonLogPath, daemonProcess, FileHandle(forWritingTo:)).
🪄 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: df3b5a84-3ac4-4c97-8ad2-041dc5d664fc
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (22)
package.jsonpackages/cli/src/cli.test.tspackages/cli/src/cli.tspackages/cli/src/menubar/claude-statusline.tspackages/cli/src/menubar/command.tspackages/cli/src/menubar/format.tspackages/cli/src/menubar/install.tspackages/cli/src/menubar/launchd.test.tspackages/cli/src/menubar/launchd.tspackages/cli/src/menubar/paths.tspackages/cli/src/menubar/state.test.tspackages/cli/src/menubar/state.tspackages/cli/src/menubar/types.tspackages/menubar/App/TokenleakUsage.swiftpackages/menubar/package.jsonpackages/registry/src/index.tspackages/registry/src/providers/codex-rate-limits.test.tspackages/registry/src/providers/codex-rate-limits.tspackages/registry/src/providers/index.tsscripts/build-menubar-app.tsscripts/check-menubar-app.tsscripts/package-menubar-app.ts
| export function toRemainingPercent(value: number | null): number | null { | ||
| return typeof value === 'number' ? clampPercent(100 - value) : null; | ||
| } | ||
|
|
||
| export function formatPercentLeft(value: number | null): string { | ||
| const remaining = toRemainingPercent(value); | ||
| return typeof remaining === 'number' ? `${Math.round(remaining)}%` : '--'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript/TypeScript, does typeof NaNreturn"number"and doesMath.round(NaN)produceNaN?
💡 Result:
Yes, typeof NaN returns "number" and Math.round(NaN) produces NaN in JavaScript/TypeScript. NaN is a special value of the Number type in JavaScript, as defined by the ECMAScript specification and confirmed by MDN documentation. The typeof operator returns "number" for all Number values, including NaN. Math functions like Math.round propagate NaN: if the input is NaN, the output is NaN. This follows IEEE 754 floating-point standards and standard JavaScript behavior for Math operations, where NaN is preserved through arithmetic and rounding operations (unlike some edge cases in other functions like Math.pow(NaN, 0)). MDN examples for similar functions (e.g., Math.floor) explicitly show Number.isNaN(value) checks returning NaN, and general documentation confirms NaN propagation in Math methods.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NaN
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/round
- 4: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Math
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/floor
- 6: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/pow
🏁 Script executed:
cd packages/cli/src/menubar && cat -n format.tsRepository: ya-nsh/tokenleak
Length of output: 1032
🏁 Script executed:
rg "clampPercent" packages/cli/src/Repository: ya-nsh/tokenleak
Length of output: 247
🏁 Script executed:
rg "formatPercentLeft|toRemainingPercent" packages/cli/src/ -A 2 -B 2Repository: ya-nsh/tokenleak
Length of output: 2463
🏁 Script executed:
fd "format.test" packages/cli/src/ --type fRepository: ya-nsh/tokenleak
Length of output: 42
🏁 Script executed:
rg "usedPercent" packages/cli/src/ -B 3 -A 1Repository: ya-nsh/tokenleak
Length of output: 4279
🏁 Script executed:
rg -t ts "interface.*Window|type.*Window" packages/cli/src/menubar/ -A 5Repository: ya-nsh/tokenleak
Length of output: 1679
Guard against non-finite numbers in percent formatting.
toRemainingPercent accepts NaN and Infinity (both satisfy typeof value === 'number'), which propagate through clampPercent (since Math.min/Math.max return NaN when given NaN) and render as "NaN%" or "Infinity%" in the output string. This could occur if an upstream API returns a malformed percentage value.
Add a finiteness check:
Proposed fix
export function toRemainingPercent(value: number | null): number | null {
- return typeof value === 'number' ? clampPercent(100 - value) : null;
+ return typeof value === 'number' && Number.isFinite(value) ? clampPercent(100 - value) : null;
}📝 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.
| export function toRemainingPercent(value: number | null): number | null { | |
| return typeof value === 'number' ? clampPercent(100 - value) : null; | |
| } | |
| export function formatPercentLeft(value: number | null): string { | |
| const remaining = toRemainingPercent(value); | |
| return typeof remaining === 'number' ? `${Math.round(remaining)}%` : '--'; | |
| } | |
| export function toRemainingPercent(value: number | null): number | null { | |
| return typeof value === 'number' && Number.isFinite(value) ? clampPercent(100 - value) : null; | |
| } | |
| export function formatPercentLeft(value: number | null): string { | |
| const remaining = toRemainingPercent(value); | |
| return typeof remaining === 'number' ? `${Math.round(remaining)}%` : '--'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/menubar/format.ts` around lines 22 - 29, The percent
formatting functions currently treat NaN/Infinity as valid numbers; update
toRemainingPercent to only process finite numbers (use Number.isFinite(value))
before calling clampPercent and returning 100 - value, and ensure
formatPercentLeft continues to rely on toRemainingPercent (or additionally check
Number.isFinite on the result) so that non-finite inputs yield null and the
display falls back to "--"; reference the functions toRemainingPercent and
formatPercentLeft (and clampPercent) when making the change.
| import { | ||
| cpSync, | ||
| existsSync, | ||
| mkdirSync, |
There was a problem hiding this comment.
Remove unused import.
mkdirSync is imported but never used. The code uses ensureMenubarDir from state.ts instead.
🧹 Proposed fix
import {
cpSync,
existsSync,
- mkdirSync,
readFileSync,
rmSync,
unlinkSync,
writeFileSync,
} from 'node:fs';📝 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.
| mkdirSync, | |
| import { | |
| cpSync, | |
| existsSync, | |
| readFileSync, | |
| rmSync, | |
| unlinkSync, | |
| writeFileSync, | |
| } from 'node:fs'; |
🧰 Tools
🪛 ESLint
[error] 4-4: 'mkdirSync' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/menubar/install.ts` at line 4, Remove the unused mkdirSync
import from the top-level imports in the menubar install module; the file uses
ensureMenubarDir from state.ts instead, so delete mkdirSync from the import list
(leave any other imports intact) to eliminate the dead import.
| function collectJsonlFiles(dir: string): string[] { | ||
| if (!existsSync(dir)) { | ||
| return []; | ||
| } | ||
|
|
||
| const files: string[] = []; | ||
| for (const entry of readdirSync(dir)) { | ||
| const fullPath = join(dir, entry); | ||
| const stats = statSync(fullPath); | ||
| if (stats.isDirectory()) { | ||
| files.push(...collectJsonlFiles(fullPath)); | ||
| } else if (entry.endsWith('.jsonl')) { | ||
| files.push(fullPath); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, does fs.statSync follow symlinks, and what are recommended patterns to avoid symlink recursion in directory traversal?
💡 Result:
Yes, fs.statSync follows symbolic links (symlinks), returning stats for the target file or directory rather than the link itself. To detect symlinks or avoid following them, use fs.lstatSync instead. Recommended patterns to avoid symlink recursion in directory traversal: - Use fs.lstatSync (or readdir with {withFileTypes: true}) to check if entries are symlinks via stats.isSymbolicLink or dirent.isSymbolicLink, and skip symlink directories. - Track visited canonical paths (via fs.realpathSync) in a Set to detect cycles. - Limit recursion depth. - For built-in recursive ops like fs.readdirSync({recursive: true}), note it follows symlinks (per issues); prefer custom loops with checks or libraries like walk-sync that handle cycles. - In path traversal security, resolve with fs.realpathSync and validate against root. Example safe traversal: const fs = require('node:fs'); const path = require('node:path'); function safeWalk(dir, visited = new Set) { const realDir = fs.realpathSync(dir); if (visited.has(realDir)) return; visited.add(realDir); const entries = fs.readdirSync(dir, { withFileTypes: true }); for (const entry of entries) { const full = path.join(dir, entry.name); if (entry.isSymbolicLink) continue; // Skip symlinks if (entry.isDirectory) { safeWalk(full, visited); } else { console.log(full); } } }
Citations:
- 1: https://nodejs.org/api/fs.html
- 2: statSync fails in detecting link nodejs/node#47633
- 3: https://docs.deno.com/api/node/fs/~/stat
- 4: https://docs.deno.com/api/node/fs/~/lstat
- 5: https://github.com/joliss/node-walk-sync
🏁 Script executed:
fd -t f "codex-rate-limits.ts" --path "packages/registry"Repository: ya-nsh/tokenleak
Length of output: 293
🏁 Script executed:
fd "codex-rate-limits.ts" packages/registryRepository: ya-nsh/tokenleak
Length of output: 112
🏁 Script executed:
cat -n packages/registry/src/providers/codex-rate-limits.ts | head -60Repository: ya-nsh/tokenleak
Length of output: 2042
🏁 Script executed:
rg "collectJsonlFiles" packages/registry/src/providers/codex-rate-limits.tsRepository: ya-nsh/tokenleak
Length of output: 205
🏁 Script executed:
cat -n packages/registry/src/providers/codex-rate-limits.ts | sed -n '45,100p'Repository: ya-nsh/tokenleak
Length of output: 1860
🏁 Script executed:
rg "collectJsonlFiles\(" packages/registry/src/providers/codex-rate-limits.ts -B 5 -A 5Repository: ya-nsh/tokenleak
Length of output: 922
🏁 Script executed:
cat -n packages/registry/src/providers/codex-rate-limits.ts | sed -n '110,160p'Repository: ya-nsh/tokenleak
Length of output: 1337
🏁 Script executed:
cat -n packages/registry/src/providers/codex-rate-limits.ts | sed -n '1,10p'Repository: ya-nsh/tokenleak
Length of output: 495
🏁 Script executed:
rg "statSync" packages/registry/src/providers/codex-rate-limits.tsRepository: ya-nsh/tokenleak
Length of output: 158
Avoid following directory symlinks during recursive scan.
Using statSync follows symlinks; a symlink cycle under CODEX_HOME/sessions can cause infinite recursion and stack overflow.
Proposed fix
function collectJsonlFiles(dir: string): string[] {
if (!existsSync(dir)) {
return [];
}
const files: string[] = [];
- for (const entry of readdirSync(dir)) {
- const fullPath = join(dir, entry);
- const stats = statSync(fullPath);
- if (stats.isDirectory()) {
- files.push(...collectJsonlFiles(fullPath));
- } else if (entry.endsWith('.jsonl')) {
- files.push(fullPath);
- }
+ for (const entry of readdirSync(dir, { withFileTypes: true })) {
+ const fullPath = join(dir, entry.name);
+ if (entry.isSymbolicLink()) {
+ continue;
+ }
+ if (entry.isDirectory()) {
+ files.push(...collectJsonlFiles(fullPath));
+ } else if (entry.isFile() && entry.name.endsWith('.jsonl')) {
+ files.push(fullPath);
+ }
}
return files;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/registry/src/providers/codex-rate-limits.ts` around lines 31 - 45,
collectJsonlFiles currently uses statSync which follows symlinks and can cause
infinite recursion if a symlinked directory cycles; update the function to use
lstatSync for each fullPath and skip entries where
lstatSync(fullPath).isSymbolicLink() is true (or otherwise avoid descending into
symlinked directories) before recursing, while still using readdirSync/join to
enumerate and checking isDirectory/isFile on the non-symlink lstat result so
.jsonl files are collected but directory symlinks are not followed.
| function toResetAtIso(value: unknown): string | null { | ||
| if (typeof value !== 'number' || !Number.isFinite(value)) { | ||
| return null; | ||
| } | ||
|
|
||
| return new Date(value * 1000).toISOString(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Per the JavaScript spec/MDN, can Date.prototype.toISOString throw RangeError for invalid or out-of-range dates?
💡 Result:
Yes, per both the ECMAScript specification (as documented in MDN) and MDN documentation, Date.prototype.toISOString throws a RangeError for invalid dates (those with NaN time values, e.g., from invalid parsing like new Date("invalid")) and for dates corresponding to years that cannot be represented in the ISO date string format (extended years beyond the standard 4-digit representation, though ECMAScript supports up to ~±275,000 years via 6-digit signed years).
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toisostring
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Invalid_date
- 3: https://bugzilla.mozilla.org/show_bug.cgi?id=649575
🏁 Script executed:
find . -name "codex-rate-limits.ts" -type fRepository: ya-nsh/tokenleak
Length of output: 114
🏁 Script executed:
cat -n packages/registry/src/providers/codex-rate-limits.ts | sed -n '45,160p'Repository: ya-nsh/tokenleak
Length of output: 3443
Make reset timestamp parsing non-throwing.
toISOString() on line 55 can throw RangeError for out-of-range dates. When it does, the exception is caught at lines 138-140, skipping all remaining records in the current file and potentially losing newer valid records.
Additionally, parseQuotaWindow() (lines 67-75) does not validate usedPercent and windowMinutes with Number.isFinite(), allowing potentially problematic numeric values to propagate.
Proposed fix
function toResetAtIso(value: unknown): string | null {
if (typeof value !== 'number' || !Number.isFinite(value)) {
return null;
}
- return new Date(value * 1000).toISOString();
+ const millis = value * 1000;
+ if (!Number.isFinite(millis)) {
+ return null;
+ }
+ const date = new Date(millis);
+ if (Number.isNaN(date.getTime())) {
+ return null;
+ }
+ return date.toISOString();
}
function parseQuotaWindow(value: unknown): QuotaWindowSnapshot | null {
@@
- if (typeof usedPercent !== 'number' || typeof windowMinutes !== 'number') {
+ if (
+ typeof usedPercent !== 'number' ||
+ !Number.isFinite(usedPercent) ||
+ typeof windowMinutes !== 'number' ||
+ !Number.isFinite(windowMinutes)
+ ) {
return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/registry/src/providers/codex-rate-limits.ts` around lines 50 - 56,
toResetAtIso can throw RangeError from toISOString and parseQuotaWindow doesn't
validate numeric inputs; update toResetAtIso to guard against non-finite and
out-of-range timestamps by wrapping new Date(value * 1000).toISOString() in a
try/catch and return null on any error (so it never throws), and update
parseQuotaWindow to validate usedPercent and windowMinutes using Number.isFinite
(and typeof === 'number') before using them—if either is invalid, treat the
parsed result as null/invalid (consistent with existing null returns) to avoid
propagating NaN/Infinity or throwing later.
…olors - Replace arc progress rings with native-style progress bars (saves space, no text clipping) - Switch to vertical card layout with per-window progress rows - Use correct brand colors: Claude orange (#D97757), Codex green (#10A37F) - Remove hover effects, glass gradients, and heavy shadows for flat macOS aesthetic - Fix settings window title bar visibility (remove fullSizeContentView) - Remove refresh button rotation animation - Replace custom menu bar glyph with SF Symbol gauge icon - Fix popover corner gap (remove manual clipShape/stroke) - Add text truncation to settings path rows
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/menubar/App/Utilities/TimerManager.swift (1)
3-27: Consider marking the class@MainActorto enforce thread safety.
Timer.invalidate()must be called from the thread where the timer was installed. Currently, this class implicitly assumes main-thread usage (caller is@MainActor AppDelegate), but there's no compile-time enforcement. Annotating the class makes this requirement explicit and enables Swift concurrency checks.♻️ Proposed change
import Foundation +@MainActor final class TimerManager { private var timer: Timer? func start(every interval: TimeInterval, fireImmediately: Bool = false, action: `@escaping` () -> Void) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/menubar/App/Utilities/TimerManager.swift` around lines 3 - 27, Mark the TimerManager class as `@MainActor` to ensure all its methods and the Timer property run on the main thread; update the declaration for final class TimerManager to be `@MainActor` final class TimerManager so calls to start(every:fireImmediately:action:), stop(), deinit and the Timer.invalidate() call are enforced to execute on the main actor by the compiler.packages/menubar/App/Utilities/KeychainHelper.swift (1)
47-54: Surface keychain delete failures to callers.At Line 53,
SecItemDeletestatus is ignored. Failed deletes (other than not-found) become silent, making credential cleanup state ambiguous.Proposed refactor
-static func delete(service: String, account: String) { +static func delete(service: String, account: String) throws { let query: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: service, kSecAttrAccount as String: account, ] - SecItemDelete(query as CFDictionary) + let status = SecItemDelete(query as CFDictionary) + guard status == errSecSuccess || status == errSecItemNotFound else { + throw NSError(domain: NSOSStatusErrorDomain, code: Int(status)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/menubar/App/Utilities/KeychainHelper.swift` around lines 47 - 54, The delete(service:account:) helper currently ignores the SecItemDelete result; change it to surface failures by returning a Bool or throwing an error: call SecItemDelete(query as CFDictionary), capture the OSStatus, and treat errSecSuccess and errSecItemNotFound as non-fatal success; for any other status return false or throw an NSError/OSStatus-based error. Update the signature of delete(service:account:) accordingly and adjust callers to handle the returned failure/throw so credential-cleanup failures are not silently ignored.packages/menubar/App/Views/PopoverContentView.swift (1)
64-73: Consider adding loading feedback for the refresh button.The refresh button calls
viewModel.refresh()but provides no visual feedback during the refresh operation. Users won't know if the action is in progress.💡 Optional: Add rotation animation during refresh
+ `@State` private var isRefreshing = false + ... Button { - viewModel.refresh() + isRefreshing = true + viewModel.refresh() + // Reset after a short delay or tie to viewModel's loading state + DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { + isRefreshing = false + } } label: { Image(systemName: "arrow.clockwise") .font(.system(size: 12, weight: .medium)) .foregroundStyle(AppTheme.textTertiary) .frame(width: 26, height: 26) .contentShape(Circle()) + .rotationEffect(.degrees(isRefreshing ? 360 : 0)) + .animation(isRefreshing ? .linear(duration: 0.5).repeatForever(autoreverses: false) : .default, value: isRefreshing) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/menubar/App/Views/PopoverContentView.swift` around lines 64 - 73, The refresh Button calls viewModel.refresh() but provides no visual feedback; add a boolean state on the view model (e.g., `@Published` var isRefreshing) that is set true at the start of viewModel.refresh() and false when it completes, then update the Button/Image UI in PopoverContentView to reflect that state: disable the Button while isRefreshing, apply a rotation animation or SwiftUI .rotationEffect with an implicit animation to the Image(systemName: "arrow.clockwise") when viewModel.isRefreshing is true, and ensure viewModel.refresh() sets isRefreshing even on errors (or uses async/await with defer to clear it). Include accessibilityLabel and isEnabled bindings so users and assistive tech see the loading state.packages/menubar/App/AppDelegate.swift (2)
150-155: FileHandle is never closed after daemon process setup.The
FileHandleopened for writing to the daemon log is assigned toprocess.standardOutputandprocess.standardErrorbut is never explicitly closed. When the daemon process terminates andstartDaemonIfNeeded()is called again, a new handle is opened without closing the previous one. Over time, this can leak file descriptors.Consider storing the handle and closing it when the process terminates, or relying on the process termination to close it automatically by not holding a separate reference.
♻️ Suggested approach
private var daemonProcess: Process? + private var daemonLogHandle: FileHandle?Then in
startDaemonIfNeeded():+ daemonLogHandle?.closeFile() + daemonLogHandle = nil + FileManager.default.createFile(atPath: daemonLogPath, contents: nil) if let handle = try? FileHandle(forWritingTo: URL(fileURLWithPath: daemonLogPath)) { _ = try? handle.seekToEnd() process.standardOutput = handle process.standardError = handle + daemonLogHandle = handle }And in
applicationWillTerminate:func applicationWillTerminate(_ notification: Notification) { refreshTimer.stop() daemonProcess?.terminate() + daemonLogHandle?.closeFile() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/menubar/App/AppDelegate.swift` around lines 150 - 155, The FileHandle opened for daemonLogPath is never closed; modify AppDelegate to store the FileHandle (e.g., a property named daemonLogHandle) when you create it for writing and assign it to process.standardOutput/standardError, then ensure you close and nil out daemonLogHandle when the daemon process exits or when applicationWillTerminate() runs (also close before reopening in startDaemonIfNeeded() to avoid leaking descriptors); reference the FileHandle creation block, daemonLogPath, process.standardOutput/process.standardError, startDaemonIfNeeded(), and applicationWillTerminate() when implementing the lifecycle cleanup.
157-163: Consider logging daemon startup failures for debuggability.When
process.run()fails, the app only beeps without any indication of what went wrong. For a menubar app, this may be intentional, but logging the error to the daemon log file oros_logwould help diagnose startup issues.♻️ Optional: Log startup errors
do { try process.run() daemonProcess = process } catch { NSSound.beep() + NSLog("Failed to start menubar daemon: \(error.localizedDescription)") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/menubar/App/AppDelegate.swift` around lines 157 - 163, When process.run() throws in the AppDelegate start-up path, replace the silent NSSound.beep() error handling with a logged failure that includes the thrown error and context; capture the caught error in the catch block and send a descriptive message with the error details to a logging facility (e.g., os_log or your daemon log) while optionally preserving the NSSound.beep() call, updating the catch that follows try process.run() and references daemonProcess/NSSound.beep() to emit the log entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/menubar/App/Utilities/KeychainHelper.swift`:
- Around line 5-24: The save(value:service:account:) currently deletes the
existing item then adds a new one which can permanently lose credentials if
SecItemAdd fails; change it to first call SecItemUpdate using the query with
kSecClassGenericPassword, kSecAttrService and kSecAttrAccount and an attributes
dictionary containing kSecValueData set to value, then if SecItemUpdate returns
errSecItemNotFound call SecItemAdd with the full attributes to create the item,
and for any other non-success status throw an NSError with the returned status;
remove the SecItemDelete step and ensure you check for errSecSuccess on both
SecItemUpdate and SecItemAdd and treat other codes as errors.
In `@packages/menubar/App/ViewModels/UsageViewModel.swift`:
- Around line 37-38: The initializer is ignoring the injected services and
creates new ClaudeUsageService and CodexUsageService instances; update the
initialization of self.claudeUsage and self.codexUsage to call
resolveUsage(from: nil) on the injected services parameter (use the injected
services' claude/codex usage service instances) instead of instantiating new
ClaudeUsageService() and CodexUsageService(), referencing the UsageViewModel
initializer's services parameter and the resolveUsage(from:) method to locate
the correct objects.
- Around line 102-118: The refresh spawns two independent tasks causing a race
between Process execution and reloadSnapshot(); instead, coordinate by
performing the reload and setting isRefreshing = false only after the Process
completes: move the reloadSnapshot() and isRefreshing update into the same Task
that runs the Process (the Task that uses cliWrapperPath, Process(),
process.run() and process.waitUntilExit()), and ensure those UI updates
(reloadSnapshot() and isRefreshing) are dispatched on the main actor (e.g., via
`@MainActor` context or DispatchQueue.main) so they happen after the CLI finishes
and on the correct thread.
In `@packages/menubar/App/Views/ArcProgressRing.swift`:
- Around line 42-49: The glow animation is only started in onAppear so when
`critical` flips true later the pulsing won't begin; update the
`ArcProgressRing` to also observe changes to `critical` (e.g., use onChange(of:
critical) or a published binding observer) and run the same
withAnimation(.easeInOut(duration: 1.4).repeatForever(autoreverses: true)) {
glow.toggle() } block when `critical` becomes true, and when `critical` becomes
false stop/reset the effect by setting `glow = false`; reference the `critical`
property, the `glow` state, and the existing withAnimation block to ensure the
same behavior is invoked on state change as on appear.
---
Nitpick comments:
In `@packages/menubar/App/AppDelegate.swift`:
- Around line 150-155: The FileHandle opened for daemonLogPath is never closed;
modify AppDelegate to store the FileHandle (e.g., a property named
daemonLogHandle) when you create it for writing and assign it to
process.standardOutput/standardError, then ensure you close and nil out
daemonLogHandle when the daemon process exits or when applicationWillTerminate()
runs (also close before reopening in startDaemonIfNeeded() to avoid leaking
descriptors); reference the FileHandle creation block, daemonLogPath,
process.standardOutput/process.standardError, startDaemonIfNeeded(), and
applicationWillTerminate() when implementing the lifecycle cleanup.
- Around line 157-163: When process.run() throws in the AppDelegate start-up
path, replace the silent NSSound.beep() error handling with a logged failure
that includes the thrown error and context; capture the caught error in the
catch block and send a descriptive message with the error details to a logging
facility (e.g., os_log or your daemon log) while optionally preserving the
NSSound.beep() call, updating the catch that follows try process.run() and
references daemonProcess/NSSound.beep() to emit the log entry.
In `@packages/menubar/App/Utilities/KeychainHelper.swift`:
- Around line 47-54: The delete(service:account:) helper currently ignores the
SecItemDelete result; change it to surface failures by returning a Bool or
throwing an error: call SecItemDelete(query as CFDictionary), capture the
OSStatus, and treat errSecSuccess and errSecItemNotFound as non-fatal success;
for any other status return false or throw an NSError/OSStatus-based error.
Update the signature of delete(service:account:) accordingly and adjust callers
to handle the returned failure/throw so credential-cleanup failures are not
silently ignored.
In `@packages/menubar/App/Utilities/TimerManager.swift`:
- Around line 3-27: Mark the TimerManager class as `@MainActor` to ensure all its
methods and the Timer property run on the main thread; update the declaration
for final class TimerManager to be `@MainActor` final class TimerManager so calls
to start(every:fireImmediately:action:), stop(), deinit and the
Timer.invalidate() call are enforced to execute on the main actor by the
compiler.
In `@packages/menubar/App/Views/PopoverContentView.swift`:
- Around line 64-73: The refresh Button calls viewModel.refresh() but provides
no visual feedback; add a boolean state on the view model (e.g., `@Published` var
isRefreshing) that is set true at the start of viewModel.refresh() and false
when it completes, then update the Button/Image UI in PopoverContentView to
reflect that state: disable the Button while isRefreshing, apply a rotation
animation or SwiftUI .rotationEffect with an implicit animation to the
Image(systemName: "arrow.clockwise") when viewModel.isRefreshing is true, and
ensure viewModel.refresh() sets isRefreshing even on errors (or uses async/await
with defer to clear it). Include accessibilityLabel and isEnabled bindings so
users and assistive tech see the loading state.
🪄 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: e709ab3f-6663-43f6-9825-548ea5b5ef29
📒 Files selected for processing (15)
packages/menubar/App/AppDelegate.swiftpackages/menubar/App/LLMUsageApp.swiftpackages/menubar/App/Services/ClaudeUsageService.swiftpackages/menubar/App/Services/CodexUsageService.swiftpackages/menubar/App/Services/UsageService.swiftpackages/menubar/App/Settings/SettingsView.swiftpackages/menubar/App/Utilities/KeychainHelper.swiftpackages/menubar/App/Utilities/Theme.swiftpackages/menubar/App/Utilities/TimerManager.swiftpackages/menubar/App/ViewModels/UsageViewModel.swiftpackages/menubar/App/Views/ArcProgressRing.swiftpackages/menubar/App/Views/PopoverContentView.swiftpackages/menubar/App/Views/UsageCard.swiftscripts/build-menubar-app.tsscripts/check-menubar-app.ts
✅ Files skipped from review due to trivial changes (1)
- scripts/build-menubar-app.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/check-menubar-app.ts
| static func save(value: Data, service: String, account: String) throws { | ||
| let query: [String: Any] = [ | ||
| kSecClass as String: kSecClassGenericPassword, | ||
| kSecAttrService as String: service, | ||
| kSecAttrAccount as String: account, | ||
| ] | ||
|
|
||
| SecItemDelete(query as CFDictionary) | ||
|
|
||
| let attributes: [String: Any] = [ | ||
| kSecClass as String: kSecClassGenericPassword, | ||
| kSecAttrService as String: service, | ||
| kSecAttrAccount as String: account, | ||
| kSecValueData as String: value, | ||
| ] | ||
|
|
||
| let status = SecItemAdd(attributes as CFDictionary, nil) | ||
| guard status == errSecSuccess else { | ||
| throw NSError(domain: NSOSStatusErrorDomain, code: Int(status)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Apple Security.framework docs: what are the documented behaviors of SecItemAdd vs SecItemUpdate for existing generic-password items, and is delete-then-add recommended?
💡 Result:
For existing generic-password items (kSecClassGenericPassword) in Apple's Security.framework Keychain Services: SecItemAdd fails with errSecDuplicateItem (-25299) if the uniqueness constraint attributes match an existing item. The primary keys are: kSecAttrService, kSecAttrAccount, kSecAttrAccessGroup (if applicable), and kSecAttrSynchronizable (if applicable). Other attributes like kSecAttrGeneric do not affect uniqueness. SecItemUpdate modifies matching items based on a pure query dictionary (search attributes only, no return keys) and an update dictionary (new attribute values like kSecValueData). It preserves properties like persistent references and add-only attributes (e.g., kSecAttrAccessControl). Delete-then-add is not recommended. Apple Developer Technical Support (DTS) explicitly advises against it in best practices: prefer SecItemCopyMatching to check existence, then SecItemUpdate if found or SecItemAdd if not. Delete-then-add loses persistent references, risks data protection issues if key material unavailable, and is less efficient. Official docs and sample code (e.g., GenericKeychain) demonstrate the check-then-update/add pattern.
Citations:
- 1: https://developer.apple.com/documentation/security/secitemadd(_:_:)
- 2: https://developer.apple.com/documentation/security/secitemupdate(_:_:)
- 3: https://developer.apple.com/forums/thread/724013
- 4: https://developer.apple.com/forums/thread/724023
- 5: https://developer.apple.com/documentation/Security/kSecClassGenericPassword
- 6: https://developer.apple.com/documentation/security/updating-and-deleting-keychain-items
- 7: https://developer.apple.com/library/archive/samplecode/GenericKeychain/Listings/GenericKeychain_KeychainPasswordItem_swift.html
- 8: https://developer.apple.com/forums/thread/86961
🏁 Script executed:
fd -t f "KeychainHelper.swift"Repository: ya-nsh/tokenleak
Length of output: 111
🏁 Script executed:
cat -n packages/menubar/App/Utilities/KeychainHelper.swiftRepository: ya-nsh/tokenleak
Length of output: 2315
Use update-first semantics in save to avoid credential loss.
At line 12, deleting before re-adding is non-atomic; if SecItemAdd fails at line 21, the credential is permanently lost. Apple's Security.framework documentation and Developer Technical Support explicitly recommend using SecItemUpdate first, then SecItemAdd only when the item is not found. The delete-then-add pattern also loses persistent references and creates data protection vulnerabilities.
Update the save function to:
- Attempt
SecItemUpdatewith only the value attribute - Fall back to
SecItemAddonly iferrSecItemNotFoundis returned - Handle all other status codes as errors
Proposed fix
static func save(value: Data, service: String, account: String) throws {
let query: [String: Any] = [
kSecClass as String: kSecClassGenericPassword,
kSecAttrService as String: service,
kSecAttrAccount as String: account,
]
-
- SecItemDelete(query as CFDictionary)
-
- let attributes: [String: Any] = [
- kSecClass as String: kSecClassGenericPassword,
- kSecAttrService as String: service,
- kSecAttrAccount as String: account,
- kSecValueData as String: value,
- ]
-
- let status = SecItemAdd(attributes as CFDictionary, nil)
- guard status == errSecSuccess else {
- throw NSError(domain: NSOSStatusErrorDomain, code: Int(status))
- }
+ let updateAttrs: [String: Any] = [
+ kSecValueData as String: value,
+ ]
+
+ let updateStatus = SecItemUpdate(query as CFDictionary, updateAttrs as CFDictionary)
+ switch updateStatus {
+ case errSecSuccess:
+ return
+ case errSecItemNotFound:
+ var addQuery = query
+ addQuery[kSecValueData as String] = value
+ let addStatus = SecItemAdd(addQuery as CFDictionary, nil)
+ guard addStatus == errSecSuccess else {
+ throw NSError(domain: NSOSStatusErrorDomain, code: Int(addStatus))
+ }
+ default:
+ throw NSError(domain: NSOSStatusErrorDomain, code: Int(updateStatus))
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/menubar/App/Utilities/KeychainHelper.swift` around lines 5 - 24, The
save(value:service:account:) currently deletes the existing item then adds a new
one which can permanently lose credentials if SecItemAdd fails; change it to
first call SecItemUpdate using the query with kSecClassGenericPassword,
kSecAttrService and kSecAttrAccount and an attributes dictionary containing
kSecValueData set to value, then if SecItemUpdate returns errSecItemNotFound
call SecItemAdd with the full attributes to create the item, and for any other
non-success status throw an NSError with the returned status; remove the
SecItemDelete step and ensure you check for errSecSuccess on both SecItemUpdate
and SecItemAdd and treat other codes as errors.
| self.claudeUsage = ClaudeUsageService().resolveUsage(from: nil) | ||
| self.codexUsage = CodexUsageService().resolveUsage(from: nil) |
There was a problem hiding this comment.
Services are instantiated twice instead of using injected ones.
The initializer accepts services parameter (Line 28) but Lines 37-38 create new ClaudeUsageService() and CodexUsageService() instances instead of using the injected services. This defeats the purpose of dependency injection and could cause issues if custom services are passed for testing.
🐛 Proposed fix
- self.claudeUsage = ClaudeUsageService().resolveUsage(from: nil)
- self.codexUsage = CodexUsageService().resolveUsage(from: nil)
+ self.claudeUsage = services.first(where: { $0.kind == .claude })?.resolveUsage(from: nil) ?? ClaudeUsageService().resolveUsage(from: nil)
+ self.codexUsage = services.first(where: { $0.kind == .codex })?.resolveUsage(from: nil) ?? CodexUsageService().resolveUsage(from: nil)📝 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.
| self.claudeUsage = ClaudeUsageService().resolveUsage(from: nil) | |
| self.codexUsage = CodexUsageService().resolveUsage(from: nil) | |
| self.claudeUsage = services.first(where: { $0.kind == .claude })?.resolveUsage(from: nil) ?? ClaudeUsageService().resolveUsage(from: nil) | |
| self.codexUsage = services.first(where: { $0.kind == .codex })?.resolveUsage(from: nil) ?? CodexUsageService().resolveUsage(from: nil) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/menubar/App/ViewModels/UsageViewModel.swift` around lines 37 - 38,
The initializer is ignoring the injected services and creates new
ClaudeUsageService and CodexUsageService instances; update the initialization of
self.claudeUsage and self.codexUsage to call resolveUsage(from: nil) on the
injected services parameter (use the injected services' claude/codex usage
service instances) instead of instantiating new ClaudeUsageService() and
CodexUsageService(), referencing the UsageViewModel initializer's services
parameter and the resolveUsage(from:) method to locate the correct objects.
| Task.detached(priority: .userInitiated) { | ||
| guard FileManager.default.isExecutableFile(atPath: cliWrapperPath) else { | ||
| return | ||
| } | ||
|
|
||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: cliWrapperPath) | ||
| process.arguments = ["menubar", "refresh", "--home", homeDirectory] | ||
| try? process.run() | ||
| process.waitUntilExit() | ||
| } | ||
|
|
||
| Task { | ||
| try? await Task.sleep(for: .milliseconds(500)) | ||
| self.reloadSnapshot() | ||
| self.isRefreshing = false | ||
| } |
There was a problem hiding this comment.
Race condition between CLI process completion and snapshot reload.
The refresh logic spawns two independent tasks: one runs the CLI and waits for exit (Lines 102-112), while the other waits a fixed 500ms then reloads the snapshot (Lines 114-118). If the CLI takes longer than 500ms, the reload will fetch stale data. If it completes faster, the fixed delay adds unnecessary latency.
Consider coordinating these tasks so the reload happens after the process completes.
🐛 Proposed fix
func refresh() {
guard !isRefreshing else {
return
}
isRefreshing = true
let cliWrapperPath = self.cliWrapperPath
let homeDirectory = self.homeDirectory
Task.detached(priority: .userInitiated) {
guard FileManager.default.isExecutableFile(atPath: cliWrapperPath) else {
+ await MainActor.run {
+ self.isRefreshing = false
+ }
return
}
let process = Process()
process.executableURL = URL(fileURLWithPath: cliWrapperPath)
process.arguments = ["menubar", "refresh", "--home", homeDirectory]
try? process.run()
process.waitUntilExit()
- }
- Task {
- try? await Task.sleep(for: .milliseconds(500))
- self.reloadSnapshot()
- self.isRefreshing = false
+ await MainActor.run {
+ self.reloadSnapshot()
+ self.isRefreshing = false
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/menubar/App/ViewModels/UsageViewModel.swift` around lines 102 - 118,
The refresh spawns two independent tasks causing a race between Process
execution and reloadSnapshot(); instead, coordinate by performing the reload and
setting isRefreshing = false only after the Process completes: move the
reloadSnapshot() and isRefreshing update into the same Task that runs the
Process (the Task that uses cliWrapperPath, Process(), process.run() and
process.waitUntilExit()), and ensure those UI updates (reloadSnapshot() and
isRefreshing) are dispatched on the main actor (e.g., via `@MainActor` context or
DispatchQueue.main) so they happen after the CLI finishes and on the correct
thread.
| guard critical else { | ||
| return | ||
| } | ||
|
|
||
| withAnimation(.easeInOut(duration: 1.4).repeatForever(autoreverses: true)) { | ||
| glow.toggle() | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical glow animation won't start if critical becomes true after initial appear.
The glow animation is only initiated in onAppear. If critical changes from false to true while the view is already displayed, the pulsing glow effect won't activate.
✨ Proposed fix to handle dynamic critical state changes
.onAppear {
withAnimation(.easeOut(duration: 0.7)) {
animatedProgress = progress
}
-
- guard critical else {
- return
- }
-
- withAnimation(.easeInOut(duration: 1.4).repeatForever(autoreverses: true)) {
- glow.toggle()
- }
}
+ .onChange(of: critical) { _, isCritical in
+ if isCritical {
+ withAnimation(.easeInOut(duration: 1.4).repeatForever(autoreverses: true)) {
+ glow = true
+ }
+ } else {
+ withAnimation(.default) {
+ glow = false
+ }
+ }
+ }
.onChange(of: progress) { _, newValue in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/menubar/App/Views/ArcProgressRing.swift` around lines 42 - 49, The
glow animation is only started in onAppear so when `critical` flips true later
the pulsing won't begin; update the `ArcProgressRing` to also observe changes to
`critical` (e.g., use onChange(of: critical) or a published binding observer)
and run the same withAnimation(.easeInOut(duration:
1.4).repeatForever(autoreverses: true)) { glow.toggle() } block when `critical`
becomes true, and when `critical` becomes false stop/reset the effect by setting
`glow = false`; reference the `critical` property, the `glow` state, and the
existing withAnimation block to ensure the same behavior is invoked on state
change as on appear.
- Add toResetAtIso() to convert Unix epoch seconds to ISO strings (Claude Code sends resets_at as numbers, not strings) - Add used_percent/reset_at field name variants for flexibility - Add primary/secondary fallback (Codex-style naming) - Add top-level five_hour/seven_day fallback - Add debug logging to diagnose statusline hook data
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/menubar/claude-statusline.ts (1)
57-69: Redundant conditional branches.Both branches perform identical operations.
Buffer.from()handles both strings and Buffers, so the type check is unnecessary.♻️ Proposed simplification
async function readStdinText(): Promise<string> { const chunks: Buffer[] = []; for await (const chunk of process.stdin) { - if (typeof chunk === 'string') { - chunks.push(Buffer.from(chunk)); - } else { - chunks.push(Buffer.from(chunk)); - } + chunks.push(Buffer.from(chunk)); } return Buffer.concat(chunks).toString('utf8'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/menubar/claude-statusline.ts` around lines 57 - 69, The function readStdinText contains a redundant type check inside the for-await loop; remove the conditional and always push Buffer.from(chunk) into chunks (since Buffer.from accepts both strings and Buffers), leaving the rest of the function (chunks declaration, for-await over process.stdin, and Buffer.concat(...).toString('utf8')) unchanged to keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli/src/menubar/claude-statusline.ts`:
- Around line 57-69: The function readStdinText contains a redundant type check
inside the for-await loop; remove the conditional and always push
Buffer.from(chunk) into chunks (since Buffer.from accepts both strings and
Buffers), leaving the rest of the function (chunks declaration, for-await over
process.stdin, and Buffer.concat(...).toString('utf8')) unchanged to keep
behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 894b40a8-5965-4276-b025-4587370615fd
📒 Files selected for processing (1)
packages/cli/src/menubar/claude-statusline.ts
Replace the fragile 3-script statusline wrapper chain with a single self-contained bridge script that uses /usr/bin/python3 to extract rate_limits from Claude Code's statusline JSON. Key improvements: - No tokenleak CLI binary spawned in the hot path (zero latency) - Rate limit extraction runs in background, user statusline renders instantly - Self-healing daemon: auto-repairs ~/.claude/settings.json when statusLine is overwritten by the user or Claude Code - Bridge version tracking: daemon auto-upgrades old bridge scripts - New claude-rate-limits.ts reader in registry (symmetric with Codex) - Atomic write pattern prevents partial reads by the Swift app Fixes Claude Code showing "Cld --" in the menu bar while Codex stats displayed correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/registry/src/providers/claude-rate-limits.ts (1)
29-37: Consider validating ISO string format forresetAt.The function accepts any non-empty string as a valid ISO date without validation. Malformed strings will be passed through and may cause issues downstream when parsed with
Date.parse()instate.ts(line 57).💡 Optional: Add basic ISO format check
function toResetAtIso(value: unknown): string | null { if (typeof value === 'string' && value.length > 0) { + // Basic sanity check - ISO strings start with a year + if (Number.isNaN(Date.parse(value))) { + return null; + } return value; } if (typeof value === 'number' && Number.isFinite(value) && value > 0) { return new Date(value * 1000).toISOString(); } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/registry/src/providers/claude-rate-limits.ts` around lines 29 - 37, The helper toResetAtIso currently accepts any non-empty string and may pass malformed dates downstream; update to validate that string is a proper ISO 8601 datetime before returning it (e.g., use a RegExp or Date.parse() sanity check), and if validation fails return null; modify function toResetAtIso so it only returns the original string when it matches the ISO format (otherwise fall back to null) to prevent invalid values reaching the consumer (see usages that parse the date, e.g., in state.ts).packages/cli/src/menubar/install.ts (1)
86-110: Consider extracting shared helpers to reduce duplication.
readClaudeSettings,writeClaudeSettings, andparseCommandStatusLineare duplicated betweeninstall.tsandstate.ts(lines 291-319). Consider extracting these to a shared module to ensure consistent behavior (e.g., error handling).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/menubar/install.ts` around lines 86 - 110, Extract the duplicated helpers into a single shared module that exports readClaudeSettings, writeClaudeSettings, and parseCommandStatusLine; create a new file that contains the current implementations (preserving behavior such as ensureMenubarDir usage, JSON parse/write formatting, and the exact type checks), export those three functions, and then replace the local implementations in both install.ts and state.ts with imports from the new module (update call sites to use the exported functions and their return types).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/menubar/command.ts`:
- Around line 52-56: The --poll parsing currently assigns Number(argv[index+1])
directly which can be NaN; update the '--poll' branch (the switch case handling
--poll) to validate the parsed value: parse Number(argv[index+1]) into a local
variable, check Number.isFinite or !Number.isNaN, and if invalid throw a
TokenleakError('--poll requires a numeric value') (or otherwise set a sane
default before clamping); then assign the validated/clamped result to
parsed.pollIntervalSeconds. Ensure you reference the existing TokenleakError and
parsed.pollIntervalSeconds symbols when making the change.
In `@packages/cli/src/menubar/install.ts`:
- Around line 86-92: readClaudeSettings currently calls JSON.parse on the
contents of paths.claudeSettingsPath and will throw on malformed JSON; wrap the
readFileSync+JSON.parse in a try/catch inside the readClaudeSettings function
(same behavior as the duplicate in state.ts) and return an empty object {} when
parsing fails, while optionally logging the error; reference the
readClaudeSettings function and the paths.claudeSettingsPath/MenubarPaths
parameter when locating where to add the try/catch.
---
Nitpick comments:
In `@packages/cli/src/menubar/install.ts`:
- Around line 86-110: Extract the duplicated helpers into a single shared module
that exports readClaudeSettings, writeClaudeSettings, and
parseCommandStatusLine; create a new file that contains the current
implementations (preserving behavior such as ensureMenubarDir usage, JSON
parse/write formatting, and the exact type checks), export those three
functions, and then replace the local implementations in both install.ts and
state.ts with imports from the new module (update call sites to use the exported
functions and their return types).
In `@packages/registry/src/providers/claude-rate-limits.ts`:
- Around line 29-37: The helper toResetAtIso currently accepts any non-empty
string and may pass malformed dates downstream; update to validate that string
is a proper ISO 8601 datetime before returning it (e.g., use a RegExp or
Date.parse() sanity check), and if validation fails return null; modify function
toResetAtIso so it only returns the original string when it matches the ISO
format (otherwise fall back to null) to prevent invalid values reaching the
consumer (see usages that parse the date, e.g., in state.ts).
🪄 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: 0ef5fada-6d30-4d41-b9ea-54805de15f1d
📒 Files selected for processing (11)
packages/cli/src/menubar/command.tspackages/cli/src/menubar/install.tspackages/cli/src/menubar/launchd.test.tspackages/cli/src/menubar/launchd.tspackages/cli/src/menubar/state.test.tspackages/cli/src/menubar/state.tspackages/cli/src/menubar/types.tspackages/registry/src/index.tspackages/registry/src/providers/claude-rate-limits.test.tspackages/registry/src/providers/claude-rate-limits.tspackages/registry/src/providers/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/registry/src/index.ts
- packages/registry/src/providers/index.ts
- packages/cli/src/menubar/launchd.test.ts
| case '--poll': | ||
| if (!argv[index + 1]) throw new TokenleakError('--poll requires a value'); | ||
| parsed.pollIntervalSeconds = Number(argv[index + 1]!); | ||
| index += 2; | ||
| break; |
There was a problem hiding this comment.
Handle NaN when parsing --poll value.
If the user passes a non-numeric value (e.g., --poll abc), Number() returns NaN, which then gets clamped by Math.max(10, NaN) → NaN and persisted to config.
🐛 Proposed fix
case '--poll':
if (!argv[index + 1]) throw new TokenleakError('--poll requires a value');
- parsed.pollIntervalSeconds = Number(argv[index + 1]!);
+ const pollValue = Number(argv[index + 1]!);
+ if (!Number.isFinite(pollValue)) {
+ throw new TokenleakError('--poll requires a numeric value');
+ }
+ parsed.pollIntervalSeconds = pollValue;
index += 2;
break;📝 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.
| case '--poll': | |
| if (!argv[index + 1]) throw new TokenleakError('--poll requires a value'); | |
| parsed.pollIntervalSeconds = Number(argv[index + 1]!); | |
| index += 2; | |
| break; | |
| case '--poll': | |
| if (!argv[index + 1]) throw new TokenleakError('--poll requires a value'); | |
| const pollValue = Number(argv[index + 1]!); | |
| if (!Number.isFinite(pollValue)) { | |
| throw new TokenleakError('--poll requires a numeric value'); | |
| } | |
| parsed.pollIntervalSeconds = pollValue; | |
| index += 2; | |
| break; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/menubar/command.ts` around lines 52 - 56, The --poll parsing
currently assigns Number(argv[index+1]) directly which can be NaN; update the
'--poll' branch (the switch case handling --poll) to validate the parsed value:
parse Number(argv[index+1]) into a local variable, check Number.isFinite or
!Number.isNaN, and if invalid throw a TokenleakError('--poll requires a numeric
value') (or otherwise set a sane default before clamping); then assign the
validated/clamped result to parsed.pollIntervalSeconds. Ensure you reference the
existing TokenleakError and parsed.pollIntervalSeconds symbols when making the
change.
| function readClaudeSettings(paths: MenubarPaths): Record<string, unknown> { | ||
| if (!existsSync(paths.claudeSettingsPath)) { | ||
| return {}; | ||
| } | ||
|
|
||
| return JSON.parse(readFileSync(paths.claudeSettingsPath, 'utf8')) as Record<string, unknown>; | ||
| } |
There was a problem hiding this comment.
Add error handling for malformed JSON.
readClaudeSettings will throw if the file contains invalid JSON. The duplicate in state.ts (lines 291-301) has a try/catch that returns {} on parse failure. This version should match.
🐛 Proposed fix
function readClaudeSettings(paths: MenubarPaths): Record<string, unknown> {
if (!existsSync(paths.claudeSettingsPath)) {
return {};
}
- return JSON.parse(readFileSync(paths.claudeSettingsPath, 'utf8')) as Record<string, unknown>;
+ try {
+ return JSON.parse(readFileSync(paths.claudeSettingsPath, 'utf8')) as Record<string, unknown>;
+ } catch {
+ return {};
+ }
}📝 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.
| function readClaudeSettings(paths: MenubarPaths): Record<string, unknown> { | |
| if (!existsSync(paths.claudeSettingsPath)) { | |
| return {}; | |
| } | |
| return JSON.parse(readFileSync(paths.claudeSettingsPath, 'utf8')) as Record<string, unknown>; | |
| } | |
| function readClaudeSettings(paths: MenubarPaths): Record<string, unknown> { | |
| if (!existsSync(paths.claudeSettingsPath)) { | |
| return {}; | |
| } | |
| try { | |
| return JSON.parse(readFileSync(paths.claudeSettingsPath, 'utf8')) as Record<string, unknown>; | |
| } catch { | |
| return {}; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/menubar/install.ts` around lines 86 - 92, readClaudeSettings
currently calls JSON.parse on the contents of paths.claudeSettingsPath and will
throw on malformed JSON; wrap the readFileSync+JSON.parse in a try/catch inside
the readClaudeSettings function (same behavior as the duplicate in state.ts) and
return an empty object {} when parsing fails, while optionally logging the
error; reference the readClaudeSettings function and the
paths.claudeSettingsPath/MenubarPaths parameter when locating where to add the
try/catch.
- Disable NSPopover's built-in animation (popover.animates = false) which was stacking with SwiftUI's spring animation causing sluggish open/close - Replace heavy spring + scaleEffect with a fast 0.15s easeOut fade - Check daemon health every 30s instead of every 5s to reduce main thread work
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/menubar/App/AppDelegate.swift`:
- Around line 157-162: Before calling FileManager.default.createFile and opening
a FileHandle for daemonLogPath, ensure the containing directory exists: use
FileManager to get the directory component of daemonLogPath and create it with
createDirectory(atPath:withIntermediateDirectories:attributes:) (allowing
intermediate directories) and handle errors; then call
FileManager.default.createFile(atPath:daemonLogPath, contents:nil) and open
FileHandle(forWritingTo: URL(fileURLWithPath: daemonLogPath)) to assign
process.standardOutput and process.standardError so the daemon logs are not lost
when the logs directory is missing.
- Around line 144-156: startDaemonIfNeeded() can spawn duplicate daemon
instances because daemonProcess is in-memory; modify it to check for an external
singleton guard before launching: look for a PID/lock file created by the CLI
daemon (or probe an existing process by executable path/port) at cliWrapperPath
and validate the PID is running (or the lock is held) before creating the
Process; if no valid PID/lock exists, start the daemon as now and ensure the
daemon creates and atomically writes a PID/lock file and removes it on exit (and
handle stale PID by verifying process liveliness), and keep references to
daemonProcess for in-app lifecycle management; update any code that relies on
concurrent safety (e.g., refreshMenubarSnapshot()) to assume a single daemon and
rely on the PID/lock check.
🪄 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: 841377a2-f3ae-4372-bbb3-1b86a472cd41
📒 Files selected for processing (2)
packages/menubar/App/AppDelegate.swiftpackages/menubar/App/Views/PopoverContentView.swift
✅ Files skipped from review due to trivial changes (1)
- packages/menubar/App/Views/PopoverContentView.swift
| private func startDaemonIfNeeded() { | ||
| guard daemonProcess?.isRunning != true else { | ||
| return | ||
| } | ||
|
|
||
| guard FileManager.default.isExecutableFile(atPath: cliWrapperPath) else { | ||
| return | ||
| } | ||
|
|
||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: cliWrapperPath) | ||
| process.arguments = ["menubar", "daemon", "--home", homeDirectory] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate menubar daemon-related sources
fd -i 'menubar' packages/cli/src
# 2) Inspect daemon command implementation for lock/pid/single-instance checks
rg -n -C4 --type=ts --type=js --type=bash \
'\bdaemon\b|\bpid\b|lock|flock|already running|singleton|launchd|status' \
packages/cli/src || true
# 3) Inspect snapshot write paths for collision protection
rg -n -C3 --type=ts \
'snapshot|writeFile|rename|atomic|lock' \
packages/cli/src/menubar || trueRepository: ya-nsh/tokenleak
Length of output: 25665
🏁 Script executed:
# Examine the runDaemon function implementation
cat -n packages/cli/src/menubar/command.ts | grep -A 30 'async function runDaemon'
# Check if there's any lock/PID logic in the daemon
rg -n -B2 -A10 'lock|pid|single.*instance|already.*running' packages/cli/src/menubar/command.ts || true
# Check the launchd plist building to see how daemon is configured
rg -n -B2 -A15 'buildDaemonPlist|buildAppPlist' packages/cli/src/menubar/Repository: ya-nsh/tokenleak
Length of output: 7077
🏁 Script executed:
# View complete buildAppPlist to see KeepAlive and all plist settings
cat -n packages/cli/src/menubar/launchd.ts | head -50
# Check if there's any daemon PID/lock logic elsewhere
rg -n 'daemon|lock|pid|\.lock|\.pid|single.*instance' packages/cli/src/menubar/ -t ts -t js
# Check if the CLI wrapper has any singleton logic before calling daemon
cat -n packages/cli/src/menubar/launchd.ts | grep -A 40 'buildCliWrapper'Repository: ya-nsh/tokenleak
Length of output: 3700
🏁 Script executed:
# Check if daemon initializes any lock/pid file
rg -n -B3 -A8 'async function runDaemon' packages/cli/src/menubar/command.ts
# Check for any file locking when writing snapshots
rg -n 'lock|flock|fcntl' packages/cli/src/menubar/ -t ts -t js
# Verify the snapshot write is truly atomic and safe for concurrent access
cat -n packages/cli/src/menubar/state.ts | sed -n '212,220p'Repository: ya-nsh/tokenleak
Length of output: 1035
Implement singleton protection in daemon startup to prevent multiple instances after app crash/restart.
The CLI daemon has no lock/PID guard. When the Swift app crashes and launchd restarts it (KeepAlive=true), the fresh app instance has an empty in-memory daemonProcess reference, causing startDaemonIfNeeded() to spawn a second daemon process. While snapshot writes use atomic rename (safe from corruption), multiple concurrent daemon instances will cause redundant API calls and potential race conditions in refreshMenubarSnapshot().
Add either:
- A PID file lock checked/created at daemon startup
- A lock file acquired by the daemon process
- An OS-level singleton check (e.g., checking existing process by name/port)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/menubar/App/AppDelegate.swift` around lines 144 - 156,
startDaemonIfNeeded() can spawn duplicate daemon instances because daemonProcess
is in-memory; modify it to check for an external singleton guard before
launching: look for a PID/lock file created by the CLI daemon (or probe an
existing process by executable path/port) at cliWrapperPath and validate the PID
is running (or the lock is held) before creating the Process; if no valid
PID/lock exists, start the daemon as now and ensure the daemon creates and
atomically writes a PID/lock file and removes it on exit (and handle stale PID
by verifying process liveliness), and keep references to daemonProcess for
in-app lifecycle management; update any code that relies on concurrent safety
(e.g., refreshMenubarSnapshot()) to assume a single daemon and rely on the
PID/lock check.
| FileManager.default.createFile(atPath: daemonLogPath, contents: nil) | ||
| if let handle = try? FileHandle(forWritingTo: URL(fileURLWithPath: daemonLogPath)) { | ||
| _ = try? handle.seekToEnd() | ||
| process.standardOutput = handle | ||
| process.standardError = handle | ||
| } |
There was a problem hiding this comment.
Ensure the daemon log directory exists before opening the log file.
On Line 157, createFile fails silently if .../logs does not exist, which causes stdout/stderr redirection to be skipped and loses daemon logs.
💡 Proposed fix
- FileManager.default.createFile(atPath: daemonLogPath, contents: nil)
- if let handle = try? FileHandle(forWritingTo: URL(fileURLWithPath: daemonLogPath)) {
+ let logURL = URL(fileURLWithPath: daemonLogPath)
+ let logDirectoryURL = logURL.deletingLastPathComponent()
+ try? FileManager.default.createDirectory(at: logDirectoryURL, withIntermediateDirectories: true)
+ if !FileManager.default.fileExists(atPath: daemonLogPath) {
+ FileManager.default.createFile(atPath: daemonLogPath, contents: nil)
+ }
+ if let handle = try? FileHandle(forWritingTo: logURL) {
_ = try? handle.seekToEnd()
process.standardOutput = handle
process.standardError = handle
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/menubar/App/AppDelegate.swift` around lines 157 - 162, Before
calling FileManager.default.createFile and opening a FileHandle for
daemonLogPath, ensure the containing directory exists: use FileManager to get
the directory component of daemonLogPath and create it with
createDirectory(atPath:withIntermediateDirectories:attributes:) (allowing
intermediate directories) and handle errors; then call
FileManager.default.createFile(atPath:daemonLogPath, contents:nil) and open
FileHandle(forWritingTo: URL(fileURLWithPath: daemonLogPath)) to assign
process.standardOutput and process.standardError so the daemon logs are not lost
when the logs directory is missing.
ya-nsh
changed the title
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Enhancements
Chores
Tests