refactor: use replaceAll in breadcrumbs#3282
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduced top-level constants Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 Fix all issues with AI agents
In @frontend/src/utils/breadcrumb.ts:
- Around line 1-6: Fix the JSDoc block for the breadcrumb formatter by removing
the extra asterisk on the parameter line: change the malformed "* * @param" to
the correct "* @param" in the JSDoc for the function that formats breadcrumb
titles (the comment above the breadcrumb formatting function), ensuring the rest
of the comment lines keep a single leading asterisk and proper spacing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/utils/breadcrumb.ts
🔇 Additional comments (2)
frontend/src/utils/breadcrumb.ts (2)
13-13: LGTM: Clean refactoring toreplaceAll.The change from
match.replace(/-/g, DATE_TOKEN)tomatch.replaceAll('-', DATE_TOKEN)is functionally equivalent and improves readability. This aligns with the ES2021 standard and addresses the SonarCloud code-smell as intended.
18-18: LGTM: Cleaner token restoration withreplaceAll.Replacing
segment.replace(new RegExp(DATE_TOKEN, 'g'), '-')withsegment.replaceAll(DATE_TOKEN, '-')removes the RegExp constructor complexity while maintaining identical behavior. This is exactly the improvement the PR set out to achieve.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/utils/breadcrumb.ts (1)
17-20: (Optional) Consider hardening againstDATE_TOKENappearing in input.
If a user-provided breadcrumb contains__DATE_HYPHEN__, it’ll be “restored” into-. If that’s possible in your domain, use a less-collidable sentinel (e.g., include uncommon delimiters) or generate a token that’s guaranteed absent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/utils/breadcrumb.ts
🔇 Additional comments (2)
frontend/src/utils/breadcrumb.ts (2)
1-6: Doc comment examples match the function behavior.
Nice to have the “date protection” example inline; it mirrors the implementation.
10-13: No action needed. The TypeScript configuration targets ES2023 (frontend/tsconfig.json), which fully supportsreplaceAll()(introduced in ES2021). No polyfill or transpilation is required for this code.Likely an incorrect or invalid review comment.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/utils/breadcrumb.ts (1)
1-2: LGTM! Constants correctly defined.The top-level
DATE_TOKENandDATE_PATTERNconstants improve readability and align with the refactoring objectives. Thegflag onDATE_PATTERNis required byreplaceAll()when using a regex—without it, a TypeError would be thrown.📝 Optional: Document the global flag requirement
Consider adding a brief comment to clarify why the
gflag is necessary:+// Note: The 'g' flag is required by String.replaceAll() when using a RegExp const DATE_PATTERN = /\d{4}-\d{1,2}(?:-\d{1,2})?/gThis can help future maintainers understand the constraint.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/utils/breadcrumb.ts
🔇 Additional comments (3)
frontend/src/utils/breadcrumb.ts (3)
14-14: LGTM! Date protection logic correctly refactored.The use of
replaceAll(DATE_PATTERN, ...)with a callback successfully protects date hyphens by replacing them withDATE_TOKEN. The nestedmatch.replaceAll('-', DATE_TOKEN)using string search is clean and correct. This refactor achieves the objective of improved readability while preserving the original date-protection behavior.
19-19: LGTM! Date hyphen restoration is correct.The
replaceAll(DATE_TOKEN, '-')correctly restores original hyphens within date segments after splitting. This is cleaner and more readable than the previousnew RegExp(DATE_TOKEN, 'g')approach, and it preserves the intended behavior.
14-14: ES2021 support is confirmed and requires no action.The project's TypeScript configuration targets ES2023, which includes full support for
String.prototype.replaceAll()(introduced in ES2021). No transpilation layer or polyfills are needed, confirming the refactor is compatible with the target environment.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@saichethana28 this works great! Thank you!
* refactor: use replaceAll in breadcrumbs * docs: add comprehensive JSDoc and refactor logic to replaceAll * docs: fix JSDoc formatting and remove extra asterisk * refactor: final polish on spacing and docs * chore: trigger final fresh CI scan * refactor: optimize constants placement and use replaceAll * refactor: achieve full replaceAll consistency and optimize regex
2 participants
Proposed change
Resolves #3253
I noticed a SonarCloud "Code Smell" regarding the use of global regular expressions for simple string replacements in our breadcrumb utility. This PR refactors that logic to use the modern
.replaceAll()method.What I did:
.replace(/-/g, ...)andnew RegExp(DATE_TOKEN, 'g')for the much cleaner.replaceAll().Why it’s better:
It makes the code a lot easier to read and gets rid of the extra complexity of the
RegExpconstructor. Plus, it moves us toward more modern ES2021 standards which are safer and more performant for this kind of work.Checklist
make check-testlocally and all tests passed