feat(j-s): Speeding #17589
feat(j-s): Speeding #17589
Conversation
WalkthroughThis pull request introduces support for speeding offenses in the judicial system by adding new properties and handling mechanisms across multiple files. The changes include adding Changes
Possibly related PRs
Suggested Labels
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: 7
🧹 Nitpick comments (5)
apps/judicial-system/api/src/app/modules/indictment-count/dto/updateIndictmentCount.input.ts (1)
67-77: Enhance validation for speed-related fields.Consider adding custom validation to ensure that
recordedSpeedandspeedLimitcontain valid numeric values when provided.+import { IsNumberString } from 'class-validator' @Allow() @IsOptional() @IsString() +@IsNumberString() @Field(() => String, { nullable: true }) readonly recordedSpeed?: string @Allow() @IsOptional() @IsString() +@IsNumberString() @Field(() => String, { nullable: true }) readonly speedLimit?: stringapps/judicial-system/backend/src/app/modules/indictment-count/models/indictmentCount.model.ts (1)
84-90: Consider adding database-level constraints for speed values.While the application validates these fields, consider adding database-level CHECK constraints to ensure data integrity.
Example migration:
ALTER TABLE indictment_count ADD CONSTRAINT chk_recorded_speed CHECK (recorded_speed ~ '^[0-9]+$'), ADD CONSTRAINT chk_speed_limit CHECK (speed_limit ~ '^[0-9]+$');apps/judicial-system/web/src/utils/hooks/useIndictmentCounts/index.ts (1)
131-145: Consider using a more concise implementation.The function logic is correct but could be more concise using a ternary operator.
- (law: number[]) => { - const article = law[0] - const paragraph = law[1] - - if (paragraph === 0) { - return formatMessage(indictmentCount.lawsBrokenTagArticleOnly, { - article, - }) - } else { - return formatMessage(indictmentCount.lawsBrokenTag, { - article, - paragraph, - }) - } - }, + ([article, paragraph]: number[]) => + formatMessage( + paragraph === 0 + ? indictmentCount.lawsBrokenTagArticleOnly + : indictmentCount.lawsBrokenTag, + paragraph === 0 ? { article } : { article, paragraph }, + ),apps/judicial-system/web/src/components/FormProvider/case.graphql (1)
222-223: Add GraphQL type definitions for the new fields.The new fields
recordedSpeedandspeedLimitshould have their types explicitly defined in the schema. Consider adding scalar type annotations (e.g., String!) to ensure type safety and proper validation.- recordedSpeed - speedLimit + recordedSpeed: String! + speedLimit: String!apps/judicial-system/web/src/utils/validate.ts (1)
309-316: Improve maintainability of traffic violation validation.Consider extracting the validation checks into separate functions for better maintainability and testability. This would make it easier to add new types of traffic violations in the future.
+const hasRequiredTrafficViolationFields = (indictmentCount: IndictmentCount) => + Boolean(indictmentCount.policeCaseNumber) && + Boolean(indictmentCount.offenses && indictmentCount.offenses?.length > 0) && + Boolean(indictmentCount.vehicleRegistrationNumber) && + Boolean(indictmentCount.lawsBroken) && + Boolean(indictmentCount.incidentDescription) && + Boolean(indictmentCount.legalArguments) const isValidTrafficViolation = (indictmentCount: IndictmentCount) => - Boolean(indictmentCount.policeCaseNumber) && - Boolean(indictmentCount.offenses && indictmentCount.offenses?.length > 0) && - Boolean(indictmentCount.vehicleRegistrationNumber) && - Boolean(indictmentCount.lawsBroken) && - Boolean(indictmentCount.incidentDescription) && - Boolean(indictmentCount.legalArguments) && - isValidSpeedingIndictmentCount(indictmentCount) + hasRequiredTrafficViolationFields(indictmentCount) && + isValidSpeedingIndictmentCount(indictmentCount)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/judicial-system/api/src/app/modules/indictment-count/dto/updateIndictmentCount.input.ts(2 hunks)apps/judicial-system/api/src/app/modules/indictment-count/models/indictmentCount.model.ts(1 hunks)apps/judicial-system/backend/migrations/20250120150345-update-indictment-count.js(1 hunks)apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts(1 hunks)apps/judicial-system/backend/src/app/modules/indictment-count/models/indictmentCount.model.ts(1 hunks)apps/judicial-system/web/src/components/FormProvider/case.graphql(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.strings.ts(2 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx(7 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCountEnum.strings.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Substance/Substance.tsx(1 hunks)apps/judicial-system/web/src/utils/hooks/useIndictmentCounts/index.ts(1 hunks)apps/judicial-system/web/src/utils/hooks/useIndictmentCounts/updateIndictmentCount.graphql(1 hunks)apps/judicial-system/web/src/utils/validate.ts(2 hunks)libs/island-ui/core/src/lib/Input/Input.mixins.ts(1 hunks)libs/judicial-system/types/src/lib/indictmentCount.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Substance/Substance.tsx
🧰 Additional context used
📓 Path-based instructions (14)
apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts (1)
Pattern 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/useIndictmentCounts/updateIndictmentCount.graphql (1)
Pattern 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/src/app/modules/indictment-count/dto/updateIndictmentCount.input.ts (1)
Pattern 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/src/app/modules/indictment-count/models/indictmentCount.model.ts (1)
Pattern 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."
libs/judicial-system/types/src/lib/indictmentCount.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."
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCountEnum.strings.ts (1)
Pattern 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/components/FormProvider/case.graphql (1)
Pattern 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."
libs/island-ui/core/src/lib/Input/Input.mixins.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."
apps/judicial-system/web/src/utils/validate.ts (1)
Pattern 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/backend/migrations/20250120150345-update-indictment-count.js (1)
Pattern 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/backend/src/app/modules/indictment-count/models/indictmentCount.model.ts (1)
Pattern 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/useIndictmentCounts/index.ts (1)
Pattern 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.strings.ts (1)
Pattern 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 (1)
Pattern 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."
📓 Learnings (5)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCountEnum.strings.ts (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#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/backend/migrations/20250120150345-update-indictment-count.js (1)
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/backend/migrations/20241022142128-update-defendant.js:1-2
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the 'judicial-system' application, it is a convention to include `'use strict'` directives at the top of migration files in `apps/judicial-system/backend/migrations/*.js`.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.strings.ts (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.tsx (4)
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: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:700-758
Timestamp: 2024-12-17T14:33:50.960Z
Learning: In the file `apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx`, the user prefers not to add range validation for the blood alcohol content input.
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 (7)
apps/judicial-system/backend/migrations/20250120150345-update-indictment-count.js (1)
26-37: LGTM! Transaction handling is correct.The down migration properly removes the columns within a transaction, ensuring data consistency.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCountEnum.strings.ts (1)
24-28: LGTM! Consistent with existing patterns.The new SPEEDING enum follows the established pattern and naming convention used for other driving offenses.
libs/judicial-system/types/src/lib/indictmentCount.ts (1)
6-6: LGTM!The addition of
SPEEDINGto the offense types and its corresponding empty substances array is well-structured and maintains type safety.Also applies to: 102-102
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.strings.ts (1)
98-115: LGTM! The message definitions are well-structured.The new messages for speeding-related fields follow the existing naming conventions, and the descriptions are clear and helpful. The version suffix in the
incidentDescriptionAutofillmessage ID correctly indicates a breaking change.Also applies to: 139-143, 145-147
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (1)
Line range hint
85-107: LGTM! Well-documented and properly handles edge cases.The
offenseLawsMapdocumentation is clear, and the legal arguments formatting correctly handles cases with no article number.Also applies to: 279-285
apps/judicial-system/web/src/utils/hooks/useIndictmentCounts/updateIndictmentCount.graphql (1)
15-16: LGTM! The GraphQL schema is properly updated.The new fields
recordedSpeedandspeedLimitare correctly added to the mutation response type.apps/judicial-system/web/src/utils/validate.ts (1)
Line range hint
1-1: Address the TODO comment by adding tests.The file lacks tests which is particularly concerning given the critical nature of validation logic in a judicial system. Consider adding comprehensive test cases for all validation scenarios.
Would you like me to help generate test cases for the validation functions?
apps/judicial-system/backend/migrations/20250120150345-update-indictment-count.js
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/migrations/20250120150345-update-indictment-count.js
Show resolved
Hide resolved
apps/judicial-system/api/src/app/modules/indictment-count/models/indictmentCount.model.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 8 Total Test Services: 0 Failed, 7 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
oddsson
requested review from
a team and
sigruntg
and removed request for
a team
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (2)
348-362: Extract placeholder strings into constants.Consider extracting the placeholder strings into named constants for better maintainability and reusability.
+const SPEED_PLACEHOLDERS = { + RECORDED_SPEED: '[Mældur hraði]', + SPEED_LIMIT: '[Leyfilegur hraði]', +} as const - const recordedSpeed = indictmentCount.recordedSpeed ?? '[Mældur hraði]' - const speedLimit = indictmentCount.speedLimit ?? '[Leyfilegur hraði]' + const recordedSpeed = indictmentCount.recordedSpeed ?? SPEED_PLACEHOLDERS.RECORDED_SPEED + const speedLimit = indictmentCount.speedLimit ?? SPEED_PLACEHOLDERS.SPEED_LIMIT
713-716: Extract speed-related cleanup into a function.Consider extracting the speed-related cleanup logic into a separate function for better readability and reusability.
+const clearSpeedingData = (offense: IndictmentCountOffense) => { + return offense === IndictmentCountOffense.SPEEDING + ? { recordedSpeed: null, speedLimit: null } + : {} +} -...(offense === IndictmentCountOffense.SPEEDING && { - recordedSpeed: null, - speedLimit: null, -}), +...clearSpeedingData(offense),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx(7 hunks)libs/island-ui/core/src/lib/Input/Input.css.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/island-ui/core/src/lib/Input/Input.css.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."
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (1)
Pattern 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."
📓 Learnings (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (6)
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.
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:700-758
Timestamp: 2024-12-17T14:33:50.960Z
Learning: In the file `apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx`, the user prefers not to add range validation for the blood alcohol content input.
Learnt from: oddsson
PR: island-is/island.is#17589
File: apps/judicial-system/api/src/app/modules/indictment-count/models/indictmentCount.model.ts:51-56
Timestamp: 2025-01-21T15:32:09.727Z
Learning: In the judicial-system application, speed-related values (recordedSpeed and speedLimit) in IndictmentCount models should be stored as strings rather than numbers.
Learnt from: oddsson
PR: island-is/island.is#17589
File: apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts:62-71
Timestamp: 2025-01-21T15:32:15.618Z
Learning: In the judicial-system application, speed-related values (recordedSpeed, speedLimit) should be stored as strings rather than numbers.
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 (3)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (3)
Line range hint
85-107: LGTM! Well-documented law mapping.The addition of the speeding offense to
offenseLawsMapis clear and follows the established pattern.
404-407: LGTM! Consistent error state management.The error message states for speed inputs follow the established pattern.
799-899:⚠️ Potential issueAdd validation for speed values.
The speed inputs need validation to ensure:
- The recorded speed is greater than the speed limit
- Both values are reasonable (e.g., within typical road speed ranges)
Add validation in the
onBlurhandler:onBlur={(event) => { const recordedSpeed = event.target.value + const speedLimit = indictmentCount.speedLimit + const recordedSpeedNum = parseInt(recordedSpeed, 10) + const speedLimitNum = speedLimit ? parseInt(speedLimit, 10) : 0 validateAndSetErrorMessage( - ['empty'], + ['empty', 'numeric'], recordedSpeed, setRecordedSpeedErrorMessage, + { + customMessage: + recordedSpeedNum <= speedLimitNum + ? 'Recorded speed must be greater than speed limit' + : recordedSpeedNum > 250 + ? 'Recorded speed seems unreasonably high' + : undefined, + } ) handleIndictmentCountChanges({ recordedSpeed, }) }}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
apps/auth-admin-web/components/Client/nav/ClientTabNav.tsx (1)
Line range hint
53-66: Use localization for help text.The help text is hardcoded in English. For consistency with the rest of the component that uses localization, these strings should be moved to the localization system.
<div className="client-tab-nav__container__help"> - Use either the simplified form or the detailed form to create a new - client + {localization.navigations['clientTabNav'].items['helpText'].text} <div className="client-tab-nav__container__help__selected"> - {activetab === ClientTab.BasicForm - ? 'You are using the simplified form' - : 'You are using the detailed form'} + {activetab === ClientTab.BasicForm + ? localization.navigations['clientTabNav'].items['basicFormSelected'].text + : localization.navigations['clientTabNav'].items['detailedFormSelected'].text} </div> </div>apps/skilavottord/ws/src/app/modules/samgongustofa/samgongustofa.service.ts (2)
Line range hint
76-102: Add type safety and improve error handling for SOAP response parsing.The current implementation has several areas for improvement:
- Define TypeScript interfaces for SOAP response structure to prevent runtime errors.
- Add null checks for nested property access.
- Use template literals for error messages.
Example implementation:
+ interface SoapEnvelope { + 'soapenv:Envelope': { + 'soapenv:Body': [{ + 'soapenv:Fault'?: [{ + 'faultstring': string[]; + }]; + 'ns1:allVehiclesForPersidnoResponse'?: [{ + 'allVehiclesForPersidnoReturn': [{ + '_': string; + }]; + }]; + }]; + }; + } - .then((allCarsResult) => { + .then((allCarsResult: SoapEnvelope) => { // ... rest of the code - logger.error( - allCarsResult['soapenv:Envelope']['soapenv:Body'][0]['soapenv:Fault'][0]['faultstring'][0], - ) + const faultString = allCarsResult?.['soapenv:Envelope']?.['soapenv:Body']?.[0]?.['soapenv:Fault']?.[0]?.['faultstring']?.[0]; + logger.error(`SOAP fault: ${faultString}`);
Line range hint
147-156: Improve error handling strategy.The current error handling implementation has several issues:
- Duplicate error handling logic across catch blocks.
- Generic error messages that could be more specific.
- No custom error types for different failure scenarios.
+ class XmlParsingError extends Error { + constructor(stage: string, originalError: Error) { + super(`XML parsing failed during ${stage}: ${originalError.message}`); + this.name = 'XmlParsingError'; + } + } + private handleXmlParsingError(stage: string, error: Error): never { + logger.error(`Failed while parsing XML to JSON during ${stage}`, { + error: error.message, + stage, + }); + throw new XmlParsingError(stage, error); + } - .catch((err) => { - logger.error( - `Failed while parsing xml to json on getUserVehiclesInformation request with error: ${err}`, - ); - throw new Error( - `Failed while parsing xml to json on getUserVehiclesInformation request with error: ${err}`, - ); - }) + .catch((err) => this.handleXmlParsingError('getUserVehiclesInformation', err))Also applies to: 269-278
🧹 Nitpick comments (6)
libs/api/domains/disability-license/src/lib/disability-license.resolver.ts (1)
22-22: Great improvement using the primitive boolean type!The change from
Promise<Boolean>toPromise<boolean>aligns with TypeScript best practices. Using the primitivebooleantype instead of the boxedBooleantype provides better type safety and can lead to improved performance.Consider updating the GraphQL return type decorator to match:
- @Query(() => Boolean) + @Query(() => Boolean, { description: 'Check if user has a disability license' })This adds helpful documentation for GraphQL clients.
libs/api/domains/directorate-of-labour/src/lib/parentalLeavePregnancyStatus.model.ts (1)
9-9: LGTM! Good improvement in type safety.The change from
Stringtostringis correct as it follows TypeScript best practices by using primitive types instead of object constructors.Consider adding JSDoc comments to document the class and its properties for better maintainability:
@ObjectType() +/** + * Represents the pregnancy status for parental leave applications. + */ export class ParentalLeavePregnancyStatus { @Field(() => Boolean) + /** Indicates if there is an active pregnancy case. */ hasActivePregnancy!: boolean @Field(() => String) + /** The expected date when the child will be born. */ expectedDateOfBirth!: string }apps/financial-aid/backend/src/app/modules/staff/test/numberOfUsersForMunicipality.spec.ts (1)
Line range hint
1-89: Consider adding boundary value test cases.While the current test coverage is good, consider adding test cases for:
- Edge cases (e.g., zero users)
- Invalid municipality IDs
- Large number of users
Example test case structure:
describe('edge cases', () => { it('should handle zero users', async () => { const municipalityId = uuid() const mockNumberOfUsers = mockStaffModel.count as jest.Mock mockNumberOfUsers.mockResolvedValueOnce(0) const then = await givenWhenThen(municipalityId) expect(then.result).toBe(0) }) })apps/auth-admin-web/components/Client/nav/ClientTabNav.tsx (1)
12-14: Maintain consistent casing in prop names.The renaming of
activeTabtoactivetabbreaks the camelCase naming convention used throughout the codebase. Consider keeping the original casing.- activeTab: activetab, + activeTab,apps/skilavottord/ws/src/app/modules/samgongustofa/samgongustofa.service.ts (2)
Line range hint
102-147: Refactor vehicle processing logic for better maintainability.The vehicle processing logic is complex and could benefit from:
- Extracting vehicle status and recyclability checks into separate methods.
- Using appropriate log levels (debug vs info).
- Using immutable array operations.
Example implementation:
+ private isVehicleDeregistered(status: string): boolean { + return status === 'Afskráð'; + } + private determineVehicleRecyclability(vehicle: any): { isRecyclable: boolean; status: string } { + const isDeregistered = this.isVehicleDeregistered(vehicle.vehiclestatus[0]); + const hasCoOwner = vehicle.otherowners[0] !== '0'; + + return { + isRecyclable: !isDeregistered && !hasCoOwner, + status: isDeregistered ? 'deregistered' : 'inUse' + }; + } - allCars['persidnolookup']['vehicleList'][0]['vehicle'].forEach((car) => { + const vehicleArr = allCars?.['persidnolookup']?.['vehicleList']?.[0]?.['vehicle']?.map((car) => { + const { isRecyclable, status } = this.determineVehicleRecyclability(car); + + logger.debug(`Vehicle ${car.permno[0]} status: ${status}, recyclable: ${isRecyclable}`); + + return new VehicleInformation( + car.permno[0], + car.type[0], + car.color[0], + car.vin[0], + car.firstregdate[0], + isRecyclable, + !car.otherowners[0], + status + ); + }) ?? [];
Line range hint
206-227: Simplify vehicle information update logic.The current implementation has complex nested conditions and type checks that could be simplified:
- Use TypeScript types for vehicle information structure.
- Extract update logic into separate methods.
- Use early returns to reduce nesting.
+ interface VehicleBasicInfo { + vehicle: { + ownerregistrationerrors: [{ ownerregistrationerror?: any[] }]; + stolens: [{ stolen?: { enddate: [string] }[] }]; + updatelocks: [{ updatelock?: { enddate: [string] }[] }]; + }; + } + private hasActiveRegistrationErrors(info: VehicleBasicInfo): boolean { + return typeof info.vehicle.ownerregistrationerrors[0].ownerregistrationerror !== 'undefined'; + } + private hasActiveStolenStatus(info: VehicleBasicInfo): boolean { + return info.vehicle.stolens[0].stolen?.some(stolen => !stolen.enddate[0].trim()) ?? false; + } + private hasActiveUpdateLocks(info: VehicleBasicInfo): boolean { + return info.vehicle.updatelocks[0].updatelock?.some(lock => !lock.enddate[0].trim()) ?? false; + } - .then((basicInfo) => { + .then((basicInfo: VehicleBasicInfo) => { + if (hasActiveRegistrationErrors(basicInfo)) { + logger.debug('Vehicle not recyclable: has registration errors'); + newVehicleArr[i].isRecyclable = false; + return newVehicleArr[i]; + } + + if (hasActiveStolenStatus(basicInfo)) { + logger.debug('Vehicle not recyclable: marked as stolen'); + newVehicleArr[i].isRecyclable = false; + return newVehicleArr[i]; + } + + if (hasActiveUpdateLocks(basicInfo)) { + logger.debug('Vehicle not recyclable: has active update locks'); + newVehicleArr[i].isRecyclable = false; + return newVehicleArr[i]; + } + + return newVehicleArr[i];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
apps/auth-admin-web/components/Admin/nav/AdminTabNav.tsx(1 hunks)apps/auth-admin-web/components/Client/nav/ClientTabNav.tsx(1 hunks)apps/contentful-apps/components/Layout.tsx(1 hunks)apps/contentful-apps/components/LocalhostWarning.tsx(1 hunks)apps/contentful-apps/components/MideindTranslationSidebar/fieldUtils/index.tsx(1 hunks)apps/contentful-apps/pages/fields/active-translations-field.tsx(1 hunks)apps/contentful-apps/pages/fields/admin-only-json-field.tsx(1 hunks)apps/contentful-apps/pages/fields/admin-only-short-text-checkbox-list-field.tsx(1 hunks)apps/contentful-apps/pages/fields/admin-only-short-text-dropdown-field.tsx(1 hunks)apps/contentful-apps/pages/fields/admin-only-short-text-field.tsx(1 hunks)apps/contentful-apps/pages/fields/admin-only-single-reference-field.tsx(1 hunks)apps/contentful-apps/pages/fields/article-related-articles-field.tsx(2 hunks)apps/contentful-apps/pages/fields/article-subarticles-field.tsx(1 hunks)apps/contentful-apps/pages/fields/content-type-notifications-field.tsx(1 hunks)apps/contentful-apps/pages/fields/embed-link-field.tsx(1 hunks)apps/contentful-apps/pages/fields/form-default-namespace-field.tsx(1 hunks)apps/contentful-apps/pages/fields/formatted-location-input-field.tsx(1 hunks)apps/contentful-apps/pages/fields/publish-date-field.tsx(1 hunks)apps/contentful-apps/pages/fields/theme-properties-field.tsx(1 hunks)apps/contentful-apps/pages/fields/time-duration-field.tsx(1 hunks)apps/contentful-apps/pages/fields/vacancy-contacts-field.tsx(1 hunks)apps/financial-aid/backend/src/app/modules/staff/test/numberOfUsersForMunicipality.spec.ts(1 hunks)apps/skilavottord/web/screens/Overview/components/ActionCard/ActionCard.tsx(1 hunks)apps/skilavottord/ws/src/app/modules/samgongustofa/samgongustofa.service.ts(8 hunks)apps/tools/contentful-role-permissions/utils/index.ts(1 hunks)apps/web/screens/PetitionView/PetitionView.tsx(1 hunks)libs/api/domains/auth-admin/src/lib/client/clients.service.spec.ts(8 hunks)libs/api/domains/directorate-of-labour/src/lib/parentalLeavePregnancyStatus.model.ts(1 hunks)libs/api/domains/disability-license/src/lib/disability-license.resolver.ts(1 hunks)libs/api/domains/driving-license/src/lib/graphql/qualityPhoto.resolver.ts(1 hunks)libs/api/domains/driving-license/src/lib/graphql/qualitySignature.resolver.ts(1 hunks)libs/application/templates/financial-aid/src/fields/TaxReturnFilesForm/taxFormContent.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (24)
- apps/contentful-apps/pages/fields/admin-only-short-text-field.tsx
- apps/contentful-apps/components/Layout.tsx
- apps/contentful-apps/pages/fields/active-translations-field.tsx
- apps/contentful-apps/pages/fields/time-duration-field.tsx
- apps/contentful-apps/pages/fields/admin-only-json-field.tsx
- apps/contentful-apps/pages/fields/admin-only-short-text-dropdown-field.tsx
- apps/contentful-apps/pages/fields/content-type-notifications-field.tsx
- apps/contentful-apps/pages/fields/publish-date-field.tsx
- apps/contentful-apps/components/MideindTranslationSidebar/fieldUtils/index.tsx
- apps/contentful-apps/pages/fields/admin-only-short-text-checkbox-list-field.tsx
- apps/contentful-apps/pages/fields/theme-properties-field.tsx
- apps/tools/contentful-role-permissions/utils/index.ts
- apps/contentful-apps/components/LocalhostWarning.tsx
- apps/auth-admin-web/components/Admin/nav/AdminTabNav.tsx
- apps/contentful-apps/pages/fields/admin-only-single-reference-field.tsx
- apps/contentful-apps/pages/fields/embed-link-field.tsx
- libs/api/domains/auth-admin/src/lib/client/clients.service.spec.ts
- apps/contentful-apps/pages/fields/formatted-location-input-field.tsx
- libs/application/templates/financial-aid/src/fields/TaxReturnFilesForm/taxFormContent.tsx
- apps/web/screens/PetitionView/PetitionView.tsx
- apps/contentful-apps/pages/fields/article-related-articles-field.tsx
- apps/contentful-apps/pages/fields/article-subarticles-field.tsx
- apps/contentful-apps/pages/fields/form-default-namespace-field.tsx
- apps/contentful-apps/pages/fields/vacancy-contacts-field.tsx
🧰 Additional context used
📓 Path-based instructions (8)
apps/auth-admin-web/components/Client/nav/ClientTabNav.tsx (1)
Pattern 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."
libs/api/domains/disability-license/src/lib/disability-license.resolver.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."
libs/api/domains/directorate-of-labour/src/lib/parentalLeavePregnancyStatus.model.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."
apps/financial-aid/backend/src/app/modules/staff/test/numberOfUsersForMunicipality.spec.ts (1)
Pattern 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."
libs/api/domains/driving-license/src/lib/graphql/qualityPhoto.resolver.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."
apps/skilavottord/web/screens/Overview/components/ActionCard/ActionCard.tsx (1)
Pattern 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/skilavottord/ws/src/app/modules/samgongustofa/samgongustofa.service.ts (1)
Pattern 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."
libs/api/domains/driving-license/src/lib/graphql/qualitySignature.resolver.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."
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (4)
apps/financial-aid/backend/src/app/modules/staff/test/numberOfUsersForMunicipality.spec.ts (1)
9-9: Good change! Using primitivenumbertype instead ofNumber.This change aligns with TypeScript best practices by using the primitive
numbertype instead of theNumberobject wrapper type, which improves type safety and performance.libs/api/domains/driving-license/src/lib/graphql/qualityPhoto.resolver.ts (1)
25-25: Good use of TypeScript primitive type!The change from
Promise<String>toPromise<string>follows TypeScript best practices by using the primitive type instead of the wrapper object type. This improves type safety and performance.Let's check if there are other instances of
Promise<String>that should be updated for consistency:✅ Verification successful
Verified: Codebase consistently uses primitive type
stringThe change from
Promise<String>toPromise<string>aligns with the existing codebase conventions. A thorough search confirms that all Promise return types consistently use the TypeScript primitive typestringrather than the object typeString.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Promise<String> type annotations that might need similar updates rg "Promise<String>" --type tsLength of output: 32
Script:
#!/bin/bash # Search with multiple patterns and case variations echo "=== Searching for Promise<String> variations ===" rg -i "promise\s*<\s*string" --type ts echo -e "\n=== Checking return type declarations ===" ast-grep --pattern 'Promise<String>' --lang ts ast-grep --pattern 'Promise < String >' --lang tsLength of output: 24022
libs/api/domains/driving-license/src/lib/graphql/qualitySignature.resolver.ts (1)
25-25: Consistent type usage across resolvers!The change from
Promise<String>toPromise<string>maintains consistency with the similar change inQualityPhotoResolver, following TypeScript best practices.apps/skilavottord/ws/src/app/modules/samgongustofa/samgongustofa.service.ts (1)
Line range hint
1-24: Consider modernizing the deprecated service.This service is marked as deprecated but contains critical business logic for vehicle recycling. Consider:
- Creating a migration plan to a modern REST-based service.
- Implementing caching for frequently accessed vehicle information.
- Using XML parsing libraries optimized for large SOAP responses.
- Adding comprehensive API documentation for the replacement service.
apps/skilavottord/web/screens/Overview/components/ActionCard/ActionCard.tsx
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ❌ 10 Total Test Services: 1 Failed, 9 Passed Test ServicesThis report shows up to 10 services
❌ Failed Tests (11)
🔻 Code Coverage Decreases vs Default Branch (6)
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
libs/infra-express-server/src/lib/infra-express-server.ts (1)
Line range hint
33-41: Add type declaration for res.locals.startEpoch.For better type safety, consider declaring the custom property in res.locals:
// Add this at the top of the file declare global { namespace Express { interface Locals { startEpoch: number; } } }libs/island-ui/core/src/lib/DatePicker/DatePicker.stories.tsx (6)
46-54: Improve type safety and testability of the WithSelectedDate story.Consider these improvements:
- Extract the date initialization to make testing deterministic
- Add proper type annotations for better type safety
+const defaultDate = new Date('2024-01-01T00:00:00.000Z') + export const WithSelectedDate = () => ( <Wrap> <DatePicker label="Date" placeholderText="Pick a date" - selected={new Date()} + selected={defaultDate} handleChange={(date: Date) => console.log(date)} /> </Wrap> )
56-67: Simplify the LocaleIS story and improve type safety.The story can be simplified and made more type-safe:
-export const LocaleIS = () => { - return ( +export const LocaleIS = () => ( <Wrap> <DatePicker label="Dagsetning" placeholderText="Veldu dagsetningu" locale="is" - handleChange={(date: Date) => console.log(date)} + handleChange={(date: Date): void => console.log(date)} /> </Wrap> - ) -} +)
72-85: Fix typo and improve type safety in the SelectYear story.The story has the following issues:
- There's a typo in the text message: "then" should be "than"
- Missing type annotations
- Date initialization should be extracted for better testability
+const currentDate = new Date('2024-01-01T00:00:00.000Z') + export const SelectYear = () => { - const toDay = new Date() + const toDay = currentDate return ( <Wrap> <DatePicker label="Date" placeholderText="Pick a year" minYear={toDay.getFullYear() - 10} maxYear={toDay.getFullYear() + 2} - handleChange={(date: Date) => console.log(date)} + handleChange={(date: Date): void => console.log(date)} /> <Text variant="small" marginTop={3}> Selecting a year will only work if <code>minYear</code> and{' '} <code>maxYear</code> are set and <code>maxYear</code> is higher{' '} - <code>then</code>{' '} + <code>than</code>{' '} <code>minYear</code> </Text> </Wrap> ) }
142-152: Improve testability and type safety of the WithErrors story.Consider extracting the date initialization and adding proper type annotations:
+const errorDate = new Date('2024-01-01T00:00:00.000Z') + export const WithErrors = () => ( <Wrap> <DatePicker id="TestError" label="Date" placeholderText="Pick a date" - selected={new Date()} + selected={errorDate} hasError errorMessage="This date is somewhat incorrect" - handleChange={(date: Date) => console.log(date)} + handleChange={(date: Date): void => console.log(date)} /> </Wrap> )
156-164: Improve testability and type safety of the Disabled story.Consider extracting the date initialization and adding proper type annotations:
+const disabledDate = new Date('2024-01-01T00:00:00.000Z') + export const Disabled = () => ( <Wrap> <DatePicker label="Date" placeholderText="Pick a date" - selected={new Date()} + selected={disabledDate} - handleChange={(date: Date) => console.log(date)} + handleChange={(date: Date): void => console.log(date)} disabled /> </Wrap> )
Line range hint
1-164: Improve overall component reusability and maintainability.The DatePicker stories demonstrate good practices for component documentation and reusability. However, consider these improvements:
- Extract common date initialization logic into a shared utility
- Add proper TypeScript return type annotations for all stories
- Consider adding more complex use cases demonstrating integration with forms
Example utility for date initialization:
// utils/testDates.ts export const getTestDate = (dateString: string = '2024-01-01T00:00:00.000Z'): Date => new Date(dateString)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/contentful-apps/components/LocalhostWarning.tsx(1 hunks)apps/contentful-apps/pages/fields/article-related-articles-field.tsx(2 hunks)apps/contentful-apps/pages/fields/embed-link-field.tsx(1 hunks)apps/contentful-apps/pages/fields/time-duration-field.tsx(1 hunks)apps/contentful-apps/pages/fields/vacancy-contacts-field.tsx(1 hunks)apps/skilavottord/web/screens/Overview/components/ActionCard/ActionCard.tsx(1 hunks)libs/application/templates/financial-aid/src/fields/TaxReturnFilesForm/taxFormContent.tsx(1 hunks)libs/infra-express-server/src/lib/infra-express-server.ts(3 hunks)libs/island-ui/core/src/lib/DatePicker/DatePicker.stories.tsx(2 hunks)libs/portals/shared-modules/delegations/src/hooks/useDynamicShadow.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/contentful-apps/pages/fields/vacancy-contacts-field.tsx
- apps/contentful-apps/pages/fields/time-duration-field.tsx
- apps/contentful-apps/components/LocalhostWarning.tsx
- libs/application/templates/financial-aid/src/fields/TaxReturnFilesForm/taxFormContent.tsx
- apps/contentful-apps/pages/fields/embed-link-field.tsx
- apps/skilavottord/web/screens/Overview/components/ActionCard/ActionCard.tsx
- apps/contentful-apps/pages/fields/article-related-articles-field.tsx
🧰 Additional context used
📓 Path-based instructions (3)
libs/portals/shared-modules/delegations/src/hooks/useDynamicShadow.tsx (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."
libs/infra-express-server/src/lib/infra-express-server.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."
libs/island-ui/core/src/lib/DatePicker/DatePicker.stories.tsx (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/shared-modules/delegations/src/hooks/useDynamicShadow.tsx (1)
Line range hint
40-52: LGTM! Well-structured and reusable hook implementation.The conversion to arrow function syntax maintains consistency while preserving the functionality. The hook demonstrates good practices:
- Strong TypeScript typing with proper interfaces
- Clean exports for effective tree-shaking
- Reusable design that can be shared across NextJS apps
libs/infra-express-server/src/lib/infra-express-server.ts (1)
44-46: LGTM! Clean and reusable health check implementation.The liveness endpoint follows RESTful practices and provides a clear health check interface.
There was a problem hiding this comment.
Nice! Looks good 🚀 Just left one small comment and then when testing I noticed that we don't highlight the new fields in the same way as other required fields. See screenshot when "ölvunarakstur" is selected with speeding. Wrote something in the speeding limit filed and then removed it, and would expect it to be highlighted in the same way as "Vínandamagn".
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (1)
348-362: Consider internationalizing the fallback values.The fallback values for
recordedSpeedandspeedLimitare hardcoded in Icelandic. Consider moving these to the string resources for better internationalization support.- const recordedSpeed = indictmentCount.recordedSpeed ?? '[Mældur hraði]' - const speedLimit = indictmentCount.speedLimit ?? '[Leyfilegur hraði]' + const recordedSpeed = indictmentCount.recordedSpeed ?? formatMessage(strings.recordedSpeedPlaceholder) + const speedLimit = indictmentCount.speedLimit ?? formatMessage(strings.speedLimitPlaceholder)
📜 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(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (1)
Pattern 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."
📓 Learnings (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (6)
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.
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:700-758
Timestamp: 2024-12-17T14:33:50.960Z
Learning: In the file `apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx`, the user prefers not to add range validation for the blood alcohol content input.
Learnt from: oddsson
PR: island-is/island.is#17589
File: apps/judicial-system/api/src/app/modules/indictment-count/models/indictmentCount.model.ts:51-56
Timestamp: 2025-01-21T15:32:09.727Z
Learning: In the judicial-system application, speed-related values (recordedSpeed and speedLimit) in IndictmentCount models should be stored as strings rather than numbers.
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: 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.
🪛 Biome (1.9.4)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
[error] 832-832: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 876-876: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (2)
Line range hint
85-107: LGTM! Clear documentation and consistent implementation.The addition of the SPEEDING offense to the offenseLawsMap follows the established pattern and is well-documented.
713-716: LGTM! Clean reset implementation.The speed values are properly reset when the speeding offense is removed, maintaining data consistency.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
Outdated
Show resolved
Hide resolved
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (1)
Pattern 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."
📓 Learnings (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (6)
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.
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:700-758
Timestamp: 2024-12-17T14:33:50.960Z
Learning: In the file `apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx`, the user prefers not to add range validation for the blood alcohol content input.
Learnt from: oddsson
PR: island-is/island.is#17589
File: apps/judicial-system/api/src/app/modules/indictment-count/models/indictmentCount.model.ts:51-56
Timestamp: 2025-01-21T15:32:09.727Z
Learning: In the judicial-system application, speed-related values (recordedSpeed and speedLimit) in IndictmentCount models should be stored as strings rather than numbers.
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: 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.
🔇 Additional comments (4)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (4)
Line range hint
85-107: Well-documented law mapping for speeding offense!The addition of the SPEEDING offense to the
offenseLawsMapis clean and follows the established pattern. The comment clearly explains the structure of the law references.
348-362: Good handling of optional speed values!The implementation gracefully handles missing speed values by using placeholders, maintaining a consistent user experience.
404-407: Consistent error state management!The error message state management for speed-related fields follows the established pattern in the codebase.
713-716: Clean state reset when removing speeding offense!Good practice to reset the speed-related fields when the speeding offense is removed.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
Show resolved
Hide resolved
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx
Show resolved
Hide resolved
* Add option SPEEDING * Add legal arguments * Add input fields * Add recordedSpeed and speedLimit to db * Enable seedLimit update * Add validation * Update indictment description * Clear recorded speed and speed limit when removing speedig * Add error handling * Fix comment * Fix lint * Remove css * Make Icon position abs * Rec speed and speed limit is now number * Remove island-ui change * Improve validation * Use Number.isNaN instead of isNaN
* Add option SPEEDING * Add legal arguments * Add input fields * Add recordedSpeed and speedLimit to db * Enable seedLimit update * Add validation * Update indictment description * Clear recorded speed and speed limit when removing speedig * Add error handling * Fix comment * Fix lint * Remove css * Make Icon position abs * Rec speed and speed limit is now number * Remove island-ui change * Improve validation * Use Number.isNaN instead of isNaN
Speeding
Asana
What
Add a new offence to indictment counts, speeding.
When speeding is selected, prosecutors need to input two values into new fields on the indictment count, recorded speed and speed limit. The rest of the indictment takes note of that and generates a description and everything else accordingly.
Why
An indictment is issued when people don't pay the fines for speeding. This enables users to handle these types of cases in RVG.
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
New Features
Documentation
Refactor