feat(j-s): show error state for invalid items in SelectableList #17504
Conversation
WalkthroughThe pull request introduces modifications to the 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
|
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17504 +/- ##
==========================================
- Coverage 35.61% 35.61% -0.01%
==========================================
Files 7017 7015 -2
Lines 150430 150367 -63
Branches 42974 42949 -25
==========================================
- Hits 53570 53546 -24
+ Misses 96860 96821 -39
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/judicial-system/web/src/components/SelectableList/SelectableList.tsx (1)
91-95: Consider extracting the invalid item filtering logic.The invalid item filtering logic is repeated in multiple places. Consider extracting it into a helper function for better maintainability.
+ const isItemValid = (item: Item) => !item.invalid + const getValidItems = (items: Item[]) => items.filter(isItemValid) // Usage in useEffect checked: (selectableItems.find((i) => i.id === item.id)?.checked && isItemValid(item)) ?? false, // Usage in select all checkbox checked={ selectableItems.length > 0 && getValidItems(selectableItems).every((item) => item.checked) } // Usage in select all handler checked: evt.target.checked && isItemValid(item),Also applies to: 122-125, 128-132
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.tsx (1)
59-73: Enhance file validation robustness.While the current implementation works, it could be improved in several ways:
- Extract the PDF validation to a named function
- Make the file extension a constant
- Add case-insensitive validation
+ const ALLOWED_FILE_EXTENSION = '.pdf' + const isPDFFile = (filename: string): boolean => + filename.toLowerCase().endsWith(ALLOWED_FILE_EXTENSION.toLowerCase()) items={policeCaseFileList?.map((p) => { - const invalid = !p.name.endsWith('.pdf') + const invalid = !isPDFFile(p.name) return { id: p.id, name: p.name, ...(invalid ? { invalid, tooltipText: formatMessage( strings.invalidPoliceCaseFileFromLOKE, ), } : {}), } })}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/judicial-system/web/src/components/SelectableList/SelectableList.tsx(6 hunks)apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.strings.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.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/components/PoliceCaseFiles/PoliceCaseFiles.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/SelectableList/SelectableList.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 (3)
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.strings.ts (1)
41-47: LGTM! Well-structured localization message.The new error message is properly defined with a clear description and follows the established naming convention.
apps/judicial-system/web/src/components/SelectableList/SelectableList.tsx (2)
24-25: LGTM! Well-defined interface extensions.The new optional properties
invalidandtooltipTextare appropriately typed and named.
189-192: LGTM! Clear visual feedback for invalid items.The implementation provides clear visual feedback with:
- Red background for invalid items
- Warning icon
- Tooltip with error message
- Disabled checkbox state
Also applies to: 208-208, 218-229
...icial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.strings.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.tsx
Outdated
Show resolved
Hide resolved
…s/handle-invalid-file-names-from-loke
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.tsx (1)
71-77: Consider improving type safety in the items mapping.The current implementation could benefit from explicit type annotations to ensure type safety and improve maintainability.
Consider this improvement:
- items={policeCaseFileList?.map((p) => { - return { + items={policeCaseFileList?.map((p): Item => { + return { id: p.id, name: p.name, ...(isIndictment ? validateFileName(p.name) : {}), } })}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.strings.ts(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.strings.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.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 (3)
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.tsx (3)
5-5: LGTM! Import changes are appropriate.The addition of
CaseTypeimport is necessary for the new validation logic.
48-56: LGTM! Validation function is well-implemented.The function provides clear validation metadata with appropriate error messaging. The implementation is correct as the actual restriction to PDF files is controlled by the caller based on the case type.
65-65: LGTM! Validation is correctly applied only to indictment cases.The implementation properly:
- Checks the case type before applying validation
- Only restricts PDF files for indictment cases
- Preserves support for other file types in non-indictment cases
Also applies to: 71-77
…s/handle-invalid-file-names-from-loke
gudjong
added
the
deprecated:automerge
thorhildurt
removed
the
deprecated:automerge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.tsx (1)
56-64: Enhance file validation robustness.The current validation might miss edge cases. Consider these improvements:
- Make the extension check case-insensitive
- Add MIME type validation when possible
const validateFileName = (filename: string) => { - const invalid = !filename.endsWith('.pdf') + const invalid = !filename.toLowerCase().endsWith('.pdf') if (!invalid) return {} return { invalid, tooltipText: formatMessage(strings.invalidPoliceCaseFileFromLOKE), } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.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 (3)
apps/judicial-system/web/src/routes/Prosecutor/components/PoliceCaseFiles/PoliceCaseFiles.tsx (3)
5-5: LGTM!The import addition is necessary and follows best practices.
54-54: LGTM!The type check correctly determines when to apply PDF-only validation, as required for indictment cases.
70-76: LGTM!The mapping elegantly applies validation only for indictment cases while preserving the required fields.
Asana task
What
Why
Screenshots / Gifs
https://github.com/user-attachments/assets/18d36b54-435e-4c03-83c7-81abb6f0a2a3
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Localization
These updates improve user experience by providing clearer guidance and preventing incorrect file selections.