feat(cli): show scan duration in terminal summaries#141
Merged
Conversation
Signed-off-by: Nikhil Gupta <100pranjalgupta@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA new ChangesHuman-readable duration formatting
Estimated code review effort: 1 (Trivial) | ~4 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 (1)
src/commands/utils/format-duration.ts (1)
1-7: 💤 Low valueConsider adding input validation for robustness.
The function doesn't validate that
msis a non-negative, finite number. While the current callers (scan duration measurements) should always provide valid values, defensive validation would prevent unexpected output if the function is reused elsewhere.🛡️ Optional: Add input validation
export function formatDuration(ms: number): string { + if (!Number.isFinite(ms) || ms < 0) { + return "0ms"; + } + if (ms < 1000) { return `${Math.round(ms)}ms`; } return `${(ms / 1000).toFixed(1)}s`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/utils/format-duration.ts` around lines 1 - 7, The formatDuration function lacks input validation and does not check whether the ms parameter is a non-negative, finite number, which could lead to unexpected output if the function is reused with invalid inputs. Add validation at the beginning of the formatDuration function to ensure the ms parameter is a valid non-negative finite number, and throw an appropriate error or return a default value if validation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/tests/audit.test.ts`:
- Line 121: The regex pattern in the expect statement for r.stderr is checking
the output of formatDuration but does not account for decimal milliseconds.
Update the regex pattern to allow optional decimal values in the milliseconds
portion (e.g., 123.456ms) by making the decimal part optional after the integer
digits in the milliseconds alternative. The seconds pattern already handles
decimals correctly, so only the milliseconds pattern needs to be updated to
include an optional decimal component.
In `@src/commands/tests/cli.test.ts`:
- Line 113: The regex pattern in the expect statement for stderr matching at
line 113 does not account for decimal milliseconds that formatDuration could
produce. Update the regex pattern from `/\d+ms|\d+\.\d+s/` to handle both
integer and decimal milliseconds (e.g., 123ms or 123.456ms) in addition to the
already-supported decimal seconds format. Modify the milliseconds portion of the
regex to make the decimal part optional, allowing it to match patterns like
\d+\.?\d*ms to capture both whole and decimal millisecond values.
In `@src/commands/utils/format-duration.ts`:
- Around line 2-3: In the format-duration.ts file, the milliseconds output in
the condition where ms < 1000 is not rounding decimal values to integers. This
causes the output to potentially return decimal milliseconds (e.g.,
`123.456ms`), which does not match the test regex pattern expecting integer
milliseconds and is inconsistent with how seconds are formatted. Round the ms
value to an integer before including it in the return string template to ensure
both consistency in formatting and compatibility with the existing test regex
patterns in cli.test.ts and audit.test.ts.
---
Nitpick comments:
In `@src/commands/utils/format-duration.ts`:
- Around line 1-7: The formatDuration function lacks input validation and does
not check whether the ms parameter is a non-negative, finite number, which could
lead to unexpected output if the function is reused with invalid inputs. Add
validation at the beginning of the formatDuration function to ensure the ms
parameter is a valid non-negative finite number, and throw an appropriate error
or return a default value if validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fcc5bf7-3193-47e1-abdf-47d8719d19de
📒 Files selected for processing (5)
src/commands/audit.tssrc/commands/scan.tssrc/commands/tests/audit.test.tssrc/commands/tests/cli.test.tssrc/commands/utils/format-duration.ts
Contributor
Author
|
Hi @Krishan27 , I've synced the branch with the latest Please let me know if you'd like any changes regarding the CodeRabbit suggestions or anything else in the implementation. Happy to update the PR if needed. Thanks for your time and review. |
Contributor
Author
|
Hi @Krishan27, @flaglint I've synced this branch with the latest main, resolved the merge conflict, and pushed the updated branch. All CI checks and the full test suite are passing after the merge. The PR should now be up to date. If you'd like any changes, including addressing the earlier CodeRabbit suggestions, I'm happy to update it. Thanks for your time and review. |
…mary - formatDuration: add Math.round() so decimal inputs (e.g. from high-res timers) produce clean integer output like 42ms rather than 42.456ms. Fixes the test regex /\d+ms/ which expects no decimal point. - audit.ts: swap process.stderr.write → stderrInfo on the Audit complete summary line so --quiet suppresses it, consistent with scan.ts. Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
Owner
|
@100NikhilBro /lgtm merged |
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
3 participants
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.
Summary
Adds scan duration information to terminal summaries for both
scanandauditcommands.Changes
Added a reusable
formatDuration()helperUpdated
scancommand summary to include:Updated
auditcommand summary to include:Added CLI test coverage for duration formatting
Added audit test coverage for duration formatting
Verification
This change only affects terminal summary output and does not modify report formats or scanner behavior.
Summary by CodeRabbit
formatDuration(ms)utility to standardize command timing display.