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:
📝 WalkthroughWalkthroughA new server-side logging utility is introduced and integrated across input handling and WebSocket components. The InputMessage interface is expanded with additional fields to support richer message types. Error handling is enhanced with try-catch wrapping and comprehensive debug logging throughout input actions and connection events. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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: 3
🧹 Nitpick comments (5)
src/server/logger.ts (4)
56-58: Log path is resolved at module-load time, beforeinit()is called.The constructor (Line 57) calls
resolveLogPath(), and the singleton is instantiated at import time (Line 186). This meansLOG_FILE_PATH,LOG_DIR, and other environment variables must be set before the module is first imported. This is a subtle ordering constraint that isn't documented.If
resolveLogPath()were called insideinit()instead, it would give callers a chance to set environment variables after import but before initialization.Proposed fix: defer path resolution to init()
class Logger { - private logPath: string; + private logPath: string = ''; private stream: fs.WriteStream | null = null; private originals: Partial<typeof console> = {}; - constructor() { - this.logPath = resolveLogPath(); - } + constructor() {} init(): void { + this.logPath = resolveLogPath(); fs.mkdirSync(path.dirname(this.logPath), { recursive: true });Also applies to: 186-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/logger.ts` around lines 56 - 58, The Logger currently resolves log paths in its constructor by calling resolveLogPath(), causing the singleton to capture LOG_FILE_PATH/LOG_DIR at import time; move that resolution into the init() method so environment variables can be set before initialization: remove the resolveLogPath() call from the constructor, add this.logPath = resolveLogPath() at the start of init(), and ensure any code paths that might use this.logPath before init() either handle undefined or throw a clear error; update references to the Logger singleton creation so it remains safe to import without needing env vars already set.
41-48: Rotation silently overwrites the oldest file without deleting it first.On Line 44, when renaming
.4→.5, if.5already exists,fs.renameSyncwill overwrite it on POSIX but may fail on some Windows configurations where the target file is locked. Since this is all wrapped in try/catch with best-effort semantics it won't crash, but the rotation could silently stop working on Windows, leaving only.1ever being rotated.Consider explicitly unlinking the oldest file before rotating:
Proposed fix
+ // Remove the oldest rotated file to make room + const oldest = `${logPath}.${MAX_ROTATED}`; + try { if (fs.existsSync(oldest)) fs.unlinkSync(oldest); } catch { /* best-effort */ } + for (let i = MAX_ROTATED - 1; i >= 1; i--) { const src = `${logPath}.${i}`; const dest = `${logPath}.${i + 1}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/logger.ts` around lines 41 - 48, Rotation can silently fail on Windows when renaming into an existing target (e.g., renaming `${logPath}.${i}` → `${logPath}.${i+1}`) because fs.renameSync may error if the destination is locked; modify the rotation loop in the code that uses MAX_ROTATED and logPath so that before renaming into a destination you attempt to remove/unlink the destination (e.g., try unlinkSync(dest) wrapped in its own try/catch) to ensure rename succeeds, and apply the same unlink-before-rename step to the final fs.renameSync(logPath, `${logPath}.1`) operation, keeping the operations best-effort (swallowing unlink errors) so behavior on POSIX is unchanged but Windows locked-file cases are handled.
86-95:write()method is fire-and-forget — no backpressure handling.
this.stream.write(...)returnsfalsewhen the internal buffer is full, signaling the caller to wait for'drain'. In the current implementation this is ignored. For a logging system driven by high-frequency input events (every click, zoom, key press), this could lead to unbounded memory growth if the filesystem I/O is slow (e.g., network-mounted home directory, slow disk).This isn't urgent for a local development tool, but worth being aware of — especially since the
InputHandlerlogs are quite chatty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/logger.ts` around lines 86 - 95, The write method currently ignores backpressure from this.stream.write; change write (or add a new async helper) to check the boolean return of this.stream.write(...) and, when it returns false, wait for the stream's 'drain' event (or reject if the stream is destroyed/errored) before returning so logs don't accumulate in memory; update the method signature (e.g., make write async or return Promise<void>) and ensure callers (or internal logger queue) await this helper, and guard against multiple concurrent waiters by using a single promise-per-drain resolver tied to this.stream.once('drain') and this.stream.once('error') to handle edge cases.
120-139: Console patching approach is fragile and has re-entrancy risk.The monkey-patching approach has a few concerns:
Stacking: If
init()is called twice (no guard exists — flagged in websocket.ts review), the "original" saved on the second call is the already-patched function, creating infinite recursion whenrestoreConsoletries to revert.Third-party interference: Any other library that patches
consoleafter this logger (e.g., a test framework, debug utility) will have its patches silently overwritten byrestoreConsole().
as constnarrowing + loop: The type assertionself.originals[name] as (...a: unknown[]) => voidon Line 137 discards type safety. This is acceptable given the dynamic nature of the patching, but aninitializedguard (suggested in the websocket.ts comment) would make this much safer overall.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/logger.ts` around lines 120 - 139, The console patching in patchConsole() is fragile and can cause recursion and clobber third-party patches; add an initialized guard (e.g., a private boolean initialized checked/set in init() and used in patchConsole()/restoreConsole()) so patchConsole() runs only once, only save originals[name] if it is not already set, and have restoreConsole() restore only the entries that this logger actually replaced (clearing the initialized flag afterwards); keep using the originals map and functions (originals, patchConsole, restoreConsole, init) but ensure you never overwrite an existing original or reapply patches when initialized is true to avoid stacking and re-entrancy issues.src/server/InputHandler.ts (1)
38-41: Input event logging is verbose and usesconsole.loginstead ofconsole.debug.Every mouse click, zoom gesture, key combo, and text input triggers a log write. On an active session this will rapidly fill the 10 MB rotation window, evicting more valuable diagnostic entries (errors, connection events). Since the PR's future scope mentions
LOG_LEVELfiltering but it isn't implemented yet, all of these will always be written.Consider using
console.debugfor these high-frequency input traces so they can be filtered out once level-based filtering is added, without requiring another code change.Example: switch input traces to debug level
- console.log(`Mouse press: ${msg.button}`); + console.debug(`Mouse press: ${msg.button}`);- console.log(`Mouse release: ${msg.button}`); + console.debug(`Mouse release: ${msg.button}`);- console.log(`Zoom: delta=${msg.delta} scaled=${scaledDelta}`); + console.debug(`Zoom: delta=${msg.delta} scaled=${scaledDelta}`);- console.log(`Pressing combo: ${msg.keys.join('+')}`); + console.debug(`Pressing combo: ${msg.keys.join('+')}`);- console.log(`Typing text (${msg.text.length} chars)`); + console.debug(`Typing text (${msg.text.length} chars)`);Also applies to: 84-84, 129-129, 155-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 38 - 41, Replace high-frequency console.log calls in InputHandler with console.debug: change the mouse press/release logs surrounding mouse.pressButton(btn) and mouse.releaseButton, and the other input traces at the locations handling zoom gestures, key combinations, and text input (the log statements referencing msg.button and similar input messages around lines shown in the diff). Use console.debug for these traces so they are filtered by log level later; keep the same message text and variable interpolation (e.g., `${msg.button}`) but swap console.log -> console.debug everywhere those input-event logs occur.
🤖 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/logger.ts`:
- Around line 147-167: The handlers in hookProcessEvents currently call
this.close() and then immediately call process.exit(), which can terminate the
process before the logger stream has flushed; fix by making this.close() return
a Promise (e.g., closeAsync or modify close to return a Promise that resolves
when stream 'finish' or 'error' fires or immediately if no stream) and change
the uncaughtException handler and shutdown function to await that Promise (or
use this.stream.once('finish', () => process.exit(...))) before calling
process.exit; also remove or change process.once('exit', () => this.close())
because exit handlers can’t perform async I/O — either perform a synchronous
flush using the underlying fd with fs.fsyncSync when available or simply omit
async close on 'exit'.
In `@src/server/websocket.ts`:
- Around line 44-46: The code currently logs raw client IPs in the
wss.on('connection') handler by reading request.socket.remoteAddress into
clientIp and outputting it via console.log; change this to mask/anonymize the IP
before logging (e.g., for IPv4 replace the last octet with 'x' and for IPv6 zero
out/mask the trailing segments), keep the existing 'unknown' fallback, and
update the log call to use the masked value instead of clientIp; also add a
short comment near the wss.on('connection') block referencing the applicable
privacy/retention policy so reviewers know this logging is intentionally
anonymized.
- Around line 23-25: The logger is being initialized inside createWsServer
causing late capture of console output and risking double-patching on repeated
calls; move the call to logger.init() out of createWsServer to the application
entry point (e.g., main/bootstrap) so console is captured early, and make
Logger.init() idempotent by adding an internal initialized guard (e.g., a
private boolean like this.initialized) that returns early if already set before
patching console methods; ensure logger.getLogPath() remains available for
logging but remove console.log from createWsServer to avoid re-init side
effects.
---
Nitpick comments:
In `@src/server/InputHandler.ts`:
- Around line 38-41: Replace high-frequency console.log calls in InputHandler
with console.debug: change the mouse press/release logs surrounding
mouse.pressButton(btn) and mouse.releaseButton, and the other input traces at
the locations handling zoom gestures, key combinations, and text input (the log
statements referencing msg.button and similar input messages around lines shown
in the diff). Use console.debug for these traces so they are filtered by log
level later; keep the same message text and variable interpolation (e.g.,
`${msg.button}`) but swap console.log -> console.debug everywhere those
input-event logs occur.
In `@src/server/logger.ts`:
- Around line 56-58: The Logger currently resolves log paths in its constructor
by calling resolveLogPath(), causing the singleton to capture
LOG_FILE_PATH/LOG_DIR at import time; move that resolution into the init()
method so environment variables can be set before initialization: remove the
resolveLogPath() call from the constructor, add this.logPath = resolveLogPath()
at the start of init(), and ensure any code paths that might use this.logPath
before init() either handle undefined or throw a clear error; update references
to the Logger singleton creation so it remains safe to import without needing
env vars already set.
- Around line 41-48: Rotation can silently fail on Windows when renaming into an
existing target (e.g., renaming `${logPath}.${i}` → `${logPath}.${i+1}`) because
fs.renameSync may error if the destination is locked; modify the rotation loop
in the code that uses MAX_ROTATED and logPath so that before renaming into a
destination you attempt to remove/unlink the destination (e.g., try
unlinkSync(dest) wrapped in its own try/catch) to ensure rename succeeds, and
apply the same unlink-before-rename step to the final fs.renameSync(logPath,
`${logPath}.1`) operation, keeping the operations best-effort (swallowing unlink
errors) so behavior on POSIX is unchanged but Windows locked-file cases are
handled.
- Around line 86-95: The write method currently ignores backpressure from
this.stream.write; change write (or add a new async helper) to check the boolean
return of this.stream.write(...) and, when it returns false, wait for the
stream's 'drain' event (or reject if the stream is destroyed/errored) before
returning so logs don't accumulate in memory; update the method signature (e.g.,
make write async or return Promise<void>) and ensure callers (or internal logger
queue) await this helper, and guard against multiple concurrent waiters by using
a single promise-per-drain resolver tied to this.stream.once('drain') and
this.stream.once('error') to handle edge cases.
- Around line 120-139: The console patching in patchConsole() is fragile and can
cause recursion and clobber third-party patches; add an initialized guard (e.g.,
a private boolean initialized checked/set in init() and used in
patchConsole()/restoreConsole()) so patchConsole() runs only once, only save
originals[name] if it is not already set, and have restoreConsole() restore only
the entries that this logger actually replaced (clearing the initialized flag
afterwards); keep using the originals map and functions (originals,
patchConsole, restoreConsole, init) but ensure you never overwrite an existing
original or reapply patches when initialized is true to avoid stacking and
re-entrancy issues.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/server/logger.ts (1)
96-98: Consider writing ERROR (and optionally WARN) toprocess.stderrinstead ofprocess.stdout.Standard convention directs error-level output to stderr so that operators can separate normal output from error output when piping/redirecting (e.g.,
node app 2>errors.log). Currently both WARN and ERROR go to stdout.Proposed diff
if (LEVELS[level] >= this.stdoutLevel) { - process.stdout.write(line); + const out = LEVELS[level] >= LEVELS.ERROR ? process.stderr : process.stdout; + out.write(line); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/logger.ts` around lines 96 - 98, The logger currently sends all output matching LEVELS[level] >= this.stdoutLevel to process.stdout.write; change this so error-level (and optionally warn-level) messages go to process.stderr.write instead. In the logging function (use the existing variables LEVELS and level), branch: if level === 'error' (and if you want warn also: level === 'warn') call process.stderr.write(line) else keep process.stdout.write(line); keep the existing stdoutLevel check but ensure error/warn bypass or are routed to stderr appropriately.
🤖 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/logger.ts`:
- Around line 62-77: The init() method currently sets this.initialized = true
before performing critical setup (fs.mkdirSync, maybeRotate,
fs.createWriteStream), so any thrown error leaves the logger permanently marked
initialized; update init() to only set this.initialized after the critical
operations succeed (i.e., after creating this.stream and completing
writeSessionHeader()/hookProcessEvents()), or alternatively wrap the setup in a
try/catch and reset this.initialized = false in the catch and rethrow/log the
error; refer to init(), this.initialized, fs.mkdirSync, maybeRotate,
fs.createWriteStream, this.stream, writeSessionHeader, and hookProcessEvents
when making the change.
- Around line 124-148: The bug is duplicate process event handlers because
close() resets this.initialized and hookProcessEvents() re-registers handlers on
every init(); fix by tracking hook registration separately: add a private flag
(e.g., this.processHooksInstalled) and in hookProcessEvents() return immediately
if it's already true, set it to true after successful registration, and do not
reset that flag in close(); keep existing process.once SIG handlers as-is but
ensure uncaughtException/unhandledRejection are only registered once via this
new guard (refer to close(), init(), and hookProcessEvents()).
In `@src/server/websocket.ts`:
- Around line 21-28: maskIp currently fails to recognize IPv4-mapped IPv6
addresses like "::ffff:192.168.1.42" and returns "unknown"; update maskIp to
first check for the IPv4-mapped prefix (e.g., detect
/^::ffff:(\d+\.\d+\.\d+\.\d+)$/i or a variant that allows optional IPv6 zone/id)
and when matched return a masked IPv4 form (same mask style as the v4 branch),
then fall back to the existing v4 and v6 regexes (preserving the v4 branch using
v4[1] and the v6 branch using v6[1]) so that ::ffff:IPv4 is handled correctly by
the maskIp function.
---
Nitpick comments:
In `@src/server/logger.ts`:
- Around line 96-98: The logger currently sends all output matching
LEVELS[level] >= this.stdoutLevel to process.stdout.write; change this so
error-level (and optionally warn-level) messages go to process.stderr.write
instead. In the logging function (use the existing variables LEVELS and level),
branch: if level === 'error' (and if you want warn also: level === 'warn') call
process.stderr.write(line) else keep process.stdout.write(line); keep the
existing stdoutLevel check but ensure error/warn bypass or are routed to stderr
appropriately.
| init(): void { | ||
| if (this.initialized) return; | ||
| this.initialized = true; | ||
|
|
||
| fs.mkdirSync(path.dirname(this.logPath), { recursive: true }); | ||
| maybeRotate(this.logPath); | ||
|
|
||
| this.stream = fs.createWriteStream(this.logPath, { flags: 'a', encoding: 'utf-8' }); | ||
| this.stream.on('error', (err) => { | ||
| process.stderr.write(`[rein-logger] stream error: ${err.message}\n`); | ||
| }); | ||
|
|
||
| this.writeSessionHeader(); | ||
| this.hookProcessEvents(); | ||
| this.info(`Log file: ${this.logPath}`); | ||
| } |
There was a problem hiding this comment.
initialized flag set before stream creation — logger permanently broken if setup throws.
If mkdirSync (line 66) or createWriteStream (line 69) throws (e.g., permission denied), initialized is already true so all future init() calls are no-ops, leaving the logger in a silently broken state with no file output and no way to recover.
Move the flag after the critical setup, or reset it on failure.
Proposed fix
init(): void {
if (this.initialized) return;
- this.initialized = true;
- fs.mkdirSync(path.dirname(this.logPath), { recursive: true });
- maybeRotate(this.logPath);
+ try {
+ fs.mkdirSync(path.dirname(this.logPath), { recursive: true });
+ maybeRotate(this.logPath);
- this.stream = fs.createWriteStream(this.logPath, { flags: 'a', encoding: 'utf-8' });
- this.stream.on('error', (err) => {
- process.stderr.write(`[rein-logger] stream error: ${err.message}\n`);
- });
+ this.stream = fs.createWriteStream(this.logPath, { flags: 'a', encoding: 'utf-8' });
+ this.stream.on('error', (err) => {
+ process.stderr.write(`[rein-logger] stream error: ${err.message}\n`);
+ });
+ } catch (err) {
+ process.stderr.write(`[rein-logger] init failed: ${err}\n`);
+ return;
+ }
+ this.initialized = true;
this.writeSessionHeader();
this.hookProcessEvents();
this.info(`Log file: ${this.logPath}`);
}📝 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.
| init(): void { | |
| if (this.initialized) return; | |
| this.initialized = true; | |
| fs.mkdirSync(path.dirname(this.logPath), { recursive: true }); | |
| maybeRotate(this.logPath); | |
| this.stream = fs.createWriteStream(this.logPath, { flags: 'a', encoding: 'utf-8' }); | |
| this.stream.on('error', (err) => { | |
| process.stderr.write(`[rein-logger] stream error: ${err.message}\n`); | |
| }); | |
| this.writeSessionHeader(); | |
| this.hookProcessEvents(); | |
| this.info(`Log file: ${this.logPath}`); | |
| } | |
| init(): void { | |
| if (this.initialized) return; | |
| try { | |
| fs.mkdirSync(path.dirname(this.logPath), { recursive: true }); | |
| maybeRotate(this.logPath); | |
| this.stream = fs.createWriteStream(this.logPath, { flags: 'a', encoding: 'utf-8' }); | |
| this.stream.on('error', (err) => { | |
| process.stderr.write(`[rein-logger] stream error: ${err.message}\n`); | |
| }); | |
| } catch (err) { | |
| process.stderr.write(`[rein-logger] init failed: ${err}\n`); | |
| return; | |
| } | |
| this.initialized = true; | |
| this.writeSessionHeader(); | |
| this.hookProcessEvents(); | |
| this.info(`Log file: ${this.logPath}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/logger.ts` around lines 62 - 77, The init() method currently sets
this.initialized = true before performing critical setup (fs.mkdirSync,
maybeRotate, fs.createWriteStream), so any thrown error leaves the logger
permanently marked initialized; update init() to only set this.initialized after
the critical operations succeed (i.e., after creating this.stream and completing
writeSessionHeader()/hookProcessEvents()), or alternatively wrap the setup in a
try/catch and reset this.initialized = false in the catch and rethrow/log the
error; refer to init(), this.initialized, fs.mkdirSync, maybeRotate,
fs.createWriteStream, this.stream, writeSessionHeader, and hookProcessEvents
when making the change.
| close(): void { | ||
| if (!this.stream) return; | ||
| this.writeSessionFooterSync(); | ||
| try { this.stream.destroy(); } catch { /* */ } | ||
| this.stream = null; | ||
| this.initialized = false; | ||
| } | ||
|
|
||
| private hookProcessEvents(): void { | ||
| process.on('uncaughtException', (err) => { | ||
| this.writeSync(`[${new Date().toISOString()}] [ERROR] [UncaughtException] ${err?.stack ?? err}\n`); | ||
| this.close(); | ||
| process.exit(1); | ||
| }); | ||
|
|
||
| process.on('unhandledRejection', (reason) => { | ||
| const msg = reason instanceof Error ? reason.stack : safeStringify(reason); | ||
| this.write('ERROR', [`[UnhandledRejection] ${msg}`]); | ||
| }); | ||
|
|
||
| const shutdown = () => { this.close(); process.exit(0); }; | ||
| process.once('SIGINT', shutdown); | ||
| process.once('SIGTERM', shutdown); | ||
| process.once('exit', () => this.close()); | ||
| } |
There was a problem hiding this comment.
close() resets initialized, allowing init() to re-register process.on handlers — handlers accumulate on each cycle.
close() sets this.initialized = false (line 129), so a subsequent init() call re-enters hookProcessEvents(). Because process.on('uncaughtException', ...) and process.on('unhandledRejection', ...) are used (not once), every close() → init() cycle adds another handler, producing duplicate log entries and duplicate process.exit() calls.
Proposed fix — track process hooks separately
class Logger {
private logPath: string;
private stream: fs.WriteStream | null = null;
private initialized = false;
+ private hooked = false;
private fileLevel = LEVELS.DEBUG;
private stdoutLevel = LEVELS.WARN; private hookProcessEvents(): void {
+ if (this.hooked) return;
+ this.hooked = true;
+
process.on('uncaughtException', (err) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/logger.ts` around lines 124 - 148, The bug is duplicate process
event handlers because close() resets this.initialized and hookProcessEvents()
re-registers handlers on every init(); fix by tracking hook registration
separately: add a private flag (e.g., this.processHooksInstalled) and in
hookProcessEvents() return immediately if it's already true, set it to true
after successful registration, and do not reset that flag in close(); keep
existing process.once SIG handlers as-is but ensure
uncaughtException/unhandledRejection are only registered once via this new guard
(refer to close(), init(), and hookProcessEvents()).
src/server/websocket.ts
Outdated
| function maskIp(ip: string | undefined): string { | ||
| if (!ip) return 'unknown'; | ||
| const v4 = ip.match(/^(\d+\.\d+\.\d+\.)\d+$/); | ||
| if (v4) return `${v4[1]}x`; | ||
| const v6 = ip.match(/^([0-9a-f]{0,4}:[0-9a-f]{0,4}:[0-9a-f]{0,4}:[0-9a-f]{0,4}):/i); | ||
| if (v6) return `${v6[1]}:x:x:x:x`; | ||
| return 'unknown'; | ||
| } |
There was a problem hiding this comment.
maskIp misses IPv4-mapped IPv6 addresses (::ffff:x.x.x.x), the most common format from Node.js remoteAddress on dual-stack servers.
On a typical dual-stack listener, request.socket.remoteAddress returns ::ffff:192.168.1.42 for IPv4 clients. Neither the v4 regex (expects a pure dotted-quad) nor the v6 regex (the trailing dotted portion breaks the colon pattern) matches this, so the function falls through to 'unknown'.
Proposed fix — handle the IPv4-mapped prefix
function maskIp(ip: string | undefined): string {
if (!ip) return 'unknown';
+ // Strip IPv4-mapped IPv6 prefix before matching
+ const raw = ip.startsWith('::ffff:') ? ip.slice(7) : ip;
- const v4 = ip.match(/^(\d+\.\d+\.\d+\.)\d+$/);
+ const v4 = raw.match(/^(\d+\.\d+\.\d+\.)\d+$/);
if (v4) return `${v4[1]}x`;
- const v6 = ip.match(/^([0-9a-f]{0,4}:[0-9a-f]{0,4}:[0-9a-f]{0,4}:[0-9a-f]{0,4}):/i);
+ const v6 = raw.match(/^([0-9a-f]{0,4}:[0-9a-f]{0,4}:[0-9a-f]{0,4}:[0-9a-f]{0,4}):/i);
if (v6) return `${v6[1]}:x:x:x:x`;
return 'unknown';
}📝 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 maskIp(ip: string | undefined): string { | |
| if (!ip) return 'unknown'; | |
| const v4 = ip.match(/^(\d+\.\d+\.\d+\.)\d+$/); | |
| if (v4) return `${v4[1]}x`; | |
| const v6 = ip.match(/^([0-9a-f]{0,4}:[0-9a-f]{0,4}:[0-9a-f]{0,4}:[0-9a-f]{0,4}):/i); | |
| if (v6) return `${v6[1]}:x:x:x:x`; | |
| return 'unknown'; | |
| } | |
| function maskIp(ip: string | undefined): string { | |
| if (!ip) return 'unknown'; | |
| // Strip IPv4-mapped IPv6 prefix before matching | |
| const raw = ip.startsWith('::ffff:') ? ip.slice(7) : ip; | |
| const v4 = raw.match(/^(\d+\.\d+\.\d+\.)\d+$/); | |
| if (v4) return `${v4[1]}x`; | |
| const v6 = raw.match(/^([0-9a-f]{0,4}:[0-9a-f]{0,4}:[0-9a-f]{0,4}:[0-9a-f]{0,4}):/i); | |
| if (v6) return `${v6[1]}:x:x:x:x`; | |
| return 'unknown'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/websocket.ts` around lines 21 - 28, maskIp currently fails to
recognize IPv4-mapped IPv6 addresses like "::ffff:192.168.1.42" and returns
"unknown"; update maskIp to first check for the IPv4-mapped prefix (e.g., detect
/^::ffff:(\d+\.\d+\.\d+\.\d+)$/i or a variant that allows optional IPv6 zone/id)
and when matched return a masked IPv4 form (same mask style as the v4 branch),
then fall back to the existing v4 and v6 regexes (preserving the v4 branch using
v4[1] and the v6 branch using v6[1]) so that ::ffff:IPv4 is handled correctly by
the maskIp function.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/server/logger.ts (1)
9-14:LEVELSmap is defined but never used — no log-level filtering.The
LEVELSrecord and its numeric values are dead code. Thewrite()method writes every message regardless of level, soDEBUGnoise cannot be suppressed. The PR objectives mention futureLOG_LEVELfiltering, but the plumbing (comparingLEVELS[level]against a threshold) is missing, making the constant misleading.Consider either wiring up filtering now or removing
LEVELSto avoid confusion.Also applies to: 58-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/logger.ts` around lines 9 - 14, The LEVELS map is unused so either remove it or wire it into the logger: add a module-level threshold (e.g. derive currentThreshold from process.env.LOG_LEVEL with a default like "INFO") and in the Logger.write() method compare LEVELS[level] against LEVELS[currentThreshold], only formatting/emitting the message when LEVELS[level] >= LEVELS[currentThreshold]; use the existing LogLevel type and LEVELS constant and ensure invalid/unknown env values fall back to the default.
🤖 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 73-93: The zoom handler currently computes scaledDelta/amount and
always calls mouse.scrollDown(amount), which can pass negative values to
mouse.scrollDown(); update the 'zoom' case in InputHandler (variables:
msg.delta, sensitivityFactor, MAX_ZOOM_STEP, scaledDelta, amount) to mirror the
scroll case: compute scaledDelta and amount, then if amount > 0 call await
mouse.scrollDown(amount) else call await mouse.scrollUp(Math.abs(amount)); keep
the LeftControl press/release (keyboard.pressKey(Key.LeftControl) /
keyboard.releaseKey(Key.LeftControl)) and the debug log intact, and ensure you
await the correct scroll direction to avoid passing negative values to
mouse.scrollDown().
In `@src/server/logger.ts`:
- Around line 69-74: The writeSync method currently reads an undocumented/async
fd from this.stream and swallows errors; instead open the file descriptor
synchronously when initializing the logger and store it (e.g., loggerFd) to use
for fs.writeSync, or if you only have a path use fs.appendFileSync(path, text);
update writeSync to use the stored fd or path-based append, remove the silent
catch so errors surface (or at least log them), and ensure the stored fd is
closed in the logger's close/destroy logic; reference the writeSync method and
the stream initialisation logic to implement this change.
- Around line 93-99: The close() method currently calls this.stream.destroy()
which can drop buffered async writes; change it to call
this.stream.end(callback) to flush pending writes (invoke writeSessionFooterSync
before ending), and implement a timeout fallback that forces destroy() if end's
callback hasn't fired within e.g. 2–5s; ensure you clear the timeout in the end
callback, handle errors passed to the callback by logging via processLogger or
this.logger, and then set this.stream = null and this.initialized = false (refer
to close(), writeSessionFooterSync(), and this.stream).
In `@src/server/websocket.ts`:
- Around line 30-36: The upgrade handler currently compares request.url directly
to '/ws', which fails when query strings are present; update the
server.on('upgrade' ...) callback to parse the request URL (e.g., using new
URL(request.url!, `http://${request.headers.host}`) or URL.parse) and compare
the pathname instead of request.url, then call wss.handleUpgrade(request,
socket, head, (ws) => { wss.emit('connection', ws, request); }) as before;
ensure you reference request.url, request.headers.host, the server.on('upgrade'
handler, and wss.handleUpgrade when making the change.
- Around line 58-63: The code uses a fragile relative path string configPath
('./src/server-config.json') for reading/writing config; change it to resolve
against the module directory so it is stable regardless of CWD (e.g., build a
resolved path using path.resolve(__dirname, ...) or path.join(__dirname, ...)
and use that resolved path in the fs.existsSync, fs.readFileSync, and
fs.writeFileSync calls), and add the missing import for path (import path from
'path') if not present; update references to configPath in this block
accordingly.
---
Duplicate comments:
In `@src/server/logger.ts`:
- Around line 37-49: The init() method sets this.initialized = true before
filesystem ops, which leaves the logger permanently marked initialized if
mkdirSync or createWriteStream throws; change init() so the initialized flag is
only set after successful creation of the directory and stream (and after
writeSessionHeader() and hookProcessEvents() succeed), and wrap the fs.mkdirSync
and fs.createWriteStream steps in a try/catch that closes/cleans any
partially-created this.stream and rethrows or logs the error so subsequent calls
to init() can retry; update references to init(), initialized, this.stream,
writeSessionHeader(), and hookProcessEvents() accordingly.
- Around line 101-117: hookProcessEvents currently re-registers persistent
handlers on each init() because close() resets initialized; to fix, ensure
handlers aren't added multiple times by either registering them once or removing
existing listeners before adding: in hookProcessEvents (and/or in close())
remove or off the existing handlers for 'uncaughtException' and
'unhandledRejection' (and the shutdown handlers if needed) using the same
callback references, or convert to process.once if you want a single-use
behavior; update init()/close() so that hookProcessEvents only attaches handlers
when they are not already present and keep the handler functions as
named/identifiable variables inside the class so you can remove them reliably.
In `@src/server/websocket.ts`:
- Around line 39-41: The code logs raw client IPs; use the existing maskIp
helper to avoid storing full addresses: import or reference maskIp, compute
const rawIp = request.socket.remoteAddress ?? 'unknown'; const clientIp =
maskIp(rawIp); then replace both console.log and logger.info calls to use the
masked clientIp (and remove any other direct uses of
request.socket.remoteAddress), ensuring the variable names clientIp and rawIp
and the maskIp function are used so reviewers can locate the change.
---
Nitpick comments:
In `@src/server/logger.ts`:
- Around line 9-14: The LEVELS map is unused so either remove it or wire it into
the logger: add a module-level threshold (e.g. derive currentThreshold from
process.env.LOG_LEVEL with a default like "INFO") and in the Logger.write()
method compare LEVELS[level] against LEVELS[currentThreshold], only
formatting/emitting the message when LEVELS[level] >= LEVELS[currentThreshold];
use the existing LogLevel type and LEVELS constant and ensure invalid/unknown
env values fall back to the default.
| case 'zoom': | ||
| if (msg.delta !== undefined && msg.delta !== 0) { | ||
| const sensitivityFactor = 0.5; | ||
| const MAX_ZOOM_STEP = 5; | ||
|
|
||
| const scaledDelta = | ||
| Math.sign(msg.delta) * | ||
| Math.min(Math.abs(msg.delta) * sensitivityFactor, MAX_ZOOM_STEP); | ||
|
|
||
| console.log(`Pressing keys:`, nutKeys); | ||
| const pressedKeys: Key[] = []; | ||
| const amount = -scaledDelta; | ||
|
|
||
| try { | ||
| for (const k of nutKeys) { | ||
| if (typeof k === "string") { | ||
| await keyboard.type(k); | ||
| logger.debug(`Zoom: delta=${msg.delta} scaled=${scaledDelta}`); | ||
|
|
||
| await keyboard.pressKey(Key.LeftControl); | ||
| try { | ||
| await mouse.scrollDown(amount); | ||
| } finally { | ||
| await keyboard.releaseKey(Key.LeftControl); | ||
| } | ||
| } | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the InputHandler.ts file and examine the exact code
find . -name "InputHandler.ts" -type fRepository: AOSSIE-Org/Rein
Length of output: 87
🏁 Script executed:
# Search for nut-js mouse scroll method usage in the codebase
rg -n "scrollDown|scrollUp" --type ts -B 2 -A 2Repository: AOSSIE-Org/Rein
Length of output: 1015
🏁 Script executed:
# Check for nut-js imports and configuration
rg -n "nut-js|from.*mouse" --type ts -B 1 -A 1 | head -50Repository: AOSSIE-Org/Rein
Length of output: 647
🏁 Script executed:
# Look for package.json to see which version of nut-js is being used
cat package.json 2>/dev/null | grep -A 2 -B 2 "nut-js"Repository: AOSSIE-Org/Rein
Length of output: 203
🏁 Script executed:
# Read the full InputHandler.ts file around the zoom section to see complete context
cat -n src/server/InputHandler.ts | sed -n '70,100p'Repository: AOSSIE-Org/Rein
Length of output: 1513
🏁 Script executed:
# Also check the scroll section to confirm the pattern
cat -n src/server/InputHandler.ts | sed -n '50,65p'Repository: AOSSIE-Org/Rein
Length of output: 871
🌐 Web query:
@nut-tree-fork/nut-js scrollDown negative value behavior documentation
💡 Result:
@nut-tree-fork/nut-js (same API surface as upstream nut.js) does not document any special/defined behavior for passing a negative value to mouse.scrollDown(amount).
What the docs do specify:
scrollDown(x)“scrollsxticks downwards” (OS-dependent distance). No constraints or negative-value semantics are mentioned. [1]- The API provides separate methods for opposite directions:
scrollDown(amount)andscrollUp(amount). [1] - The type signature is just
amount: number(no “must be positive” note, and no “negative reverses direction” note). [2] - At the provider-interface level, the method is simply
scrollDown(amount): Promise<void>—again, no negative semantics described. [3]
Practical takeaway: treat negative inputs as undefined / provider-dependent (might do nothing, throw, or scroll the opposite direction depending on the underlying provider/OS). For reliable “scroll up”, call mouse.scrollUp(Math.abs(x)) instead of mouse.scrollDown(-x). [1][2]
Sources:
[1] nut.js “Mouse Control” docs (scrollDown/scrollUp) (nutjs.dev)
[2] TypeDoc for MouseClass.scrollDown(amount: number) (nut-tree.github.io)
[3] TypeDoc for MouseProviderInterface.scrollDown(amount) (nut-tree.github.io)
Citations:
- 1: https://nutjs.dev/docs/apidoc/mouse
- 2: https://nut-tree.github.io/apidoc/classes/_nut_tree_nut_js.MouseClass.html
- 3: https://nut-tree.github.io/apidoc/interfaces/_nut_tree_nut_js.MouseProviderInterface.html?utm_source=openai
mouse.scrollDown(amount) receives a negative value—inconsistent with the scroll case and undefined behavior.
The zoom case (line 88) passes a potentially negative amount to scrollDown(), which is undefined behavior in nut-js. The scroll case just above (lines 52–56) already demonstrates the correct pattern: check the sign and call scrollDown() or scrollUp() accordingly. The zoom implementation should follow the same approach.
Proposed fix
- const amount = -scaledDelta;
-
logger.debug(`Zoom: delta=${msg.delta} scaled=${scaledDelta}`);
await keyboard.pressKey(Key.LeftControl);
try {
- await mouse.scrollDown(amount);
+ const absAmount = Math.abs(scaledDelta);
+ if (scaledDelta > 0) {
+ await mouse.scrollDown(absAmount);
+ } else {
+ await mouse.scrollUp(absAmount);
+ }
} finally {
await keyboard.releaseKey(Key.LeftControl);
}📝 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 'zoom': | |
| if (msg.delta !== undefined && msg.delta !== 0) { | |
| const sensitivityFactor = 0.5; | |
| const MAX_ZOOM_STEP = 5; | |
| const scaledDelta = | |
| Math.sign(msg.delta) * | |
| Math.min(Math.abs(msg.delta) * sensitivityFactor, MAX_ZOOM_STEP); | |
| console.log(`Pressing keys:`, nutKeys); | |
| const pressedKeys: Key[] = []; | |
| const amount = -scaledDelta; | |
| try { | |
| for (const k of nutKeys) { | |
| if (typeof k === "string") { | |
| await keyboard.type(k); | |
| logger.debug(`Zoom: delta=${msg.delta} scaled=${scaledDelta}`); | |
| await keyboard.pressKey(Key.LeftControl); | |
| try { | |
| await mouse.scrollDown(amount); | |
| } finally { | |
| await keyboard.releaseKey(Key.LeftControl); | |
| } | |
| } | |
| break; | |
| case 'zoom': | |
| if (msg.delta !== undefined && msg.delta !== 0) { | |
| const sensitivityFactor = 0.5; | |
| const MAX_ZOOM_STEP = 5; | |
| const scaledDelta = | |
| Math.sign(msg.delta) * | |
| Math.min(Math.abs(msg.delta) * sensitivityFactor, MAX_ZOOM_STEP); | |
| logger.debug(`Zoom: delta=${msg.delta} scaled=${scaledDelta}`); | |
| await keyboard.pressKey(Key.LeftControl); | |
| try { | |
| const absAmount = Math.abs(scaledDelta); | |
| if (scaledDelta > 0) { | |
| await mouse.scrollDown(absAmount); | |
| } else { | |
| await mouse.scrollUp(absAmount); | |
| } | |
| } finally { | |
| await keyboard.releaseKey(Key.LeftControl); | |
| } | |
| } | |
| break; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/InputHandler.ts` around lines 73 - 93, The zoom handler currently
computes scaledDelta/amount and always calls mouse.scrollDown(amount), which can
pass negative values to mouse.scrollDown(); update the 'zoom' case in
InputHandler (variables: msg.delta, sensitivityFactor, MAX_ZOOM_STEP,
scaledDelta, amount) to mirror the scroll case: compute scaledDelta and amount,
then if amount > 0 call await mouse.scrollDown(amount) else call await
mouse.scrollUp(Math.abs(amount)); keep the LeftControl press/release
(keyboard.pressKey(Key.LeftControl) / keyboard.releaseKey(Key.LeftControl)) and
the debug log intact, and ensure you await the correct scroll direction to avoid
passing negative values to mouse.scrollDown().
| private writeSync(text: string): void { | ||
| try { | ||
| const fd = (this.stream as unknown as { fd: number }).fd; | ||
| if (typeof fd === 'number' && fd >= 0) fs.writeSync(fd, text); | ||
| } catch { /**/ } | ||
| } |
There was a problem hiding this comment.
writeSync relies on an internal, undocumented fd property of WriteStream.
fs.WriteStream does not expose fd as a stable public property — it's set asynchronously after the 'open' event. If writeSync is called before the stream is fully open (e.g., immediately after createWriteStream), or if the Node.js internals change, fd will be undefined and the write silently fails. The try/catch masks the failure.
A safer approach is to open the fd yourself synchronously and keep it alongside the stream, or use fs.appendFileSync with the path:
Proposed fix
private writeSync(text: string): void {
try {
- const fd = (this.stream as unknown as { fd: number }).fd;
- if (typeof fd === 'number' && fd >= 0) fs.writeSync(fd, text);
- } catch { /**/ }
+ fs.appendFileSync(this.logPath, text);
+ } catch { /* best-effort */ }
}📝 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.
| private writeSync(text: string): void { | |
| try { | |
| const fd = (this.stream as unknown as { fd: number }).fd; | |
| if (typeof fd === 'number' && fd >= 0) fs.writeSync(fd, text); | |
| } catch { /**/ } | |
| } | |
| private writeSync(text: string): void { | |
| try { | |
| fs.appendFileSync(this.logPath, text); | |
| } catch { /* best-effort */ } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/logger.ts` around lines 69 - 74, The writeSync method currently
reads an undocumented/async fd from this.stream and swallows errors; instead
open the file descriptor synchronously when initializing the logger and store it
(e.g., loggerFd) to use for fs.writeSync, or if you only have a path use
fs.appendFileSync(path, text); update writeSync to use the stored fd or
path-based append, remove the silent catch so errors surface (or at least log
them), and ensure the stored fd is closed in the logger's close/destroy logic;
reference the writeSync method and the stream initialisation logic to implement
this change.
| close(): void { | ||
| if (!this.stream) return; | ||
| this.writeSessionFooterSync(); | ||
| try { this.stream.destroy(); } catch { /**/ } | ||
| this.stream = null; | ||
| this.initialized = false; | ||
| } |
There was a problem hiding this comment.
stream.destroy() in close() discards any buffered async writes.
destroy() does not flush pending data — any recent calls to this.stream.write() (e.g., from unhandledRejection at line 110) that are still buffered will be silently dropped. Consider using stream.end(callback) with a timeout fallback instead of destroy().
Proposed fix
close(): void {
if (!this.stream) return;
this.writeSessionFooterSync();
- try { this.stream.destroy(); } catch { /**/ }
- this.stream = null;
- this.initialized = false;
+ const s = this.stream;
+ this.stream = null;
+ this.initialized = false;
+ try { s.end(); } catch { /**/ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/logger.ts` around lines 93 - 99, The close() method currently
calls this.stream.destroy() which can drop buffered async writes; change it to
call this.stream.end(callback) to flush pending writes (invoke
writeSessionFooterSync before ending), and implement a timeout fallback that
forces destroy() if end's callback hasn't fired within e.g. 2–5s; ensure you
clear the timeout in the end callback, handle errors passed to the callback by
logging via processLogger or this.logger, and then set this.stream = null and
this.initialized = false (refer to close(), writeSessionFooterSync(), and
this.stream).
| server.on('upgrade', (request: IncomingMessage, socket: Socket, head: Buffer) => { | ||
| const pathname = request.url; | ||
|
|
||
| if (pathname === '/ws') { | ||
| if (request.url === '/ws') { | ||
| wss.handleUpgrade(request, socket, head, (ws) => { | ||
| wss.emit('connection', ws, request); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Upgrade handler compares request.url as a raw string — query parameters will break the match.
If a client connects with ws://host/ws?token=abc, request.url is '/ws?token=abc', which won't equal '/ws'. Consider parsing the pathname:
Proposed fix
server.on('upgrade', (request: IncomingMessage, socket: Socket, head: Buffer) => {
- if (request.url === '/ws') {
+ const { pathname } = new URL(request.url ?? '', `http://${request.headers.host}`);
+ if (pathname === '/ws') {📝 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.
| server.on('upgrade', (request: IncomingMessage, socket: Socket, head: Buffer) => { | |
| const pathname = request.url; | |
| if (pathname === '/ws') { | |
| if (request.url === '/ws') { | |
| wss.handleUpgrade(request, socket, head, (ws) => { | |
| wss.emit('connection', ws, request); | |
| }); | |
| } | |
| }); | |
| server.on('upgrade', (request: IncomingMessage, socket: Socket, head: Buffer) => { | |
| const { pathname } = new URL(request.url ?? '', `http://${request.headers.host}`); | |
| if (pathname === '/ws') { | |
| wss.handleUpgrade(request, socket, head, (ws) => { | |
| wss.emit('connection', ws, request); | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/websocket.ts` around lines 30 - 36, The upgrade handler currently
compares request.url directly to '/ws', which fails when query strings are
present; update the server.on('upgrade' ...) callback to parse the request URL
(e.g., using new URL(request.url!, `http://${request.headers.host}`) or
URL.parse) and compare the pathname instead of request.url, then call
wss.handleUpgrade(request, socket, head, (ws) => { wss.emit('connection', ws,
request); }) as before; ensure you reference request.url, request.headers.host,
the server.on('upgrade' handler, and wss.handleUpgrade when making the change.
| const configPath = './src/server-config.json'; | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const current = fs.existsSync(configPath) ? JSON.parse(fs.readFileSync(configPath, 'utf-8')) : {}; | ||
| const newConfig = { ...current, ...msg.config }; | ||
|
|
||
| fs.writeFileSync(configPath, JSON.stringify(newConfig, null, 2)); | ||
| const current = fs.existsSync(configPath) | ||
| ? JSON.parse(fs.readFileSync(configPath, 'utf-8')) | ||
| : {}; | ||
| fs.writeFileSync(configPath, JSON.stringify({ ...current, ...msg.config }, null, 2)); |
There was a problem hiding this comment.
Relative config path './src/server-config.json' is fragile.
This resolves relative to the process's current working directory, not the project root or the file's location. If the server is started from a different directory (e.g., in production or CI), the read/write will target the wrong path or fail silently (the existsSync fallback returns {}). Consider resolving relative to __dirname or a known root.
Proposed fix
- const configPath = './src/server-config.json';
+ const configPath = path.join(__dirname, '../server-config.json');(Requires adding import path from 'path'; if not already imported.)
📝 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 configPath = './src/server-config.json'; | |
| // eslint-disable-next-line @typescript-eslint/no-require-imports | |
| const current = fs.existsSync(configPath) ? JSON.parse(fs.readFileSync(configPath, 'utf-8')) : {}; | |
| const newConfig = { ...current, ...msg.config }; | |
| fs.writeFileSync(configPath, JSON.stringify(newConfig, null, 2)); | |
| const current = fs.existsSync(configPath) | |
| ? JSON.parse(fs.readFileSync(configPath, 'utf-8')) | |
| : {}; | |
| fs.writeFileSync(configPath, JSON.stringify({ ...current, ...msg.config }, null, 2)); | |
| const configPath = path.join(__dirname, '../server-config.json'); | |
| // eslint-disable-next-line `@typescript-eslint/no-require-imports` | |
| const current = fs.existsSync(configPath) | |
| ? JSON.parse(fs.readFileSync(configPath, 'utf-8')) | |
| : {}; | |
| fs.writeFileSync(configPath, JSON.stringify({ ...current, ...msg.config }, null, 2)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/websocket.ts` around lines 58 - 63, The code uses a fragile
relative path string configPath ('./src/server-config.json') for reading/writing
config; change it to resolve against the module directory so it is stable
regardless of CWD (e.g., build a resolved path using path.resolve(__dirname,
...) or path.join(__dirname, ...) and use that resolved path in the
fs.existsSync, fs.readFileSync, and fs.writeFileSync calls), and add the missing
import for path (import path from 'path') if not present; update references to
configPath in this block accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/logger.ts (1)
18-34: LGTM — platform-aware log path resolution with sensible env-var overrides.One note: the path is resolved eagerly in the constructor (line 59), which runs at import time (line 166). If
LOG_FILE_PATH/LOG_DIRare set dynamically after this module is first imported, they won't take effect. Consider lazily resolving ininit()if this is a concern for the project's startup sequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/logger.ts` around lines 18 - 34, The module currently resolves the log path eagerly via resolveLogPath() from the logger constructor (runs at import time), so runtime changes to LOG_FILE_PATH/LOG_DIR after import are ignored; modify the logger to lazily resolve the path inside the init() method instead: remove/avoid calling resolveLogPath() in the constructor, move any path-dependent setup to init(), and call resolveLogPath() (or a new getLogPath wrapper) during init() so environment changes made prior to init() are respected; update references in the constructor and init() to ensure the logger's file handle/transport is created using the lazily resolved path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/logger.ts`:
- Around line 62-77: The init() method sets this.initialized = true before
running setup calls that can throw (fs.mkdirSync, maybeRotate,
fs.createWriteStream), which leaves the logger permanently marked initialized if
setup fails; move the this.initialized = true assignment to after the stream is
successfully created and any other setup (i.e., after createWriteStream and
after writeSessionHeader/hookProcessEvents), or wrap the setup block (mkdirSync,
maybeRotate, createWriteStream, stream.on, writeSessionHeader,
hookProcessEvents) in a try/catch and reset this.initialized = false (and
rethrow or log) on error so subsequent calls to init() can retry and the logger
is not left broken.
- Around line 132-148: The hookProcessEvents() function is adding duplicate
process handlers on every init; add a private hooked: boolean field and at the
top of hookProcessEvents() return early if hooked is true, then set hooked =
true after installing handlers; also change process.on(...) for
'uncaughtException' and 'unhandledRejection' to process.once(...) so each
handler runs only once (still call this.close() and process.exit as before), and
keep the existing process.once(...) installs for SIGINT/SIGTERM/exit; reference
hookProcessEvents(), process.once/process.on, and the class-level close()/init()
methods when making these changes.
- Around line 101-106: The writeSync method uses the undocumented asynchronous
fd property on this.stream which may be undefined; change writeSync to
synchronously append to the configured file path instead: replace the fd check
and fs.writeSync call in writeSync with fs.appendFileSync(this.logPath, text)
(keeping the existing try/catch behavior) so writes do not silently fail when
the stream isn't open; reference the writeSync method and this.logPath to locate
where to make the change.
- Around line 124-130: Replace the unsafe stream.destroy() call in close() with
a proper stream.end(callback) flush and a timeout fallback: call
writeSessionFooterSync(), then call this.stream.end(() => { this.stream = null;
/* cleanup */ }) and also set a short setTimeout fallback that forcibly destroys
the stream if the callback doesn't fire within e.g. 500ms; only null out stream
after end completes (or fallback fires). Do not rely on toggling initialized to
prevent duplicate process handlers; introduce a private hooked = false flag, set
hooked = true inside hookProcessEvents() when handlers are registered, and have
hookProcessEvents() and init() check hooked to avoid re-registering handlers;
update close() so it performs stream shutdown but does not simply reset
initialized = false to avoid re-triggering hookProcessEvents() (or if you must
clear handlers, clear hooked appropriately when you explicitly unhook).
---
Nitpick comments:
In `@src/server/logger.ts`:
- Around line 18-34: The module currently resolves the log path eagerly via
resolveLogPath() from the logger constructor (runs at import time), so runtime
changes to LOG_FILE_PATH/LOG_DIR after import are ignored; modify the logger to
lazily resolve the path inside the init() method instead: remove/avoid calling
resolveLogPath() in the constructor, move any path-dependent setup to init(),
and call resolveLogPath() (or a new getLogPath wrapper) during init() so
environment changes made prior to init() are respected; update references in the
constructor and init() to ensure the logger's file handle/transport is created
using the lazily resolved path.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/server/InputHandler.ts (2)
52-73: Concurrent scroll axes viaPromise.allmay cause input ordering issues.Running vertical and horizontal scroll operations concurrently with
Promise.all(line 72) could produce interleaved or unpredictable OS-level input events. Consider executing them sequentially instead, since they are low-latency operations and sequential execution guarantees deterministic ordering.Proposed sequential execution
- const promises: Promise<any>[] = []; - if (typeof msg.dy === 'number' && msg.dy !== 0) { if (msg.dy > 0) { - promises.push(mouse.scrollDown(msg.dy)); + await mouse.scrollDown(msg.dy); } else { - promises.push(mouse.scrollUp(-msg.dy)); + await mouse.scrollUp(-msg.dy); } } if (typeof msg.dx === 'number' && msg.dx !== 0) { if (msg.dx > 0) { - promises.push(mouse.scrollRight(msg.dx)); + await mouse.scrollRight(msg.dx); } else { - promises.push(mouse.scrollLeft(-msg.dx)); + await mouse.scrollLeft(-msg.dx); } } - - if (promises.length) { - await Promise.all(promises); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 52 - 73, The handler currently runs vertical and horizontal scrolls concurrently using Promise.all (the promises array and await Promise.all(promises)), which can cause OS-level input ordering issues; change the logic in InputHandler's 'scroll' case to execute scrolls sequentially instead of pushing both into promises—call mouse.scrollDown/scrollUp first (if dy present) and await it, then call mouse.scrollRight/scrollLeft (if dx present) and await that, ensuring deterministic ordering of mouse.scrollDown/scrollUp and mouse.scrollRight/scrollLeft operations.
135-150: Combo with multiple string-only keys won't produce a true simultaneous key combo.When all keys in the combo resolve to strings (e.g.,
['a', 'b']), each is individuallytype()d (press+release) sequentially rather than held together. This is fine for the typical modifier+character pattern (e.g., Ctrl+C), since modifiers are held viapressKeywhile the character is typed. Just be aware that purely character-based combos won't behave as simultaneous key presses — if that's intentional, a brief comment in the code would help clarify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 135 - 150, The current loop over nutKeys calls keyboard.type for string entries which presses and releases immediately, so multi-string combos (e.g., ['a','b']) are not held simultaneously; update the logic in the try/finally around nutKeys, keyboard.type, keyboard.pressKey, pressedKeys, and keyboard.releaseKey so that when a combo contains multiple string keys (or when the intent is to hold multiple keys) you call keyboard.pressKey for those string entries and push them to pressedKeys, then after pressing all required keys perform any single-character typing if needed, finally releasing pressedKeys in the finally block; alternatively, if you intend the current sequential behavior to be deliberate, add a short comment next to the nutKeys handling explaining that string entries are typed sequentially and not held.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/InputHandler.ts`:
- Around line 76-96: The zoom handler currently computes scaledDelta and amount
and calls mouse.scrollDown(amount) which can pass a negative value; update the
'zoom' case in InputHandler.ts to branch on scaledDelta/amount like the existing
'scroll' case: after computing scaledDelta and amount, if amount > 0 call await
mouse.scrollDown(amount) else call await mouse.scrollUp(Math.abs(amount)),
keeping the existing keyboard.pressKey(Key.LeftControl)/releaseKey try/finally
block and logger.debug call (refer to the 'zoom' case, scaledDelta, amount,
mouse.scrollDown, mouse.scrollUp, keyboard.pressKey, keyboard.releaseKey to
locate the change).
---
Nitpick comments:
In `@src/server/InputHandler.ts`:
- Around line 52-73: The handler currently runs vertical and horizontal scrolls
concurrently using Promise.all (the promises array and await
Promise.all(promises)), which can cause OS-level input ordering issues; change
the logic in InputHandler's 'scroll' case to execute scrolls sequentially
instead of pushing both into promises—call mouse.scrollDown/scrollUp first (if
dy present) and await it, then call mouse.scrollRight/scrollLeft (if dx present)
and await that, ensuring deterministic ordering of mouse.scrollDown/scrollUp and
mouse.scrollRight/scrollLeft operations.
- Around line 135-150: The current loop over nutKeys calls keyboard.type for
string entries which presses and releases immediately, so multi-string combos
(e.g., ['a','b']) are not held simultaneously; update the logic in the
try/finally around nutKeys, keyboard.type, keyboard.pressKey, pressedKeys, and
keyboard.releaseKey so that when a combo contains multiple string keys (or when
the intent is to hold multiple keys) you call keyboard.pressKey for those string
entries and push them to pressedKeys, then after pressing all required keys
perform any single-character typing if needed, finally releasing pressedKeys in
the finally block; alternatively, if you intend the current sequential behavior
to be deliberate, add a short comment next to the nutKeys handling explaining
that string entries are typed sequentially and not held.
1 participant
Captures all console output to a log file so users can attach it when reporting issues.
Log file location
Uses
os.homedir()to resolve a universal path across all platforms:~/.rein/log.txt~/AppData/Roaming/.rein/log.txtThe exact path is printed to the terminal on every
npm run devAppends across sessions. Rotates at 10 MB, keeps 5 old files.
Key edge cases
safeStringify, won't crash the loggerFuture scope
/settingspage so users can copy them without finding the file manuallySummary by CodeRabbit
Bug Fixes
Chores