Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(j-s): Merge case with another that is not in RVG #17516

Merged
merged 41 commits into from
Jan 21, 2025
Merged

Conversation

oddsson
Copy link
Member

@oddsson oddsson commented Jan 15, 2025

Merge case with another that is not in RVG

Asana

What

This PR implements a Input box to update the DB field added in #17507. This is a part in making it possible to merge a case with another case that is not in RVG.

Screenshots / Gifs

Screenshot 2025-01-16 at 11 42 27

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

Release Notes

  • New Features

    • Added ability to merge cases in the Indictments Conclusion section.
    • Introduced custom dropdown components for improved select input styling.
  • Improvements

    • Enhanced case number input with validation and error handling.
    • Streamlined select component imports and rendering.
    • Added clear indicator for select inputs.
    • Updated navigation behavior to include specific case IDs in routing.
  • User Interface

    • New input field for merging cases outside the Réttarvörslugátt.
    • Improved dropdown component with custom styling and interactions.

Copy link
Contributor

coderabbitai bot commented Jan 15, 2025

Walkthrough

This pull request introduces modifications to several components in the judicial system web application, focusing on enhancing case management and select component functionality. The changes include refactoring the CourtDocuments component, creating new select components, adding case merging capabilities, and improving the select input interface. The modifications streamline the code, introduce new validation logic for merging cases, and provide more flexible handling of select inputs.

Changes

File Change Summary
apps/judicial-system/web/src/components/CourtDocuments/CourtDocuments.tsx Simplified react-select imports, removed custom component definitions
apps/judicial-system/web/src/components/SelectComponents/SelectComponents.tsx Added new custom dropdown components: ClearIndicator, DropdownIndicator, Placeholder, SingleValue, and Option
apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/Conclusion.strings.ts Added two new messages for case merging: mergeCaseNumberLabel and mergeCaseNumberPlaceholder
apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/Conclusion.tsx Introduced state variables and validation logic for merge case number
apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/SelectConnectedCase.tsx Updated props and selection logic to support merge case number
libs/island-ui/core/src/lib/Select/Select.tsx Added ClearIndicator to select components with clear functionality
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx Updated nextUrl to include specific case ID for navigation
apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx Removed unused MessageDescriptor import
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx Removed Box component from imports and JSX structure
apps/judicial-system/web/src/routes/Court/components/ReceptionAndAssignment/ReceptionAndAssignment.tsx Removed CourtCaseInfo import

Sequence Diagram

sequenceDiagram
    participant User
    participant ConclusionComponent
    participant SelectConnectedCase
    participant ValidationUtility

    User->>ConclusionComponent: Enter merge case number
    ConclusionComponent->>ValidationUtility: Validate merge case number
    ValidationUtility-->>ConclusionComponent: Validation result
    ConclusionComponent->>SelectConnectedCase: Update connected case selection
    SelectConnectedCase->>ConclusionComponent: Confirm case merge
Loading

Possibly related PRs

Suggested labels

automerge


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@datadog-island-is
Copy link

datadog-island-is bot commented Jan 15, 2025

Datadog Report

All test runs e418c60 🔗

10 Total Test Services: 0 Failed, 10 Passed
🔻 Test Sessions change in coverage: 3 decreased, 197 no change

Test Services
This report shows up to 10 services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
air-discount-scheme-backend 0 0 0 63 0 23.99s N/A Link
air-discount-scheme-web 0 0 0 2 0 7.03s N/A Link
api 0 0 0 4 0 2.4s N/A Link
api-catalogue-services 0 0 0 23 0 8.9s N/A Link
api-domains-air-discount-scheme 0 0 0 6 0 16.2s N/A Link
api-domains-assets 0 0 0 3 0 9.49s 1 no change Link
api-domains-auth-admin 0 0 0 18 0 9.96s 1 no change Link
api-domains-communications 0 0 0 5 0 31.79s N/A Link
api-domains-criminal-record 0 0 0 5 0 8.45s N/A Link
api-domains-driving-license 0 0 0 23 0 27.1s N/A Link

🔻 Code Coverage Decreases vs Default Branch (3)

  • message-queue - jest 67.46% (-0.8%) - Details
  • services-university-gateway - jest 45.28% (-0.03%) - Details
  • services-auth-delegation-api - jest 50.45% (-0.03%) - Details

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 7.14286% with 65 lines in your changes missing coverage. Please review.

Project coverage is 35.57%. Comparing base (33c3794) to head (bee03c7).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...b/src/components/CourtDocuments/CourtDocuments.tsx 0.00% 22 Missing ⚠️
...routes/Court/Indictments/Conclusion/Conclusion.tsx 0.00% 20 Missing ⚠️
...urt/Indictments/Conclusion/SelectConnectedCase.tsx 0.00% 16 Missing ⚠️
...c/components/SelectComponents/SelectComponents.tsx 41.66% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17516      +/-   ##
==========================================
- Coverage   35.57%   35.57%   -0.01%     
==========================================
  Files        7027     7028       +1     
  Lines      150434   150444      +10     
  Branches    42943    42946       +3     
==========================================
  Hits        53522    53522              
- Misses      96912    96922      +10     
Flag Coverage Δ
judicial-system-backend 55.84% <ø> (ø)
services-auth-admin-api 52.51% <ø> (ø)
services-auth-delegation-api 58.41% <ø> (-0.10%) ⬇️
services-auth-ids-api 52.54% <ø> (-0.02%) ⬇️
services-auth-public-api 49.40% <ø> (ø)
services-user-notification 46.57% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...m/web/src/components/DefenderInfo/DefenderInfo.tsx 1.85% <ø> (ø)
...components/Table/PastCasesTable/PastCasesTable.tsx 81.48% <ø> (ø)
...Court/Indictments/Conclusion/Conclusion.strings.ts 0.00% <ø> (ø)
.../ReceptionAndAssignment/ReceptionAndAssignment.tsx 0.00% <ø> (ø)
...s/Prosecutor/Indictments/Processing/Processing.tsx 0.00% <ø> (ø)
libs/island-ui/core/src/lib/Select/Select.tsx 59.52% <ø> (ø)
...c/components/SelectComponents/SelectComponents.tsx 41.66% <41.66%> (ø)
...urt/Indictments/Conclusion/SelectConnectedCase.tsx 0.00% <0.00%> (ø)
...routes/Court/Indictments/Conclusion/Conclusion.tsx 0.00% <0.00%> (ø)
...b/src/components/CourtDocuments/CourtDocuments.tsx 2.12% <0.00%> (+0.46%) ⬆️

... and 5 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33c3794...bee03c7. Read the comment docs.

@oddsson oddsson changed the title J s/merge outside fe feat(j-s): Merge case with another that is not in RVG Jan 16, 2025
@oddsson oddsson marked this pull request as ready for review January 17, 2025 14:47
@oddsson oddsson requested review from a team as code owners January 17, 2025 14:47
@oddsson oddsson requested a review from snaerth January 17, 2025 14:47
Copy link
Member

@disaerna disaerna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI file 💯

@oddsson oddsson removed the request for review from snaerth January 17, 2025 14:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🔭 Outside diff range comments (2)
libs/island-ui/core/src/lib/Select/Select.tsx (1)

Line range hint 120-124: Consider adding ARIA labels for clear button.

The ClearIndicator should have appropriate ARIA labels to improve accessibility.

 ClearIndicator,
+// Add to ClearIndicator component:
+aria-label="Clear selection"
+role="button"
apps/judicial-system/web/src/components/CourtDocuments/CourtDocuments.tsx (1)

Line range hint 45-67: Add error handling for document operations.

The document manipulation functions (handleRemoveDocument, handleAddDocument, handleSubmittedBy) should include error handling for failed operations.

 const handleAddDocument = (document: string) => {
+  try {
     const updatedCourtDocuments = [
       ...(workingCase.courtDocuments || []),
       { name: document, submittedBy: undefined },
     ]
 
     setUpdateIndex(undefined)
 
     setAndSendCaseToServer(
       [{ courtDocuments: updatedCourtDocuments, force: true }],
       workingCase,
       setWorkingCase,
-    )
+    ).catch((error) => {
+      // Handle error appropriately
+      console.error('Failed to add document:', error);
+    })
+  } catch (error) {
+    console.error('Error processing document:', error);
+  }
 }
🧹 Nitpick comments (7)
apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/SelectConnectedCase.tsx (2)

40-40: Consider adding JSDoc for the setConnectedCase function.

Adding documentation for the function's behavior when handling null values would improve maintainability.

+/**
+ * Sets the connected case ID in the working case state.
+ * @param connectedCaseId - The ID of the case to connect, or null to clear the connection
+ */
const setConnectedCase = async (connectedCaseId: string | null) => {

Also applies to: 43-43


70-111: Improve readability by extracting rendering conditions.

The nested conditional rendering could be simplified by extracting the conditions into named variables or separate render methods.

+const renderLoadingState = () => (
+  <Box textAlign="center" paddingY={2} paddingX={3} marginBottom={2} key="loading-dots">
+    <LoadingDots />
+  </Box>
+)
+
+const renderWarningMessage = (title: string, message: string) => (
+  <AlertMessage type="warning" title={title} message={message} />
+)
+
+const renderSelect = () => (
+  <Select
+    name="connectedCase"
+    label={formatMessage(strings.connectedCaseLabel)}
+    options={connectedCases}
+    value={defaultConnectedCase}
+    placeholder={formatMessage(strings.connectedCasePlaceholder)}
+    onChange={(selectedOption) => {
+      setConnectedCase((selectedOption?.value as string) || null)
+    }}
+    isDisabled={connectedCasesLoading || Boolean(mergeCaseNumber)}
+    isClearable
+  />
+)

 if (!mergeAllowed) {
-  return (
-    <AlertMessage
-      type="warning"
-      title={formatMessage(strings.cannotBeMergedTitle)}
-      message={formatMessage(strings.cannotBeMergedMessage)}
-    />
-  )
+  return renderWarningMessage(
+    formatMessage(strings.cannotBeMergedTitle),
+    formatMessage(strings.cannotBeMergedMessage)
+  )
 } else if (connectedCasesLoading) {
-  return (
-    <Box textAlign="center" paddingY={2} paddingX={3} marginBottom={2} key="loading-dots">
-      <LoadingDots />
-    </Box>
-  )
+  return renderLoadingState()
 } else {
   return connectedCases.length === 0 ? (
-    <AlertMessage
-      type="warning"
-      title={formatMessage(strings.noConnectedCasesTitle)}
-      message={formatMessage(strings.noConnectedCasesMessage)}
-    />
+    renderWarningMessage(
+      formatMessage(strings.noConnectedCasesTitle),
+      formatMessage(strings.noConnectedCasesMessage)
+    )
   ) : (
-    <Select
-      name="connectedCase"
-      label={formatMessage(strings.connectedCaseLabel)}
-      options={connectedCases}
-      value={defaultConnectedCase}
-      placeholder={formatMessage(strings.connectedCasePlaceholder)}
-      onChange={(selectedOption) => {
-        setConnectedCase((selectedOption?.value as string) || null)
-      }}
-      isDisabled={connectedCasesLoading || Boolean(mergeCaseNumber)}
-      isClearable
-    />
+    renderSelect()
   )
 }
apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/Conclusion.tsx (2)

233-237: Consider adding cleanup in useEffect.

The effect hook should clean up when the component unmounts.

 useEffect(() => {
   if (workingCase.mergeCaseNumber) {
     setMergeCaseNumber(workingCase.mergeCaseNumber)
   }
+  return () => {
+    setMergeCaseNumber(undefined)
+  }
 }, [workingCase.mergeCaseNumber])

239-245: Consider extracting validation logic to a custom hook.

The validation logic could be reusable across components.

+const useCaseNumberValidation = () => {
+  const validateCaseNumber = (value: string) => {
+    const validation = validate([[value, ['S-case-number']]])
+    return {
+      isValid: validation.isValid,
+      errorMessage: validation.isValid ? undefined : validation.errorMessage,
+    }
+  }
+  return validateCaseNumber
+}

-const handleMergeCaseNumberBlur = (value: string) => {
-  const validation = validate([[value, ['S-case-number']]])
-
-  setMergeCaseNumberErrorMessage(
-    validation.isValid ? undefined : validation.errorMessage,
-  )
-}
+const validateCaseNumber = useCaseNumberValidation()
+const handleMergeCaseNumberBlur = (value: string) => {
+  const { errorMessage } = validateCaseNumber(value)
+  setMergeCaseNumberErrorMessage(errorMessage)
+}
apps/judicial-system/web/src/components/SelectComponents/SelectComponents.tsx (1)

1-47: Consider grouping related components in a styled-components pattern.

The components share similar patterns and styling approaches. Consider organizing them using styled-components to reduce repetition and improve maintainability.

Example refactor:

const StyledText = styled(Text)`
  // shared text styles
`

const SelectComponent = {
  DropdownIndicator: (props: DropdownIndicatorProps<ReactSelectOption>) => (
    <components.DropdownIndicator {...props}>
      <Icon type="cheveron" width={12} height={12} color="blue400" />
    </components.DropdownIndicator>
  ),
  // ... other components following the same pattern
}

export const { DropdownIndicator, Placeholder, SingleValue, Option } = SelectComponent
apps/judicial-system/web/src/components/CourtDocuments/CourtDocuments.tsx (2)

Line range hint 144-207: Extract inline styles to a separate styles object or file.

The select component contains complex inline styles that would be better maintained in a separate styles object or file.

Example:

// selectStyles.ts
export const selectStyles = {
  control: (baseStyles) => ({
    ...baseStyles,
    border: 'none',
    // ... other control styles
  }),
  // ... other style objects
}

// In component:
styles={selectStyles}

Line range hint 31-33: Optimize state management with useCallback.

The state management for submittedByMenuIsOpen and updateIndex could be optimized using useCallback to prevent unnecessary rerenders.

+import { useCallback } from 'react';
+
 const CourtDocuments: FC<Props> = ({ workingCase, setWorkingCase }) => {
-  const [submittedByMenuIsOpen, setSubmittedByMenuIsOpen] = useState<number>(-1)
-  const [updateIndex, setUpdateIndex] = useState<number | undefined>(undefined)
+  const [submittedByMenuIsOpen, setSubmittedByMenuIsOpen] = useState<number>(-1);
+  const [updateIndex, setUpdateIndex] = useState<number | undefined>(undefined);
+
+  const handleMenuOpen = useCallback((index: number) => {
+    setSubmittedByMenuIsOpen(index);
+  }, []);
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10f297e and 932c879.

📒 Files selected for processing (6)
  • apps/judicial-system/web/src/components/CourtDocuments/CourtDocuments.tsx (2 hunks)
  • apps/judicial-system/web/src/components/SelectComponents/SelectComponents.tsx (1 hunks)
  • apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/Conclusion.strings.ts (1 hunks)
  • apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/Conclusion.tsx (7 hunks)
  • apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/SelectConnectedCase.tsx (3 hunks)
  • libs/island-ui/core/src/lib/Select/Select.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/Conclusion.strings.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/components/SelectComponents/SelectComponents.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/CourtDocuments/CourtDocuments.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/Court/Indictments/Conclusion/Conclusion.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/Select/Select.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."
apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/SelectConnectedCase.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/Court/Indictments/Conclusion/Conclusion.tsx (1)
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 (6)
apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/SelectConnectedCase.tsx (2)

21-21: LGTM! Well-typed prop definition.

The optional mergeCaseNumber prop is properly typed with TypeScript, allowing for both string and null values.


24-28: LGTM! Clean component parameter destructuring.

The component parameters are well-structured and follow React best practices.

apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/Conclusion.strings.ts (1)

102-113: LGTM! Well-structured message definitions.

The new messages for merge case number functionality are properly defined with clear IDs, default messages, and helpful descriptions.

apps/judicial-system/web/src/routes/Court/Indictments/Conclusion/Conclusion.tsx (2)

110-112: LGTM! Clean state management implementation.

The state variables for merge case number and error message are well-defined using React hooks.


533-560: LGTM! Well-structured form implementation.

The form elements are properly organized with appropriate error handling and user feedback.

libs/island-ui/core/src/lib/Select/Select.tsx (1)

120-120: Verify ClearIndicator behavior across select variants.

The ClearIndicator is added to both select variants. Ensure consistent behavior between CreatableReactSelect and ReactSelect, especially with keyboard navigation and screen readers.

✅ Verification successful

ClearIndicator implementation follows accessibility best practices

The ClearIndicator is properly integrated with react-select's base implementation, which includes built-in keyboard navigation and screen reader support. The component is consistently implemented across both select variants with proper ARIA attributes for error states and announcements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any existing accessibility-related tests
rg -l "accessibility|aria|screen.?reader" --type ts --type tsx

Length of output: 92


Script:

#!/bin/bash
# Find TypeScript files and search for accessibility patterns
fd -e ts -e tsx | xargs rg -l "accessibility|aria|screen.?reader"

# Search for ClearIndicator implementation
ast-grep --pattern 'const ClearIndicator = $_'

# Look for aria attributes in Select component
fd -e ts -e tsx | xargs rg "aria-|role=" -A 2 -B 2

Length of output: 66072


Script:

#!/bin/bash
# Search for ClearIndicator implementation and related accessibility code in Select files
rg -A 5 -B 5 "ClearIndicator|aria-|role=|onKeyDown|tabIndex" "libs/island-ui/core/src/lib/Select/"

# Look for any test files related to Select component
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | grep -i select

Length of output: 12388

@oddsson oddsson enabled auto-merge January 20, 2025 10:51
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
apps/judicial-system/web/src/components/SelectComponents/SelectComponents.tsx (3)

24-32: Consider adding ARIA label for better accessibility.

The component is well-implemented, but adding an aria-label to the icon would improve accessibility for screen readers.

-      <Icon type="filled" icon="chevronDown" color="blue400" />
+      <Icon 
+        type="filled" 
+        icon="chevronDown" 
+        color="blue400" 
+        aria-label="Toggle dropdown menu"
+      />

52-58: Consider adding visual feedback for keyboard navigation.

While the component is well-implemented, consider adding visual feedback for keyboard navigation to improve accessibility.

 export const Option = (props: OptionProps<ReactSelectOption>) => {
   return (
     <components.Option {...props}>
-      <Text variant="small">{props.children}</Text>
+      <Text 
+        variant="small"
+        color={props.isFocused ? "blue400" : undefined}
+      >
+        {props.children}
+      </Text>
     </components.Option>
   )
 }

1-58: Consider creating a custom Select component for reusability.

These well-implemented select components could be composed into a higher-level custom Select component that encapsulates all these customizations. This would make it easier to maintain consistent select behavior across the application.

Example usage pattern:

// CustomSelect.tsx
import { Props } from 'react-select';
import * as SelectComponents from './SelectComponents';

export const CustomSelect = (props: Props<ReactSelectOption>) => (
  <Select
    {...props}
    components={{
      ClearIndicator: SelectComponents.ClearIndicator,
      DropdownIndicator: SelectComponents.DropdownIndicator,
      Placeholder: SelectComponents.Placeholder,
      SingleValue: SelectComponents.SingleValue,
      Option: SelectComponents.Option,
    }}
  />
);
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 932c879 and 43911bf.

📒 Files selected for processing (2)
  • apps/judicial-system/web/src/components/CourtDocuments/CourtDocuments.tsx (3 hunks)
  • apps/judicial-system/web/src/components/SelectComponents/SelectComponents.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/judicial-system/web/src/components/CourtDocuments/CourtDocuments.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/components/SelectComponents/SelectComponents.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (4)
apps/judicial-system/web/src/components/SelectComponents/SelectComponents.tsx (4)

1-13: LGTM! Well-organized imports with proper type safety.

The imports are logically grouped and include all necessary components and types for implementing custom react-select components.


14-22: LGTM! Well-implemented clear indicator component.

The component properly extends react-select's base component while maintaining type safety and following React best practices.


34-42: LGTM! Well-structured placeholder component.

The component maintains consistent styling using the Text component while properly extending react-select's base component.


44-50: LGTM! Clean implementation of single value component.

The component maintains consistent typography using the Text component while properly extending react-select's base functionality.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (1)

Line range hint 267-280: Add error handling for national registry lookup.

The national registry lookup could benefit from better error handling and user feedback.

Consider adding error handling:

  useEffect(() => {
    if (!civilClaimantNationalIdUpdate) {
      return
    }

    const items = personData?.items || []
    const person = items[0]

    setNationalIdNotFound(items.length === 0)
+   
+   if (items.length > 1) {
+     // Handle multiple matches
+     console.warn('Multiple matches found for national ID')
+   }

    const update: UpdateCivilClaimant = {
      civilClaimantId: civilClaimantNationalIdUpdate.civilClaimantId || '',
      nationalId: civilClaimantNationalIdUpdate.nationalId,
      ...(person?.name ? { name: person.name } : {}),
    }

    handleSetAndSendCivilClaimantToServer(update)
  }, [personData])
🧹 Nitpick comments (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (2)

Line range hint 65-77: Consider simplifying the initialize function.

Based on the retrieved learning from PR #15421, the updateCase method has its own error handling, making the additional error handling in the initialize function unnecessary.

Consider simplifying the function:

  const initialize = useCallback(async () => {
-   if (!workingCase.court) {
-     await updateCase(workingCase.id, {
-       courtId: user?.institution?.defaultCourtId,
-     })
-     refreshCase()
-   }
+   if (!workingCase.court && user?.institution?.defaultCourtId) {
+     await updateCase(workingCase.id, { courtId: user.institution.defaultCourtId })
+     refreshCase()
+   }
  }, [
    refreshCase,
    updateCase,
    user?.institution?.defaultCourtId,
    workingCase.court,
    workingCase.id,
  ])

Line range hint 79-89: Consider optimistic updates for better UX.

Based on the learning from PR #17198, consider updating the UI state before making the API call in the handleNavigationTo function for a more responsive user experience.

Consider this approach:

  const handleNavigationTo = useCallback(
    async (destination: string) => {
      if (workingCase.state === CaseState.NEW) {
+       setWorkingCase((prev) => ({ ...prev, state: CaseState.OPEN }))
        await transitionCase(
          workingCase.id,
          CaseTransition.OPEN,
          setWorkingCase,
        )
      }
      router.push(`${destination}/${workingCase.id}`)
    },
    [router, setWorkingCase, transitionCase, workingCase],
  )
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 43911bf and b4ed289.

📒 Files selected for processing (1)
  • apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
📓 Learnings (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (3)
Learnt from: gudjong
PR: island-is/island.is#15421
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx:55-61
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The `updateCase` method in the `apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx` file has its own error handling, and additional error handling in the `initialize` function is not necessary.
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
Learnt from: thorhildurt
PR: island-is/island.is#17198
File: apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx:42-50
Timestamp: 2024-12-11T14:25:44.741Z
Learning: In `IndictmentOverview.tsx`, when updating the punishment type, update the UI state before making the API call to immediately reflect the change.
🔇 Additional comments (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (2)

Line range hint 1-44: Well-structured component with proper TypeScript usage!

The component follows NextJS best practices with proper type definitions and organized imports.


686-686: LGTM! Proper route handling.

The updated nextUrl correctly includes the working case ID, ensuring proper navigation flow.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
apps/judicial-system/web/src/components/CourtDocuments/CourtDocuments.tsx (2)

157-204: Consider extracting select styles to a separate file.

The current implementation has a large amount of inline styles. Moving these to a dedicated styles file would:

  • Improve code maintainability
  • Make the component more focused on its logic
  • Allow style reuse across other select components

Example structure:

// selectStyles.ts
export const getSelectStyles = (theme, isOpen) => ({
  control: (baseStyles) => ({...}),
  menu: (baseStyles) => ({...}),
  // ... other style objects
})

213-218: Improve type safety in the onChange handler.

The current implementation uses type casting which could be made safer.

-onChange={(option) => {
-  handleSubmittedBy(
-    index,
-    (option as ReactSelectOption)?.value as UserRole,
-  )
-}}
+onChange={(option: ReactSelectOption | null) => {
+  if (option?.value) {
+    handleSubmittedBy(index, option.value as UserRole)
+  }
+}}
apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx (2)

Line range hint 38-67: Simplify conditional logic in title and tooltip rendering

The getSectionTitle and renderTooltip functions contain nested conditionals that could be simplified for better maintainability.

Consider refactoring using lookup objects:

const SECTION_TITLES = {
  restriction: {
    prosecution: defenderInfo.restrictionCases.sections.defender.heading,
    default: defenderInfo.restrictionCases.sections.defender.title,
  },
  investigation: {
    prosecution: defenderInfo.investigationCases.sections.defender.heading,
    default: defenderInfo.investigationCases.sections.defender.title,
  },
}

const getSectionTitle = () => {
  const caseType = isRestrictionCase(workingCase.type) ? 'restriction' : 'investigation'
  const userRole = isProsecutionUser(user) ? 'prosecution' : 'default'
  const message = SECTION_TITLES[caseType][userRole]
  
  return formatMessage(message, 
    caseType === 'investigation' && userRole === 'default'
      ? {
          defenderType: workingCase.sessionArrangements === SessionArrangements.ALL_PRESENT_SPOKESPERSON
            ? 'Talsmaður'
            : 'Verjandi',
        }
      : undefined
  )
}

Also applies to: 69-91


Line range hint 126-196: Extract defender access radio group into a separate component

The radio button group for defender access control could be extracted into a reusable component to improve maintainability and reduce duplication.

Consider creating a new component:

interface DefenderAccessRadioGroupProps {
  caseType: CaseType
  currentValue: RequestSharedWithDefender
  onChange: (value: RequestSharedWithDefender) => void
  disabled: boolean
}

const DefenderAccessRadioGroup: FC<DefenderAccessRadioGroupProps> = ({
  caseType,
  currentValue,
  onChange,
  disabled,
}) => {
  const { formatMessage } = useIntl()
  const isRestriction = isRestrictionCase(caseType)
  
  const options = [
    {
      value: RequestSharedWithDefender.READY_FOR_COURT,
      labelKey: isRestriction
        ? defenderInfo.restrictionCases.sections.defenderRequestAccess.labelReadyForCourt
        : defenderInfo.investigationCases.sections.defenderRequestAccess.labelReadyForCourt,
    },
    // ... other options
  ]
  
  return (
    <>
      {options.map(({ value, labelKey }) => (
        <Box key={value} marginTop={2}>
          <RadioButton
            name="defender-access"
            id={`defender-access-${value}`}
            label={formatMessage(labelKey)}
            checked={currentValue === value}
            onChange={() => onChange(value)}
            large
            backgroundColor="white"
            disabled={disabled}
          />
        </Box>
      ))}
    </>
  )
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b4ed289 and af6429f.

📒 Files selected for processing (4)
  • apps/judicial-system/web/src/components/CourtDocuments/CourtDocuments.tsx (3 hunks)
  • apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx (1 hunks)
  • apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx (1 hunks)
  • apps/judicial-system/web/src/routes/Court/components/ReceptionAndAssignment/ReceptionAndAssignment.tsx (0 hunks)
💤 Files with no reviewable changes (1)
  • apps/judicial-system/web/src/routes/Court/components/ReceptionAndAssignment/ReceptionAndAssignment.tsx
✅ Files skipped from review due to trivial changes (1)
  • apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.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/CourtDocuments/CourtDocuments.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (4)
apps/judicial-system/web/src/components/CourtDocuments/CourtDocuments.tsx (1)

19-25: Well-structured component separation!

Good job extracting the select-related components into a separate file. This improves code organization and reusability.

apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx (3)

1-2: Clean import statements approved!

The removal of the unused MessageDescriptor import simplifies the code while maintaining full i18n functionality through useIntl.


Line range hint 1-196: Implementation follows NextJS best practices

The component demonstrates good adherence to NextJS patterns:

  • Strong TypeScript usage for type safety
  • Efficient state management with React hooks
  • Clean component structure and proper separation of concerns

Line range hint 89-96: Verify the usage of force parameter in case updates

The handleAdvocateChange function always sets force: true when updating the case. Please verify if forcing the update is necessary in all scenarios, as this might bypass important validations.

✅ Verification successful

The force parameter usage is appropriate in this context

The force parameter is correctly used to ensure explicit user actions in the UI are properly reflected in the case updates, particularly for critical fields like rulings, custody decisions, and date updates. This is a deliberate design choice backed by test cases and consistent usage patterns across similar components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other occurrences of force parameter to understand its usage pattern
rg "force:\s*true" -A 5 -B 5

Length of output: 65876

@oddsson oddsson added this pull request to the merge queue Jan 21, 2025
Merged via the queue into main with commit 5f81d55 Jan 21, 2025
259 checks passed
@oddsson oddsson deleted the j-s/merge-outside-fe branch January 21, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants