fix: add structured debug logging to silent catch blocks in update-check#229
fix: add structured debug logging to silent catch blocks in update-check#229iamyhe wants to merge 2 commits into
Conversation
WalkthroughThis PR adds optional debug logging to the update-notifier cache logic in src/lib/update-check.ts. A new helper checks process.argv for --debug or --verbose flags; when enabled, cache read/parse and cache write failures emit debug messages to stderr while preserving the existing silent-failure behavior. A module doc comment was also reformatted. ChangesUpdate-check debug logging
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
src/lib/update-check.ts (1)
120-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
argvdefault parameter never actually gets injected — DI intent unfulfilled.
isDebugLoggingEnabledaccepts anargvparameter for testability, but both call sites (Line 136, Line 154) invoke it with no arguments, so it always falls back to reading the realprocess.argv. Unlike every other dependency in this module (env,now,readFile,stderr, etc., all resolved viaresolveUpdateCheckDeps/ResolvedUpdateCheckDeps),argvisn't threaded through the deps object, so tests can't exercise the debug-logging branch without mutating the globalprocess.argv. This directly contradicts the PR's own stated goal of "injectingargvas a dependency ... to improve testability."♻️ Suggested fix: thread argv through the deps object
-function isDebugLoggingEnabled(argv: string[] = process.argv): boolean { - return argv.includes('--debug') || argv.includes('--verbose'); -} +function isDebugLoggingEnabled(resolved: ResolvedUpdateCheckDeps): boolean { + return resolved.argv.includes('--debug') || resolved.argv.includes('--verbose'); +}Add
argv: string[]toUpdateCheckDeps/ResolvedUpdateCheckDeps(defaulting toprocess.argvinresolveUpdateCheckDeps), then callisDebugLoggingEnabled(resolved)at both call sites.Also applies to: 136-136, 154-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/update-check.ts` around lines 120 - 123, Thread argv through the dependency injection path in update-check.ts: add argv to UpdateCheckDeps and ResolvedUpdateCheckDeps, default it to process.argv in resolveUpdateCheckDeps, and update the call sites of isDebugLoggingEnabled so they use the resolved deps instead of relying on the function’s default parameter. This keeps isDebugLoggingEnabled testable without touching global process.argv and aligns it with the other injected dependencies in this module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/update-check.ts`:
- Around line 135-138: The debug write in readUpdateCheckCache is appending an
explicit newline even though resolved.stderr already terminates the message,
causing double spacing in debug output. Update the debug logging strings in this
catch block (and any similar resolved.stderr calls in update-check.ts) to be
newline-free so the output formatting matches the advisory path.
---
Nitpick comments:
In `@src/lib/update-check.ts`:
- Around line 120-123: Thread argv through the dependency injection path in
update-check.ts: add argv to UpdateCheckDeps and ResolvedUpdateCheckDeps,
default it to process.argv in resolveUpdateCheckDeps, and update the call sites
of isDebugLoggingEnabled so they use the resolved deps instead of relying on the
function’s default parameter. This keeps isDebugLoggingEnabled testable without
touching global process.argv and aligns it with the other injected dependencies
in this module.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 16e4c2a9-8257-4487-9bb1-4ed1a6199d77
📒 Files selected for processing (1)
src/lib/update-check.ts
| } catch (err) { | ||
| if (isDebugLoggingEnabled()) { | ||
| resolved.stderr(`[debug] readUpdateCheckCache: ${err instanceof Error ? err.message : String(err)}\n`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect resolveUpdateCheckDeps / default stderr implementation for newline handling
rg -n -B2 -A8 'resolveUpdateCheckDeps|stderr:' src/lib/update-check.tsRepository: TestSprite/testsprite-cli
Length of output: 2763
🏁 Script executed:
#!/bin/bash
sed -n '130,160p' src/lib/update-check.ts
printf '\n---\n'
sed -n '280,295p' src/lib/update-check.tsRepository: TestSprite/testsprite-cli
Length of output: 2016
Drop the explicit \n in the debug writes. resolved.stderr already adds a newline, so the debug messages print with a doubled blank line. Keep those strings newline-free to match the advisory call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/update-check.ts` around lines 135 - 138, The debug write in
readUpdateCheckCache is appending an explicit newline even though
resolved.stderr already terminates the message, causing double spacing in debug
output. Update the debug logging strings in this catch block (and any similar
resolved.stderr calls in update-check.ts) to be newline-free so the output
formatting matches the advisory path.
1 participant
What does this PR do?
This PR adds structured debug logging to the silent catch blocks in the
update-checkmodule to improve maintainability and troubleshooting.Key Highlights:
isDebugLoggingEnabledhelper for cleaner logic.argvdependency to follow the project's dependency-injection pattern and improve testability.Type of change
Summary by CodeRabbit
New Features
Bug Fixes