fix(regulations-admin): Update text amending #16859
Conversation
thordurhhh
added
the
deprecated:automerge
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![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 (2)
libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (2)
Line range hint
147-186: Add defensive checks for empty arrays in title formattingWhile the logic for formatting titles is correct, consider adding defensive checks for empty arrays to prevent unnecessary prefixes.
Consider this improvement:
const amendingTitles = uniq( amendingArray.map( (item) => `${removeRegNamePrefix(item.name)} ${removeRegPrefix(item.regTitle)}`, ), ) - const prefixedAmendingTitles = amendingTitles.map( - (title, i) => `${i === 0 ? `${PREFIX_AMENDING}` : ''}${title}`, - ) + const prefixedAmendingTitles = amendingTitles.length + ? amendingTitles.map( + (title, i) => `${i === 0 ? `${PREFIX_AMENDING}` : ''}${title}`, + ) + : [] - const repealTitles = repealArray.map( - (item, i) => - `${i === 0 ? `${PREFIX_REPEALING}` : ''}${removeRegNamePrefix( - item.name, - )} ${removeRegPrefix(item.regTitle)}`, - ) + const repealTitles = repealArray.length + ? repealArray.map( + (item, i) => + `${i === 0 ? `${PREFIX_REPEALING}` : ''}${removeRegNamePrefix( + item.name, + )} ${removeRegPrefix(item.regTitle)}`, + ) + : []
Line range hint
1-11: Enhance type safety with explicit return typesWhile the TypeScript usage is good, consider adding explicit return type annotations to exported functions for better type safety and documentation.
Example improvement:
export const formatAmendingRegTitle = (draft: RegDraftForm): string => { // ... } export const formatAmendingRegBody = ( name: string, repeal?: boolean, diff?: HTMLText | string | undefined, regTitle?: string, appendixes?: AppendixDraftForm[], ): HTMLText[] => { // ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (1)
libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (1)
26-26: LGTM: Grammatical correction in the repealing prefix
The constant update improves the grammatical accuracy of the Icelandic text used for repealing regulation titles.
Datadog ReportAll test runs ✅ 99 Total Test Services: 0 Failed, 97 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (2)
Line range hint
1-27: Add explicit TypeScript type annotationsConsider adding explicit type annotations to improve type safety and code maintainability:
- export const formatAmendingRegTitle = (draft: RegDraftForm) => { + export const formatAmendingRegTitle = (draft: RegDraftForm): string => { - const formatAmendingRegBody = ( + export const formatAmendingRegBody = ( name: string, repeal?: boolean, diff?: HTMLText | string | undefined, regTitle?: string, appendixes?: AppendixDraftForm[], -) => { +): HTMLText[] => {
Line range hint
196-200: Improve error handling for undefined diff parameterWhen
repealis false anddiffis undefined, the function silently returns an empty array. Consider adding validation or logging to handle this case explicitly.if (!diff) { + console.warn('Diff is undefined for non-repeal regulation'); return [] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (2)
26-26: LGTM: Grammatical correction in repealing prefix
The modification to PREFIX_REPEALING improves the grammatical structure of the repealing regulation titles in Icelandic.
Line range hint 1-11: LGTM: Effective tree-shaking practices
The code follows good practices for tree-shaking and reusability:
- Individual lodash imports enable better tree-shaking
- Named exports allow for selective imports
- Utility functions are modular and reusable
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Update text amending
Why
its not correct.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes