chore(j-s): adding new client logic for offenses #17857
Conversation
…s/offense/rename
…and.is into j-s/create-new-offenses-structure
WalkthroughThis pull request enables the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
|
|
View your CI Pipeline Execution ↗ for commit 3dcb43f.
☁️ Nx Cloud last updated this comment at |
…s/create-new-offenses-structure
…s/create-new-offenses-structure
…s/create-new-offenses-structure
...-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescriptionReason.ts
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDecriptionReason.spec.ts (1)
1-126:⚠️ Potential issueFix typo in filename.
The filename contains a typo:
getIncidentDecriptionReasonshould begetIncidentDescriptionReason(missing 's' in 'Description').
🧹 Nitpick comments (23)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Substances/DeprecatedSubstances.tsx (3)
35-47: Simplify disabled state logic.The disabled state check can be simplified using optional chaining and nullish coalescing.
- disabled: indictmentCount.substances - ? indictmentCount.substances[sub] !== undefined - : false, + disabled: indictmentCount.substances?.[sub] !== undefined ?? false,
112-116: Simplify amount access with optional chaining.The amount access logic can be simplified using optional chaining.
- amount={ - (indictmentCount.substances && - indictmentCount.substances[substance]) || - '' - } + amount={indictmentCount.substances?.[substance] ?? ''}🧰 Tools
🪛 Biome (1.9.4)
[error] 113-114: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
109-121: Remove unnecessary braces in JSX.The braces around the Substance component are unnecessary.
- { <Substance substance={substance} amount={amount} onUpdateAmount={handleUpdateSubstanceAmount} onDelete={handleDeleteSubstance} - ></Substance> - } + />🧰 Tools
🪛 Biome (1.9.4)
[error] 113-114: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescription.ts (1)
66-154: Consider verifying placeholders for i18n completeness.
Most placeholders like[Vettvangur]and[Dagsetning]appear to be placeholders for missing data. Double-check that all relevant placeholders are captured and localized as needed. Otherwise, no major issues identified.apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (1)
564-835: Test coverage for new offense-based logic.
The conditional display of the new<Offenses />component vs. legacy code is well structured. Ensure thorough integration tests to verify both paths—particularly the feature-flag-enabled scenario.apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescriptionReason.ts (2)
17-61: Consider extracting string concatenation logic.The offense description concatenation logic is complex and could benefit from being extracted into a separate helper function for better maintainability.
+const getOffenseDescription = ( + offense: Offense, + index: number, + offenses: Offense[], + formatMessage: IntlShape['formatMessage'], +) => { + let description = '' + if ( + (offenses.length > 1 && index === offenses.length - 1) || + (offenses.length > 2 && + index === offenses.length - 2 && + offense.offense === IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING) + ) { + description += ' og ' + } else if (index > 0) { + description += ', ' + } + switch (offense.offense) { + // ... switch cases ... + } + return description +} export const getIncidentDescriptionReason = ( offenses: Offense[], formatMessage: IntlShape['formatMessage'], ) => { - let reason = offenses + let reason = offenses .filter((o) => o.offense !== IndictmentCountOffense.SPEEDING) - .reduce((acc, o, index) => { - if ( - (offenses.length > 1 && index === offenses.length - 1) || - (offenses.length > 2 && - index === offenses.length - 2 && - o.offense === IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING) - ) { - acc += ' og ' - } else if (index > 0) { - acc += ', ' - } - switch (o.offense) { - // ... switch cases ... - } - return acc - }, '') + .reduce((acc, o, index) => + acc + getOffenseDescription(o, index, offenses, formatMessage), '')
63-83: Simplify substance handling with array methods.The substance handling logic could be simplified using array methods for better readability.
- const substances = offenses - .filter((o) => o.substances) - .flatMap(({ substances }) => Object.entries(substances as SubstanceMap)) - reason += substances.reduce((acc, substance, index) => { + const substanceEntries = offenses + .filter((o) => o.substances) + .flatMap(({ substances }) => Object.entries(substances as SubstanceMap)) + + if (substanceEntries.length === 0) return reason + + reason += ` (${formatMessage(strings.incidentDescriptionSubstancesPrefixAutofill)} ` + + substanceEntries + .map((substance, index) => + formatMessage(substanceStrings[substance[0] as Substance], { + amount: substance[1], + })) + .join(index === substanceEntries.length - 1 ? ' og ' : ', ') + + ')'apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.ts (1)
13-90: Consider adding deprecation notice.Since this file is marked for removal, add a deprecation notice to warn other developers.
+/** + * @deprecated This function will be removed in favor of getIncidentDescriptionReason. + * Use getIncidentDescriptionReason with the new Offense type instead. + */ export const getDeprecatedIncidentDescriptionReason = ( deprecatedOffenses: IndictmentCountOffense[], substances: SubstanceMap, formatMessage: IntlShape['formatMessage'], ) => {apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Substances/Substances.tsx (2)
98-98: Use optional chaining for safer access.Replace the logical OR with optional chaining for better type safety.
- amount={(substances && substances[substance]) || ''} + amount={substances?.[substance] ?? ''}🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
89-106: Consider extracting substance list to a separate component.The substance list rendering logic could be extracted to improve readability and maintainability.
+interface SubstanceListProps { + substances: SubstanceMap + offenseType: string + onUpdateAmount: (substanceId: SubstanceEnum, amount: string) => void + onDelete: (substanceId: SubstanceEnum) => void +} + +const SubstanceList: FC<SubstanceListProps> = ({ + substances, + offenseType, + onUpdateAmount, + onDelete, +}) => ( + <div className={styles.gridRow}> + {(Object.keys(substances) as SubstanceEnum[]) + .filter((s) => offenseSubstances[offenseType].includes(s)) + .map((substance) => ( + <div key={substance}> + <Substance + substance={substance} + amount={substances?.[substance] ?? ''} + onUpdateAmount={onUpdateAmount} + onDelete={onDelete} + /> + </div> + ))} + </div> +)🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.spec.ts (1)
6-120: Add test case for substance handling.The test suite lacks coverage for the substance handling logic. Consider adding a test case before removal.
test('should return a description with substances', () => { const offenses = [IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING] const substances = { [Substance.AMPHETAMINE]: '0.5', [Substance.COCAINE]: '0.3', } const result = getDeprecatedIncidentDescriptionReason( offenses, substances, formatMessage, ) expect(result).toContain('amfetamín 0,5') expect(result).toContain('kókaín 0,3') })apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.spec.tsx (1)
4-4: Consider using a relative import path.The import path
@island.is/judicial-system-web/src/graphql/schemaappears to be an absolute path. Consider using a relative import path for better maintainability and to avoid potential path resolution issues.-import { IndictmentCountOffense as offense } from '@island.is/judicial-system-web/src/graphql/schema' +import { IndictmentCountOffense as offense } from '../../../../graphql/schema'apps/judicial-system/api/infra/judicial-system-api.ts (1)
49-51: Consider a phased rollout strategy.Enabling the
OFFENSE_ENDPOINTSfeature flag across all environments (dev, staging, prod) simultaneously could be risky. Consider a phased rollout strategy by enabling it first in dev, then staging, and finally in production after thorough testing.apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/SpeedingOffenseFields.tsx (1)
55-55: Specify radix parameter in parseInt calls.When using
parseInt, it's best practice to specify the radix parameter to ensure consistent parsing behavior across browsers.-const recordedSpeed = parseInt(event.target.value) +const recordedSpeed = parseInt(event.target.value, 10)Also applies to: 71-71, 115-115
apps/judicial-system/web/src/utils/hooks/useIndictmentCounts/index.ts (1)
109-110: Add type annotations for better code clarity.Consider adding explicit type annotations to improve code readability and maintainability:
- const updatedOffenses = [...offenses, newOffense] + const updatedOffenses: Offense[] = [...offenses, newOffense] - const updatedOffenses = offenses.filter((o) => o.id !== offenseId) + const updatedOffenses: Offense[] = offenses.filter((o) => o.id !== offenseId)Also applies to: 126-126
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescription.spec.ts (2)
8-8: Move feature flag constant to a shared location.Consider moving the
IS_OFFENSE_ENDPOINT_ENABLEDconstant to a shared configuration file to maintain consistency across tests.
42-45: Improve type safety by using type assertions consistently.Instead of using type assertion with
as, consider using type predicates or type guards for better type safety:-] as Offense[], +] satisfies Offense[],This change should be applied to all similar occurrences in the test file.
Also applies to: 77-80, 103-105, 132-135
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/Offenses.tsx (3)
29-40: Optimize the sorting function for better performance.The current sorting implementation can be simplified:
-const sortByCreatedDate = (o1: Offense, o2: Offense) => { - if (!(o1.created && o2.created)) { - return 0 - } - if (o1.created < o2.created) { - return -1 - } - if (o1.created > o2.created) { - return 1 - } - return 0 -} +const sortByCreatedDate = (o1: Offense, o2: Offense) => { + return (o1.created && o2.created) ? o1.created.localeCompare(o2.created) : 0 +}
91-94: Remove unnecessary useMemo for simple array access.The
useMemohook is unnecessary here as it's just accessing an array property:- const offenses = useMemo( - () => indictmentCount.offenses ?? [], - [indictmentCount.offenses], - ) + const offenses = indictmentCount.offenses ?? []
239-240: Enhance input validation for blood alcohol content.The current mask validation could be improved:
- mask={'9,99'} - maskPlaceholder={null} + mask={'9,99'} + maskPlaceholder={null} + beforeMaskedStateChange={({ nextState }) => { + const value = nextState.value + if (value && !value.match(/^[0-9],[0-9]{0,2}$/)) { + return { value: '', selection: { start: 0, end: 0 } } + } + return nextState + }}apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (1)
139-157: Extract offense check logic into a separate function.Consider extracting the offense check logic for better maintainability:
+const hasDriversLicenseOffense = (offense: IndictmentCountOffense) => + [ + IndictmentCountOffense.DRUNK_DRIVING, + IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING, + IndictmentCountOffense.PRESCRIPTION_DRUGS_DRIVING, + ].includes(offense) +const checkDriversLicenseSuspension = ( + indictmentCounts: TIndictmentCount[], + isOffenseEndpointEnabled: boolean, +) => + isOffenseEndpointEnabled + ? indictmentCounts?.some((count) => + count.offenses?.some((o) => hasDriversLicenseOffense(o.offense)), + ) + : indictmentCounts?.some((count) => + count.deprecatedOffenses?.some(hasDriversLicenseOffense), + ) - const requestDriversLicenseSuspension = isOffenseEndpointEnabled - ? indictmentCounts?.some((count) => { - return count.offenses?.some((o) => - [ - IndictmentCountOffense.DRUNK_DRIVING, - IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING, - IndictmentCountOffense.PRESCRIPTION_DRUGS_DRIVING, - ].includes(o.offense), - ) - }) - : indictmentCounts?.some((count) => - count.deprecatedOffenses?.some((offense) => - [ - IndictmentCountOffense.DRUNK_DRIVING, - IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING, - IndictmentCountOffense.PRESCRIPTION_DRUGS_DRIVING, - ].includes(offense), - ), - ) + const requestDriversLicenseSuspension = checkDriversLicenseSuspension( + indictmentCounts, + isOffenseEndpointEnabled, + )apps/judicial-system/web/src/utils/validate.ts (1)
320-330: Consider adding JSDoc to document the helper function.The
hasOffenseshelper function is well-structured but lacks documentation explaining its purpose and return value.Add JSDoc documentation:
+/** + * Checks if an indictment count has any offenses, supporting both new and deprecated structures. + * @param indictmentCount - The indictment count to check + * @returns boolean - True if the indictment count has any offenses + */ const hasOffenses = (indictmentCount: IndictmentCount) => {charts/judicial-system/values.prod.yaml (1)
33-33: Verify production readiness before enabling the feature.The
OFFENSE_ENDPOINTSfeature is being enabled in the production environment. Before deploying, please ensure:
- The feature has been thoroughly tested in staging.
- A rollback plan is in place.
- All dependent components are ready for production.
Consider implementing a gradual rollout strategy using a percentage-based feature flag if high traffic is expected.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/judicial-system/api/infra/judicial-system-api.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsx(4 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx(7 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.spec.tsx(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx(12 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/Offenses.tsx(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/SpeedingOffenseFields.tsx(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Substances/DeprecatedSubstances.tsx(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Substances/Substances.tsx(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.spec.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDecriptionReason.spec.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescription.spec.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescription.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescriptionReason.ts(1 hunks)apps/judicial-system/web/src/utils/hooks/useIndictmentCounts/index.ts(3 hunks)apps/judicial-system/web/src/utils/validate.ts(1 hunks)charts/judicial-system-services/judicial-system-api/values.dev.yaml(1 hunks)charts/judicial-system-services/judicial-system-api/values.prod.yaml(1 hunks)charts/judicial-system-services/judicial-system-api/values.staging.yaml(1 hunks)charts/judicial-system/values.dev.yaml(1 hunks)charts/judicial-system/values.prod.yaml(1 hunks)charts/judicial-system/values.staging.yaml(1 hunks)libs/judicial-system/types/src/lib/feature.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`libs/**/*`: "Confirm that the code adheres to the following...
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."
libs/judicial-system/types/src/lib/feature.ts
`apps/**/*`: "Confirm that the code adheres to the following...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/api/infra/judicial-system-api.tsapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.spec.tsapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDecriptionReason.spec.tsapps/judicial-system/web/src/utils/hooks/useIndictmentCounts/index.tsapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescription.spec.tsapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Substances/DeprecatedSubstances.tsxapps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsxapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/SpeedingOffenseFields.tsxapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.tsapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsxapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.spec.tsxapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescription.tsapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/Offenses.tsxapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Substances/Substances.tsxapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescriptionReason.tsapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsxapps/judicial-system/web/src/utils/validate.ts
🧠 Learnings (6)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/SpeedingOffenseFields.tsx (2)
Learnt from: oddsson
PR: island-is/island.is#17589
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:0-0
Timestamp: 2025-01-21T15:32:24.472Z
Learning: In the judicial-system web app, speed-related values (recordedSpeed, speedLimit) should be stored and handled as strings rather than numbers to maintain consistency with other similar fields and the backend data model.
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.spec.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/Offenses.tsx (2)
Learnt from: oddsson
PR: island-is/island.is#16731
File: apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx:172-186
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `IndictmentOverview.tsx`, since the defendants data does not change, using `useMemo` to memoize the filtering logic is unnecessary.
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (3)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
Learnt from: thorhildurt
PR: island-is/island.is#17198
File: apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx:42-50
Timestamp: 2024-12-11T14:25:44.741Z
Learning: In `IndictmentOverview.tsx`, when updating the punishment type, update the UI state before making the API call to immediately reflect the change.
Learnt from: oddsson
PR: island-is/island.is#16731
File: apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx:172-186
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `IndictmentOverview.tsx`, since the defendants data does not change, using `useMemo` to memoize the filtering logic is unnecessary.
apps/judicial-system/web/src/utils/validate.ts (1)
Learnt from: oddsson
PR: island-is/island.is#17589
File: apps/judicial-system/web/src/utils/validate.ts:303-308
Timestamp: 2025-01-21T15:32:39.384Z
Learning: In the judicial system web app, speed-related fields (recordedSpeed and speedLimit) in indictment counts should use simple Boolean validation checks rather than complex format/range validation.
🪛 Biome (1.9.4)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Substances/DeprecatedSubstances.tsx
[error] 113-114: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Substances/Substances.tsx
[error] 98-98: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
[error] 320-320: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (21)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getIncidentDescription.ts (2)
24-47: Straightforward helper function.
This function cleanly derives the reason and determines if speeding applies, referencing deprecated offenses.
49-64: Clear structure for offense-based description.
This helper provides a nice separation of logic for new offenses.apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (4)
72-73: Expanded function signature looks correct.
Adding theupdatedOffensesparameter inonChangealigns with the new logic.
79-80: Signature change for updateIndictmentCountState is consistent.
No issues spotted in the updated signature.
303-305: Good extension of handleIndictmentCountChanges arguments.
The addition ofupdatedOffensesis well integrated here to unify logic for old and new offenses.
372-396: Confirm async iteration consistency in handleSubtypeChange.
The async call todeleteOffensewithin aforEachis generally safe here, but be aware thatforEachwon’t await all deletions before proceeding. If sequential or fully awaited deletions are required, consider refactoring to afor ofloop orPromise.all().libs/judicial-system/types/src/lib/feature.ts (1)
3-3: Feature addition looks good.
No concerns about addingOFFENSE_ENDPOINTSto the enum.apps/judicial-system/web/src/utils/hooks/useIndictmentCounts/index.ts (2)
7-10: LGTM!Clean import organization for the new
Offensetype.
123-127: LGTM!The state update logic correctly handles the optional
updatedOffensesparameter using the spread operator.apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/Offenses.tsx (1)
98-106: LGTM!Good use of
useMemofor computing offense options, as it involves transformation and depends on multiple dependencies.apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (2)
84-85: LGTM!Clean implementation of feature flag using the
FeatureContext.
242-248: LGTM!The state update logic correctly handles the optional
updatedOffensesparameter when mapping indictment counts.apps/judicial-system/web/src/utils/validate.ts (2)
303-318: LGTM! The function correctly handles both new and deprecated offense structures.The implementation aligns with the learning from PR #17589 by using simple Boolean validation for speed-related fields.
331-338: LGTM! The validation logic is comprehensive.The function maintains all necessary validation checks while incorporating the new offense structure.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsx (2)
107-109: LGTM! Feature flag is properly implemented.The feature context is correctly retrieved and checked at the component level.
305-311: LGTM! The function call is updated to use named parameters.The update to use object parameters improves code readability and maintainability.
charts/judicial-system-services/judicial-system-api/values.dev.yaml (1)
33-33: LGTM! Feature flag is consistently enabled.The OFFENSE_ENDPOINTS feature is properly enabled in the development environment.
charts/judicial-system-services/judicial-system-api/values.prod.yaml (1)
33-33: LGTM! Feature flag is consistently enabled across environments.The OFFENSE_ENDPOINTS feature is properly enabled in the production environment, maintaining consistency with other environments.
charts/judicial-system-services/judicial-system-api/values.staging.yaml (1)
33-33: Verify the feature flag implementation.The
OFFENSE_ENDPOINTSfeature is being enabled in the staging environment. Please ensure that:
- The feature flag is properly handled in the codebase.
- The feature has been thoroughly tested in lower environments.
Run the following script to verify the feature flag implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the feature flag implementation. # Test: Search for feature flag usage. Expect: Feature flag checks in components. rg -A 5 $'OFFENSE_ENDPOINTS'Length of output: 6623
OFFENSE_ENDPOINTS feature flag implementation verified.
- The configuration in
charts/judicial-system-services/judicial-system-api/values.staging.yamlcorrectly setsHIDDEN_FEATURESto'OFFENSE_ENDPOINTS'.- Multiple components in both the API (e.g.,
apps/judicial-system/api/infra/judicial-system-api.ts) and the front-end (e.g., usage inapps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsxand other related files) are checking for this feature flag.- The same flag is consistently enabled in dev, staging, and production configurations, supporting the intended feature rollout.
Please ensure that end-to-end tests in lower environments confirm the expected behavior.
charts/judicial-system/values.dev.yaml (1)
33-33: LGTM! Configuration is consistent across environments.The
OFFENSE_ENDPOINTSfeature flag is consistently enabled in the development environment, aligning with the configuration in other environments.charts/judicial-system/values.staging.yaml (1)
33-33: LGTM! Configuration is consistent across environments.The
OFFENSE_ENDPOINTSfeature flag is consistently enabled in the staging environment, aligning with the configuration in other environments.
...-system/web/src/routes/Prosecutor/Indictments/Indictment/Substances/DeprecatedSubstances.tsx
Show resolved
Hide resolved
...-system/web/src/routes/Prosecutor/Indictments/Indictment/Substances/DeprecatedSubstances.tsx
Show resolved
Hide resolved
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
Outdated
Show resolved
Hide resolved
...l-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/SpeedingOffenseFields.tsx
Show resolved
Hide resolved
...l-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/SpeedingOffenseFields.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (4)
251-252: Extract feature flag name to a constant.Consider extracting the feature flag name to a constant to maintain consistency and ease future updates:
+const FEATURE_FLAG = Feature.OFFENSE_ENDPOINTS; -const isOffenseEndpointEnabled = features.includes(Feature.OFFENSE_ENDPOINTS) +const isOffenseEndpointEnabled = features.includes(FEATURE_FLAG)
302-360: Simplify state updates and improve performance.The function handles both old and new offense logic well, but there are a few improvements possible:
Consider splitting the function into two separate handlers for better maintainability:
handleDeprecatedOffenseChangeshandleNewOffenseChangesThe substances reduction logic has been improved to use Object.assign, which is good for performance.
564-572: Consider memoizing the Offenses component.The Offenses component receives multiple props and might trigger unnecessary re-renders. Consider memoizing it:
+const MemoizedOffenses = memo(Offenses) -<Offenses +<MemoizedOffenses workingCase={workingCase} setWorkingCase={setWorkingCase} indictmentCount={indictmentCount} updateIndictmentCountState={updateIndictmentCountState} handleIndictmentCountChanges={handleIndictmentCountChanges} />
910-949: Optimize textarea handling for better performance.The incident description textarea might cause performance issues with frequent updates. Consider:
- Debouncing the onChange handler
- Using a controlled component pattern with local state
+const [localDescription, setLocalDescription] = useState(indictmentCount.incidentDescription ?? '') +const debouncedUpdate = useCallback( + debounce((value: string) => { + updateIndictmentCountState( + indictmentCount.id, + { incidentDescription: value }, + setWorkingCase, + ) + }, 300), + [indictmentCount.id, updateIndictmentCountState, setWorkingCase] +)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx(12 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`apps/**/*`: "Confirm that the code adheres to the following...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
🧠 Learnings (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (3)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
Learnt from: thorhildurt
PR: island-is/island.is#17198
File: apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx:42-50
Timestamp: 2024-12-11T14:25:44.741Z
Learning: In `IndictmentOverview.tsx`, when updating the punishment type, update the UI state before making the API call to immediately reflect the change.
Learnt from: oddsson
PR: island-is/island.is#16731
File: apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx:172-186
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `IndictmentOverview.tsx`, since the defendants data does not change, using `useMemo` to memoize the filtering logic is unnecessary.
🔇 Additional comments (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (1)
1-8: LGTM! Props interface and imports updated correctly.The changes properly introduce the new feature flag context and offense-related types, with appropriate updates to the Props interface to support both old and new offense handling.
Also applies to: 26-27, 34-36, 39-42, 65-81
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
Show resolved
Hide resolved
thorhildurt
force-pushed
the
j-s/offense/supporting-offense-property-in-client
branch
from
7b6bb20 to
0dd683f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (1)
142-160: Extract offense list to improve maintainability.The list of offenses that trigger driver's license suspension is duplicated. Consider extracting it to a constant.
+const DRIVER_LICENSE_SUSPENSION_OFFENSES = [ + IndictmentCountOffense.DRUNK_DRIVING, + IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING, + IndictmentCountOffense.PRESCRIPTION_DRUGS_DRIVING, +] as const; const requestDriversLicenseSuspension = isOffenseEndpointEnabled ? indictmentCounts?.some((count) => { return count.offenses?.some((o) => - [ - IndictmentCountOffense.DRUNK_DRIVING, - IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING, - IndictmentCountOffense.PRESCRIPTION_DRUGS_DRIVING, - ].includes(o.offense), + DRIVER_LICENSE_SUSPENSION_OFFENSES.includes(o.offense), ) }) : indictmentCounts?.some((count) => count.deprecatedOffenses?.some((offense) => - [ - IndictmentCountOffense.DRUNK_DRIVING, - IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING, - IndictmentCountOffense.PRESCRIPTION_DRUGS_DRIVING, - ].includes(offense), + DRIVER_LICENSE_SUSPENSION_OFFENSES.includes(offense), ), )apps/judicial-system/web/src/utils/validate.ts (1)
306-321: Extract common validation logic to improve maintainability.The speed validation logic could be more DRY. Consider extracting the common validation logic.
+const isSpeedingOffense = (offense: IndictmentCountOffense) => + offense === IndictmentCountOffense.SPEEDING; +const validateSpeedingFields = (indictmentCount: IndictmentCount) => + Boolean(indictmentCount.recordedSpeed) && Boolean(indictmentCount.speedLimit); const isValidSpeedingIndictmentCount = (indictmentCount: IndictmentCount) => { if (indictmentCount.offenses) { - return indictmentCount.offenses.some( - (o) => o.offense === IndictmentCountOffense.SPEEDING, - ) - ? Boolean(indictmentCount.recordedSpeed) && - Boolean(indictmentCount.speedLimit) - : true + return indictmentCount.offenses.some((o) => isSpeedingOffense(o.offense)) + ? validateSpeedingFields(indictmentCount) + : true } - return indictmentCount.deprecatedOffenses?.includes( - IndictmentCountOffense.SPEEDING, - ) - ? Boolean(indictmentCount.recordedSpeed) && - Boolean(indictmentCount.speedLimit) - : true + return indictmentCount.deprecatedOffenses?.some(isSpeedingOffense) + ? validateSpeedingFields(indictmentCount) + : true }apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.ts (3)
18-64: Refactor complex string concatenation logic for better maintainability.The current implementation combines filtering, reduction, and complex conditionals in a way that's difficult to maintain. Consider breaking this down into smaller, more focused functions.
Here's a suggested refactor:
const isLastOrSecondToLastDrugOffense = ( index: number, offenses: IndictmentCountOffense[], currentOffense: IndictmentCountOffense, ) => (offenses.length > 1 && index === offenses.length - 1) || (offenses.length > 2 && index === offenses.length - 2 && currentOffense === IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING) const getJoinWord = ( index: number, offenses: IndictmentCountOffense[], currentOffense: IndictmentCountOffense, ) => { if (isLastOrSecondToLastDrugOffense(index, offenses, currentOffense)) { return ' og ' } return index > 0 ? ', ' : '' } const getOffenseDescription = ( offense: IndictmentCountOffense, formatMessage: IntlShape['formatMessage'], hasIllegalDrugs: boolean, ) => { switch (offense) { // ... existing switch cases ... } } export const getDeprecatedIncidentDescriptionReason = ( deprecatedOffenses: IndictmentCountOffense[], substances: SubstanceMap, formatMessage: IntlShape['formatMessage'], ) => { const nonSpeedingOffenses = deprecatedOffenses.filter( (offense) => offense !== IndictmentCountOffense.SPEEDING, ) const hasIllegalDrugs = nonSpeedingOffenses.includes( IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING, ) return nonSpeedingOffenses.reduce((acc, offense, index) => { const joinWord = getJoinWord(index, nonSpeedingOffenses, offense) const description = getOffenseDescription(offense, formatMessage, hasIllegalDrugs) return acc + joinWord + description }, '') }
65-88: Enhance type safety for substance formatting.The substance formatting logic could benefit from stronger type safety and better separation of concerns.
Consider creating a dedicated type and helper for substance formatting:
type FormattedSubstance = { substance: Substance amount: number isLast: boolean isFirst: boolean } const formatSubstance = ( { substance, amount, isFirst, isLast }: FormattedSubstance, formatMessage: IntlShape['formatMessage'], ) => { const prefix = isFirst ? ` (${formatMessage(strings.incidentDescriptionSubstancesPrefixAutofill)} ` : '' const separator = isLast ? ' og ' : ', ' const suffix = isLast ? ')' : '' return ( prefix + (isFirst ? '' : separator) + formatMessage(substanceStrings[substance], { amount }) + suffix ) } // Usage in the main function: const substanceDescription = relevantSubstances .map(([substance, amount], index) => ({ substance, amount, isFirst: index === 0, isLast: index === relevantSubstances.length - 1, })) .map((params) => formatSubstance(params, formatMessage)) .join('')
89-90: Add input validation.Consider adding input validation to handle edge cases gracefully.
Add validation at the start of the function:
export const getDeprecatedIncidentDescriptionReason = ( deprecatedOffenses: IndictmentCountOffense[], substances: SubstanceMap, formatMessage: IntlShape['formatMessage'], ) => { + if (!deprecatedOffenses?.length) { + return '' + } + + if (!substances || !formatMessage) { + throw new Error('Missing required parameters') + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx(8 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.ts(1 hunks)apps/judicial-system/web/src/utils/validate.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`apps/**/*`: "Confirm that the code adheres to the following...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.tsapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsxapps/judicial-system/web/src/utils/validate.ts
🧠 Learnings (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
apps/judicial-system/web/src/utils/validate.ts (1)
Learnt from: oddsson
PR: island-is/island.is#17589
File: apps/judicial-system/web/src/utils/validate.ts:303-308
Timestamp: 2025-01-21T15:32:39.384Z
Learning: In the judicial system web app, speed-related fields (recordedSpeed and speedLimit) in indictment counts should use simple Boolean validation checks rather than complex format/range validation.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: linting (judicial-system-web,judicial-system-types,judicial-system-digital-mailbox-api,judicial-s...
- GitHub Check: build (judicial-system-backend)
- GitHub Check: build (judicial-system-api,judicial-system-digital-mailbox-api,judicial-system-message-handler,ju...
- GitHub Check: formatting
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: tests (judicial-system-api,judicial-system-auth,judicial-system-consts,judicial-system-digital-ma...
- GitHub Check: linting-workspace
🔇 Additional comments (5)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (2)
10-10: LGTM! Feature flag implementation follows best practices.The feature flag setup using React context follows NextJS best practices for feature toggling.
Also applies to: 14-14, 84-85
215-219: LGTM! State updates follow React best practices.The changes correctly handle offense updates and follow React best practices for state management.
Also applies to: 247-249, 257-257
apps/judicial-system/web/src/utils/validate.ts (2)
293-296: LGTM! Function signature update follows TypeScript best practices.The addition of the
isOffenseEndpointEnabledparameter is properly typed and follows TypeScript best practices.
323-337: LGTM! Offense validation logic is well-structured.The offense validation logic correctly handles the feature flag and follows TypeScript best practices.
Also applies to: 339-342
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.ts (1)
1-12: LGTM! Well-organized imports with proper type safety.The imports are properly organized with clear separation between external packages, shared types, and local modules.
...b/src/routes/Prosecutor/Indictments/Indictment/lib/getDeprecatedIncidentDescriptionReason.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/judicial-system/web/src/utils/formHelper.ts (1)
307-322:⚠️ Potential issueFix potential bug in
findFirstInvalidStep.The
findFirstInvalidStepfunction doesn't pass theisOffenseEndpointEnabledparameter when calling validation functions. This could lead to incorrect validation results for theINDICTMENTS_INDICTMENT_ROUTEstep.Apply this diff to fix the issue:
export const findFirstInvalidStep = (steps: string[], theCase: Case) => { const validations = stepValidations() const validationFunctions = Object.entries(validations) const stepsToCheck = validationFunctions.filter(([key]) => steps.includes(key), ) - if (stepsToCheck.every(([, validationFn]) => validationFn(theCase))) { + if (stepsToCheck.every(([key, validationFn]) => { + // Special handling for INDICTMENTS_INDICTMENT_ROUTE + if (key === constants.INDICTMENTS_INDICTMENT_ROUTE) { + return validationFn(theCase, true) // or get the flag from a context/config + } + return validationFn(theCase) + })) { return steps[steps.length - 1] } const [key] = - stepsToCheck.find(([, validationFn]) => !validationFn(theCase)) ?? [] + stepsToCheck.find(([key, validationFn]) => { + // Special handling for INDICTMENTS_INDICTMENT_ROUTE + if (key === constants.INDICTMENTS_INDICTMENT_ROUTE) { + return !validationFn(theCase, true) // or get the flag from a context/config + } + return !validationFn(theCase) + }) ?? [] return key }Note: The actual value of
isOffenseEndpointEnabledshould be obtained from your feature flag system or configuration.
🧹 Nitpick comments (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (1)
134-184: Enhance the driver's license suspension request logic.The current implementation has nested ternary operators which could be simplified for better readability and maintainability.
Consider refactoring to use early returns or a helper function:
-const requestDriversLicenseSuspension = isOffenseEndpointEnabled - ? indictmentCounts?.some((count) => { - return count.offenses?.some((o) => - [ - IndictmentCountOffense.DRUNK_DRIVING, - IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING, - IndictmentCountOffense.PRESCRIPTION_DRUGS_DRIVING, - ].includes(o.offense), - ) - }) - : indictmentCounts?.some((count) => - count.deprecatedOffenses?.some((offense) => - [ - IndictmentCountOffense.DRUNK_DRIVING, - IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING, - IndictmentCountOffense.PRESCRIPTION_DRUGS_DRIVING, - ].includes(offense), - ), - ) +const SUSPENSION_OFFENSES = [ + IndictmentCountOffense.DRUNK_DRIVING, + IndictmentCountOffense.ILLEGAL_DRUGS_DRIVING, + IndictmentCountOffense.PRESCRIPTION_DRUGS_DRIVING, +]; + +const hasOffenseRequiringSuspension = (count: TIndictmentCount) => { + if (isOffenseEndpointEnabled) { + return count.offenses?.some((o) => SUSPENSION_OFFENSES.includes(o.offense)); + } + return count.deprecatedOffenses?.some((offense) => SUSPENSION_OFFENSES.includes(offense)); +}; + +const requestDriversLicenseSuspension = indictmentCounts?.some(hasOffenseRequiringSuspension);apps/judicial-system/web/src/utils/validate.ts (1)
303-318: Simplify the speeding validation logic.The current implementation has nested conditionals that could be simplified.
Consider refactoring to use early returns:
-const isValidSpeedingIndictmentCount = (indictmentCount: IndictmentCount) => { - if (indictmentCount.offenses) { - return indictmentCount.offenses.some( - (o) => o.offense === IndictmentCountOffense.SPEEDING, - ) - ? Boolean(indictmentCount.recordedSpeed) && - Boolean(indictmentCount.speedLimit) - : true - } - return indictmentCount.deprecatedOffenses?.includes( - IndictmentCountOffense.SPEEDING, - ) - ? Boolean(indictmentCount.recordedSpeed) && - Boolean(indictmentCount.speedLimit) - : true -} +const isValidSpeedingIndictmentCount = (indictmentCount: IndictmentCount) => { + const hasSpeedingOffense = indictmentCount.offenses + ? indictmentCount.offenses.some((o) => o.offense === IndictmentCountOffense.SPEEDING) + : indictmentCount.deprecatedOffenses?.includes(IndictmentCountOffense.SPEEDING); + + if (!hasSpeedingOffense) { + return true; + } + + return Boolean(indictmentCount.recordedSpeed) && Boolean(indictmentCount.speedLimit); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx(8 hunks)apps/judicial-system/web/src/utils/formHelper.ts(2 hunks)apps/judicial-system/web/src/utils/hooks/useSections/index.ts(7 hunks)apps/judicial-system/web/src/utils/validate.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`apps/**/*`: "Confirm that the code adheres to the following...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/utils/hooks/useSections/index.tsapps/judicial-system/web/src/utils/validate.tsapps/judicial-system/web/src/utils/formHelper.tsapps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx
🧠 Learnings (2)
apps/judicial-system/web/src/utils/validate.ts (1)
Learnt from: oddsson
PR: island-is/island.is#17589
File: apps/judicial-system/web/src/utils/validate.ts:303-308
Timestamp: 2025-01-21T15:32:39.384Z
Learning: In the judicial system web app, speed-related fields (recordedSpeed and speedLimit) in indictment counts should use simple Boolean validation checks rather than complex format/range validation.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
🔇 Additional comments (8)
apps/judicial-system/web/src/utils/formHelper.ts (2)
176-176: LGTM! Type signature updated for feature flag support.The type signature update for
INDICTMENTS_INDICTMENT_ROUTEcorrectly adds the optionalisOffenseEndpointEnabledparameter to support the new offense endpoints feature.
258-259: LGTM! Implementation updated to match type signature.The implementation correctly passes the
isOffenseEndpointEnabledparameter to the validation function.apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (2)
10-14: LGTM! Clean feature context integration.The Feature context is properly integrated with clear imports and type definitions.
212-266: Verify the impact of updating offenses.The function now accepts an optional
updatedOffensesparameter which is used to update the indictment count. Ensure that all callers are updated to handle this new parameter correctly.Run this script to find all callers of
handleUpdateIndictmentCount:❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Find all callers of handleUpdateIndictmentCount ast-grep --pattern 'handleUpdateIndictmentCount($$$)'Length of output: 55
I'll run a query using a grep command to search for any references to
handleUpdateIndictmentCountin the repository. This will help us verify if there are any callers and confirm whether they pass the newupdatedOffensesparameter. Please verify the results manually if needed.
🏁 Script executed:
#!/bin/bash # Description: Search for all references to handleUpdateIndictmentCount using rg rg -n "handleUpdateIndictmentCount\\("Length of output: 39
Action Required: Manual Verification of Updated Offenses Parameter
It appears that our search for callers of
handleUpdateIndictmentCountdid not return any direct references. This suggests either the function is only used as an inline callback (or passed as a prop) within its own module, or it isn’t being invoked elsewhere in the repository. Please manually verify the following:
- Check if
handleUpdateIndictmentCountis passed to any child components or event handlers that might call it.- Confirm that any such consumers are updated (or will be updated) to correctly handle the new optional
updatedOffensesparameter.apps/judicial-system/web/src/utils/validate.ts (2)
293-294: LGTM! Clean function signature update.The optional
isOffenseEndpointEnabledparameter is properly added to the function signature.
320-330: LGTM! Clean helper function implementation.The
hasOffenseshelper function is well-implemented with clear logic for both feature-enabled and feature-disabled cases.apps/judicial-system/web/src/utils/hooks/useSections/index.ts (2)
64-66: LGTM! Clean feature context integration.The Feature context is properly integrated and the feature flag is correctly extracted.
524-526: LGTM! Proper feature flag usage.The
isOffenseEndpointEnabledflag is correctly passed to the form validation function.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
apps/judicial-system/web/src/utils/validate.ts (2)
1-1:⚠️ Potential issueAdd unit tests for validation functions.
The TODO comment indicates missing tests. Given the critical nature of validation logic in a judicial system, comprehensive test coverage is essential.
Would you like me to help generate unit tests for these validation functions? I can create test cases covering various scenarios including edge cases.
331-338: 💡 Verification agent🧩 Analysis chain
Consider adding type validation for offenses.
The
isValidTrafficViolationfunction should validate that the offenses array contains valid offense types.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any custom offense types used in the codebase # that need to be validated ast-grep --pattern 'IndictmentCountOffense = { $$$ }' rg -A 5 'offense ='Length of output: 12987
Offense Type Validation in isValidTrafficViolation
After reviewing the code and examining the usage of the
IndictmentCountOffensethroughout the repository, it’s clear that while the off‐ense types (e.g.IndictmentCountOffense.SPEEDING,IndictmentCountOffense.DRUNK_DRIVING, etc.) are used consistently across multiple modules, theisValidTrafficViolationfunction inapps/judicial-system/web/src/utils/validate.tsonly relies on helpers likehasOffenses(likely checking only for an existing offenses array) without explicitly validating that each offense in the array is one of the allowed types.Actionable Points:
- Update
isValidTrafficViolationto incorporate runtime checks confirming that every offense inindictmentCount.offensesis a valid value from theIndictmentCountOffenseset.- Consider centralizing the list of valid offenses so that any additions or removals update all relevant validations uniformly.
🧹 Nitpick comments (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsx (1)
291-323: Consider adding TypeScript interface for getIncidentDescription parameters.The function implementation looks good, but we could improve type safety by defining an interface for the parameters object.
+interface IncidentDescriptionParams { + indictmentCount: typeof updatedIndictmentCount; + formatMessage: typeof formatMessage; + crimeScene: CrimeScene; + subtypesRecord?: Record<string, IndictmentSubtype[]>; + isOffenseEndpointEnabled: boolean; +} const handleUpdateIndictmentCount = ( policeCaseNumber: string, crimeScene: CrimeScene, subtypes?: Record<string, IndictmentSubtype[]>, ) => {apps/judicial-system/web/src/utils/validate.ts (1)
302-317: Consider using early returns for better readability.The nested conditional structure in
isValidSpeedingIndictmentCountcan be simplified.- const isValidSpeedingIndictmentCount = (indictmentCount: IndictmentCount) => { - if (indictmentCount.offenses) { - return indictmentCount.offenses.some( - (o) => o.offense === IndictmentCountOffense.SPEEDING, - ) - ? Boolean(indictmentCount.recordedSpeed) && - Boolean(indictmentCount.speedLimit) - : true - } - return indictmentCount.deprecatedOffenses?.includes( - IndictmentCountOffense.SPEEDING, - ) - ? Boolean(indictmentCount.recordedSpeed) && - Boolean(indictmentCount.speedLimit) - : true - } + const isValidSpeedingIndictmentCount = (indictmentCount: IndictmentCount) => { + const isSpeedingOffense = indictmentCount.offenses + ? indictmentCount.offenses.some((o) => o.offense === IndictmentCountOffense.SPEEDING) + : indictmentCount.deprecatedOffenses?.includes(IndictmentCountOffense.SPEEDING); + + if (!isSpeedingOffense) { + return true; + } + + return Boolean(indictmentCount.recordedSpeed) && Boolean(indictmentCount.speedLimit); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsx(4 hunks)apps/judicial-system/web/src/utils/validate.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`apps/**/*`: "Confirm that the code adheres to the following...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsxapps/judicial-system/web/src/utils/validate.ts
🧠 Learnings (1)
apps/judicial-system/web/src/utils/validate.ts (1)
Learnt from: oddsson
PR: island-is/island.is#17589
File: apps/judicial-system/web/src/utils/validate.ts:303-308
Timestamp: 2025-01-21T15:32:39.384Z
Learning: In the judicial system web app, speed-related fields (recordedSpeed and speedLimit) in indictment counts should use simple Boolean validation checks rather than complex format/range validation.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (judicial-system-api,judicial-system-digital-mailbox-api,judicial-system-message-handler,ju...
- GitHub Check: tests (judicial-system-backend)
🔇 Additional comments (4)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsx (2)
12-12: LGTM! Clean feature flag implementation.The feature flag setup follows React best practices by using context and proper type imports.
Also applies to: 18-18, 107-108
474-642: LGTM! Well-structured component rendering.The component follows NextJS best practices with:
- Proper form validation
- Good error handling
- Clean UI components structure
🧰 Tools
🪛 Biome (1.9.4)
[error] 525-528: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 531-534: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/judicial-system/web/src/utils/validate.ts (2)
292-292: LGTM! Function signature updated to support feature flag.The addition of the optional
isOffenseEndpointEnabledparameter allows for graceful feature rollout.
319-329: LGTM! Well-structured feature flag handling.The
hasOffensesfunction cleanly handles both new and deprecated offense data structures based on the feature flag.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
Show resolved
Hide resolved
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Offenses/Offenses.tsx
Show resolved
Hide resolved
…s/offense/supporting-offense-property-in-client
…ttps://github.com/island-is/island.is into j-s/offense/supporting-offense-property-in-client
…s/offense/supporting-offense-property-in-client
thorhildurt
deleted the
j-s/offense/supporting-offense-property-in-client
branch
Step 3.2/4: Implement client functionality for new offense endpoints
What
offensesdeprecatedOffensesWhy
indictmentCount.For more details see technical docs here.
Checklist:
Summary by CodeRabbit
New Features
FeatureContextto manage feature visibility within components.Refactor
Tests