chore(j-s): Use Table component for past cases #16939
Conversation
…e-announcement-defenders
…e-announcement-defenders
…e-announcement-defenders
…d-is/island.is into j-s/service-announcement-defenders
…e-announcement-defenders
…e-announcement-defenders
WalkthroughThe pull request involves the removal of the Changes
Possibly related PRs
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 ✅ 99 Total Test Services: 0 Failed, 91 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (4) |
oddsson
changed the title
…cases-table-refactoring
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
apps/judicial-system/web/src/routes/Shared/Cases/ActiveCases.tsx (2)
60-73: LGTM! Consider type safety improvement.The context menu items generation is well-structured and follows React best practices. The code is more concise with the implicit return.
Consider adding explicit return type annotation for better type safety:
- generateContextMenuItems={(row) => [ + generateContextMenuItems={(row): ContextMenuItem[] => [
Line range hint
1-124: Consider centralizing sorting state management.The Table component accepts sortable configuration for multiple columns (defendants, created, courtDate). Consider implementing a centralized sorting state management solution if this sorting logic is shared across different table instances in the application.
This would:
- Ensure consistent sorting behavior across tables
- Make it easier to persist sort preferences
- Reduce duplicate code
You could achieve this by:
- Creating a custom hook (e.g.,
useTableSort)- Using React Context for global sorting preferences
- Implementing a state management solution (e.g., Redux, Zustand) if sorting state needs to be shared widely
apps/judicial-system/web/src/components/Table/Table.tsx (4)
101-105: Consider adding TypeScript return type annotationThe function implementation looks good, but could benefit from explicit TypeScript return type annotation for better type safety.
- const handleCaseClick = (theCase: CaseListEntry) => { + const handleCaseClick = (theCase: CaseListEntry): void => {
107-123: Add TypeScript return type annotation for renderProsecutorTextFor better type safety and documentation, consider adding an explicit return type.
- const renderProsecutorText = ( + const renderProsecutorText = ( state?: CaseState | null, prosecutorName?: string | null, - ) => { + ): ReactNode => {
125-146: Extract date formatting logic to a utility functionThe date formatting logic in renderPostponedOrCourtDateText could be extracted to a reusable utility function to improve maintainability and reduce duplication.
+ const formatCourtDate = (date: string): string => { + const parsedDate = parseISO(date) + return `${formatDate(parsedDate, 'd.M.y')} kl. ${formatDate(parsedDate, 'kk:mm')}` + } const renderPostponedOrCourtDateText = ( postponedIndefinitelyExplanation?: string | null, caseState?: CaseState | null, courtDate?: string | null, - ) => { + ): ReactNode => { // ... if (!isCompletedCase(caseState) && courtDate) { return ( <Text fontWeight="medium" variant="small"> - {`${formatMessage(strings.hearing)} ${formatDate( - parseISO(courtDate), - 'd.M.y', - )} kl. ${formatDate(parseISO(courtDate), 'kk:mm')}`} + {`${formatMessage(strings.hearing)} ${formatCourtDate(courtDate)}`} </Text> ) } // ... }
Line range hint
1-324: Consider splitting mobile and desktop table implementationsThe current Table component handles both mobile and desktop views internally. Consider splitting these into separate components (MobileTable and DesktopTable) and using a container component for the responsive logic. This would:
- Improve maintainability by separating concerns
- Make the code more modular and easier to test
- Reduce cognitive complexity
apps/judicial-system/web/src/routes/Shared/Cases/Cases.tsx (1)
350-350: Consider consistent loading state handling across tablesWhile the
PastCasesTablehas been updated to remove theloadingprop, the loading state is still checked in the parent condition. This creates an inconsistency with other table components that explicitly receive the loading prop.Consider wrapping the
PastCasesTablewithTableWrapperfor consistent loading state handling:-<PastCasesTable cases={pastCases} /> +<TableWrapper loading={loading}> + <PastCasesTable cases={pastCases} /> +</TableWrapper>apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx (1)
65-76: Consider making the "Case Number" and "Type" columns sortableThe columns "Defendant" and "Created" are sortable, but "Case Number" and "Type" are not. For a consistent user experience, consider enabling sorting on these columns if applicable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/judicial-system/web/src/components/Table/PastCasesTable/MobilePastCase.tsx(0 hunks)apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx(2 hunks)apps/judicial-system/web/src/components/Table/Table.css.ts(0 hunks)apps/judicial-system/web/src/components/Table/Table.tsx(5 hunks)apps/judicial-system/web/src/routes/Shared/Cases/ActiveCases.tsx(1 hunks)apps/judicial-system/web/src/routes/Shared/Cases/Cases.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- apps/judicial-system/web/src/components/Table/PastCasesTable/MobilePastCase.tsx
- apps/judicial-system/web/src/components/Table/Table.css.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/components/Table/Table.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/Shared/Cases/ActiveCases.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/Shared/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."
📓 Learnings (1)
apps/judicial-system/web/src/routes/Shared/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 (5)
apps/judicial-system/web/src/routes/Shared/Cases/Cases.tsx (1)
350-350: Verify Table component integration
Let's verify that the PastCasesTable component is properly integrated with the new Table component as per PR objectives.
✅ Verification successful
Let me gather more information about the PastCasesTable implementation.
PastCasesTable is properly integrated with the Table component
The component is correctly using the new Table component:
- It imports the base Table component from the same directory (
import Table from '../Table') - It properly implements the Table interface with thead, data, and columns props
- The component is exported and used correctly in the Cases view
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PastCasesTable uses the new Table component
# Expected: Find import and usage of the new Table component in PastCasesTable
# Check if PastCasesTable imports and uses the Table component
ast-grep --pattern 'import { Table } from "@island.is/judicial-system-web/src/components/Table"'
# Check the implementation
rg -A 10 "export.*PastCasesTable"
Length of output: 622
Script:
#!/bin/bash
# Check the actual implementation of PastCasesTable component
cat apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx
# Also check if there are any other Table imports in this file
rg -l "import.*Table" apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx
Length of output: 5504
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx (4)
33-34: Imports updated to include necessary modules
The Table component and useContextMenu hook have been imported correctly, which are required for the new implementation.
40-40: Simplified component props for PastCasesTable
The PastCasesTable component now only accepts cases as props, simplifying its interface and improving maintainability.
43-43: Integrated openCaseInNewTabMenuItem from context
The openCaseInNewTabMenuItem is now obtained from the useContextMenu hook, ensuring consistent context menu behavior throughout the application.
60-165: Refactored to use the new Table component
The component has been refactored to utilize the new Table component, streamlining the table's rendering logic and enhancing code reuse.
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx
Show resolved
Hide resolved
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx
Show resolved
Hide resolved
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16939 +/- ##
==========================================
- Coverage 35.70% 35.68% -0.03%
==========================================
Files 6939 6938 -1
Lines 147119 147339 +220
Branches 41840 42142 +302
==========================================
+ Hits 52534 52578 +44
- Misses 94585 94761 +176
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.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
apps/judicial-system/web/src/components/Table/PastCasesTable/TestData.ts (1)
6-70: Add JSDoc documentation for the test data.Adding documentation would help other developers understand the purpose and structure of this test data.
Consider adding documentation:
+/** + * Mock case data for testing the PastCasesTable component. + * Includes various case states and scenarios to test different table behaviors. + * @type {TestCase[]} + */ export const cases: TestCase[] = [apps/judicial-system/web/src/components/Table/PastCasesTable/PastCases.spec.tsx (4)
12-12: Consider using the main package path for UserEvent type import.Instead of importing from the internal dist/types path, prefer importing from the main package path for better maintainability.
-import { UserEvent } from '@testing-library/user-event/dist/types/setup/setup' +import { UserEvent } from '@testing-library/user-event'
166-166: Fix typo in test description.There's a typo in "acending" which should be "descending" to match the actual test behavior.
- test('should order the table data by created in acending order when the user clicks the created table header twice', async () => { + test('should order the table data by created in descending order when the user clicks the created table header twice', async () => {
116-122: Enhance test assertions with custom error messages.Consider adding custom error messages to the expect statements to make test failures more descriptive.
Example improvement:
- expect(tableRows[0]).toHaveTextContent('D. M. Kil') + expect(tableRows[0]).toHaveTextContent('D. M. Kil', + 'First row should contain "D. M. Kil" when sorted by name ascending')Also applies to: 137-143, 158-164, 179-185
133-133: Consider using click twice instead of dblClick for more reliable tests.The
dblClickevent can be flaky in some environments due to timing issues. Consider using two separate click events with a small delay between them.- await user.dblClick(await screen.findByTestId('defendantsSortButton')) + const button = await screen.findByTestId('defendantsSortButton') + await user.click(button) + await user.click(button)Also applies to: 175-175
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCases.spec.ts(0 hunks)apps/judicial-system/web/src/components/Table/PastCasesTable/PastCases.spec.tsx(1 hunks)apps/judicial-system/web/src/components/Table/PastCasesTable/TestData.ts(1 hunks)apps/judicial-system/web/src/routes/Shared/Cases/Cases.spec.tsx(0 hunks)
💤 Files with no reviewable changes (2)
- apps/judicial-system/web/src/components/Table/PastCasesTable/PastCases.spec.ts
- apps/judicial-system/web/src/routes/Shared/Cases/Cases.spec.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCases.spec.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/TestData.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."
🔇 Additional comments (3)
apps/judicial-system/web/src/components/Table/PastCasesTable/TestData.ts (1)
1-4: LGTM! Clean and focused imports.
The imports are correctly typed and only include the necessary enums from the GraphQL schema.
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCases.spec.tsx (2)
27-93: Well-structured and comprehensive test suite for getDurationDate!
The test suite effectively covers:
- Edge cases with REJECTED and DISMISSED states
- Different date combinations and fallbacks
- Null handling scenarios
95-102: Clean test setup with proper isolation!
Good practice clearing localStorage before each test to prevent state leakage between tests.
apps/judicial-system/web/src/components/Table/PastCasesTable/TestData.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/web/src/components/Table/PastCasesTable/TestData.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/judicial-system/web/src/components/Table/Table.spec.tsx (1)
36-40: Add error handling to the helper functionWhile the helper function is a good abstraction, consider adding error handling for cases where the button is not found.
const clickButtonByTestId = async (testId: string) => { await act(async () => { + const button = await screen.findByTestId(testId).catch(() => { + throw new Error(`Button with testId "${testId}" not found`) + }) - await user.click(await screen.findByTestId(testId)) + await user.click(button) }) }apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx (2)
Line range hint
53-58: Enhance sort implementation robustnessThe current sorting implementation could be improved to handle edge cases better and provide more efficient date comparison.
Consider this implementation:
const pastCasesData = useMemo( () => cases.sort((a: CaseListEntry, b: CaseListEntry) => - (b['created'] ?? '').localeCompare(a['created'] ?? ''), + new Date(b.created || '').getTime() - new Date(a.created || '').getTime() ), [cases], )
88-164: Consider extracting complex cell renderersThe column definitions contain complex inline components, particularly the state cell renderer (lines 114-150). Consider extracting these into separate components for better maintainability and reusability.
Example refactor:
const StateCell: FC<{ row: CaseListEntry }> = ({ row }) => { const { formatMessage } = useIntl() const { user } = useContext(UserContext) const indictmentCaseStateTag = getIndictmentCaseStateTag(row, user) return ( <> <Box marginRight={row.appealState ? 1 : 0} marginBottom={row.appealState ? 1 : 0}> {/* ... existing state tag logic ... */} </Box> {row.appealState && <TagAppealState {...row} />} </> ) } // Then in columns definition: { cell: (row) => <StateCell row={row} /> }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx(2 hunks)apps/judicial-system/web/src/components/Table/Table.spec.tsx(3 hunks)apps/judicial-system/web/src/components/Tags/utils.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/components/Table/Table.spec.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/Tags/utils.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."
📓 Learnings (2)
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#16939
File: apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx:126-131
Timestamp: 2024-11-19T21:47:09.093Z
Learning: The `CaseListEntry` interface in `@island.is/judicial-system-web/src/graphql/schema` includes the properties `isValidToDateInThePast`, `indictmentRulingDecision`, and `indictmentDecision`.
apps/judicial-system/web/src/components/Tags/utils.ts (2)
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.
Learnt from: unakb
PR: island-is/island.is#15378
File: apps/judicial-system/web/src/routes/Court/Indictments/Summary/Summary.tsx:86-100
Timestamp: 2024-11-12T15:15:11.835Z
Learning: User unakb prefers explicit case handling in switch statements for key functionalities like `getRulingDecisionTagColor` to ensure clarity and avoid assumptions that a case was overlooked.
🔇 Additional comments (8)
apps/judicial-system/web/src/components/Tags/utils.ts (4)
10-10: LGTM! Import changes align with the refactoring.
The addition of CaseListEntry type is consistent with the PR's objective of refactoring the table component.
Line range hint 57-124: LGTM! Helper functions maintain consistent patterns.
The helper functions follow established patterns for tag generation with explicit case handling, which aligns with the team's preferences as indicated in the retrieved learnings.
43-43: Verify courtDate type compatibility.
The removal of .date access suggests a type change in the courtDate property. Ensure that the courtDate type in CaseListEntry matches the expected type in getReceivedIndictmentStateTag.
✅ Verification successful
Let me analyze the search results to verify the courtDate type compatibility.
The search results show:
- In CaseListEntry:
readonly courtDate?: string- In getReceivedIndictmentStateTag:
courtDate?: string | null- The actual usage in the code:
return getReceivedIndictmentStateTag(indictmentDecision, courtDate)The types are compatible - both the source courtDate from CaseListEntry and the parameter in getReceivedIndictmentStateTag are optional strings (with the latter also accepting null).
Let me generate the final response:
The courtDate type is compatible with getReceivedIndictmentStateTag parameter.
The removal of .date access is correct since:
- CaseListEntry.courtDate is of type
string | undefined - getReceivedIndictmentStateTag accepts
string | null | undefined - No type conversion is needed for the direct parameter pass
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify courtDate type definition and usage
# Search for courtDate type definition in CaseListEntry
ast-grep --pattern 'type CaseListEntry = {
$$$
courtDate$_
$$$
}'
# Search for all usages of courtDate in the codebase
rg 'courtDate' -A 2 -B 2
Length of output: 140079
Line range hint 18-30: Verify type compatibility between Case and CaseListEntry.
Let's ensure that all required properties (state, indictmentReviewer, indictmentRulingDecision, indictmentDecision, courtDate) are properly typed in CaseListEntry.
✅ Verification successful
Type compatibility verified between Case and CaseListEntry
The CaseListEntry model in the backend properly defines all the required properties that are being destructured in the utils.ts file:
state(type: CaseState)indictmentReviewer(type: User)indictmentRulingDecision(type: CaseIndictmentRulingDecision)indictmentDecision(type: IndictmentDecision)courtDate(type: string)
The change from Case to CaseListEntry is type-safe as all the necessary properties are available with the correct types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type compatibility between Case and CaseListEntry
# Search for CaseListEntry type definition
ast-grep --pattern 'type CaseListEntry = {
$$$
}'
# Search for Case type definition for comparison
ast-grep --pattern 'type Case = {
$$$
}'
Length of output: 98
Script:
#!/bin/bash
# Let's try different approaches to find the type definitions
# Search for files containing CaseListEntry
echo "=== Files containing CaseListEntry ==="
rg "CaseListEntry" -l
# Search for the actual type definition with context
echo -e "\n=== CaseListEntry definition ==="
rg "CaseListEntry" -A 10
# Search for Case type similarly
echo -e "\n=== Files containing Case type ==="
rg "type Case =" -l
echo -e "\n=== Case type definition ==="
rg "type Case =" -A 10
# Try searching in TypeScript interface definitions
echo -e "\n=== Interface definitions ==="
rg "interface.*Case" -A 10
Length of output: 137477
Script:
#!/bin/bash
# Let's search for the CaseListEntry type definition in the GraphQL schema
echo "=== Searching for CaseListEntry type definition in GraphQL schema ==="
rg -A 20 "CaseListEntry.*{" apps/judicial-system/web/src/graphql/schema.ts
# Let's also check for any imports of CaseListEntry to understand its source
echo -e "\n=== Checking CaseListEntry imports ==="
rg "import.*CaseListEntry.*from" -A 1
Length of output: 3976
Script:
#!/bin/bash
# Let's fix the regex and search for CaseListEntry definition in schema
echo "=== Searching for CaseListEntry definition in schema ==="
rg -A 20 "export (type|class) CaseListEntry" apps/judicial-system/web/src/graphql/schema.ts apps/judicial-system/api/src/app/modules/case-list/models/caseList.model.ts
# Let's also check the properties used in the utils.ts file
echo -e "\n=== Checking properties used in utils.ts ==="
rg -A 10 "const \{.*\} = caseListEntry" apps/judicial-system/web/src/components/Tags/utils.ts
Length of output: 2713
Script:
#!/bin/bash
# Let's try to find the destructured properties in utils.ts with a different pattern
echo "=== Checking properties used in utils.ts ==="
rg -A 10 "const.*state.*indictmentReviewer.*indictmentRulingDecision.*indictmentDecision.*courtDate" apps/judicial-system/web/src/components/Tags/utils.ts
# Let's also check if these properties are defined in the CaseListEntry model
echo -e "\n=== Checking specific properties in CaseListEntry model ==="
rg "readonly (state|indictmentReviewer|indictmentRulingDecision|indictmentDecision|courtDate)" apps/judicial-system/api/src/app/modules/case-list/models/caseList.model.ts
Length of output: 777
apps/judicial-system/web/src/components/Table/Table.spec.tsx (2)
Line range hint 4-34: LGTM: Proper test setup with UserEvent
The changes follow React Testing Library best practices by using UserEvent for simulating user interactions and properly cleaning up between tests.
92-98: LGTM: Clean test refactoring
The test refactoring improves readability while maintaining the same test coverage and assertions.
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx (2)
33-33: LGTM: Good move towards standardization
The introduction of the shared Table component aligns with the PR objective of standardizing table implementations across the project.
82-87: LGTM: Well-structured context menu implementation
The context menu implementation follows good practices with:
- Clean separation of concerns
- Conditional menu items based on row state
- Proper type safety
…cases-table-refactoring
gudjong
added
the
deprecated:automerge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx (2)
Line range hint
37-56: Consider leveraging Table component's built-in sortingThe current implementation uses
useMemofor sorting, while the Table component supports sorting through its props (as evidenced by thesortableconfiguration in the thead). Consider removing the manual sort and letting the Table component handle it.- const pastCasesData = useMemo( - () => - cases.sort((a: CaseListEntry, b: CaseListEntry) => - (b['created'] ?? '').localeCompare(a['created'] ?? ''), - ), - [cases], - )
111-147: Consider extracting state tag logic into a separate componentThe state tag rendering logic is complex and could benefit from being extracted into a dedicated component for better maintainability and reusability.
type StateTagProps = Pick<CaseListEntry, | 'type' | 'state' | 'isValidToDateInThePast' | 'indictmentRulingDecision' | 'indictmentDecision' | 'appealState' | 'appealRulingDecision' >; const CaseStateTag: FC<StateTagProps & { user: User }> = ({ type, state, ...props }) => { const indictmentCaseStateTag = getIndictmentCaseStateTag(props, user); return ( <> <Box marginRight={props.appealState ? 1 : 0} marginBottom={props.appealState ? 1 : 0}> {isIndictmentCase(type) ? ( <CaseTag color={indictmentCaseStateTag.color} text={formatMessage(indictmentCaseStateTag.text)} /> ) : ( <TagCaseState caseState={state} caseType={type} isCourtRole={isDistrictCourtUser(user)} {...props} /> )} </Box> {props.appealState && ( <TagAppealState appealState={props.appealState} appealRulingDecision={props.appealRulingDecision} /> )} </> ); };apps/judicial-system/web/src/components/Table/Table.tsx (3)
101-105: Add TypeScript parameter type annotationThe function implementation is good and follows single responsibility principle. However, consider adding explicit type annotation for better type safety:
- const handleCaseClick = (theCase: CaseListEntry) => { + const handleCaseClick = (theCase: CaseListEntry): void => {
125-146: Consider simplifying date formatting logicThe court date formatting could be extracted into a separate utility function to improve readability and reusability.
+ const formatCourtDateTime = (date: string): string => + `${formatMessage(strings.hearing)} ${formatDate(parseISO(date), 'd.M.y')} kl. ${formatDate(parseISO(date), 'kk:mm')}` const renderPostponedOrCourtDateText = ( postponedIndefinitelyExplanation?: string | null, caseState?: CaseState | null, courtDate?: string | null, ) => { if (postponedIndefinitelyExplanation) { return <Text>{formatMessage(strings.postponed)}</Text> } if (!isCompletedCase(caseState) && courtDate) { return ( <Text fontWeight="medium" variant="small"> - {`${formatMessage(strings.hearing)} ${formatDate( - parseISO(courtDate), - 'd.M.y', - )} kl. ${formatDate(parseISO(courtDate), 'kk:mm')}`} + {formatCourtDateTime(courtDate)} </Text> ) } return null }
204-220: Add data-testid for testingConsider adding a data-testid attribute to the MobileCase component for better testability:
<MobileCase + data-testid={`mobile-case-${theCase.id}`} onClick={() => handleCaseClick(theCase)} theCase={theCase} isCourtRole={isDistrictCourtUser(user)} isLoading={isOpeningCaseId === theCase.id && showLoading} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx(2 hunks)apps/judicial-system/web/src/components/Table/Table.tsx(5 hunks)apps/judicial-system/web/src/components/Tags/utils.ts(3 hunks)apps/judicial-system/web/src/routes/Shared/Cases/Cases.spec.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/web/src/routes/Shared/Cases/Cases.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/web/src/components/Tags/utils.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/components/Table/Table.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/components/Table/PastCasesTable/PastCasesTable.tsx (2)
Learnt from: oddsson
PR: island-is/island.is#16939
File: apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx:126-131
Timestamp: 2024-11-19T21:47:09.093Z
Learning: The `CaseListEntry` interface in `@island.is/judicial-system-web/src/graphql/schema` includes the properties `isValidToDateInThePast`, `indictmentRulingDecision`, and `indictmentDecision`.
Learnt from: oddsson
PR: island-is/island.is#16939
File: apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx:136-139
Timestamp: 2024-11-20T10:15:04.980Z
Learning: The properties `isValidToDateInThePast`, `indictmentRulingDecision`, and `indictmentDecision` are defined in the `CaseListEntry` interface used in the `PastCasesTable` component.
apps/judicial-system/web/src/components/Table/Table.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#16939
File: apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx:136-139
Timestamp: 2024-11-20T10:15:04.980Z
Learning: The properties `isValidToDateInThePast`, `indictmentRulingDecision`, and `indictmentDecision` are defined in the `CaseListEntry` interface used in the `PastCasesTable` component.
🔇 Additional comments (7)
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx (3)
25-31: LGTM! Clean interface simplification
The removal of loading and testid props along with the introduction of the Table component aligns well with the standardization goals. The simplified Props interface improves maintainability.
Also applies to: 33-35
59-84: LGTM! Well-structured table configuration
The table header configuration with sortable columns and context menu integration is well implemented. The use of specialized components for different column types promotes reusability.
Line range hint 1-173: LGTM! Follows NextJS best practices
The component implementation adheres to NextJS best practices with proper use of hooks, context, and TypeScript. The modular approach with the Table component improves maintainability and reusability.
apps/judicial-system/web/src/components/Table/Table.tsx (4)
10-15: LGTM: New type imports enhance type safety
The addition of these type imports from the judicial system types package improves type safety and provides better case state validation capabilities.
107-123: LGTM: Well-structured prosecutor text rendering
The function has good null checks and clear conditional logic for rendering prosecutor information.
148-169: LGTM: Well-implemented duration date rendering
The function has good type safety and clear conditional logic for rendering duration dates.
260-260: LGTM: Consistent click handling
The desktop view correctly uses the centralized handleCaseClick function, maintaining consistency with the mobile view.
lodmfjord
removed
the
deprecated:automerge
gudjong
added
the
deprecated:automerge
Use Table component for past cases
Asana
What
Refactor the past cases table to use the new Table component.
Why
All tabels should use this component.
Checklist:
Summary by CodeRabbit
New Features
PastCasesTablecomponent interface by removing the loading prop.Tablecomponent with improved click handling and modularized rendering logic.Bug Fixes
Tests
Casescomponent by removing unnecessary sorting tests while retaining core functionality tests.Tablecomponent tests with new sorting functionality and improved structure.Chores
MobilePastCasecomponent to streamline the codebase.