refactor(logging): replace console.* with Winston logger across server files#268
refactor(logging): replace console.* with Winston logger across server files#268Muneerali199 wants to merge 2 commits into
Conversation
…r files
- src/utils/logger.ts: remove console.log/console.error monkey-patch and
dead _originalConsoleLog/_originalConsoleError variables; each server
module now imports logger explicitly, making log routing predictable
- src/server/websocket.ts: remove duplicate console.error on ws 'error'
event (logger.error was already called on same line); downgrade
high-frequency 'Upgrade request received' log from info to debug
- src/server/InputHandler.ts: import logger; replace 16 raw console.*
calls with logger.error/warn/debug as appropriate; fix mislabeled
error message in scroll throttle path ('move' -> 'scroll'); downgrade
operational key/combo trace logs to debug level
- src/server/ydotool.ts: import logger; replace 3 console.* calls;
console.warn was previously unpatched and silently lost to stderr
- src/server/tokenStore.ts: import logger; replace console.error
WalkthroughServer-side logging was migrated from direct Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InputHandler
participant YDotool
participant OS_Mouse
participant Logger
Client->>InputHandler: send move(delta)
InputHandler->>YDotool: attempt moveRelative(delta)
YDotool-->>InputHandler: success / failure
alt success
InputHandler->>Logger: debug move succeeded (ydotool)
else failure
InputHandler->>OS_Mouse: compute absolute from currentPos + delta and move
OS_Mouse-->>InputHandler: success / failure
InputHandler->>Logger: warn/error with context
end
InputHandler->>Logger: debug completion or throttled warning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/InputHandler.ts (1)
277-296:⚠️ Potential issue | 🟠 MajorStop logging raw key input and combo contents.
These debug/warn lines write actual keystrokes and shortcut contents into the server logs. Because single-character input is handled through
msg.key, a password or token entered over the remote-control channel will be reconstructed in the logs one event at a time. Log only redacted metadata here.As per coding guidelines, "No exposed API keys or sensitive data".🛡️ Proposed fix
- logger.debug(`Processing key: ${msg.key}`) + logger.debug("Processing key input") ... - logger.debug(`Unmapped key: ${msg.key}`) + logger.debug("Unmapped key received") ... - logger.warn(`Unknown key in combo: ${k}`) + logger.warn("Unknown key in combo") ... - logger.debug(`Pressing keys: ${nutKeys.join(", ")}`) + logger.debug(`Pressing combo with ${nutKeys.length} accepted keys`) ... - logger.debug(`Combo complete: ${msg.keys.join("+")}`) + logger.debug(`Combo complete (${nutKeys.length} accepted keys)`)Also applies to: 324-358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 277 - 296, Replace any logs that include raw keystroke or combo contents (e.g., the logger.debug calling `Processing key: ${msg.key}`, the `logger.debug('Unmapped key: ${msg.key}')` and the `logger.warn` that prints the error context including `msg.key`) so they never emit plaintext `msg.key` or reconstructed combos; instead log only non-sensitive metadata (e.g., event type, message length, redacted placeholder like "<REDACTED_KEY>", or a hashed/length indicator). Update the key-handling block around the `nutKey = KEY_MAP[msg.key.toLowerCase()]`, `keyboard.pressKey`, `keyboard.releaseKey`, and `keyboard.type(msg.key)` paths to remove direct `msg.key` interpolation in logs and apply the same redaction strategy to the similar logging in the subsequent combo handling section (the code referenced at lines ~324-358). Ensure error logs still capture useful context but never include the raw `msg.key` contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/ydotool.ts`:
- Around line 36-41: The warning currently flattens all errors into "ydotool not
available"; update the catch block that handles execFileAsync("which",
["ydotool"]) so it logs the actual failure reason by including the caught error
(err) in the logger.warn call (alongside the existing message), while still
setting isYdotoolAvailable = false and lastFailureTime = now; reference the
variables isYdotoolAvailable, lastFailureTime and the logger.warn invocation in
src/server/ydotool.ts to find and modify the catch body.
---
Outside diff comments:
In `@src/server/InputHandler.ts`:
- Around line 277-296: Replace any logs that include raw keystroke or combo
contents (e.g., the logger.debug calling `Processing key: ${msg.key}`, the
`logger.debug('Unmapped key: ${msg.key}')` and the `logger.warn` that prints the
error context including `msg.key`) so they never emit plaintext `msg.key` or
reconstructed combos; instead log only non-sensitive metadata (e.g., event type,
message length, redacted placeholder like "<REDACTED_KEY>", or a hashed/length
indicator). Update the key-handling block around the `nutKey =
KEY_MAP[msg.key.toLowerCase()]`, `keyboard.pressKey`, `keyboard.releaseKey`, and
`keyboard.type(msg.key)` paths to remove direct `msg.key` interpolation in logs
and apply the same redaction strategy to the similar logging in the subsequent
combo handling section (the code referenced at lines ~324-358). Ensure error
logs still capture useful context but never include the raw `msg.key` contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5bd1219-32ed-4b65-82c0-899f5d425df0
📒 Files selected for processing (5)
src/server/InputHandler.tssrc/server/tokenStore.tssrc/server/websocket.tssrc/server/ydotool.tssrc/utils/logger.ts
💤 Files with no reviewable changes (1)
- src/utils/logger.ts
…otool warn - InputHandler.ts: remove raw msg.key and combo key values from debug/warn log messages to prevent accidental keystroke capture (e.g. passwords typed over the remote channel); log only non-sensitive metadata instead - ydotool.ts: include caught error message in the 'ydotool not available' warning so the actual failure reason is visible in logs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/InputHandler.ts`:
- Around line 90-92: The logger.error calls in InputHandler (multiple locations
shown) and ydotool.ts are interpolating err.message into strings which strips
stack/structured info; update each occurrence (the logger.error(...) calls
around the pending move handling and the other listed line groups) to pass the
original Error object as the first argument (and any metadata as the second
argument, e.g., { context: "pendingMove" } or similar) so Winston can preserve
stack traces and structured error details; search for the pattern
logger.error(`Error...${err instanceof Error ? err.message : String(err)}`) and
replace it by passing err directly to logger.error(err, { /* metadata */ }) in
the relevant functions/methods in InputHandler and the error log in ydotool.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bcfd50fa-c087-4199-a512-58b348a40f7d
📒 Files selected for processing (2)
src/server/InputHandler.tssrc/server/ydotool.ts
| logger.error( | ||
| `Error processing pending move event: ${err instanceof Error ? err.message : String(err)}`, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
For Winston 3.x using format.errors({ stack: true }), what is the recommended way to log a caught Error while preserving its stack trace and metadata, instead of interpolating err.message into the log message string?
💡 Result:
With format.errors({ stack: true }) enabled, pass the actual Error instance to Winston, and put any extra context/metadata in the second argument (or as extra fields on an info object). The errors() formatter is designed to detect an Error and copy its .message (and .stack when stack: true) onto the log info. [1]
try {
// ...
} catch (err) {
logger.error(err, { requestId, userId, op: "doThing" }); // keeps stack + metadata
}Alternative (same idea, using an info object):
logger.error({ message: err, requestId, userId, op: "doThing" });Both patterns avoid interpolating err.message into a string and preserve the stack trace via format.errors({ stack: true }). [1][2]
Sources:
[1] logform errors format docs (allows passing an Error directly; adds stack when enabled) (github.com)
[2] Example usage showing logger.error(new Error(...), { ...meta }) / logger.error({ message: new Error(...), ... }) (app.unpkg.com)
Citations:
- 1: https://github.com/winstonjs/logform?utm_source=openai
- 2: https://app.unpkg.com/winston-graylog2%402.1.1/files/readme.md?utm_source=openai
🏁 Script executed:
# First, verify the files exist and check their sizes
wc -l src/server/InputHandler.ts src/utils/logger.ts src/server/ydotool.ts 2>/dev/null || echo "Some files may not exist"Repository: AOSSIE-Org/Rein
Length of output: 157
🏁 Script executed:
# Check the specific lines mentioned in InputHandler.ts
head -100 src/server/InputHandler.ts | tail -20Repository: AOSSIE-Org/Rein
Length of output: 649
🏁 Script executed:
# Check the logger configuration
cat -n src/utils/logger.ts | head -50Repository: AOSSIE-Org/Rein
Length of output: 1985
🏁 Script executed:
# Check all flagged lines in InputHandler.ts
cat -n src/server/InputHandler.ts | sed -n '88,114p'Repository: AOSSIE-Org/Rein
Length of output: 1108
🏁 Script executed:
# Check other flagged line ranges
cat -n src/server/InputHandler.ts | sed -n '145,150p;169,174p;183,188p;198,204p;238,244p;292,298p;346,352p;365,371p'Repository: AOSSIE-Org/Rein
Length of output: 2050
🏁 Script executed:
# Check ydotool.ts for similar error logging patterns
cat -n src/server/ydotool.tsRepository: AOSSIE-Org/Rein
Length of output: 2612
Pass Error objects directly to logger instead of interpolating error messages.
Interpolating err.message into template literals discards the original Error, preventing format.errors({ stack: true }) from preserving stack traces and structured error details. Pass the Error object directly; Winston will extract the message and stack automatically.
This applies to multiple locations: InputHandler.ts lines 90–92, 111–113, 147–149, 171–173, 185–187, 200–202, 240–242, 294–296, 348–350, 367–369, and ydotool.ts line 70–72.
Current pattern (incorrect):
logger.error(`Error: ${err instanceof Error ? err.message : String(err)}`)Correct pattern:
logger.error(err, { context: "additional metadata" })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/InputHandler.ts` around lines 90 - 92, The logger.error calls in
InputHandler (multiple locations shown) and ydotool.ts are interpolating
err.message into strings which strips stack/structured info; update each
occurrence (the logger.error(...) calls around the pending move handling and the
other listed line groups) to pass the original Error object as the first
argument (and any metadata as the second argument, e.g., { context:
"pendingMove" } or similar) so Winston can preserve stack traces and structured
error details; search for the pattern logger.error(`Error...${err instanceof
Error ? err.message : String(err)}`) and replace it by passing err directly to
logger.error(err, { /* metadata */ }) in the relevant functions/methods in
InputHandler and the error log in ydotool.ts.
Summary
Closes #213
All raw
console.*calls in server-side modules have been replaced with explicit Winston logger calls. The global monkey-patch inlogger.ts(which silently re-routedconsole.log/console.errorbut leftconsole.warn/console.infounpatched) has been removed in favour of each module importing the logger directly.Changes
src/utils/logger.tsconsole.log/console.errormonkey-patch and the dead_originalConsoleLog/_originalConsoleErrorvariables.src/server/websocket.tsconsole.error("WebSocket error:", error)on thews.on("error")handler;logger.errorwas already called on the same line."Upgrade request received"log frominfo→debugto reduce noise in production logs.src/server/InputHandler.tsloggerfrom../utils/logger.console.log/console.warn/console.errorcalls withlogger.debug/logger.warn/logger.errorrespectively."Error processing pending move event"→"Error processing pending scroll event".console.logtologger.debug.src/server/ydotool.tslogger.console.*calls. Notably, theconsole.warnfallback message was previously unpatched by the monkey-patch and silently dropped to native stderr — it now reaches the log file.src/server/tokenStore.tslogger.console.error("Failed to persist tokens:", e)withlogger.error(...).What was NOT changed
electron/main.cjs— separate OS process with no Winston access; itsconsole.logcalls are intentional.src/hooks/,src/routes/) — browser context, out of scope.test-input.ts— standalone test script, out of scope.Summary by CodeRabbit
Bug Fixes
Chores