feat(j-s): Service announcement for defenders #16731
Conversation
…e-announcement-defenders
WalkthroughThe pull request introduces enhancements across multiple components in the judicial system application. It adds a new export for localized service announcement strings, integrates a new Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16731 +/- ##
==========================================
- Coverage 36.44% 36.42% -0.02%
==========================================
Files 6852 6855 +3
Lines 143478 143442 -36
Branches 40949 40933 -16
==========================================
- Hits 52290 52255 -35
+ Misses 91188 91187 -1 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 28 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 34 Total Test Services: 0 Failed, 32 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
oddsson
changed the title
…e-announcement-defenders
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
libs/island-ui/core/src/lib/Box/Box.tsx (2)
33-33: LGTM! Consider adding JSDoc documentation.The addition of the
alignSelfprop follows the existing patterns and enhances the component's flexibility in flex layouts. Consider adding JSDoc documentation to describe the new prop's purpose and accepted values.export const Box = forwardRef<HTMLElement, BoxProps & TestSupport>( ( { + /** Controls how the box aligns itself within its container when used in a flex layout */ alignSelf,Also applies to: 97-97
Line range hint
8-73: Consider organizing props into logical groups.The props destructuring could be more maintainable by grouping related properties. This would make it easier to understand and maintain the component's API.
Consider organizing props into groups like this:
interface BoxLayoutProps { display?: ResponsiveProp<'block' | 'flex' | /* ... */>; position?: ResponsiveProp<'static' | 'relative' | /* ... */>; // ... other layout props } interface BoxFlexProps { flexDirection?: ResponsiveProp<'row' | 'column' | /* ... */>; flexWrap?: ResponsiveProp<'nowrap' | 'wrap' | /* ... */>; alignItems?: ResponsiveProp<'flex-start' | 'center' | /* ... */>; alignSelf?: ResponsiveProp<'auto' | 'flex-start' | /* ... */>; // ... other flex props } interface BoxSpacingProps { margin?: ResponsiveProp<Space | number>; padding?: ResponsiveProp<Space | number>; // ... other spacing props } type BoxProps = BoxLayoutProps & BoxFlexProps & BoxSpacingProps & { // ... other props };libs/island-ui/core/src/lib/AlertMessage/AlertMessage.tsx (2)
57-74: Consider re-exporting the AlertMessageProps interface.Since this is a reusable UI component, consumers might need the type definitions for proper TypeScript usage. Consider re-exporting the
AlertMessagePropsinterface to maintain good TypeScript practices and improve developer experience.-interface AlertMessageProps { +export interface AlertMessageProps { type: AlertMessageType testid?: string }
101-106: Consider simplifying nested Box components.The nested Box components for message rendering could be simplified to reduce DOM depth and improve maintainability.
<Box display="flex" width="full" flexDirection="column" alignSelf="center" > {title && ( <Text as="h5" variant="h5" marginBottom={message ? 1 : 0}> {title} </Text> )} - {message && ( - <Box display="flex" alignItems="center"> - {isValidElement(message) ? ( - message - ) : ( - <Box flexGrow={1}> - <Text variant="small">{message}</Text> - </Box> - )} - </Box> - )} + {message && ( + isValidElement(message) ? message : ( + <Text variant="small">{message}</Text> + ) + )} </Box>Also applies to: 112-122
libs/island-ui/core/src/lib/Box/useBoxStyles.ts (1)
Line range hint
38-359: Architecture Review: Well-implemented component enhancementThe addition of the
alignSelfproperty follows best practices for reusable UI components by:
- Maintaining TypeScript type safety throughout the implementation
- Supporting responsive styling consistently with other properties
- Ensuring proper tree-shaking through careful style reference handling
- Following established patterns for maximum reusability across NextJS apps
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (1)
88-88: Simplify the logic for determininghasLawsBrokenandhasMergeCases.The logic for determining
hasLawsBrokenandhasMergeCasescan be simplified by directly using thelawsBroken.sizeandworkingCase.mergedCases?.lengthchecks, respectively. This eliminates the need for separate variable declarations.Apply this diff to simplify the logic:
-const hasLawsBroken = lawsBroken.size > 0 -const hasMergeCases = - workingCase.mergedCases && workingCase.mergedCases.length > 0 +const hasLawsBroken = lawsBroken.size > 0 +const hasMergeCases = workingCase.mergedCases?.length > 0Also applies to: 100-102
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/judicial-system/web/src/components/index.ts(1 hunks)apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx(5 hunks)libs/island-ui/core/src/lib/AlertMessage/AlertMessage.tsx(4 hunks)libs/island-ui/core/src/lib/Box/Box.tsx(2 hunks)libs/island-ui/core/src/lib/Box/useBoxStyles.css.ts(1 hunks)libs/island-ui/core/src/lib/Box/useBoxStyles.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/judicial-system/web/src/components/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/Shared/IndictmentOverview/IndictmentOverview.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/island-ui/core/src/lib/AlertMessage/AlertMessage.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/core/src/lib/Box/Box.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/core/src/lib/Box/useBoxStyles.css.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/core/src/lib/Box/useBoxStyles.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."
📓 Learnings (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (1)
Learnt from: unakb
PR: island-is/island.is#14712
File: apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/IndictmentOverview.tsx:0-0
Timestamp: 2024-10-08T15:39:04.351Z
Learning: The judicial system application uses a toaster notification system to handle errors globally.
🔇 Additional comments (15)
libs/island-ui/core/src/lib/AlertMessage/AlertMessage.tsx (2)
1-1: Well-optimized imports for better tree-shaking!
The specific React imports (FC, PropsWithChildren, ReactNode, isValidElement) instead of the entire React module will result in better tree-shaking and smaller bundle sizes.
76-78: Excellent TypeScript implementation!
The component properly uses TypeScript's FC and PropsWithChildren types, providing good type safety and documentation for consumers.
apps/judicial-system/web/src/components/index.ts (1)
66-66: LGTM! Verify the export usage.
The new export follows the established patterns and conventions. The naming is consistent, and it's properly placed among related exports.
Let's verify the usage of this export:
✅ Verification successful
Let me gather more context about the usage pattern by checking the import statement and surrounding code:
Let me check one more thing to verify the import pattern:
Export is correctly implemented and properly used
The verification shows that:
- The
serviceAnnouncementStringsexport is properly imported inIndictmentOverview.tsx - It's used correctly to format messages for service status notifications
- The strings file contains the required
serviceStatusSuccessmessage that's being used - The implementation follows the established i18n patterns using
react-intl
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the serviceAnnouncementStrings export is being used correctly
# Test: Search for imports of serviceAnnouncementStrings
rg "import.*serviceAnnouncementStrings.*from.*components" -A 3
# Test: Search for direct usage of serviceAnnouncementStrings
rg "serviceAnnouncementStrings\." -A 3
Length of output: 532
Script:
#!/bin/bash
# Check the import statement and surrounding context in IndictmentOverview.tsx
rg -B 5 -A 10 "serviceAnnouncementStrings" "apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx"
# Check if there are any other files importing from components/index.ts
rg "from '\.\.\/\.\.\/\.\.\/components'" "apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx"
Length of output: 1167
Script:
#!/bin/bash
# Check the import statement pattern without escaping
rg "from '@island.is/judicial-system-web/src/components'" -B 3 "apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx"
# Check the ServiceAnnouncement component's strings file to verify the exported string
cat "apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.strings.ts"
Length of output: 2265
libs/island-ui/core/src/lib/Box/useBoxStyles.ts (3)
38-38: LGTM: Type-safe interface extension
The alignSelf property is properly typed using ResponsiveProp and follows the established pattern for responsive styling properties.
102-102: LGTM: Consistent parameter addition
The alignSelf parameter is correctly positioned among related flex properties, maintaining the established parameter ordering pattern.
351-359: LGTM: Proper responsive style resolution
The implementation correctly:
- Handles responsive breakpoints using
resolveResponsiveProp - Follows the established pattern for conditional style application
- Maintains tree-shaking capability through direct style references
libs/island-ui/core/src/lib/Box/useBoxStyles.css.ts (2)
329-338: LGTM! The alignSelf implementation follows the established patterns.
The implementation correctly defines the standard flex alignment values and exports them using the consistent pattern found throughout the codebase.
340-367: LGTM! Responsive variants are properly implemented.
The responsive variants (sm, md, lg, xl) are correctly implemented using themeUtils.responsiveStyle, maintaining consistency with other flex-related properties in the codebase.
✅ Verification successful
Responsive variants for alignSelf follow the same pattern as alignItems - Verified!
The implementation of alignSelf responsive variants (sm, md, lg, xl) perfectly matches the pattern used for alignItems, using the same themeUtils.responsiveStyle approach with consistent breakpoint handling. Both sets of variants use identical structure:
- Same
styleVariantswrapper - Same
mapToStylePropertyusage - Identical responsive breakpoint implementation (sm, md, lg, xl)
- Consistent property mapping approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the implementation follows the same pattern as other flex properties
echo "Checking pattern consistency with other flex properties..."
rg --type ts "export const align(Items|Self)[A-Z][a-z]+ = styleVariants\(" -A 5
Length of output: 4337
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (7)
49-52: LGTM!
The ServiceAnnouncementProps interface is well-defined and accurately captures the required properties for the ServiceAnnouncement component.
39-39: Confirm the usage of the imported types.
The Defendant, ServiceStatus, and Subpoena types are imported correctly from the @island.is/judicial-system-web/src/graphql/schema package. Ensure that these types are being used appropriately in the newly added code segments, such as the ServiceAnnouncementProps interface and the ServiceAnnouncement component.
To verify the usage of the imported types, run the following script:
Also applies to: 41-42
5-5: Confirm the usage of the AlertMessage component.
The AlertMessage component is imported correctly from the @island.is/island-ui/core package. Ensure that the component is being used as per the updated implementation, considering the removal of the action prop and other changes made to the component.
To verify the usage of the AlertMessage component, run the following script:
15-15: Confirm the usage of the isSuccessfulServiceStatus function.
The isSuccessfulServiceStatus function is imported correctly from the @island.is/judicial-system/types package. Ensure that the function is being used appropriately to check the service status of subpoenas.
To verify the usage of the isSuccessfulServiceStatus function, run the following script:
✅ Verification successful
Let me try another search to find the usage of isSuccessfulServiceStatus:
The isSuccessfulServiceStatus function is correctly used in the codebase
The function is properly imported from @island.is/judicial-system/types and is used appropriately in several key places:
- In
IndictmentOverview.tsxto conditionally render service announcements when subpoena service is successful - In
IndictmentCaseFilesList.tsxto control the display of PDF buttons for service certificates - In the backend services to handle service status notifications and case responses
The implementation in judicial-system/types shows it's a boolean function that checks if a service status is in a list of successful statuses, which is the expected behavior for this use case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the `isSuccessfulServiceStatus` function.
# Test: Search for the function usage. Expect: Usage in the conditional rendering logic.
rg -A 5 $'isSuccessfulServiceStatus'
Length of output: 17640
Script:
#!/bin/bash
# Search for isSuccessfulServiceStatus function usage
ast-grep --pattern 'isSuccessfulServiceStatus($_)'
# Also try with ripgrep for any string matches
rg -A 5 'isSuccessfulServiceStatus'
Length of output: 9105
7-10: Ensure consistent usage of the imported utility functions.
The formatDate and normalizeAndFormatNationalId utility functions are imported correctly. Verify that these functions are being used consistently throughout the file, especially in the newly added code segments.
To check the usage of the utility functions, run the following script:
✅ Verification successful
Let me generate another script to check for the usage of formatDate since the previous output didn't show its usage:
Both utility functions are used correctly and consistently in the codebase
The verification shows that:
formatDateis used correctly in theIndictmentOverview.tsxfile to format service dates with the 'Pp' format patternnormalizeAndFormatNationalIdis used consistently for national ID comparisons in multiple places within the file- Both functions are imported from the correct package and their implementations in
@island.is/judicial-system/formattersare properly defined and tested
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the utility functions.
# Test: Search for the usage of `formatDate`. Expect: Consistent usage.
rg -A 3 $'formatDate'
# Test: Search for the usage of `normalizeAndFormatNationalId`. Expect: Consistent usage.
rg -A 3 $'normalizeAndFormatNationalId'
Length of output: 17640
Script:
#!/bin/bash
# Search for formatDate usage in the IndictmentOverview.tsx file
rg -A 3 "formatDate" "apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx"
# Also check the implementation of formatDate
ast-grep --pattern 'export const formatDate = $_'
Length of output: 12714
34-34: Confirm the usage of the serviceAnnouncementStrings import.
The serviceAnnouncementStrings import is added correctly from the @island.is/judicial-system-web/src/components package. Verify that the imported strings are being used appropriately in the ServiceAnnouncement component.
To check the usage of the serviceAnnouncementStrings, run the following script:
✅ Verification successful
Let me generate another script to check the usage of serviceAnnouncementStrings in the ServiceAnnouncement component:
Let me check the ServiceAnnouncement.strings.ts file to verify the strings:
The serviceAnnouncementStrings import is correctly used in the codebase
The import is properly used in the IndictmentOverview component where it's referenced to display service status messages. The strings are defined in ServiceAnnouncement.strings.ts and contain localized messages for different service announcement states like success, failure, expiry etc. The usage is consistent with the component's purpose of showing subpoena service status notifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the `serviceAnnouncementStrings` import.
# Test: Search for the usage of `serviceAnnouncementStrings`. Expect: Usage in the `ServiceAnnouncement` component.
rg -A 5 $'serviceAnnouncementStrings'
Length of output: 17640
Script:
#!/bin/bash
# Check the ServiceAnnouncement component and its strings
ast-grep --pattern 'export const strings = {
$$$
}'
# Also check the ServiceAnnouncement component implementation
rg -A 5 'ServiceAnnouncement'
Length of output: 47928
Script:
#!/bin/bash
# Check the ServiceAnnouncement strings file
cat apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.strings.ts
Length of output: 1981
173-187: Ensure the conditional rendering logic is correct.
The logic for conditionally rendering the ServiceAnnouncement component based on the user role and subpoena service status appears to be correct. It checks if the user is a defense user and maps through the defendants and their subpoenas to render the component only when the service status is successful.
To double-check the conditional rendering logic, run the following script:
✅ Verification successful
Let me gather more information about the conditional rendering logic.
Let me search for the ServiceAnnouncement component definition and its usage.
Let me check the ServiceAnnouncement strings and component implementation.
Based on the comprehensive review of the code, I can now provide the final response:
The conditional rendering logic is correctly implemented and secure.
The implementation shows that:
- The
isDefenceUser(user)check correctly gates access to defence-only content - The
isSuccessfulServiceStatus(subpoena.serviceStatus)properly filters subpoenas to show only successfully served ones - The
ServiceAnnouncementcomponent has proper error handling, loading states, and type safety - Each announcement is uniquely keyed using both defendant and subpoena IDs
- The Box wrapper with marginBottom provides consistent spacing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the conditional rendering logic for `ServiceAnnouncement`.
# Test: Search for the rendering logic. Expect: Correct user role and service status checks.
rg -A 10 $'isDefenceUser\(user\) &&'
Length of output: 17640
Script:
#!/bin/bash
# Search for isDefenceUser function definition and usage
rg -A 5 "isDefenceUser"
# Search for isSuccessfulServiceStatus function definition and usage
rg -A 5 "isSuccessfulServiceStatus"
# Search for ServiceAnnouncement component definition
ast-grep --pattern 'const ServiceAnnouncement = $_'
Length of output: 56498
Script:
#!/bin/bash
# Search for ServiceAnnouncement component definition and imports
rg -A 5 "ServiceAnnouncement"
# Search for the file containing ServiceAnnouncement strings
fd -g "*ServiceAnnouncement.strings*"
Length of output: 8048
Script:
#!/bin/bash
# Get the ServiceAnnouncement strings content
cat apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.strings.ts
# Get the ServiceAnnouncement component implementation
cat apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx
Length of output: 7144
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx
Show resolved
Hide resolved
…e-announcement-defenders
…d-is/island.is into j-s/service-announcement-defenders
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (1)
48-81: Consider performance optimizations for the ServiceAnnouncement component.The component implementation is clean and well-structured. Consider these improvements:
- Memoize helper functions to prevent unnecessary recalculations
- Add validation for required subpoena fields
Apply this diff to optimize the component:
interface ServiceAnnouncementProps { defendant: Defendant subpoena: Subpoena } const ServiceAnnouncement: FC<ServiceAnnouncementProps> = (props) => { const { defendant, subpoena } = props const { formatMessage } = useIntl() - const getTitle = (defendantName?: string | null): string => { + const getTitle = useCallback((defendantName?: string | null): string => { const successMessage = formatMessage( serviceAnnouncementStrings.serviceStatusSuccess, ) return defendantName ? `${successMessage} - ${defendantName}` : successMessage - } + }, [formatMessage]) - const getMessage = ( + const getMessage = useCallback(( servedBy?: string | null, serviceDate?: string | null, - ): string => { + ): string => { return [servedBy, formatDate(serviceDate, 'Pp')].filter(Boolean).join(', ') - } + }, []) + if (!subpoena.serviceDate || !subpoena.servedBy) { + return null + } return ( <AlertMessage type="success" title={getTitle(defendant.name)} message={getMessage(subpoena.servedBy, subpoena.serviceDate)} /> ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.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/IndictmentOverview/IndictmentOverview.tsx (1)
Learnt from: unakb
PR: island-is/island.is#14712
File: apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/IndictmentOverview.tsx:0-0
Timestamp: 2024-10-08T15:39:04.351Z
Learning: The judicial system application uses a toaster notification system to handle errors globally.
🔇 Additional comments (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (1)
Line range hint 5-41: LGTM! Import statements are well-organized.
The new imports are logically grouped and necessary for the service announcement feature.
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx
Outdated
Show resolved
Hide resolved
…e-announcement-defenders
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (2)
48-81: Consider extracting string formatting logic to separate utility functions.The component implementation is solid, but the string formatting logic in
getTitleandgetMessagecould be extracted to make the component more focused and easier to test.Consider this refactor:
+const formatServiceTitle = (successMessage: string, defendantName?: string | null): string => { + return defendantName ? `${successMessage} - ${defendantName}` : successMessage +} + +const formatServiceMessage = (servedBy?: string | null, serviceDate?: string | null): string => { + return [servedBy, formatDate(serviceDate, 'Pp')].filter(Boolean).join(', ') +} + const ServiceAnnouncement: FC<ServiceAnnouncementProps> = (props) => { const { defendant, subpoena } = props const { formatMessage } = useIntl() - - const getTitle = (defendantName?: string | null): string => { - const successMessage = formatMessage( - serviceAnnouncementStrings.serviceStatusSuccess, - ) - - return defendantName - ? `${successMessage} - ${defendantName}` - : successMessage - } - - const getMessage = ( - servedBy?: string | null, - serviceDate?: string | null, - ): string => { - return [servedBy, formatDate(serviceDate, 'Pp')].filter(Boolean).join(', ') - } + const successMessage = formatMessage(serviceAnnouncementStrings.serviceStatusSuccess) return ( <AlertMessage type="success" - title={getTitle(defendant.name)} - message={getMessage(subpoena.servedBy, subpoena.serviceDate)} + title={formatServiceTitle(successMessage, defendant.name)} + message={formatServiceMessage(subpoena.servedBy, subpoena.serviceDate)} /> ) }
172-185: Optimize nested map operations for better performance.The nested map operations could be simplified using
flatMapandfilterfor better performance and readability.Consider this optimization:
- {isDefenceUser(user) && - workingCase.defendants?.map((defendant) => - defendant.subpoenas?.map( - (subpoena) => - isSuccessfulServiceStatus(subpoena.serviceStatus) && ( - <Box marginBottom={2} key={`${defendant.id}${subpoena.id}`}> - <ServiceAnnouncement - defendant={defendant} - subpoena={subpoena} - /> - </Box> - ), - ), - )} + {isDefenceUser(user) && + workingCase.defendants?.flatMap((defendant) => + (defendant.subpoenas ?? []) + .filter((subpoena) => isSuccessfulServiceStatus(subpoena.serviceStatus)) + .map((subpoena) => ( + <Box marginBottom={2} key={`${defendant.id}${subpoena.id}`}> + <ServiceAnnouncement + defendant={defendant} + subpoena={subpoena} + /> + </Box> + )) + )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.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/IndictmentOverview/IndictmentOverview.tsx (1)
Learnt from: unakb
PR: island-is/island.is#14712
File: apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/IndictmentOverview.tsx:0-0
Timestamp: 2024-10-08T15:39:04.351Z
Learning: The judicial system application uses a toaster notification system to handle errors globally.
🔇 Additional comments (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (1)
Line range hint 5-41: LGTM: Import statements are well-organized.
The new imports are properly grouped and all are utilized within the component implementations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (2)
67-72: Consider enhancing the getMessage function for better maintainability.The current implementation combines formatting and filtering in a single line. Consider separating these concerns for better readability and maintainability.
- const getMessage = ( - servedBy?: string | null, - serviceDate?: string | null, - ): string => { - return [servedBy, formatDate(serviceDate, 'Pp')].filter(Boolean).join(', ') - } + const getMessage = ( + servedBy?: string | null, + serviceDate?: string | null, + ): string => { + const formattedDate = serviceDate ? formatDate(serviceDate, 'Pp') : null + const messageParts = [servedBy, formattedDate] + return messageParts.filter(Boolean).join(', ') + }
173-187: Consider optimizing the service announcement rendering logic.The current implementation uses nested array operations. Consider using reduce for better performance and readability.
- {isDefenceUser(user) && - workingCase.defendants?.map((defendant) => - (defendant.subpoenas ?? []) - .filter((subpoena) => - isSuccessfulServiceStatus(subpoena.serviceStatus), - ) - .map((subpoena) => ( - <Box key={`${defendant.id}${subpoena.id}`} marginBottom={2}> - <ServiceAnnouncement - defendant={defendant} - subpoena={subpoena} - /> - </Box> - )), - )} + {isDefenceUser(user) && + workingCase.defendants?.reduce<React.ReactNode[]>((acc, defendant) => { + const announcements = (defendant.subpoenas ?? []) + .filter((subpoena) => isSuccessfulServiceStatus(subpoena.serviceStatus)) + .map((subpoena) => ( + <Box key={`${defendant.id}${subpoena.id}`} marginBottom={2}> + <ServiceAnnouncement + defendant={defendant} + subpoena={subpoena} + /> + </Box> + )); + return [...acc, ...announcements]; + }, [])}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.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/IndictmentOverview/IndictmentOverview.tsx (1)
Learnt from: unakb
PR: island-is/island.is#14712
File: apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/IndictmentOverview.tsx:0-0
Timestamp: 2024-10-08T15:39:04.351Z
Learning: The judicial system application uses a toaster notification system to handle errors globally.
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (2)
48-81: Consider performance and error handling improvements.While the implementation is correct, consider these enhancements:
- Wrap with error boundary to gracefully handle formatting errors
- Memoize the formatted date to prevent unnecessary recalculations
- Use React.memo since this is a presentational component
+import React from 'react' + interface ServiceAnnouncementProps { defendant: Defendant subpoena: Subpoena } -const ServiceAnnouncement: FC<ServiceAnnouncementProps> = (props) => { +const ServiceAnnouncement: FC<ServiceAnnouncementProps> = React.memo((props) => { const { defendant, subpoena } = props const { formatMessage } = useIntl() + const formattedDate = useMemo( + () => formatDate(subpoena.serviceDate, 'Pp'), + [subpoena.serviceDate] + ) const getTitle = (defendantName?: string | null): string => { const successMessage = formatMessage( serviceAnnouncementStrings.serviceStatusSuccess, ) return defendantName ? `${successMessage} - ${defendantName}` : successMessage } const getMessage = ( servedBy?: string | null, serviceDate?: string | null, ): string => { - return [servedBy, formatDate(serviceDate, 'Pp')].filter(Boolean).join(', ') + return [servedBy, formattedDate].filter(Boolean).join(', ') } return ( <AlertMessage type="success" title={getTitle(defendant.name)} message={getMessage(subpoena.servedBy, subpoena.serviceDate)} /> ) -} +}) + +ServiceAnnouncement.displayName = 'ServiceAnnouncement'
172-186: Add type guard for null safety.While the implementation works, we should add explicit type guards to handle potential null values more safely.
{isDefenceUser(user) && - workingCase.defendants?.map((defendant) => + (workingCase.defendants ?? []).map((defendant) => (defendant.subpoenas ?? []) .filter((subpoena) => isSuccessfulServiceStatus(subpoena.serviceStatus), ) .map((subpoena) => ( <Box key={`${defendant.id}${subpoena.id}`} marginBottom={2}> <ServiceAnnouncement defendant={defendant} subpoena={subpoena} /> </Box> )), )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.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/IndictmentOverview/IndictmentOverview.tsx (2)
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-08T12:52:18.973Z
Learning: In `IndictmentOverview.tsx`, since the defendants data does not change, using `useMemo` to memoize the filtering logic is unnecessary.
Learnt from: unakb
PR: island-is/island.is#14712
File: apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/IndictmentOverview.tsx:0-0
Timestamp: 2024-10-08T15:39:04.351Z
Learning: The judicial system application uses a toaster notification system to handle errors globally.
🔇 Additional comments (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (1)
Line range hint 5-41: LGTM! Import statements are well-organized.
The new imports are properly organized and all are utilized in the implementation.
…e-announcement-defenders
…d-is/island.is into j-s/service-announcement-defenders
…e-announcement-defenders
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (2)
48-81: LGTM! Consider enhancing type safety for string formatting.The ServiceAnnouncement component is well-implemented with proper null handling and string formatting.
Consider adding type safety for the formatMessage result:
-const successMessage = formatMessage( - serviceAnnouncementStrings.serviceStatusSuccess, -) +const successMessage = formatMessage( + serviceAnnouncementStrings.serviceStatusSuccess, +) as string
172-186: Consider simplifying the nested map/filter for better readability.While the code is functionally correct, the nested map/filter operations could be simplified for better readability.
Consider this more readable approach:
-workingCase.defendants?.map((defendant) => - (defendant.subpoenas ?? []) - .filter((subpoena) => - isSuccessfulServiceStatus(subpoena.serviceStatus), - ) - .map((subpoena) => ( - <Box key={`${defendant.id}${subpoena.id}`} marginBottom={2}> - <ServiceAnnouncement - defendant={defendant} - subpoena={subpoena} - /> - </Box> - )), +workingCase.defendants?.flatMap((defendant) => + (defendant.subpoenas ?? []) + .filter(subpoena => isSuccessfulServiceStatus(subpoena.serviceStatus)) + .map(subpoena => ( + <Box key={`${defendant.id}${subpoena.id}`} marginBottom={2}> + <ServiceAnnouncement + defendant={defendant} + subpoena={subpoena} + /> + </Box> + )) +)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx(5 hunks)libs/island-ui/core/src/lib/AlertMessage/AlertMessage.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/island-ui/core/src/lib/AlertMessage/AlertMessage.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.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/IndictmentOverview/IndictmentOverview.tsx (3)
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-10T17:05:28.408Z
Learning: In `IndictmentOverview.tsx`, since the defendants data does not change, using `useMemo` to memoize the filtering logic is unnecessary.
Learnt from: unakb
PR: island-is/island.is#14712
File: apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/IndictmentOverview.tsx:0-0
Timestamp: 2024-11-10T17:05:23.796Z
Learning: The judicial system application uses a toaster notification system to handle errors globally.
Learnt from: unakb
PR: island-is/island.is#14712
File: apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/IndictmentOverview.tsx:0-0
Timestamp: 2024-11-10T17:05:23.796Z
Learning: The judicial system application uses a toaster notification system to handle errors globally.
🔇 Additional comments (1)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (1)
Line range hint 5-41: LGTM! Import statements are well-organized.
The new imports are properly organized and all are utilized within the component.
gudjong
added
automerge
…e-announcement-defenders
…d-is/island.is into j-s/service-announcement-defenders
oddsson
added
automerge
* Checkpoint * Refactor AlertMessage * Format date * Cleanup * Cleanup * Merge * Add key * Refactor * Remove console.log * Import React * Add action prop bacj * Add action prop bacj * Add action prop bacj * Fix import --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: Guðjón Guðjónsson <gudjon@kolibri.is>
Service announcement for defenders
Asana
What
When a subpoena has been served to a defendant, the defendant's defender needs to see that, who served the subpoena and when. This PR implements that. Note that we don't differentiate between how the service was done, i.e. if the subpoena was served to the defender or through island.is, we just say "Service complete".
Island.is change
This PR also refactors the
AlertMessagecomponent in Island UI. The refactoring isactionprop. This prop is not being used by anyone and should not be a part of this component according to the design in Figma.messageprop is not passed down.I also added the
alignSelfproperty touseBoxStylesScreenshots / Gifs
Alert Banner
Before
After (No change)
Only title
Before
After
Only message
Before
After (No change)
Checklist:
Summary by CodeRabbit
Release Notes
New Features
ServiceAnnouncementcomponent to display service alerts in the Indictment Overview.alignSelfproperty to the Box component for enhanced layout control.Enhancements
Styling Updates
alignSelfproperty in the Box component.