chore(j-s): Show send to prison admin file #17148
Conversation
|
Warning Rate limit exceeded@oddsson has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces several enhancements to 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
|
…end-to-prison-admin-file
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx (2)
Line range hint
17-89: Consider extracting utility functions to a separate fileThe mapping and rendering utility functions could be moved to a separate file (e.g.,
ServiceAnnouncement.utils.ts) to improve modularity and make them reusable across other components.
Line range hint
91-94: Enhance type safety for messages and AlertMessageConsider these type safety improvements:
- Define a specific type for the messages array
- Use a discriminated union type for the AlertMessage variant
type ServiceMessage = string; type AlertVariant = 'info' | 'warning' | 'success'; interface ServiceAnnouncementProps { subpoena: Subpoena; defendantName?: string | null; } const messages: ServiceMessage[] = subpoena ? mapServiceStatusMessages(subpoena, formatMessage, lawyer) : [];Also applies to: 124-125
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (2)
48-53: Consider renaming theFileSectioninterface to avoid confusion with the component.Having both an interface and a component named
FileSectioncan be confusing and may lead to naming conflicts. Consider renaming the interface to better reflect its purpose, such asFileSectionProps.Apply this diff to rename the interface:
-interface FileSection { +interface FileSectionProps { title: string onOpenFile: (fileId: string) => void files?: CaseFile[] shouldRender?: boolean }And update the component's props accordingly:
-const FileSection: FC<FileSection> = (props: FileSection) => { +const FileSection: FC<FileSectionProps> = (props: FileSectionProps) => {
79-80: Simplify the conditional return inFileSectioncomponent.The early return for the condition
!files?.length || !shouldRendercan be made more concise and readable.Apply this diff to simplify the return statement:
-if (!files?.length || !shouldRender) { +if (!shouldRender || !files?.length) { return null }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.strings.ts(1 hunks)apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx(9 hunks)apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx(1 hunks)libs/judicial-system/types/src/lib/user.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.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."
libs/judicial-system/types/src/lib/user.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."
apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.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/IndictmentCaseFilesList/IndictmentCaseFilesList.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 (6)
apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx (1)
Line range hint 96-146: Well-structured component with proper state management
The component demonstrates good practices:
- Clean separation of concerns using custom hooks
- Proper handling of loading and error states
- Clear conditional rendering logic
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (1)
169-170: Verify that useFilteredCaseFiles and useFilePermissions hooks are memoized correctly.
Ensure that the dependencies for the useMemo hooks within useFilteredCaseFiles and useFilePermissions are accurately specified to prevent unnecessary re-renders.
Run the following script to check for potential issues with dependencies:
✅ Verification successful
Let me search for the specific hooks mentioned in the review comment.
Let me try a different search approach to find these hooks.
Both hooks are properly memoized with appropriate dependencies
Looking at the implementations:
-
useFilteredCaseFilesusesuseMemowith[caseFiles]as dependency, which is correct since it only needs to recompute when the input case files change. -
useFilePermissionsusesuseMemowith[user, workingCase.hasCivilClaims]as dependencies, which is appropriate as the permissions only need to be recalculated when the user or civil claims status changes.
Both hooks have been implemented with proper memoization patterns and their dependency arrays contain the minimal set of values needed for their computations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for missing dependencies in useMemo hooks.
# Search for useMemo hooks and list their dependencies.
rg 'useMemo\(([\s\S]*?)\[\s*(.*?)\s*\]\s*\)' -g '*.tsx' -o --replace '$2' --context 2
Length of output: 6967
Script:
#!/bin/bash
# Search for useFilteredCaseFiles and useFilePermissions implementations
ast-grep --pattern 'const useFilteredCaseFiles = $_' -l typescript
ast-grep --pattern 'const useFilePermissions = $_' -l typescript
# Show the implementations if found
rg -A 10 "const useFilteredCaseFiles = " --type typescript
rg -A 10 "const useFilePermissions = " --type typescript
Length of output: 325
Script:
#!/bin/bash
# Search for the hooks in tsx files
rg -A 10 "const useFilteredCaseFiles = " --type-add 'tsx:*.{tsx,ts}' --type tsx
rg -A 10 "const useFilePermissions = " --type-add 'tsx:*.{tsx,ts}' --type tsx
# Also search for any useMemo usage within these hooks
rg "useFilteredCaseFiles|useFilePermissions" -B 5 -A 15 --type-add 'tsx:*.{tsx,ts}' --type tsx
Length of output: 10787
libs/judicial-system/types/src/lib/user.ts (4)
88-88:
Potential null pointer exception due to removed optional chaining.
Same concern as before; ensure that user.institution is defined before accessing type.
Apply this diff:
- user.institution.type === InstitutionType.DISTRICT_COURT
+ user?.institution?.type === InstitutionType.DISTRICT_COURTLikely invalid or redundant comment.
112-113:
Potential null pointer exception due to removed optional chaining.
Accessing user.institution.type without checking if user.institution is defined may cause an error.
Apply this diff:
- (user.institution.type === InstitutionType.PRISON ||
- user.institution.type === InstitutionType.PRISON_ADMIN)
+ (user?.institution?.type === InstitutionType.PRISON ||
+ user.institution?.type === InstitutionType.PRISON_ADMIN)Likely invalid or redundant comment.
73-74:
Potential null pointer exception due to removed optional chaining.
The removal of optional chaining (user?.institution?.type) may lead to runtime errors if user or user.institution is undefined. Ensure that user and user.institution are always defined before accessing their properties.
Apply this diff to restore optional chaining:
- user.institution.type === InstitutionType.PROSECUTORS_OFFICE &&
- user.institution.id === '8f9e2f6d-6a00-4a5e-b39b-95fd110d762e'
+ user?.institution?.type === InstitutionType.PROSECUTORS_OFFICE &&
+ user.institution?.id === '8f9e2f6d-6a00-4a5e-b39b-95fd110d762e'Likely invalid or redundant comment.
102-102:
Potential null pointer exception due to removed optional chaining.
Ensure safe access to user.institution.type.
Apply this diff:
- user.institution.type === InstitutionType.COURT_OF_APPEALS
+ user?.institution?.type === InstitutionType.COURT_OF_APPEALSLikely invalid or redundant comment.
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx
Outdated
Show resolved
Hide resolved
...udicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.strings.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17148 +/- ##
==========================================
- Coverage 35.74% 35.73% -0.02%
==========================================
Files 6937 6934 -3
Lines 148180 148118 -62
Branches 42255 42244 -11
==========================================
- Hits 52971 52930 -41
+ Misses 95209 95188 -21
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 73 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
|
…end-to-prison-admin-file
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (5)
48-54: Consider renaming theFileSectioninterface toFileSectionProps.To improve clarity and avoid confusion between the
FileSectioncomponent and its props interface, consider renaming theFileSectioninterface toFileSectionProps.
75-75: Simplify the component's type annotations.The explicit type annotation
(props: FileSection)is redundant since the props type is already specified inFC<FileSection>. You can simplify the component definition:-const FileSection: FC<FileSection> = (props: FileSection) => { +const FileSection: FC<FileSection> = (props) => {
240-260: UseFileSectioncomponent for consistency in rendering.In the "Ruling and Court Records" section, files are rendered directly using
<RenderFiles>instead of theFileSectioncomponent. For consistency and to reduce code duplication, consider using theFileSectioncomponent here.
306-317: Consider unifying file section rendering for uploaded case files.The "Uploaded Case Files" section uses
CaseFileTablefor rendering, whereas other sections use theFileSectioncomponent withRenderFiles. For consistency, consider adaptingFileSectionto accommodateCaseFileTableor explain the necessity of using a different component.
92-128: Add unit tests for new hooks to ensure reliability.The newly introduced hooks
useFilteredCaseFilesanduseFilePermissionsare critical for file filtering and permission management. Adding unit tests for these hooks will help ensure they function correctly across various cases and user roles.Also applies to: 130-147
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.strings.ts(1 hunks)apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.strings.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.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/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx
Show resolved
Hide resolved
…end-to-prison-admin-file
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (1)
77-92: Consider memoizing the FileSection component.The component is well-implemented, but since it's used multiple times in the parent component, it could benefit from memoization to prevent unnecessary re-renders.
-const FileSection: FC<FileSection> = (props: FileSection) => { +const FileSection: FC<FileSection> = React.memo((props: FileSection) => { const { title, files, onOpenFile, shouldRender = true } = props // ... rest of the component -} +})
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (3)
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (3)
50-55: LGTM! Well-structured interface for the FileSection component.
The FileSection interface is well-defined with clear, optional properties that support the component's functionality.
94-129: LGTM! Implementation aligns with PR objectives.
The useFilteredCaseFiles hook effectively handles the filtering of prison admin files, addressing the PR's main objective of displaying uploaded files in the file lists.
326-331: Verify prison admin file access implementation.
While the permission check is in place, we should verify that the backend API also enforces these access controls to prevent unauthorized access to prison admin files.
✅ Verification successful
Let me check the backend permission guards more thoroughly.
Let me try a different approach to check the backend routes and guards.
Let me check one final aspect - the route that uses these guards.
Backend properly enforces prison admin file access controls
The backend implementation includes multiple layers of security for prison admin files:
CreateDefendantCaseFileGuardspecifically validates the SENT_TO_PRISON_ADMIN_FILE category- The file controller uses multiple guards including role checks and case access verification
- The guard throws a ForbiddenException for unauthorized access attempts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for backend permission checks for prison admin files
rg -A 5 "SENT_TO_PRISON_ADMIN_FILE" --type ts
Length of output: 3251
Script:
#!/bin/bash
# Search for backend permission checks and routes related to prison admin files
ast-grep --pattern 'class $_ implements $_ {
$$$
async canActivate($_, $_) {
$$$
}
$$$
}'
# Also check for route decorators and guards
rg "@UseGuards.*CreateDefendantCaseFile" -A 5 --type ts
Length of output: 161
Script:
#!/bin/bash
# Search for file-related controllers and routes
rg -A 10 "@Controller.*file" --type ts
# Check for file-related guards
rg -A 10 "export class.*Guard" --type ts | rg -A 10 "file|File"
Length of output: 10392
Script:
#!/bin/bash
# Search for file controller and its routes in judicial system
rg -A 15 "@Controller.*file" apps/judicial-system/backend/src/app/modules/file/
# Check for routes using the CreateDefendantCaseFileGuard
rg -B 5 "CreateDefendantCaseFileGuard" apps/judicial-system/backend/src/app/modules/file/
Length of output: 2286
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx
Outdated
Show resolved
Hide resolved
…end-to-prison-admin-file
…d-is/island.is into j-s/show-send-to-prison-admin-file
Show send to prison admin file
Asana
What
When public prosecutor staff send cases to prison admin, they can upload a file with the request. That has been implemented but we forgot to show the uploaded file in file lists.
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
New Features
FileSectioncomponent for improved case file organization.Bug Fixes
Refactor