Merged
Conversation
- Deletes stale files - Audits and updates docs for accuracy and relevance - Improves test coverage
…wards KISS and DRY principles
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces centralized utilities and base command patterns to reduce duplication across the CLI. It exports reusable flags ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
There was a problem hiding this comment.
Pull request overview
This PR is a codebase-wide refactor that eliminates duplication by extracting common patterns into shared utilities and base-class methods, in line with KISS and DRY principles.
Changes:
- Extracted shared flags (
durationFlag,rewindFlag) and utility functions (buildHistoryParams,interpolateMessage,formatMessageTimestamp,formatPresenceAction) to reduce inline repetition across dozens of commands. - Added base-class helper methods (
handleCommandError,configureRewind,parseJsonFlag,setupRoomStatusHandler,initializeSpace,waitForConnection,waitForCursorsChannelAttachment) to consolidate repeated patterns in command catch blocks and setup flows. - Introduced
StatsBaseCommandabstract class to unify the duplicate stats polling/display logic previously duplicated betweenstats/app.tsandstats/account.ts.
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/flags.ts |
Added shared durationFlag and rewindFlag exports |
src/utils/output.ts |
Added formatMessageTimestamp and formatPresenceAction helpers |
src/utils/message.ts |
New file: extracted interpolateMessage from command classes |
src/utils/history.ts |
New file: extracted buildHistoryParams from history commands |
src/base-command.ts |
Added handleCommandError, configureRewind, parseJsonFlag helpers |
src/chat-base-command.ts |
Added setupRoomStatusHandler helper; removed _chatRealtimeClient field |
src/spaces-base-command.ts |
Added waitForConnection, initializeSpace, waitForCursorsChannelAttachment helpers |
src/stats-base-command.ts |
New file: abstract base for stats commands |
src/commands/stats/app.ts |
Refactored to extend StatsBaseCommand |
src/commands/stats/account.ts |
Refactored to extend StatsBaseCommand |
src/commands/spaces/** |
Replaced inline setup/cleanup with shared helpers |
src/commands/rooms/** |
Replaced inline status handling and error blocks with helpers |
src/commands/channels/** |
Replaced inline rewind/error/timestamp logic with helpers |
src/commands/logs/** |
Replaced inline history params and error handling with helpers |
test/unit/commands/logs/connection-lifecycle/subscribe.test.ts |
Updated expectation from undefined to {} for channel options |
test/e2e/rooms/rooms-e2e.test.ts |
Updated expected ready signal from "Connected to room:" to "Subscribed to room:" |
AGENTS.md |
Updated documentation to reference new shared flags and helpers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/commands/spaces/cursors/get-all.ts (1)
168-188:⚠️ Potential issue | 🟠 MajorKeep the live-update fallback reachable.
The new stabilization wait rejects before the
getAll()fallbacktry/catchruns. If the connection stays non-connected for 5 seconds, the command now exits with an error instead of returning the cursor updates already collected incursorMap. Move the wait inside thetry { ... getAll() ... } catch { ... }block, or downgrade the timeout to a warning and continue with the live snapshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/cursors/get-all.ts` around lines 168 - 188, The connection-stabilization Promise currently lives outside the try/catch so a timeout rejection escapes the getAll() fallback; move the wait (the block that checks this.realtimeClient!.connection.state and awaits the Promise with setTimeout/once) inside the same try that calls this.space!.cursors.getAll() so any timeout rejection is caught and the catch block can return the existing cursorMap/live snapshot, or alternatively change the timeout rejection to a non-throwing warning that logs and continues to call this.space!.cursors.getAll(); touch the symbols this.realtimeClient!.connection.state, the stabilization Promise, this.space!.cursors.getAll(), and cursorMap when making the change.src/commands/spaces/locations/set.ts (1)
144-155:⚠️ Potential issue | 🟡 MinorE2E error handling outputs misleading success status.
When an error occurs in the E2E code path, the catch block still outputs
success: true, which masks the actual failure. This could make debugging E2E test failures difficult.🛡️ Suggested fix to output accurate status
} catch { // If an error occurs in E2E mode, just exit cleanly after showing what we can if (this.shouldOutputJson(flags)) { this.log( this.formatJsonOutput( - { success: true, location, spaceName }, + { success: false, error: "Failed to set location", location, spaceName }, flags, ), ); } // Don't call this.error() in E2E mode as it sets exit code to 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locations/set.ts` around lines 144 - 155, The catch block currently swallows the thrown error and emits a JSON payload with success: true; update the catch to capture the error (e.g., catch (err)) and when this.shouldOutputJson(flags) is true call this.formatJsonOutput with success: false and include the error details (err.message and/or err.stack) alongside location and spaceName so E2E consumers see the real failure status, but still avoid calling this.error() to keep exit code behavior unchanged.
🧹 Nitpick comments (11)
src/utils/output.ts (1)
27-34: Consider whether timestamp0should be treated as valid.The condition
!timestamp && timestamp !== 0explicitly allows0as a valid timestamp, which would produce1970-01-01T00:00:00.000Z. While this is technically correct behavior, a Unix timestamp of 0 is almost never a valid message timestamp in practice and could mask bugs where 0 is passed erroneously.If
0is not expected as a valid input, simplifying toif (!timestamp)would treat0the same asnull/undefinedand fall back to the current time.💡 Optional simplification
export function formatMessageTimestamp( timestamp: number | Date | undefined | null, ): string { - if (!timestamp && timestamp !== 0) return new Date().toISOString(); + if (!timestamp) return new Date().toISOString(); return timestamp instanceof Date ? timestamp.toISOString() : new Date(timestamp).toISOString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/output.ts` around lines 27 - 34, The formatMessageTimestamp function currently treats 0 as a valid timestamp; update formatMessageTimestamp so a timestamp value of 0 is treated like null/undefined (i.e., fall back to current time) by simplifying the check to if (!timestamp) and keep the rest of the logic (Date instance -> toISOString, otherwise new Date(timestamp).toISOString()); update any tests or callers that rely on 0 being preserved.src/commands/rooms/typing/keystroke.ts (1)
93-106: Consider extracting common status handling logic.This command maintains custom
onStatusChangehandling because it needs to triggerkeystroke()on attach. However, the status logging and Failed handling (lines 100-106, 172-179) duplicates logic fromsetupRoomStatusHandler.Consider enhancing
setupRoomStatusHandlerto accept an optionalonAttachedcallback, which would allow reuse while preserving the custom keystroke behavior:this.setupRoomStatusHandler(room, flags, { roomName, onAttached: async () => { // Custom keystroke logic here } });This would further reduce duplication across room commands that need custom attach behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/rooms/typing/keystroke.ts` around lines 93 - 106, Extract the duplicated status-change and Failed handling by extending setupRoomStatusHandler to accept an options object with an optional onAttached callback; move the existing logging and Failed->reason logic from this command's room.onStatusChange handler into setupRoomStatusHandler and, in this file, call setupRoomStatusHandler(room, flags, { roomName, onAttached: async () => { await this.keystroke(...) } }) so the custom keystroke behavior runs on attach while all status logging and error-reason extraction remains centralized in setupRoomStatusHandler; update references to onStatusChange/room.error/RoomStatus.Failed accordingly.src/commands/spaces/locks/subscribe.ts (1)
62-66: Move or rename the post-init progress message.After Line 62 returns, the command is already connected and has entered the space, so
Connecting to space...is no longer true. Emit that progress line beforeinitializeSpace()or change it to the next real step to keep the CLI output accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locks/subscribe.ts` around lines 62 - 66, The progress message "Connecting to space..." is emitted after await this.initializeSpace(...) so it reports a step that's already completed; either move the log call to before the await to show the actual connection step, or change the message after initializeSpace to reflect the new state (e.g., "Connected to space" or "Entered space")—update the call site that invokes this.log(progress(resource(...))) in subscribe.ts near initializeSpace, keeping the guard this.shouldOutputJson(flags) unchanged.src/commands/spaces/locks/get.ts (1)
39-45: LetinitializeSpace()own the enter step too.You're still duplicating one of the helper's responsibilities here. Passing
enterSpace: truekeeps the lock command on the same initialization path as the other space commands and avoids the flow drifting later.♻️ Suggested simplification
- await this.initializeSpace(flags, spaceName, { - enterSpace: false, - setupConnectionLogging: false, - }); - - await this.space!.enter(); + await this.initializeSpace(flags, spaceName, { + enterSpace: true, + setupConnectionLogging: false, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locks/get.ts` around lines 39 - 45, The code calls this.space!.enter() after initializeSpace even though initializeSpace supports an enterSpace option; remove the explicit enter call and call initializeSpace with enterSpace: true (e.g., await this.initializeSpace(flags, spaceName, { enterSpace: true, setupConnectionLogging: false })) so the helper owns entering the space and the flow matches other space commands (drop the this.space!.enter() line).src/commands/rooms/presence/enter.ts (1)
68-78: Quote stripping before JSON parsing may indicate upstream issue.The manual trimming and quote removal before
parseJsonFlagsuggests shell quoting artifacts are reaching the command. This workaround handles it, but consider whetherparseJsonFlagshould handle these cases consistently, or if the issue is in how the shell passes arguments.If this pattern is needed in multiple commands, consider moving the quote-stripping logic into
parseJsonFlagitself for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/rooms/presence/enter.ts` around lines 68 - 78, The code is pre-stripping surrounding quotes from rawData before calling parseJsonFlag; move this normalization into parseJsonFlag so all callers (including this PresenceData flow) get consistent handling—update parseJsonFlag to trim whitespace and remove a single pair of matching surrounding single or double quotes before attempting JSON parsing, then remove the duplicate quote-stripping block in enter.ts (the rawData/trimmed handling and slice logic) and continue to call parseJsonFlag(trimmed, "data", flags) (or pass rawData unchanged) so PresenceData parsing uses the centralized behavior.src/commands/spaces/members/enter.ts (1)
276-285: Consider using centralized error handling for consistency.Other commands in this PR use
handleCommandErrorfor catch blocks. While the inline approach works, consider aligning with the centralized pattern for consistency.♻️ Optional: Use centralized error handler
} catch (error) { - const errorMsg = `Error: ${error instanceof Error ? error.message : String(error)}`; - this.logCliEvent(flags, "error", "unhandledError", errorMsg, { - error: errorMsg, - }); - if (this.shouldOutputJson(flags)) { - this.jsonError({ error: errorMsg, success: false }, flags); - } else { - this.error(errorMsg); - } + this.handleCommandError(error, flags, "member", { spaceName }); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/members/enter.ts` around lines 276 - 285, The catch block in enter.ts currently builds errorMsg, logs via this.logCliEvent and branches on this.shouldOutputJson/this.jsonError/this.error; replace that inline handling with a call to the centralized handler used elsewhere: handleCommandError. In the catch for the command's main function, pass the command instance (this), the parsed flags object (flags), and the caught error, and include the same contextual metadata you were passing to logCliEvent (e.g., { event: "unhandledError", error: ... } or similar) so logs and JSON output remain consistent; remove the inline this.logCliEvent/this.jsonError/this.error calls and rely on handleCommandError to perform consistent logging and output. Ensure you import/resolve handleCommandError the same way other commands do.src/commands/spaces/cursors/get-all.ts (1)
47-58: Extract the enter/poll sequence intoSpacesBaseCommand.This same 5-second enter-and-poll flow now exists here and in
src/commands/spaces/locations/get-all.ts. Pulling it behind a shared helper would better match the PR’s DRY goal and make future connection-state fixes one-place changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/cursors/get-all.ts` around lines 47 - 58, The enter-and-poll connection sequence (the initializeSpace call with enterSpace:false, the optional progress log via shouldOutputJson, and awaiting this.space!.enter() plus the 5s poll) is duplicated and should be extracted into a shared helper on SpacesBaseCommand (e.g., ensureSpaceConnected or enterAndPollSpace). Add a new method on SpacesBaseCommand that accepts flags and spaceName, performs initializeSpace(..., { enterSpace:false, setupConnectionLogging:false }), emits the same progress log when shouldOutputJson(flags) is false, calls await this.space!.enter(), and runs the existing 5-second poll/connection-state logic; then replace the duplicated block in get-all.ts (and the same block in locations/get-all.ts) with a call to that new helper.src/stats-base-command.ts (2)
143-150: Signal handlers don't terminate the process.The
cleanupfunction clears the interval and logs a message but doesn't callprocess.exit(). The command relies on the never-resolving Promise at line 172 to keep running. After SIGINT/SIGTERM, the interval is cleared but the process may hang until Node.js detects no more work. Consider explicitly exiting after cleanup.♻️ Suggested fix to exit cleanly
const cleanup = () => { if (this.pollInterval) { clearInterval(this.pollInterval); this.pollInterval = undefined; } this.log("\nUnsubscribed from live stats"); + process.exit(0); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stats-base-command.ts` around lines 143 - 150, The cleanup callback for live stats (cleanup) clears this.pollInterval and logs but does not terminate the process; update cleanup (the function referenced by the SIGINT/SIGTERM handlers) to call process.exit(0) (or process.exit(1) on error) after clearing the interval and logging so the command exits immediately; ensure you reference this.pollInterval and the same cleanup function used by the signal handlers so no dangling Promise keeps the process alive.
61-66: Avoid mutating the input flags object.Directly modifying
flags.unitmutates the input parameter, which can lead to unexpected behavior if the flags object is used elsewhere. Consider using a local variable instead.♻️ Suggested fix using a local variable
protected async runStats( flags: BaseFlags, controlApi: ControlApi, ): Promise<void> { + let unit = flags.unit as string; if (flags.live && flags.unit !== "minute") { this.warn( "Live stats only support minute intervals. Using minute interval.", ); - flags.unit = "minute"; + unit = "minute"; } await this.showAuthInfoIfNeeded(flags); this.statsDisplay = new StatsDisplay({ intervalSeconds: flags.interval as number, json: this.shouldOutputJson(flags), live: flags.live as boolean, startTime: flags.live ? new Date() : undefined, - unit: flags.unit as "day" | "hour" | "minute" | "month", + unit: unit as "day" | "hour" | "minute" | "month", ...this.getStatsDisplayOptions(), });Note: You'll also need to pass
unitto the fetch methods or store it as instance state if used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stats-base-command.ts` around lines 61 - 66, Avoid mutating the incoming flags object: read flags.unit into a local variable (e.g., let unit = flags.unit), and if (flags.live && unit !== "minute") set unit = "minute" and call this.warn(...). Replace any later uses of flags.unit with this local unit (or assign it to instance state like this.unit) and ensure any fetch methods receive this unit value as an argument rather than reading flags.unit directly.src/commands/spaces/cursors/subscribe.ts (1)
168-170: Consider removing the empty finally block.The finally block only contains a comment stating cleanup is handled elsewhere. Since it performs no actual work, it can be safely removed to reduce noise.
♻️ Suggested cleanup
} catch (error) { this.handleCommandError(error, flags, "cursor", { spaceName }); - } finally { - // Cleanup is now handled by base class finally() method } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/cursors/subscribe.ts` around lines 168 - 170, Remove the empty finally block in the async block inside the subscribe command (the finally that only contains the comment "Cleanup is now handled by base class finally() method"); edit the function/method where this try...finally appears (in src/commands/spaces/cursors/subscribe.ts, inside the subscribe handler/execute method) and delete the entire finally clause so only the try (and any catch) remain, leaving cleanup to the base class.src/commands/spaces/locations/set.ts (1)
273-275: Consider removing the empty finally block.Similar to the cursors/subscribe.ts file, this finally block only contains a comment and performs no work.
♻️ Suggested cleanup
} catch (error) { const errorMsg = `Error: ${error instanceof Error ? error.message : String(error)}`; this.logCliEvent(flags, "location", "fatalError", errorMsg, { error: errorMsg, }); if (this.shouldOutputJson(flags)) { this.jsonError({ error: errorMsg, success: false }, flags); } else { this.error(errorMsg); } - } finally { - // Cleanup is now handled by base class finally() method } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locations/set.ts` around lines 273 - 275, Remove the empty finally block in src/commands/spaces/locations/set.ts (the try...finally that only contains the comment "// Cleanup is now handled by base class finally() method"); delete the entire finally { ... } block so there is no no-op finally, or if you want to preserve the note move that comment into the surrounding function/method's header or above the try so behavior is documented without an empty block.
🤖 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/chat-base-command.ts`:
- Around line 91-135: The handler in setupRoomStatusHandler currently suppresses
error output for RoomStatus.Failed when shouldOutputJson(flags) is true; emit a
JSON-friendly error instead of doing nothing: after you call logCliEvent for the
Failed case (or inside the statusChange.current === RoomStatus.Failed branch),
when shouldOutputJson(flags) is true call this.error(...) or an appropriate JSON
error emission path with the same message used for the human output (e.g.,
`Failed to attach to room ${options.roomName}: ${reasonMsg || "Unknown error"}`)
so JSON consumers receive the failure notification; reference
setupRoomStatusHandler, RoomStatus.Failed, shouldOutputJson, logCliEvent and
this.error to locate where to add the JSON-specific emission.
In `@src/commands/spaces/locations/get-all.ts`:
- Around line 123-124: The code incorrectly destructures the result of
locations.getAll() as `{ items: locationsFromSpace }`; change it to assign the
raw result to locationsFromSpace (e.g., `const locationsFromSpace = await
this.space!.locations.getAll()`), then normalize that value before use to
support both array and object shapes (check Array.isArray(locationsFromSpace)
and, if an object keyed by connectionId, extract/flatten its values) so
downstream checks and iteration work correctly; update any logic using
locationsFromSpace accordingly (see the locations.getAll call and how
subscribe.ts handles the returned shape for guidance).
In `@src/spaces-base-command.ts`:
- Around line 148-156: The waitForConnection() function has a race where it
checks connection.state === "connected" before registering the connection event
handlers, so a fast transition can be missed; fix by registering the same
listeners currently added later (the "connected" and error/timeout handlers used
around the listener registration block) at the start of waitForConnection()
(before checking connection.state), then immediately check connection.state and
handle the already-connected case by calling this.logCliEvent(flags,
"connection", "connected", "Realtime connection established.") and returning;
ensure you still remove/cleanup those listeners on resolve/reject to avoid
leaks.
---
Outside diff comments:
In `@src/commands/spaces/cursors/get-all.ts`:
- Around line 168-188: The connection-stabilization Promise currently lives
outside the try/catch so a timeout rejection escapes the getAll() fallback; move
the wait (the block that checks this.realtimeClient!.connection.state and awaits
the Promise with setTimeout/once) inside the same try that calls
this.space!.cursors.getAll() so any timeout rejection is caught and the catch
block can return the existing cursorMap/live snapshot, or alternatively change
the timeout rejection to a non-throwing warning that logs and continues to call
this.space!.cursors.getAll(); touch the symbols
this.realtimeClient!.connection.state, the stabilization Promise,
this.space!.cursors.getAll(), and cursorMap when making the change.
In `@src/commands/spaces/locations/set.ts`:
- Around line 144-155: The catch block currently swallows the thrown error and
emits a JSON payload with success: true; update the catch to capture the error
(e.g., catch (err)) and when this.shouldOutputJson(flags) is true call
this.formatJsonOutput with success: false and include the error details
(err.message and/or err.stack) alongside location and spaceName so E2E consumers
see the real failure status, but still avoid calling this.error() to keep exit
code behavior unchanged.
---
Nitpick comments:
In `@src/commands/rooms/presence/enter.ts`:
- Around line 68-78: The code is pre-stripping surrounding quotes from rawData
before calling parseJsonFlag; move this normalization into parseJsonFlag so all
callers (including this PresenceData flow) get consistent handling—update
parseJsonFlag to trim whitespace and remove a single pair of matching
surrounding single or double quotes before attempting JSON parsing, then remove
the duplicate quote-stripping block in enter.ts (the rawData/trimmed handling
and slice logic) and continue to call parseJsonFlag(trimmed, "data", flags) (or
pass rawData unchanged) so PresenceData parsing uses the centralized behavior.
In `@src/commands/rooms/typing/keystroke.ts`:
- Around line 93-106: Extract the duplicated status-change and Failed handling
by extending setupRoomStatusHandler to accept an options object with an optional
onAttached callback; move the existing logging and Failed->reason logic from
this command's room.onStatusChange handler into setupRoomStatusHandler and, in
this file, call setupRoomStatusHandler(room, flags, { roomName, onAttached:
async () => { await this.keystroke(...) } }) so the custom keystroke behavior
runs on attach while all status logging and error-reason extraction remains
centralized in setupRoomStatusHandler; update references to
onStatusChange/room.error/RoomStatus.Failed accordingly.
In `@src/commands/spaces/cursors/get-all.ts`:
- Around line 47-58: The enter-and-poll connection sequence (the initializeSpace
call with enterSpace:false, the optional progress log via shouldOutputJson, and
awaiting this.space!.enter() plus the 5s poll) is duplicated and should be
extracted into a shared helper on SpacesBaseCommand (e.g., ensureSpaceConnected
or enterAndPollSpace). Add a new method on SpacesBaseCommand that accepts flags
and spaceName, performs initializeSpace(..., { enterSpace:false,
setupConnectionLogging:false }), emits the same progress log when
shouldOutputJson(flags) is false, calls await this.space!.enter(), and runs the
existing 5-second poll/connection-state logic; then replace the duplicated block
in get-all.ts (and the same block in locations/get-all.ts) with a call to that
new helper.
In `@src/commands/spaces/cursors/subscribe.ts`:
- Around line 168-170: Remove the empty finally block in the async block inside
the subscribe command (the finally that only contains the comment "Cleanup is
now handled by base class finally() method"); edit the function/method where
this try...finally appears (in src/commands/spaces/cursors/subscribe.ts, inside
the subscribe handler/execute method) and delete the entire finally clause so
only the try (and any catch) remain, leaving cleanup to the base class.
In `@src/commands/spaces/locations/set.ts`:
- Around line 273-275: Remove the empty finally block in
src/commands/spaces/locations/set.ts (the try...finally that only contains the
comment "// Cleanup is now handled by base class finally() method"); delete the
entire finally { ... } block so there is no no-op finally, or if you want to
preserve the note move that comment into the surrounding function/method's
header or above the try so behavior is documented without an empty block.
In `@src/commands/spaces/locks/get.ts`:
- Around line 39-45: The code calls this.space!.enter() after initializeSpace
even though initializeSpace supports an enterSpace option; remove the explicit
enter call and call initializeSpace with enterSpace: true (e.g., await
this.initializeSpace(flags, spaceName, { enterSpace: true,
setupConnectionLogging: false })) so the helper owns entering the space and the
flow matches other space commands (drop the this.space!.enter() line).
In `@src/commands/spaces/locks/subscribe.ts`:
- Around line 62-66: The progress message "Connecting to space..." is emitted
after await this.initializeSpace(...) so it reports a step that's already
completed; either move the log call to before the await to show the actual
connection step, or change the message after initializeSpace to reflect the new
state (e.g., "Connected to space" or "Entered space")—update the call site that
invokes this.log(progress(resource(...))) in subscribe.ts near initializeSpace,
keeping the guard this.shouldOutputJson(flags) unchanged.
In `@src/commands/spaces/members/enter.ts`:
- Around line 276-285: The catch block in enter.ts currently builds errorMsg,
logs via this.logCliEvent and branches on
this.shouldOutputJson/this.jsonError/this.error; replace that inline handling
with a call to the centralized handler used elsewhere: handleCommandError. In
the catch for the command's main function, pass the command instance (this), the
parsed flags object (flags), and the caught error, and include the same
contextual metadata you were passing to logCliEvent (e.g., { event:
"unhandledError", error: ... } or similar) so logs and JSON output remain
consistent; remove the inline this.logCliEvent/this.jsonError/this.error calls
and rely on handleCommandError to perform consistent logging and output. Ensure
you import/resolve handleCommandError the same way other commands do.
In `@src/stats-base-command.ts`:
- Around line 143-150: The cleanup callback for live stats (cleanup) clears
this.pollInterval and logs but does not terminate the process; update cleanup
(the function referenced by the SIGINT/SIGTERM handlers) to call process.exit(0)
(or process.exit(1) on error) after clearing the interval and logging so the
command exits immediately; ensure you reference this.pollInterval and the same
cleanup function used by the signal handlers so no dangling Promise keeps the
process alive.
- Around line 61-66: Avoid mutating the incoming flags object: read flags.unit
into a local variable (e.g., let unit = flags.unit), and if (flags.live && unit
!== "minute") set unit = "minute" and call this.warn(...). Replace any later
uses of flags.unit with this local unit (or assign it to instance state like
this.unit) and ensure any fetch methods receive this unit value as an argument
rather than reading flags.unit directly.
In `@src/utils/output.ts`:
- Around line 27-34: The formatMessageTimestamp function currently treats 0 as a
valid timestamp; update formatMessageTimestamp so a timestamp value of 0 is
treated like null/undefined (i.e., fall back to current time) by simplifying the
check to if (!timestamp) and keep the rest of the logic (Date instance ->
toISOString, otherwise new Date(timestamp).toISOString()); update any tests or
callers that rely on 0 being preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3788981e-8864-4c67-a9f5-f79790308154
📒 Files selected for processing (53)
AGENTS.mdsrc/base-command.tssrc/chat-base-command.tssrc/commands/channels/history.tssrc/commands/channels/occupancy/subscribe.tssrc/commands/channels/presence/enter.tssrc/commands/channels/presence/subscribe.tssrc/commands/channels/publish.tssrc/commands/channels/subscribe.tssrc/commands/logs/channel-lifecycle/subscribe.tssrc/commands/logs/connection-lifecycle/history.tssrc/commands/logs/connection-lifecycle/subscribe.tssrc/commands/logs/history.tssrc/commands/logs/push/history.tssrc/commands/logs/push/subscribe.tssrc/commands/logs/subscribe.tssrc/commands/rooms/list.tssrc/commands/rooms/messages/history.tssrc/commands/rooms/messages/reactions/remove.tssrc/commands/rooms/messages/reactions/send.tssrc/commands/rooms/messages/reactions/subscribe.tssrc/commands/rooms/messages/send.tssrc/commands/rooms/messages/subscribe.tssrc/commands/rooms/occupancy/get.tssrc/commands/rooms/occupancy/subscribe.tssrc/commands/rooms/presence/enter.tssrc/commands/rooms/presence/subscribe.tssrc/commands/rooms/reactions/send.tssrc/commands/rooms/reactions/subscribe.tssrc/commands/rooms/typing/keystroke.tssrc/commands/rooms/typing/subscribe.tssrc/commands/spaces/cursors/get-all.tssrc/commands/spaces/cursors/set.tssrc/commands/spaces/cursors/subscribe.tssrc/commands/spaces/locations/get-all.tssrc/commands/spaces/locations/set.tssrc/commands/spaces/locations/subscribe.tssrc/commands/spaces/locks/acquire.tssrc/commands/spaces/locks/get-all.tssrc/commands/spaces/locks/get.tssrc/commands/spaces/locks/subscribe.tssrc/commands/spaces/members/enter.tssrc/commands/spaces/members/subscribe.tssrc/commands/stats/account.tssrc/commands/stats/app.tssrc/flags.tssrc/spaces-base-command.tssrc/stats-base-command.tssrc/utils/history.tssrc/utils/message.tssrc/utils/output.tstest/e2e/rooms/rooms-e2e.test.tstest/unit/commands/logs/connection-lifecycle/subscribe.test.ts
umair-ably
force-pushed
the
DX-928-repo-update
branch
from
1c42c11 to
2d57732
Compare
sacOO7
reviewed
Mar 6, 2026
Contributor
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Nice, seems we have addressed duplicate validation snippets, error handling and other parts of the code.
I will still try to cross-check
- code patterns, snippets, or logic that are duplicated or very similar across the codebase.
- Determine whether these can be refactored into reusable utilities, shared helpers, or extracted into common methods within the same class hierarchy or parent classes.
- Focus especially on areas such as validation logic, error handling, data transformations, conditional patterns, and repeated service or repository interactions.
sacOO7
approved these changes
Mar 6, 2026
Contributor
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Thanks for addressing feedback : )
LGTM !
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Repo wide refactor to identify areas of duplication and steer back towards KISS and DRY principles
Summary by CodeRabbit
Bug Fixes
Refactor