feat(j-s): Add indictment count subtypes to IndictmentCount #17129
Conversation
WalkthroughThis pull request introduces significant updates to the judicial system application, primarily adding a new optional property, Changes
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: 2
🧹 Outside diff range and nitpick comments (7)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (1)
Line range hint
372-385: Consider removing unnecessary AnimatePresence wrapper.Based on the code structure, the
AnimatePresencecomponent appears to be unnecessary here as it wraps a single static component without any exit animations being utilized. This aligns with NextJS best practices for optimizing component rendering.Consider this simplified structure:
- <AnimatePresence> <IndictmentCount indictmentCount={indictmentCount} workingCase={workingCase} onDelete={index > 0 ? handleDeleteIndictmentCount : undefined} onChange={handleUpdateIndictmentCount} setWorkingCase={setWorkingCase} updateIndictmentCountState={updateIndictmentCountState} /> - </AnimatePresence>apps/judicial-system/backend/migrations/20241204124025-update-indictment-count.js (1)
4-17: Consider adding transaction timeout for large tables.The migration is well-structured with proper transaction handling. However, for large tables, consider adding a transaction timeout to prevent long-running migrations from being stuck.
async up(queryInterface, Sequelize) { - await queryInterface.sequelize.transaction((transaction) => + await queryInterface.sequelize.transaction({ + timeout: 30000 // 30 seconds + }, (transaction) => queryInterface.addColumn(apps/judicial-system/api/src/app/modules/indictment-count/models/indictmentCount.model.ts (1)
6-9: LGTM! Consider centralizing enum type registrations.The changes are well-implemented following NestJS and GraphQL best practices. The field addition and enum registration are correct.
Consider moving all enum type registrations to a centralized location (e.g.,
graphql-types.ts) to maintain better organization as the number of enums grows. This would help prevent scattered enum registrations across different files.Example structure:
// graphql-types.ts import { registerEnumType } from '@nestjs/graphql' import { IndictmentCountOffense, IndictmentSubtype } from '@island.is/judicial-system/types' export const registerGraphQLEnums = () => { registerEnumType(IndictmentCountOffense, { name: 'IndictmentCountOffense' }) registerEnumType(IndictmentSubtype, { name: 'IndictmentSubtype' }) }Also applies to: 12-12, 48-50
apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts (1)
13-16: Consider organizing importsThe imports look good, but consider grouping related imports together. The IndictmentCountOffense and IndictmentSubtype could be imported in a single line since they're from the same module.
-import { - IndictmentCountOffense, - IndictmentSubtype, -} from '@island.is/judicial-system/types' +import { IndictmentCountOffense, IndictmentSubtype } from '@island.is/judicial-system/types'apps/judicial-system/api/src/app/modules/indictment-count/dto/updateIndictmentCount.input.ts (1)
7-10: Consider organizing importsSimilar to the DTO file, consider consolidating the related type imports.
-import { - IndictmentCountOffense, - IndictmentSubtype, -} from '@island.is/judicial-system/types' +import { IndictmentCountOffense, IndictmentSubtype } from '@island.is/judicial-system/types'apps/judicial-system/backend/src/app/modules/indictment-count/models/indictmentCount.model.ts (1)
14-17: Consider organizing importsSimilar to other files, consider consolidating the related type imports.
-import { - IndictmentCountOffense, - IndictmentSubtype, -} from '@island.is/judicial-system/types' +import { IndictmentCountOffense, IndictmentSubtype } from '@island.is/judicial-system/types'apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (1)
349-351: Consider adding explicit type annotation for better type safetyThe implementation is solid, particularly the use of Set for managing unique subtypes. However, the type safety could be enhanced.
Consider adding an explicit type annotation for the Set:
- const currentSubtypes = new Set( + const currentSubtypes = new Set<IndictmentSubtype>( indictmentCount.indictmentCountSubtypes ?? [], )Also applies to: 415-428
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
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(2 hunks)apps/judicial-system/backend/migrations/20241204124025-update-indictment-count.js(1 hunks)apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts(2 hunks)apps/judicial-system/backend/src/app/modules/indictment-count/models/indictmentCount.model.ts(2 hunks)apps/judicial-system/web/src/components/FormProvider/case.graphql(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.css.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.strings.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx(5 hunks)apps/judicial-system/web/src/utils/hooks/useIndictmentCounts/updateIndictmentCount.graphql(1 hunks)libs/judicial-system/types/src/lib/case.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.css.ts
🧰 Additional context used
📓 Path-based instructions (11)
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/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/judicial-system/types/src/lib/case.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/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/backend/migrations/20241204124025-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/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.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/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."
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/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/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/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 (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (1)
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/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (1)
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.
🪛 Biome (1.9.4)
apps/judicial-system/backend/migrations/20241204124025-update-indictment-count.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (9)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/Indictment.tsx (1)
383-383: LGTM! The tag format change is consistent.
The change from self-closing to standard closing tag format is valid and maintains functionality.
apps/judicial-system/web/src/utils/hooks/useIndictmentCounts/updateIndictmentCount.graphql (1)
14-14: LGTM! Verify field usage in frontend components.
The addition of indictmentCountSubtypes to the mutation response is correct and aligns with the model changes.
Let's verify the field usage in frontend components:
✅ Verification successful
Field is properly used in frontend components
The indictmentCountSubtypes field is correctly utilized in the relevant components:
- Used in
IndictmentCount.tsxfor displaying and updating subtypes - Included in the case query schema (
case.graphql) - Properly integrated with the mutation response
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using this mutation response field
rg -t tsx -t ts "indictmentCountSubtypes" apps/judicial-system/web/src/
Length of output: 99
Script:
#!/bin/bash
# Search for components using this mutation response field, using ripgrep without file type restrictions
rg "indictmentCountSubtypes" apps/judicial-system/web/src/
Length of output: 727
apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts (1)
57-61: LGTM! Property decorators are properly configured
The new indictmentCountSubtypes property is well-defined with appropriate decorators for validation and API documentation.
apps/judicial-system/api/src/app/modules/indictment-count/dto/updateIndictmentCount.input.ts (1)
60-65: LGTM! GraphQL field and validation decorators are properly configured
The new indictmentCountSubtypes property is well-defined with appropriate decorators for GraphQL and validation.
apps/judicial-system/backend/src/app/modules/indictment-count/models/indictmentCount.model.ts (1)
76-82: Verify database migration for the new column
The column definition looks good with proper type configuration. However, let's verify that a corresponding migration exists for adding this column.
✅ Verification successful
Migration exists and is properly configured
The migration file 20241204124025-update-indictment-count.js correctly adds the indictment_count_subtypes column with appropriate configuration:
- Uses ARRAY(STRING) type which matches the model definition
- Sets
allowNull: trueas specified in the model - Includes both up and down migrations for proper version control
- Uses a transaction for data integrity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for migration file that adds indictmentCountSubtypes column
# Look for migration files that might contain the column addition
rg -l "indictment_count_subtypes" apps/judicial-system/backend/migrations/
Length of output: 156
Script:
#!/bin/bash
# Let's check the content of the migration file to verify it properly adds the column
cat apps/judicial-system/backend/migrations/20241204124025-update-indictment-count.js
Length of output: 618
apps/judicial-system/web/src/components/FormProvider/case.graphql (1)
219-219: LGTM! Verify GraphQL schema definition
The addition of indictmentCountSubtypes field to the Case query is correct and aligns with the PR objective.
Let's verify the GraphQL schema definition:
✅ Verification successful
GraphQL schema is properly defined and integrated
The field indictmentCountSubtypes is correctly defined and integrated across the codebase:
- Enum
IndictmentSubtypeis defined inlibs/judicial-system/types/src/lib/case.ts - The field is properly typed in both API and backend models (
IndictmentSubtype[]) - It's correctly used in the frontend components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the IndictmentSubtype enum and field are properly defined in the schema
# Search for IndictmentSubtype enum definition
rg -l "enum IndictmentSubtype" --type graphql
# Search for indictmentCountSubtypes field type definition
rg -l "indictmentCountSubtypes.*IndictmentSubtype" --type graphql
Length of output: 179
Script:
#!/bin/bash
# Let's try searching without file type restriction since GraphQL files might have different extensions
# Search for schema files and enum definitions
fd -e gql -e graphql
# Search for IndictmentSubtype enum definition
rg "enum IndictmentSubtype"
# Search for indictmentCountSubtypes field type definition
rg "indictmentCountSubtypes.*:"
Length of output: 21181
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.strings.ts (1)
144-148: LGTM! Translation strings are well-structured
The new message entry follows the established pattern and naming conventions, with clear description and proper translation.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (2)
8-8: LGTM! Import statements are well-organized
The new imports are properly structured and align with the feature requirements for adding indictment count subtypes.
Also applies to: 14-18, 21-21, 29-29
479-507: Verify the business logic for subtype visibility
The implementation looks good, but please verify:
- Should we hide the subtype selection when there's only one subtype?
- Should we display a message when there's only one subtype?
The current implementation only shows the selection when there are multiple subtypes (> 1).
✅ Verification successful
Let me search for more context about how subtypes are handled in the codebase.
Let me try a different search approach to find subtype-related code.
Let me search for indictment-related files to understand the context better.
Let me search for the specific implementation of indictment subtypes to understand how they are handled.
Based on the code analysis, I can now provide a definitive response about the subtype visibility logic.
The current implementation is correct
The condition subtypes.length > 1 for showing the subtype selection is the intended behavior because:
- The code shows this is part of a judicial system where indictment subtypes are managed carefully
- When there's only one subtype, showing a selection UI would be unnecessary since there's no choice to make
- The implementation is consistent with other parts of the codebase where subtypes are handled
The code doesn't show any message display when there's only one subtype, which is also correct as it would add unnecessary UI complexity for a non-choice scenario.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in other components to verify consistency
ast-grep --pattern 'subtypes.length > 1'
Length of output: 168
Script:
#!/bin/bash
# Search for subtype-related patterns and their handling
rg -A 5 "subtype" --type typescript
Length of output: 70
Script:
#!/bin/bash
# Search for subtype-related patterns in all files
rg -A 5 "subtype"
# Also search for any files containing "indictment" in their name
fd "indictment" -i
Length of output: 80343
Script:
#!/bin/bash
# Search for indictment-related files and their content
rg -A 5 "indictmentSubtype" --type ts
# Also search for any files containing indictment in their name
fd "indictment" -i
Length of output: 67326
Script:
#!/bin/bash
# Search for the IndictmentSubtype enum definition
rg -A 10 "enum IndictmentSubtype" --type ts
# Also search for any files that define indictment subtype handling logic
rg -A 5 "subtypes.length" --type ts
Length of output: 2432
apps/judicial-system/backend/migrations/20241204124025-update-indictment-count.js
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 28 Total Test Services: 0 Failed, 27 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/backend/migrations/20241204124025-update-indictment-count.js (1)
4-17: Consider adding array validation constraints.The transaction usage is good for ensuring atomic operations. However, consider adding constraints to prevent potential issues:
- Maximum array length to prevent memory issues
- String validation for array elements to ensure they match expected subtypes
{ type: Sequelize.ARRAY(Sequelize.STRING), allowNull: true, defaultValue: [], + validate: { + len: [0, 10], // Adjust max length as needed + isValidSubtype(value) { + if (value && value.length) { + const validSubtypes = ['TYPE1', 'TYPE2']; // Add your valid subtypes + if (!value.every(type => validSubtypes.includes(type))) { + throw new Error('Invalid subtype detected'); + } + } + } + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/backend/migrations/20241204124025-update-indictment-count.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/migrations/20241204124025-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."
🪛 Biome (1.9.4)
apps/judicial-system/backend/migrations/20241204124025-update-indictment-count.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (2)
apps/judicial-system/backend/migrations/20241204124025-update-indictment-count.js (2)
1-2: Remove redundant 'use strict' directive.
JavaScript modules are automatically in strict mode, making this directive unnecessary.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
19-24: LGTM! Clean rollback implementation.
The down migration correctly removes the column, providing a clean rollback path.
…island.is into j-s/select-indictment-subtype
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx (2)
Line range hint
15-82: Consider improving string formatting readability.The helper functions are well-structured and handle all cases properly. However, the string concatenation in
mapServiceStatusMessagescould be more readable.Consider refactoring the string concatenation using template literals and optional chaining:
- `${subpoena.servedBy ? subpoena.servedBy : ''}${ - subpoena.serviceDate - ? ` - ${formatDate(subpoena.serviceDate, 'Pp')}` - : '' - }`, + `${subpoena.servedBy ?? ''}${ + subpoena.serviceDate ? ` - ${formatDate(subpoena.serviceDate, 'Pp')}` : '' + }`,
Line range hint
100-143: Consider improving conditional rendering and key generation.While the component implementation is solid, there are two areas for improvement:
- The AlertMessage type prop condition could be simplified
- The key prop in the Text component mapping could be more unique to prevent potential issues with duplicate messages
Consider these improvements:
- type={ - !subpoena?.serviceStatus - ? 'info' - : subpoena?.serviceStatus === ServiceStatus.FAILED || - subpoena?.serviceStatus === ServiceStatus.EXPIRED - ? 'warning' - : 'success' - } + type={ + !subpoena?.serviceStatus ? 'info' : + [ServiceStatus.FAILED, ServiceStatus.EXPIRED].includes(subpoena.serviceStatus) ? 'warning' : 'success' + } - <Text variant="small" key={`${msg}-${subpoena.created}`}> + <Text variant="small" key={`${msg}-${subpoena.created}-${messages.indexOf(msg)}`}>apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (2)
349-351: Consider memoizing the subtypes calculation.The subtypes calculation could benefit from memoization to prevent unnecessary recalculations on re-renders.
- const subtypes = indictmentCount.policeCaseNumber - ? workingCase.indictmentSubtypes[indictmentCount.policeCaseNumber] - : [] + const subtypes = useMemo(() => + indictmentCount.policeCaseNumber + ? workingCase.indictmentSubtypes[indictmentCount.policeCaseNumber] + : [], + [indictmentCount.policeCaseNumber, workingCase.indictmentSubtypes] + )
479-511: Consider accessibility improvements for the checkboxes.While the implementation is functional, consider adding ARIA labels and role attributes for better screen reader support.
<div className={styles.indictmentSubtypesContainter} + role="group" + aria-label={formatMessage(strings.selectIndictmentSubtype)} > {subtypes.map((subtype: IndictmentSubtype) => ( <div className={styles.indictmentSubtypesItem} key={`${subtype}-${indictmentCount.id}`} > <Checkbox name={`${subtype}-${indictmentCount.id}`} value={subtype} label={capitalize(indictmentSubtypes[subtype])} + aria-label={`${formatMessage(strings.selectIndictmentSubtype)} ${capitalize(indictmentSubtypes[subtype])}`} checked={indictmentCount.indictmentCountSubtypes?.includes(subtype) ?? false} onChange={(evt) => { handleSubtypeChange(subtype, evt.target.checked) }} backgroundColor="white" large filled /> </div> ))} </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
apps/judicial-system/api/infra/judicial-system-api.ts(1 hunks)apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.css.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx(5 hunks)apps/judicial-system/web/src/utils/hooks/useSections/index.ts(5 hunks)charts/judicial-system/values.prod.yaml(1 hunks)charts/judicial-system/values.staging.yaml(1 hunks)charts/services/judicial-system-api/values.prod.yaml(1 hunks)charts/services/judicial-system-api/values.staging.yaml(1 hunks)libs/judicial-system/types/src/lib/feature.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.css.ts
🧰 Additional context used
📓 Path-based instructions (5)
libs/judicial-system/types/src/lib/feature.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/api/infra/judicial-system-api.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/ServiceAnnouncement/ServiceAnnouncement.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/judicial-system/web/src/utils/hooks/useSections/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.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."
🔇 Additional comments (13)
apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx (3)
Line range hint 1-13: LGTM! Well-organized imports following best practices.
The imports are properly organized, with clear separation between external dependencies and internal modules. Type imports are correctly used.
Line range hint 84-90: LGTM! Clean error handling implementation.
The error rendering function is concise and properly handles internationalization.
Line range hint 92-98: LGTM! Well-defined TypeScript interface.
The props interface is properly typed and follows TypeScript best practices.
apps/judicial-system/web/src/utils/hooks/useSections/index.ts (3)
11-11: Imports are correctly updated
The addition of Feature to the imports ensures that feature flags are properly utilized within the hook.
23-23: Importing FeatureContext for feature flag access
Importing FeatureContext enables the hook to access the application's feature flags, allowing for conditional logic based on enabled features.
66-66: Using useContext to access features
Accessing features from FeatureContext via useContext is appropriate and aligns with React best practices for context usage.
libs/judicial-system/types/src/lib/feature.ts (1)
3-3: Adding MULTIPLE_INDICTMENT_SUBTYPES to the Feature enum
The new enum member MULTIPLE_INDICTMENT_SUBTYPES correctly extends the Feature enum, supporting the introduction of the new feature flag within the system.
charts/services/judicial-system-api/values.prod.yaml (1)
33-33: Verify the enablement of MULTIPLE_INDICTMENT_SUBTYPES in production
The HIDDEN_FEATURES environment variable now includes 'MULTIPLE_INDICTMENT_SUBTYPES', which activates this feature in the production environment. Please confirm that this feature is fully tested and intended to be released to production users.
charts/services/judicial-system-api/values.staging.yaml (1)
33-33: Enabling MULTIPLE_INDICTMENT_SUBTYPES feature in staging
The environment variable HIDDEN_FEATURES is updated to include 'MULTIPLE_INDICTMENT_SUBTYPES', correctly enabling the new feature in the staging environment for further testing.
apps/judicial-system/api/infra/judicial-system-api.ts (1)
50-51: Confirm activation of MULTIPLE_INDICTMENT_SUBTYPES feature flag in production
The HIDDEN_FEATURES configuration now includes 'MULTIPLE_INDICTMENT_SUBTYPES' for both staging and production environments:
HIDDEN_FEATURES: {
dev: '',
staging: 'MULTIPLE_INDICTMENT_SUBTYPES',
prod: 'MULTIPLE_INDICTMENT_SUBTYPES',
},Ensure that the feature is fully vetted and that enabling it in the production environment is intentional.
charts/judicial-system/values.prod.yaml (1)
33-33: LGTM! Feature flag consistently applied in production.
The feature flag configuration matches the staging environment, ensuring consistent behavior across environments.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (2)
8-8: LGTM! Dependencies properly imported.
The imports are well-organized and include all necessary components and utilities for the new subtype functionality.
Also applies to: 14-18, 21-21, 29-29
415-428: LGTM! Event handler implementation is clean and efficient.
The handleSubtypeChange function effectively manages the state updates using Set operations for uniqueness.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (2)
76-76: Consider memoizing feature checksWhile the current implementation is correct, consider memoizing feature checks to prevent unnecessary rerenders when other context values change.
- const { features } = useContext(FeatureContext) + const { features } = useContext(FeatureContext) + const memoizedFeatures = useMemo( + () => ({ + hasMultipleIndictmentSubtypes: features.includes(Feature.MULTIPLE_INDICTMENT_SUBTYPES) + }), + [features] + )
Line range hint
1-300: Consider breaking down the component for better maintainabilityThe component handles multiple responsibilities and contains several conditional rendering blocks. Consider extracting these sections into separate components for better maintainability and reusability:
- Create separate components for each file type section (indictments, criminal records, etc.)
- Create a common FileSection component to reduce duplication
Example refactor for a file section:
interface FileSectionProps { title: string; files: CaseFile[]; onOpenFile: (fileId: string) => void; } const FileSection: FC<FileSectionProps> = ({ title, files, onOpenFile }) => { if (!files?.length) return null; return ( <Box marginBottom={5}> <Text variant="h4" as="h4" marginBottom={1}> {title} </Text> <RenderFiles caseFiles={files} onOpenFile={onOpenFile} /> </Box> ); };apps/judicial-system/web/src/utils/hooks/useCaseList/index.tsx (2)
53-55: Consider extracting traffic violation check to a separate functionThe traffic violation check combines feature flag and business logic. Consider extracting this into a separate function for better maintainability and reusability.
+const isTrafficViolationEnabled = ( + features: Feature[], + caseData: Case +): boolean => { + return ( + features.includes(Feature.MULTIPLE_INDICTMENT_SUBTYPES) || + isTrafficViolationCase(caseData) + ) +} const openCase = useCallback( (caseToOpen: Case, openCaseInNewTab?: boolean) => { let routeTo = null - const isTrafficViolation = - features.includes(Feature.MULTIPLE_INDICTMENT_SUBTYPES) || - isTrafficViolationCase(caseToOpen) + const isTrafficViolation = isTrafficViolationEnabled(features, caseToOpen)
Line range hint
134-137: Add missing 'features' dependency to useCallbackThe
openCasecallback usesfeaturesfrom context but doesn't include it in the dependency array. This could lead to stale closures where the callback uses outdated feature flags.useCallback( (caseToOpen: Case, openCaseInNewTab?: boolean) => { // ... callback implementation }, - [router, user], + [router, user, features], )apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (3)
79-81: Consider extracting feature check logic into a custom hookThe feature flag check combined with the traffic violation case check could be moved to a custom hook for better maintainability and reusability.
+const useIsTrafficViolationCase = (workingCase: Case) => { + const { features } = useContext(FeatureContext) + return features.includes(Feature.MULTIPLE_INDICTMENT_SUBTYPES) || + isTrafficViolationCase(workingCase) +} const Processing: FC = () => { // ... - const isTrafficViolationCaseCheck = - features.includes(Feature.MULTIPLE_INDICTMENT_SUBTYPES) || - isTrafficViolationCase(workingCase) + const isTrafficViolationCaseCheck = useIsTrafficViolationCase(workingCase)
Line range hint
116-119: Consider enhancing form validation with a validation schemaThe current validation relies on a single function. Consider using a schema validation library like Zod or Yup for more robust form validation.
import { z } from 'zod' const processingSchema = z.object({ court: z.string().min(1, 'Court is required'), hasCivilClaims: z.boolean(), civilClaimants: z.array( z.object({ nationalId: z.string().min(11).nullable(), name: z.string().min(1, 'Name is required'), }) ).optional(), }) const stepIsValid = () => { try { processingSchema.parse(workingCase) return true } catch { return false } }
Line range hint
95-112: Consider adding type safety to navigation pathsThe navigation paths are currently stringly typed. Consider creating a type-safe routing solution.
type RouteConfig = { INDICTMENTS_CASE_FILE_ROUTE: string INDICTMENTS_TRAFFIC_VIOLATION_ROUTE: string INDICTMENTS_CASE_FILES_ROUTE: string } const ROUTES: RouteConfig = { INDICTMENTS_CASE_FILE_ROUTE: '/indictments/case-file', INDICTMENTS_TRAFFIC_VIOLATION_ROUTE: '/indictments/traffic-violation', INDICTMENTS_CASE_FILES_ROUTE: '/indictments/case-files', } as const const handleNavigationTo = useCallback( async (destination: keyof RouteConfig) => { // ... existing code router.push(`${ROUTES[destination]}/${workingCase.id}`) }, [router, setWorkingCase, transitionCase, workingCase], )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx(3 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/CaseFiles/CaseFiles.tsx(3 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx(3 hunks)apps/judicial-system/web/src/utils/hooks/useCaseList/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/CaseFiles/CaseFiles.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/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.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/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.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/judicial-system/web/src/utils/hooks/useCaseList/index.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/Processing/Processing.tsx (1)
Learnt from: gudjong
PR: island-is/island.is#15421
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx:55-61
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The `updateCase` method in the `apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx` file has its own error handling, and additional error handling in the `initialize` function is not necessary.
🔇 Additional comments (5)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (2)
8-8: LGTM: Clean import organization
The new imports follow the project's organization pattern, separating external and internal dependencies appropriately.
Also applies to: 19-19
82-84: Verify feature flag integration across the application
The logic for showing traffic violation case files is well-implemented, combining the new feature flag with existing case type checks for backward compatibility.
Let's verify consistent feature flag usage across related components:
✅ Verification successful
Feature flag integration is consistently implemented
The feature flag MULTIPLE_INDICTMENT_SUBTYPES is consistently used across the application in combination with isTrafficViolationCase checks. The pattern features.includes(Feature.MULTIPLE_INDICTMENT_SUBTYPES) || isTrafficViolationCase(workingCase) is uniformly applied in:
- IndictmentCaseFilesList component
- Processing route component
- CaseFiles route component
- useSections hook
- useCaseList hook
The implementation maintains backward compatibility while properly integrating the new feature flag.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent MULTIPLE_INDICTMENT_SUBTYPES feature flag usage
# Search for feature flag usage
rg -A 2 "Feature.MULTIPLE_INDICTMENT_SUBTYPES" --type typescript
# Search for potential traffic violation case handling
ast-grep --pattern 'isTrafficViolationCase($_)'
Length of output: 1454
Script:
#!/bin/bash
# Let's check for the Feature enum definition and other feature flag usages
rg -l "enum Feature" --type ts
# Then check the content of the file containing Feature enum
rg -A 5 "MULTIPLE_INDICTMENT_SUBTYPES" --type ts
Length of output: 4444
apps/judicial-system/web/src/utils/hooks/useCaseList/index.tsx (1)
13-13: LGTM: Feature context integration is well-structured
The Feature import and FeatureContext integration are properly implemented, following React's context best practices.
Also applies to: 27-27, 45-45
apps/judicial-system/web/src/routes/Prosecutor/Indictments/CaseFiles/CaseFiles.tsx (1)
8-11: LGTM: Consistent feature context implementation
The Feature import and FeatureContext integration follow the same pattern as other files, maintaining consistency across the codebase.
Also applies to: 14-14, 36-36
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (1)
Line range hint 252-282: Consider adding loading state for civil claimant updates
The civil claimant update logic could experience race conditions when multiple updates occur in quick succession. Consider adding a loading state to prevent concurrent updates.
const [nationalIdNotFound, setNationalIdNotFound] = useState<boolean>(false)
+const [isUpdatingCivilClaimant, setIsUpdatingCivilClaimant] = useState<boolean>(false)
const handleUpdateCivilClaimant = useCallback(
async (updatedCivilClaimant: UpdateCivilClaimantInput) => {
+ if (isUpdatingCivilClaimant) return
+ setIsUpdatingCivilClaimant(true)
try {
updateCivilClaimantState(updatedCivilClaimant, setWorkingCase)
await updateCivilClaimant(updatedCivilClaimant)
+ } finally {
+ setIsUpdatingCivilClaimant(false)
+ }
},
- [updateCivilClaimant, setWorkingCase, updateCivilClaimantState],
+ [updateCivilClaimant, setWorkingCase, updateCivilClaimantState, isUpdatingCivilClaimant],
)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/CaseFiles/CaseFiles.tsx
Show resolved
Hide resolved
…-indictment-subtype
…-indictment-subtype
…island.is into j-s/select-indictment-subtype
* Add frontend changes for selecting indictment subtypes in indictment count * Create migration * Update DB on change * Refactor * Refactor * chore: nx format:write update dirty files * Add feature flag * Refactor * Refactor * Add featur flag --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: unakb <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Add indictment count subtypes to IndictmentCount
Asana
What
Add checkboxes to IndictmentCount component so that users can select what subtypes are applicable to each indictment count.
Why
This is one step in making other types, besides traffic violations, work in RVG
Screenshots / Gifs
Screen.Recording.2024-12-04.at.15.02.11.mov
Checklist:
Summary by CodeRabbit
New Features
indictmentCountSubtypesfor managing multiple indictment subtypes in various components and APIs.indictmentCountSubtypes, providing more detailed data.Bug Fixes
Indictmentcomponent.Documentation
Style
Chores
MULTIPLE_INDICTMENT_SUBTYPESfeature flag.