chore(j-s): Move DefenderCasesTable to new table component #17420
Conversation
WalkthroughThe pull request introduces changes to the Judicial System web application, focusing on the Defender Cases section. The modifications include adding a new variable to simplify user role checks, simplifying type declarations, removing a CSS file, and refactoring the DefenderCasesTable component to utilize a new Table component. Additionally, there are updates to import paths for certain components due to a reorganization of the directory structure. These changes aim to streamline the code structure and improve the rendering of case-related information for defender users. 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 ReportBranch report: ✅ 0 Failed, 333 Passed, 0 Skipped, 1m 16.03s Total Time |
apps/judicial-system/web/src/routes/Defender/Cases/components/DefenderCasesTable.tsx
Outdated
Show resolved
Hide resolved
oddsson
changed the title
![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/routes/Defender/Cases/components/DefenderCasesTable.tsx (1)
96-101: SimplifyColumnCaseTypeprops by passing onlytypeBased on prior implementations and retrieved learnings, it's sufficient to pass only the
typeprop toColumnCaseType. ThedecisionandparentCaseIdprops may not be necessary and can be omitted to simplify the component usage.This suggestion is informed by learnings from previous pull requests.
Apply this diff to simplify the props:
<ColumnCaseType type={row.type} - decision={row.decision} - parentCaseId={row.parentCaseId} />apps/judicial-system/web/src/routes/Defender/Cases/Cases.tsx (1)
21-21: Simplify component type declarationRemoved the unnecessary generic type parameter
<unknown>from theFCtype. UsingFCwithout a type parameter is appropriate when the component does not have explicit props.Apply this diff to simplify the type declaration:
-export const Cases: FC<unknown> = () => { +export const Cases: FC = () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/judicial-system/web/src/components/ContextMenu/ContextMenuOptions/WithdrawAppealMenuOption.tsx(1 hunks)apps/judicial-system/web/src/routes/Defender/Cases/Cases.tsx(2 hunks)apps/judicial-system/web/src/routes/Defender/Cases/components/DefenderCasesTable.css.ts(0 hunks)apps/judicial-system/web/src/routes/Defender/Cases/components/DefenderCasesTable.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/web/src/routes/Defender/Cases/components/DefenderCasesTable.css.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/judicial-system/web/src/components/ContextMenu/ContextMenuOptions/WithdrawAppealMenuOption.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/Defender/Cases/Cases.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/Defender/Cases/components/DefenderCasesTable.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/Defender/Cases/Cases.tsx (1)
Learnt from: gudjong
PR: island-is/island.is#16760
File: apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx:112-118
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In `apps/judicial-system/web/src/routes/Prosecutor/components/CasesAwaitingConfirmationTable/CasesAwaitingConfirmationTable.tsx`, `apps/judicial-system/web/src/routes/Court/components/CasesAwaitingAssignmentTable/CasesAwaitingAssignmentTable.tsx`, and one instance in `apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx`, it's correct to pass only the `type` prop to `ColumnCaseType` without the `decision` and `parentCaseId` props.
🔇 Additional comments (2)
apps/judicial-system/web/src/routes/Defender/Cases/components/DefenderCasesTable.tsx (1)
106-123: LGTM!The use of
BoxwithflexWrap="wrap"ensures that tags are displayed side by side when space allows and wrap to multiple lines when needed, enhancing responsiveness and user experience.apps/judicial-system/web/src/components/ContextMenu/ContextMenuOptions/WithdrawAppealMenuOption.tsx (1)
45-45: Enhance readability by introducingisDefencevariableStoring the result of
isDefenceUser(user)in theisDefencevariable improves readability and avoids redundant function calls within the condition.
There was a problem hiding this comment.
Niice! Looks good to me, only had one non-blocking question and then I noticed this when testing the checkbox selection:
When there are cases assigned to a defender, for a given selection combo, we only show the header. Not sure if we want that or if we rather want to show the standard info alert when no cases exist for a given selection?
apps/judicial-system/web/src/routes/Defender/Cases/components/DefenderCasesTable.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/judicial-system/web/src/routes/Defender/Cases/components/DefenderCasesTable.tsx (3)
26-29: Add JSDoc documentation for the Props interface.Consider adding JSDoc documentation to describe the purpose of each prop and their expected values. This would improve maintainability and help other developers understand the component's API better.
+/** + * Props for the DefenderCasesTable component + * @property {CaseListEntry[]} cases - Array of cases to display in the table + * @property {boolean} [showingCompletedCases] - Flag to indicate if completed cases are being displayed + */ interface Props { cases: CaseListEntry[] showingCompletedCases?: boolean }
47-81: Consider memoizing the table configuration.The table header configuration and context menu items generator could be memoized to prevent unnecessary re-renders.
+const tableHeaders = useMemo( + () => [ + { + title: formatMessage(tables.caseNumber), + }, + // ... rest of the headers + ], + [formatMessage, showingCompletedCases] +); +const generateContextMenuItems = useCallback( + (row: CaseListEntry) => [ + openCaseInNewTabMenuItem(row.id), + ...(shouldDisplayWithdrawAppealOption(row) + ? [withdrawAppealMenuOption(row.id)] + : []), + ], + [openCaseInNewTabMenuItem, shouldDisplayWithdrawAppealOption, withdrawAppealMenuOption] +);
82-147: Enhance accessibility with ARIA labels.Add appropriate ARIA labels to improve accessibility for screen readers, especially for interactive elements and status indicators.
{ cell: (row) => ( <TagContainer> <TagCaseState + aria-label={`Case state: ${row.state}`} caseState={row.state} // ... other props /> {row.appealState && ( <TagAppealState + aria-label={`Appeal state: ${row.appealState}`} appealState={row.appealState} // ... other props /> )} </TagContainer> ), }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/judicial-system/web/src/components/PageTitle/PageTitle.tsx(1 hunks)apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx(2 hunks)apps/judicial-system/web/src/components/Tags/TagAppealState/TagAppealState.tsx(1 hunks)apps/judicial-system/web/src/components/Tags/TagContainer/TagContainer.tsx(1 hunks)apps/judicial-system/web/src/components/index.ts(1 hunks)apps/judicial-system/web/src/routes/Defender/Cases/components/DefenderCasesTable.tsx(2 hunks)apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesAwaitingReview.tsx(1 hunks)apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx(1 hunks)apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx(3 hunks)apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- apps/judicial-system/web/src/components/PageTitle/PageTitle.tsx
- apps/judicial-system/web/src/components/Tags/TagContainer/TagContainer.tsx
- apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx
- apps/judicial-system/web/src/components/Tags/TagAppealState/TagAppealState.tsx
- apps/judicial-system/web/src/components/index.ts
- apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesAwaitingReview.tsx
🧰 Additional context used
📓 Path-based instructions (4)
apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.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/Table/PastCasesTable/PastCasesTable.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/Defender/Cases/components/DefenderCasesTable.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/PublicProsecutor/Tables/CasesReviewed.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/Shared/Cases/PrisonCases.tsx (3)
Learnt from: gudjong
PR: island-is/island.is#16760
File: apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx:112-118
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In `apps/judicial-system/web/src/routes/Prosecutor/components/CasesAwaitingConfirmationTable/CasesAwaitingConfirmationTable.tsx`, `apps/judicial-system/web/src/routes/Court/components/CasesAwaitingAssignmentTable/CasesAwaitingAssignmentTable.tsx`, and one instance in `apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx`, it's correct to pass only the `type` prop to `ColumnCaseType` without the `decision` and `parentCaseId` props.
Learnt from: thorhildurt
PR: island-is/island.is#17285
File: apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx:223-225
Timestamp: 2024-12-18T17:15:59.181Z
Learning: The system currently only allows adding one defendant, so the punishment type logic in the table can default to the first defendant, with a plan to support multiple defendants in the future.
Learnt from: oddsson
PR: island-is/island.is#14673
File: apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx:79-79
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The implementation of `mapIndictmentCaseStateToTagVariant` in `TagCaseState.tsx`, which maps only `CaseState.ACCEPTED` to a specific tag and defaults others to an "unknown" status, is intentional as per the application's requirements.
apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#14673
File: apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx:79-79
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The implementation of `mapIndictmentCaseStateToTagVariant` in `TagCaseState.tsx`, which maps only `CaseState.ACCEPTED` to a specific tag and defaults others to an "unknown" status, is intentional as per the application's requirements.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests (judicial-system-backend)
🔇 Additional comments (3)
apps/judicial-system/web/src/routes/Defender/Cases/components/DefenderCasesTable.tsx (1)
4-24: LGTM! Well-organized imports.The imports are logically grouped and follow the project structure correctly. All imported components and utilities are properly utilized in the implementation.
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx (1)
119-141: Clean refactor using TagContainer!The replacement of the Box component with TagContainer improves the semantic structure while maintaining the existing conditional rendering logic.
apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx (1)
161-182: Consistent implementation of TagContainer!The refactor maintains the existing business logic while improving the semantic structure of the tag grouping.
oddsson
added
the
deprecated:automerge
Move DefenderCasesTable to new table component
Asana
What
A few months ago we created a Table component. Since then we've been gradually moving all tables to that component and this PR implements that for the DefenderCasesTable.
Why
Having one Table component for all tables is cleaner and less fragile than the alternative.
Checklist:
Summary by CodeRabbit
New Features
TagContainercomponent for improved tag layout.Refactor
Code Cleanup
Performance