Added a Prefix to the watchdog #490 #494
Conversation
WalkthroughThe changes involve updates to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for terminal7 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dazzling-kringle-090742 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/shell.ts (1)
Line range hint
309-319: LGTM! Consider adding JSDoc for improved documentation.The changes to the
startWatchdogmethod enhance its flexibility and improve logging clarity. The new object parameter with optional properties is a good approach.Consider adding JSDoc comments to document the new method signature and parameters:
/** * Starts a watchdog that will reject if not stopped within the given time * @param {Object} options - The options object * @param {number} [options.timeout] - The timeout in milliseconds * @param {string} [options.prefix='Waiting'] - The prefix for the log message * @returns {Promise<void>} */ startWatchdog({ timeout, prefix = 'Waiting' }: { timeout?: number; prefix?: string } = {}): Promise<void> { // ... existing implementation }src/peerbook.ts (1)
174-174: LGTM! Consider using a constant for the timeout value.The update to
this.shell.startWatchdog({ timeout: 3000 })is correct and aligns with the PR objectives. It improves the method call's clarity by using an object with a namedtimeoutproperty.For better maintainability and consistency, consider defining a constant for the timeout value:
const REGISTRATION_TIMEOUT_MS = 3000; // Then use it in the method call this.shell.startWatchdog({ timeout: REGISTRATION_TIMEOUT_MS })This approach would make it easier to adjust the timeout value in the future if needed and improve code readability.
src/commands.ts (1)
244-244: LGTM! Consider destructuring for clarity.The update to use an object parameter for
startWatchdogis a good change for consistency and future flexibility.Consider destructuring the parameter for improved readability:
- shell.startWatchdog({ timeout }).catch(e => gate.handleFailure(e)) + shell.startWatchdog({ timeout: timeout }).catch(e => gate.handleFailure(e))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/commands.ts (6 hunks)
- src/peerbook.ts (1 hunks)
- src/shell.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
src/shell.ts (1)
Line range hint
309-319: Verify usage of updatedstartWatchdogmethodLet's ensure that all calls to
startWatchdoghave been updated to use the new signature.✅ Verification successful
Let's update the verification script to correctly search TypeScript files.
Verified: All
startWatchdogmethod calls are using the updated signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for calls to startWatchdog and verify they use the new signature # Test: Search for startWatchdog calls echo "Searching for startWatchdog calls:" rg --type typescript "startWatchdog\(" -A 3 echo "\nVerifying startWatchdog import/usage:" rg --type typescript "import.*Shell.*from" -A 10Length of output: 334
Script:
#!/bin/bash # Description: Search for calls to startWatchdog and verify they use the new signature # Test: Search for startWatchdog calls in .ts and .tsx files echo "Searching for startWatchdog calls in TypeScript files:" rg "startWatchdog\(" -g "*.ts" -g "*.tsx" -A 3 echo "\nVerifying startWatchdog import/usage in TypeScript files:" rg "import.*Shell.*from" -g "*.ts" -g "*.tsx" -A 10Length of output: 4557
src/commands.ts (4)
641-641: LGTM! Consistent with earlier changes.The update to use an object parameter for
startWatchdogis consistent with the earlier changes, maintaining a uniform approach across the codebase.
664-664: LGTM! Verify the extended timeout.The update to use an object parameter for
startWatchdogis consistent with earlier changes. However, the timeout value has increased significantly from 10000 to 120000 milliseconds.Please confirm if this extended timeout (2 minutes) is intentional and necessary for this operation. If it's a typo, consider reverting to the previous timeout value.
1084-1084: LGTM! Verify the extended timeout.The update to use an object parameter for
startWatchdogis consistent with earlier changes. The timeout value is set to 180000 milliseconds (3 minutes).Please confirm if this extended timeout (3 minutes) is intentional and necessary for the login process. If it's excessive, consider reducing it to improve user experience.
Line range hint
1-1084: Overall changes look good, but consider reviewing timeout values.The updates to
startWatchdogcalls improve consistency across the codebase. However, there are significant variations in timeout values:
connectCMD: 10000 mssubscribeCMD(first occurrence): 10000 mssubscribeCMD(second occurrence): 120000 msloginCMD: 180000 msConsider reviewing these timeout values to ensure they are appropriate for each operation. If possible, define constants for these timeout values at the top of the file or in a configuration file for easier management and consistency.
To help verify the appropriateness of these timeout values, you could add logging or telemetry to measure the actual time taken for these operations in real-world usage. This data can help fine-tune the timeout values for optimal user experience.
src/shell.ts
Outdated
| @@ -316,7 +316,7 @@ export class Shell { | |||
| this.watchdogResolve = resolve | |||
| this.startHourglass(timeout) | |||
There was a problem hiding this comment.
Thanks for the PR!
the startHourglass is where the progress bar is printed and it also needs to be updated: receive the prefix and print it before the progress bar.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/shell.ts (2 hunks)
🔇 Additional comments (2)
src/shell.ts (2)
319-319: LGTM! Improved logging readability.The use of template literals improves code readability.
309-317: LGTM! Verify callers ofstartWatchdog.The parameter changes improve the API by using named parameters, making it more flexible and self-documenting. However, this is a breaking change that requires updating existing callers.
Let's verify the impact on existing callers:
✅ Verification successful
Based on the
rgresults, I can now generate the final response as I have enough information to verify the impact on callers.All callers of
startWatchdoghave been properly updated to use the new object parameter syntaxThe verification shows that all callers across the codebase (
src/peerbook.ts,src/commands.ts,src/terminal7.ts, andsrc/gate.ts) are using the new object parameter syntax correctly. Examples:
- Using with timeout:
startWatchdog({ timeout: 3000 })- Using without parameters:
startWatchdog()- Using with configurable timeout:
startWatchdog({ timeout })🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to startWatchdog to ensure they're updated # Expected: All calls should use the new object parameter syntax rg "startWatchdog" --type tsLength of output: 930
src/shell.ts
Outdated
| startHourglass({ timeout, prefix = 'Waiting' } : { timeout: number; prefix?: string } ) { | ||
| if (this.timer) return | ||
| this.map.showLog(true) | ||
| const len = 20, | ||
| interval = timeout / len | ||
| let i = 0 | ||
| this.timer = window.setInterval(() => { | ||
| const dots = Math.max(0, len - i) // i should never be > len, but just in case | ||
| this.t.write(`\r\x1B[K${" ".repeat(i)}ᗧ${"·".repeat(dots)}🍒\x1B[?25l`) | ||
| this.t.write(`\r\x1B[K${prefix}${" ".repeat(i)}ᗧ${"·".repeat(dots)}🍒\x1B[?25l`) |
There was a problem hiding this comment.
Consider making parameter types consistent with startWatchdog.
While the changes implement the requested prefix feature, there's an inconsistency in how parameters are handled between startWatchdog and startHourglass:
- In
startWatchdog, bothtimeoutandprefixare optional - In
startHourglass,timeoutis required butprefixis optional
This could lead to confusion since these methods are closely related.
Consider making both parameters optional in startHourglass to match startWatchdog:
- startHourglass({ timeout, prefix = 'Waiting' } : { timeout: number; prefix?: string } ) {
+ startHourglass({ timeout, prefix = 'Waiting' } : { timeout?: number; prefix?: string } = {} ) {
if (this.timer) return
this.map.showLog(true)
const len = 20,
- interval = timeout / len
+ interval = (timeout || terminal7.conf.net.timeout) / lenCommittable suggestion was skipped due to low confidence.
|
Hi @daonb, |
Summary by CodeRabbit
New Features
startWatchdogandstartHourglassmethods for better flexibility and clarity in timeout handling with customizable prefixes.Bug Fixes
Documentation