Conversation
📝 WalkthroughWalkthroughThis PR systematically removes console.log debugging statements and logging side effects throughout the codebase across API routes, CLI, desktop application, and streaming services. No functional changes or control-flow modifications are introduced. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/cli/src/server.ts (1)
17-20:⚠️ Potential issue | 🟡 MinorEmpty conditional block silently swallows non-zero exit codes.
Removing the log from this branch left a no-op
ifblock. Per coding guidelines, error conditions should use prefixed console logging ([domain/operation] message), and errors should never be swallowed silently. A child process exiting with a non-zero/non-130 code is an error condition worth surfacing.Suggested fix
child.on("exit", (code) => { if (code !== null && code !== 0 && code !== 130) { + console.error(`[cli/server] Child process exited with code ${code}`); } });Alternatively, if the intent is truly to suppress all output, remove the entire
exithandler to avoid dead code.As per coding guidelines, "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly" and "Use prefixed console logging with consistent context pattern: [domain/operation] message for … error conditions".
apps/desktop/scripts/upgrade-claude-binary.ts (1)
33-59:⚠️ Potential issue | 🟠 MajorThis developer-facing CLI script is now completely silent — consider structured logging instead of removing all output.
The PR goal is to remove debug logs, but this script is a manually-invoked CLI tool where informational output (current version → new version, staged files) is essential UX. The coding guideline requires prefixed console logging for significant operations. Replace the removed
console.logcalls with guideline-compliant structured logs rather than stripping them entirely.Example of what the script could retain:
const currentVersion = existsSync(versionFile) ? readFileSync(versionFile, "utf-8").split("\n")[0].trim() : "(none)"; run(`bun run scripts/download-claude-binary.ts${versionFlag}`); const newVersion = readFileSync(versionFile, "utf-8").split("\n")[0].trim(); console.log(`[upgrade-claude-binary] ${currentVersion} → ${newVersion}`); run("git add resources/bin/darwin-arm64/claude"); const status = run("git status --short resources/bin/darwin-arm64/"); console.log(`[upgrade-claude-binary] staged:\n${status}`);As per coding guidelines, "Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions".
🤖 Fix all issues with AI agents
In `@apps/desktop/scripts/upgrade-claude-binary.ts`:
- Around line 52-58: The code computes _newVersion by calling
readFileSync(versionFile, "utf-8") and runs run("git status --short ...")
producing _status but neither variable is used; remove these dead statements to
avoid unnecessary I/O and subprocess calls. Delete the line that assigns to
_newVersion and the line that assigns to _status (or, if you intended to log or
assert them, replace the assignments with a meaningful use such as logging via
processLogger or comparing against an expected value) and keep the calls that
must remain (e.g., run("git add ...") and readFileSync only if _newVersion is
actually needed elsewhere).
- Around line 40-45: The block reads versionFile into a block-scoped
_currentVersion that is never used and the else branch is empty; remove the dead
code or make it meaningful by declaring a variable outside the block and
handling the missing-file case. Concretely: either delete the whole if/else, or
change to a top-level let currentVersion; then inside the existsSync check
assign currentVersion = readFileSync(versionFile,
"utf-8").split("\n")[0].trim(); and replace/remove the empty else (or add
logging/handling there). Update any subsequent logic to reference currentVersion
instead of _currentVersion and remove the unused variable _currentVersion.
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/claude-session-scanner/claude-session-reader.ts`:
- Around line 165-167: The outer catch that currently swallows errors from
createReadStream/readline should log the error with the project's prefixed
context pattern and then rethrow (do not silently return []); find the catch
block around the createReadStream/readline logic in claude-session-reader.ts and
replace the bare return [] with a console.error (or the project's console
wrapper) that includes a clear prefix like "[claude-session-reader/read]" and
the caught error details, then rethrow the error so callers can handle it.
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/claude-session-scanner/claude-session-scanner.ts`:
- Around line 90-97: In readSessionMeta, do not silently swallow errors in the
catch that surrounds fsOpen/fsRead; before returning null log the error with a
consistent prefixed context (e.g. "[claude-session-scanner/readSessionMeta]")
and include the caught _err (and any fd-close error if applicable), then proceed
with cleanup and return null (or rethrow if preferred). Locate the catch block
around fsOpen/fsRead and update it to call the module's logger or console.error
with the prefixed message and error details, ensure fd is still closed in the
inner try/catch and that close errors are also logged with the same prefix.
- Around line 110-112: The catch block in the claude-session-scanner handling
the readdir of the projects directory silently swallows errors; change the catch
from `catch (_err) { return []; }` to log the error with context (e.g.,
`console.error('claude-session-scanner: failed reading projects directory',
err)`) before returning the empty array so permission or other failures are
visible; locate the catch that wraps the `readdir` call in the
claude-session-scanner logic and replace the underscore-suppressed error with a
real error variable and a contextual console.error then return [] as before.
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`:
- Around line 477-478: Four catch blocks are currently swallowing errors (around
the proxy stop call and store update/delete calls); change each to log the error
using the same pattern as other handlers in this file—call console.error with
the “[chat/session]” prefix and a short contextual message plus the caught error
(e.g. console.error("[chat/session] failed to stop proxy request", err)), then
continue with the existing "best-effort" handling; apply this to the catch
blocks handling the stop operation and the three store update/delete operations
so they consistently log errors instead of being silent.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts`:
- Around line 53-64: The stdout/stderr handlers contain dead for-loops that
split text into lines but do nothing; remove the unnecessary loops and the
text.trimEnd().split("\n") allocation inside the child.stdout?.on("data", ...)
and child.stderr?.on("data", ...) callbacks and keep just the combined += text
logic (referencing the child.stdout/stderr callbacks and the combined variable)
to avoid pointless allocations and clarify intent.
In `@apps/desktop/src/main/lib/local-network-permission.ts`:
- Around line 20-22: The socket error handler and related error paths currently
swallow errors; update the socket.on("error") handler, the socket.send callback,
and the surrounding catch block to log errors using the required prefixed
context pattern (e.g., "[local-network-permission/socket] <message>") before
handling or rethrowing them so errors are not silent, and ensure logs include
the error object or message for debugging; also remove/collapse the empty
if/else branch (the no-op conditional near the socket send logic) to eliminate
dead code and simplify flow.
In `@apps/desktop/src/main/terminal-host/index.ts`:
- Around line 69-79: The log function (log) was turned into a no-op and computes
timestamp/_prefix but never emits anything; restore operational logging by
emitting at least warn and error messages (and optionally info when needed)
using console.warn/console.error (or a provided logger) with the constructed
prefix and timestamp, include the message and serialized data (e.g.,
JSON.stringify(data, ...)) so exceptions, auth failures, socket errors and
shutdown events are visible, and keep the if (data !== undefined) branch to
append structured data while preserving the original prefix format; do not
swallow errors silently—ensure error-level logs always run and include
contextual information before any rethrow/handling.
In `@apps/desktop/src/main/windows/main.ts`:
- Around line 136-140: The listen callback for notificationsApp was replaced
with a no-op, removing startup visibility; update the notificationsApp.listen
call (where server is assigned) to provide a callback that logs startup success
(including bound port and host) using the prefixed context pattern like
"[notifications/listen] ..." and also handle/report errors if the listen API
surfaces them; ensure you reference PORTS.NOTIFICATIONS and the server variable
so the log shows the actual port/host and that the notification server started.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.tsx`:
- Line 42: The TaskContextMenu is being rendered with a no-op delete handler
(TaskContextMenu key={row.id} task={task} onDelete={() => {}}) which leaves the
Delete menu item visible but non-functional; either wire this up to the real
deletion flow (call the existing delete function — e.g., deleteTask(task.id) or
dispatch the tasks store/action, show a confirmation modal and success/error
toast) or explicitly disable/hide the Delete item by passing a prop/flag or
omitting onDelete so the menu renders disabled until implemented; update the
TasksTableView to replace the empty onDelete handler with the proper delete call
or a disabling prop on TaskContextMenu.
In `@apps/streams/src/index.ts`:
- Around line 26-29: The serve callback currently swallows the startup event for
the streams proxy; update the listener passed to serve (where proxyServer is
created with serve({ fetch: app.fetch, port: env.STREAMS_PORT }, (_info) => {}))
to emit a prefixed console log indicating the proxy is ready, e.g. use the
required pattern "[streams/proxy] proxy listening on port <port>" or similar,
including env.STREAMS_PORT and any relevant _info details; ensure this log is
printed from the callback so the bind/ready event is visible in runtime logs.
In `@packages/durable-session/src/materialize.ts`:
- Around line 77-79: The catch currently swallows errors silently; change it to
capture the error variable (e.g., catch (err)) and log it with a prefixed
context using console.warn or console.debug instead of doing nothing — for
example: console.warn("[durable-session/materialize] Skipping malformed chunk:",
err); keep the existing behavior of skipping the chunk but ensure the error and
context are logged (update the catch in materialize.ts where the malformed chunk
is handled).
🧹 Nitpick comments (8)
apps/desktop/src/lib/window-loader.ts (1)
33-34: Remove the empty event handler and stale comment.The
did-finish-loadlistener is now a no-op, and the comment "Log successful loads" no longer describes reality. Keeping an empty listener just adds a dead event subscription.♻️ Proposed fix
- // Log successful loads - props.browserWindow.webContents.on("did-finish-load", () => {}); - - // Log and handle load failures + // Log and handle load failuresapps/cli/src/cli.tsx (1)
335-337: Sensible default —program.help()is the standard Commander.js pattern.Nit: the
asynckeyword is no longer needed sinceprogram.help()is synchronous and the previousawaitcalls were removed.Optional cleanup
-program.action(async () => { +program.action(() => { program.help(); });apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
66-67:callIdandstartedAtare only consumed insideDEBUG_TERMINALblocks.After the log removal, these two variables (and the module-level
createOrAttachCallCounter) are only referenced inside theif (DEBUG_TERMINAL)guards at lines 148–155. Consider moving them inside that block so they aren't computed on every call when debugging is off.♻️ Suggested change
.mutation(async ({ input }) => { - const callId = ++createOrAttachCallCounter; - const startedAt = Date.now(); const { paneId, ... @@ -146,8 +144,10 @@ } if (DEBUG_TERMINAL) { + const callId = ++createOrAttachCallCounter; console.warn("[Terminal Router] createOrAttach failed:", { callId, paneId, - durationMs: Date.now() - startedAt, + // durationMs removed — startedAt was only tracked for this log error: error instanceof Error ? error.message : String(error), }); }If you still want
durationMs, wrap bothstartedAtandcallIdinside a singleDEBUG_TERMINALguard at the top of the mutation so the cost is only incurred when debugging.apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts (1)
189-192: Consider removing the_reasonparameter entirely instead of just silencing it.The
_reasonparameter is passed at three call sites (lines 261, 298, 345) but never used. Prefixing with_suppresses the lint warning but leaves dead arguments scattered across the file. Since this PR is already touching these areas, removing the parameter and its arguments would be a cleaner outcome.♻️ Proposed fix (function signature)
const resolveLocalStartPoint = async ( - _reason: string, checkOriginRefs: boolean, ): Promise<LocalStartPointResult> => {And update call sites:
- const localResult = await resolveLocalStartPoint( - "Remote unavailable", - true, - ); + const localResult = await resolveLocalStartPoint(true);- const localResult = await resolveLocalStartPoint( - "No remote configured", - false, - ); + const localResult = await resolveLocalStartPoint(false);- const localResult = await resolveLocalStartPoint( - "Fetch failed and remote tracking ref unavailable", - true, - ); + const localResult = await resolveLocalStartPoint(true);apps/desktop/src/lib/trpc/routers/ai-chat/utils/claude-session-scanner/claude-session-scanner.ts (1)
139-142: Per-filestatand per-directoryreaddirerrors are also silently swallowed.These are less critical since they occur in batch loops and individual failures are semi-expected (race conditions, transient locks). However, a
console.warnat minimum would help diagnose intermittent scan failures without being as noisy as the debug logs that were removed.As per coding guidelines: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."
apps/desktop/src/shared/debug.ts (2)
26-29:debugLogis now a no-op but theif (isDebugEnabled)block and documentation still suggest otherwise.The empty
ifblock is dead code. If this is meant to be a permanent no-op, clean it up; if temporary, add a comment explaining why.If permanent no-op
export function debugLog(_namespace: string, ..._args: unknown[]): void { - if (isDebugEnabled) { - } + // Intentionally disabled — debug logging removed }
1-14: File-level documentation is now misleading.The header comment describes how to enable debug logs and what output to expect, but the function is a no-op. Either update the docs to reflect the current state or remove them to avoid confusion for developers who set
SUPERSET_DEBUG=1expecting output.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalRestore.ts (1)
52-65:paneIdis destructured but never used in the hook body.With the current implementation,
paneIdremains unused. Either prefix it as_paneIdto explicitly signal intent, or remove it from the interface if no caller requires passing it.Proposed fix
export function useTerminalRestore({ - paneId, + _paneId, xtermRef,Or remove from interface if safe:
export interface UseTerminalRestoreOptions { - paneId: string; xtermRef: React.MutableRefObject<XTerm | null>;
| if (existsSync(versionFile)) { | ||
| const currentVersion = readFileSync(versionFile, "utf-8") | ||
| const _currentVersion = readFileSync(versionFile, "utf-8") | ||
| .split("\n")[0] | ||
| .trim(); | ||
| console.log(`Current version: ${currentVersion}`); | ||
| } else { | ||
| console.log("No existing binaries found"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Dead code: _currentVersion is computed inside a block scope and never used; else branch is empty.
The variable is block-scoped to the if body, so it can't even be referenced later. The empty else is a no-op. Either remove the entire block or retain meaningful logging per the project's guideline.
Proposed fix
const versionFile = join(BIN_DIR, "VERSION");
- if (existsSync(versionFile)) {
- const _currentVersion = readFileSync(versionFile, "utf-8")
- .split("\n")[0]
- .trim();
- } else {
- }📝 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.
| if (existsSync(versionFile)) { | |
| const currentVersion = readFileSync(versionFile, "utf-8") | |
| const _currentVersion = readFileSync(versionFile, "utf-8") | |
| .split("\n")[0] | |
| .trim(); | |
| console.log(`Current version: ${currentVersion}`); | |
| } else { | |
| console.log("No existing binaries found"); | |
| } | |
| const versionFile = join(BIN_DIR, "VERSION"); |
🤖 Prompt for AI Agents
In `@apps/desktop/scripts/upgrade-claude-binary.ts` around lines 40 - 45, The
block reads versionFile into a block-scoped _currentVersion that is never used
and the else branch is empty; remove the dead code or make it meaningful by
declaring a variable outside the block and handling the missing-file case.
Concretely: either delete the whole if/else, or change to a top-level let
currentVersion; then inside the existsSync check assign currentVersion =
readFileSync(versionFile, "utf-8").split("\n")[0].trim(); and replace/remove the
empty else (or add logging/handling there). Update any subsequent logic to
reference currentVersion instead of _currentVersion and remove the unused
variable _currentVersion.
| const _newVersion = readFileSync(versionFile, "utf-8").split("\n")[0].trim(); | ||
|
|
||
| // Stage the binary | ||
| console.log("\nStaging binary in git..."); | ||
| run("git add resources/bin/darwin-arm64/claude"); | ||
|
|
||
| // Show status | ||
| console.log("\nGit status:"); | ||
| const status = run("git status --short resources/bin/darwin-arm64/"); | ||
| console.log(status || " (no changes)"); | ||
|
|
||
| console.log("\n✓ Upgrade complete!"); | ||
| console.log("\nNext steps:"); | ||
| console.log( | ||
| ` git commit -m "chore: upgrade claude binary to v${newVersion}"`, | ||
| ); | ||
| console.log(" git push"); | ||
| const _status = run("git status --short resources/bin/darwin-arm64/"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unused variables _newVersion and _status should be removed or put to use.
Both values are computed (incurring I/O and a subprocess) but discarded. If the intent is to remove debug logs, remove the statements entirely rather than assigning to throwaway variables. Keeping dead I/O is confusing and wasteful.
Proposed fix — remove dead statements
- // Get new version
- const _newVersion = readFileSync(versionFile, "utf-8").split("\n")[0].trim();
-
// Stage the binary
run("git add resources/bin/darwin-arm64/claude");
-
- // Show status
- const _status = run("git status --short resources/bin/darwin-arm64/");📝 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.
| const _newVersion = readFileSync(versionFile, "utf-8").split("\n")[0].trim(); | |
| // Stage the binary | |
| console.log("\nStaging binary in git..."); | |
| run("git add resources/bin/darwin-arm64/claude"); | |
| // Show status | |
| console.log("\nGit status:"); | |
| const status = run("git status --short resources/bin/darwin-arm64/"); | |
| console.log(status || " (no changes)"); | |
| console.log("\n✓ Upgrade complete!"); | |
| console.log("\nNext steps:"); | |
| console.log( | |
| ` git commit -m "chore: upgrade claude binary to v${newVersion}"`, | |
| ); | |
| console.log(" git push"); | |
| const _status = run("git status --short resources/bin/darwin-arm64/"); | |
| // Stage the binary | |
| run("git add resources/bin/darwin-arm64/claude"); |
🤖 Prompt for AI Agents
In `@apps/desktop/scripts/upgrade-claude-binary.ts` around lines 52 - 58, The code
computes _newVersion by calling readFileSync(versionFile, "utf-8") and runs
run("git status --short ...") producing _status but neither variable is used;
remove these dead statements to avoid unnecessary I/O and subprocess calls.
Delete the line that assigns to _newVersion and the line that assigns to _status
(or, if you intended to log or assert them, replace the assignments with a
meaningful use such as logging via processLogger or comparing against an
expected value) and keep the calls that must remain (e.g., run("git add ...")
and readFileSync only if _newVersion is actually needed elsewhere).
| } catch (_err) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Silently swallowing file I/O errors violates project guidelines.
The outer catch covers createReadStream and the readline loop — errors here could indicate permission issues, race conditions, or disk failures. Returning [] silently makes these invisible to operators.
Replace with a prefixed log per the project's logging convention:
Proposed fix
- } catch (_err) {
+ } catch (err) {
+ console.error("[claude-session-reader/readClaudeSessionMessages]", err);
return [];
}Based on learnings: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly." As per coding guidelines: "Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions."
📝 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.
| } catch (_err) { | |
| return []; | |
| } | |
| } catch (err) { | |
| console.error("[claude-session-reader/readClaudeSessionMessages]", err); | |
| return []; | |
| } |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/claude-session-scanner/claude-session-reader.ts`
around lines 165 - 167, The outer catch that currently swallows errors from
createReadStream/readline should log the error with the project's prefixed
context pattern and then rethrow (do not silently return []); find the catch
block around the createReadStream/readline logic in claude-session-reader.ts and
replace the bare return [] with a console.error (or the project's console
wrapper) that includes a clear prefix like "[claude-session-reader/read]" and
the caught error details, then rethrow the error so callers can handle it.
| } catch (_err) { | ||
| if (fd !== undefined) { | ||
| try { | ||
| await fsClose(fd); | ||
| } catch (closeErr) { | ||
| console.debug(`[claude-scanner] Failed to close fd:`, closeErr); | ||
| } | ||
| } catch (_closeErr) {} | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Silent error swallowing in readSessionMeta hides file I/O failures.
This catch covers fsOpen/fsRead errors. Silently returning null makes permission errors, corrupt files, and other I/O issues invisible. Add a prefixed log before returning.
Proposed fix
- } catch (_err) {
+ } catch (err) {
+ console.error("[claude-session-scanner/readSessionMeta]", filePath, err);
if (fd !== undefined) {
try {
await fsClose(fd);
} catch (_closeErr) {}
}
return null;
}Based on learnings: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly." As per coding guidelines: "Use prefixed console logging with consistent context pattern: [domain/operation] message."
📝 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.
| } catch (_err) { | |
| if (fd !== undefined) { | |
| try { | |
| await fsClose(fd); | |
| } catch (closeErr) { | |
| console.debug(`[claude-scanner] Failed to close fd:`, closeErr); | |
| } | |
| } catch (_closeErr) {} | |
| } | |
| return null; | |
| } | |
| } catch (err) { | |
| console.error("[claude-session-scanner/readSessionMeta]", filePath, err); | |
| if (fd !== undefined) { | |
| try { | |
| await fsClose(fd); | |
| } catch (_closeErr) {} | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/claude-session-scanner/claude-session-scanner.ts`
around lines 90 - 97, In readSessionMeta, do not silently swallow errors in the
catch that surrounds fsOpen/fsRead; before returning null log the error with a
consistent prefixed context (e.g. "[claude-session-scanner/readSessionMeta]")
and include the caught _err (and any fd-close error if applicable), then proceed
with cleanup and return null (or rethrow if preferred). Locate the catch block
around fsOpen/fsRead and update it to call the module's logger or console.error
with the prefixed message and error details, ensure fd is still closed in the
inner try/catch and that close errors are also logged with the same prefix.
| } catch (_err) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Silent catch on readdir of the projects directory hides real failures.
If ~/.claude/projects is inaccessible due to permissions (rather than simply not existing), this silently returns an empty list with no diagnostic trace. A prefixed console.error would preserve the "remove debug logs" intent while keeping error-condition visibility.
Proposed fix
- } catch (_err) {
+ } catch (err) {
+ console.error("[claude-session-scanner/buildIndex] failed to read projects dir", err);
return [];
}As per coding guidelines: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."
📝 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.
| } catch (_err) { | |
| return []; | |
| } | |
| } catch (err) { | |
| console.error("[claude-session-scanner/buildIndex] failed to read projects dir", err); | |
| return []; | |
| } |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/claude-session-scanner/claude-session-scanner.ts`
around lines 110 - 112, The catch block in the claude-session-scanner handling
the readdir of the projects directory silently swallows errors; change the catch
from `catch (_err) { return []; }` to log the error with context (e.g.,
`console.error('claude-session-scanner: failed reading projects directory',
err)`) before returning the empty array so permission or other failures are
visible; locate the catch that wraps the `readdir` call in the
claude-session-scanner logic and replace the underscore-suppressed error with a
real error variable and a contextual console.error then return [] as before.
| function log( | ||
| level: "info" | "warn" | "error", | ||
| message: string, | ||
| _message: string, | ||
| data?: unknown, | ||
| ) { | ||
| const timestamp = new Date().toISOString(); | ||
| const prefix = `[${timestamp}] [terminal-host] [${level.toUpperCase()}]`; | ||
| const _prefix = `[${timestamp}] [terminal-host] [${level.toUpperCase()}]`; | ||
| if (data !== undefined) { | ||
| console.log(`${prefix} ${message}`, data); | ||
| } else { | ||
| console.log(`${prefix} ${message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Turning the daemon's only log function into a no-op removes all operational visibility.
This is a persistent background daemon — the kind of process where logging is most critical. With this change, uncaught exceptions (line 804), auth failures (line 249), socket errors (line 627), handler errors (line 562), and shutdown events are all silently swallowed. Diagnosing production issues in a headless daemon without any log output will be extremely difficult.
Additionally, the function body is now dead code: timestamp and _prefix are computed but unused, and the if/else block is empty.
If the goal is to reduce noise from info-level logs, consider retaining error and warn output instead of removing all logging:
Proposed fix: retain error/warn logging
function log(
level: "info" | "warn" | "error",
- _message: string,
+ message: string,
data?: unknown,
) {
const timestamp = new Date().toISOString();
- const _prefix = `[${timestamp}] [terminal-host] [${level.toUpperCase()}]`;
- if (data !== undefined) {
- } else {
+ const prefix = `[${timestamp}] [terminal-host] [${level.toUpperCase()}]`;
+ if (level === "info") return; // suppress debug/info noise
+ if (data !== undefined) {
+ console.error(prefix, message, data);
+ } else {
+ console.error(prefix, message);
}
}As per coding guidelines: "Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions" and "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/terminal-host/index.ts` around lines 69 - 79, The log
function (log) was turned into a no-op and computes timestamp/_prefix but never
emits anything; restore operational logging by emitting at least warn and error
messages (and optionally info when needed) using console.warn/console.error (or
a provided logger) with the constructed prefix and timestamp, include the
message and serialized data (e.g., JSON.stringify(data, ...)) so exceptions,
auth failures, socket errors and shutdown events are visible, and keep the if
(data !== undefined) branch to append structured data while preserving the
original prefix format; do not swallow errors silently—ensure error-level logs
always run and include contextual information before any rethrow/handling.
| const server = notificationsApp.listen( | ||
| PORTS.NOTIFICATIONS, | ||
| "127.0.0.1", | ||
| () => { | ||
| console.log( | ||
| `[notifications] Listening on http://127.0.0.1:${PORTS.NOTIFICATIONS}`, | ||
| ); | ||
| }, | ||
| () => {}, | ||
| ); |
There was a problem hiding this comment.
Retain a startup log for the notification server listen callback.
The notification server binding to a port is a significant operational event. Replacing the callback with a no-op removes visibility into whether the server started successfully, which complicates debugging connectivity issues with notifications.
As per coding guidelines: "Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions."
Suggested change
const server = notificationsApp.listen(
PORTS.NOTIFICATIONS,
"127.0.0.1",
- () => {},
+ () => {
+ console.log(`[notifications] Server listening on 127.0.0.1:${PORTS.NOTIFICATIONS}`);
+ },
);📝 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.
| const server = notificationsApp.listen( | |
| PORTS.NOTIFICATIONS, | |
| "127.0.0.1", | |
| () => { | |
| console.log( | |
| `[notifications] Listening on http://127.0.0.1:${PORTS.NOTIFICATIONS}`, | |
| ); | |
| }, | |
| () => {}, | |
| ); | |
| const server = notificationsApp.listen( | |
| PORTS.NOTIFICATIONS, | |
| "127.0.0.1", | |
| () => { | |
| console.log(`[notifications] Server listening on 127.0.0.1:${PORTS.NOTIFICATIONS}`); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/windows/main.ts` around lines 136 - 140, The listen
callback for notificationsApp was replaced with a no-op, removing startup
visibility; update the notificationsApp.listen call (where server is assigned)
to provide a callback that logs startup success (including bound port and host)
using the prefixed context pattern like "[notifications/listen] ..." and also
handle/report errors if the listen API surfaces them; ensure you reference
PORTS.NOTIFICATIONS and the server variable so the log shows the actual
port/host and that the notification server started.
| console.log("Delete task:", task.id); | ||
| }} | ||
| > | ||
| <TaskContextMenu key={row.id} task={task} onDelete={() => {}}> |
There was a problem hiding this comment.
The "Delete" menu item is now a visible but non-functional control.
Replacing the debug log with a no-op is fine for the scope of this PR, but note that users can still click "Delete" in TaskContextMenu and nothing will happen — no deletion, no toast, no feedback. Consider either wiring up actual delete logic or hiding/disabling the menu item until it's implemented, to avoid a confusing UX.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.tsx`
at line 42, The TaskContextMenu is being rendered with a no-op delete handler
(TaskContextMenu key={row.id} task={task} onDelete={() => {}}) which leaves the
Delete menu item visible but non-functional; either wire this up to the real
deletion flow (call the existing delete function — e.g., deleteTask(task.id) or
dispatch the tasks store/action, show a confirmation modal and success/error
toast) or explicitly disable/hide the Delete item by passing a prop/flag or
omitting onDelete so the menu renders disabled until implemented; update the
TasksTableView to replace the empty onDelete handler with the proper delete call
or a disabling prop on TaskContextMenu.
| const proxyServer = serve( | ||
| { fetch: app.fetch, port: env.STREAMS_PORT }, | ||
| (info) => { | ||
| console.log(`[streams] Proxy running on http://localhost:${info.port}`); | ||
| }, | ||
| (_info) => {}, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Server startup should retain a prefixed log per coding guidelines.
The serve listener callback signals that the proxy is ready — this is a significant operational event, not debug noise. Silencing it removes the only confirmation that the streams proxy bound successfully.
Suggested fix
const proxyServer = serve(
{ fetch: app.fetch, port: env.STREAMS_PORT },
- (_info) => {},
+ (info) => {
+ console.log(`[streams/proxy] Server listening on port ${info.port}`);
+ },
);As per coding guidelines, "Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions".
📝 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.
| const proxyServer = serve( | |
| { fetch: app.fetch, port: env.STREAMS_PORT }, | |
| (info) => { | |
| console.log(`[streams] Proxy running on http://localhost:${info.port}`); | |
| }, | |
| (_info) => {}, | |
| ); | |
| const proxyServer = serve( | |
| { fetch: app.fetch, port: env.STREAMS_PORT }, | |
| (info) => { | |
| console.log(`[streams/proxy] Server listening on port ${info.port}`); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In `@apps/streams/src/index.ts` around lines 26 - 29, The serve callback currently
swallows the startup event for the streams proxy; update the listener passed to
serve (where proxyServer is created with serve({ fetch: app.fetch, port:
env.STREAMS_PORT }, (_info) => {})) to emit a prefixed console log indicating
the proxy is ready, e.g. use the required pattern "[streams/proxy] proxy
listening on port <port>" or similar, including env.STREAMS_PORT and any
relevant _info details; ensure this log is printed from the callback so the
bind/ready event is visible in runtime logs.
| } catch (_err) { | ||
| // Silently skip malformed chunks | ||
| } |
There was a problem hiding this comment.
Silently swallowing errors violates coding guidelines.
The catch block now discards errors with no logging at all. The coding guideline states: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly." Additionally, the guideline and retrieved learning both require prefixed console logging ([domain/operation] message) for error conditions.
Replacing console.debug with nothing goes too far — downgrade to console.warn or console.debug with proper context instead of removing it entirely.
Proposed fix
- } catch (_err) {
- // Silently skip malformed chunks
+ } catch (err) {
+ console.debug("[durable-session/materialize] Skipping malformed chunk", err);
}As per coding guidelines: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly" and "Use prefixed console logging with consistent context pattern: [domain/operation] message for … error conditions." Based on learnings: "Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions."
🤖 Prompt for AI Agents
In `@packages/durable-session/src/materialize.ts` around lines 77 - 79, The catch
currently swallows errors silently; change it to capture the error variable
(e.g., catch (err)) and log it with a prefixed context using console.warn or
console.debug instead of doing nothing — for example:
console.warn("[durable-session/materialize] Skipping malformed chunk:", err);
keep the existing behavior of skipping the chunk but ensure the error and
context are logged (update the catch in materialize.ts where the malformed chunk
is handled).
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit