fix(admin): compare lists always visible #16528
Conversation
WalkthroughThe changes involve modifications to the conditional rendering logic within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (1)
Line range hint
226-236: Consider lazy loading CompareLists component.Since
CompareListsis now always rendered whenallowedToProcessis true, consider lazy loading it to optimize initial bundle size and improve performance.-import CompareLists from '../shared-components/compareLists' +const CompareLists = lazy(() => import('../shared-components/compareLists')) const ParliamentaryRoot = ({ allowedToProcess }: { allowedToProcess: boolean }) => { // ... existing code ... return ( // ... existing code ... {allowedToProcess && ( <Box> - <CompareLists collectionId={collection?.id} /> + <Suspense fallback={<SkeletonSingleRow />}> + <CompareLists collectionId={collection?.id} /> + </Suspense> {/* ... rest of the code ... */} </Box> )} ) }libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (3)
Line range hint
41-45: Consider enhancing type definitions and documentation.The component's props could be more explicitly defined using an interface, which would improve maintainability and documentation.
+interface ConstituencyProps { + /** Indicates whether the user has permission to process collections */ + allowedToProcess: boolean; +} -export const Constituency = ({ - allowedToProcess, -}: { - allowedToProcess: boolean -}) => { +export const Constituency = ({ + allowedToProcess, +}: ConstituencyProps) => {
Line range hint
134-141: Consider simplifying the conditional rendering.The nested conditions could be simplified using a single condition or extracted into a helper function for better readability.
- {allowedToProcess && - collectionStatus === CollectionStatus.InInitialReview && ( - <CreateCollection - collectionId={collection?.id} - areaId={areaId} - /> - )} + {shouldShowCreateCollection(allowedToProcess, collectionStatus) && ( + <CreateCollection + collectionId={collection?.id} + areaId={areaId} + /> + )}Consider adding this helper function:
const shouldShowCreateCollection = ( allowedToProcess: boolean, status: CollectionStatus ): boolean => allowedToProcess && status === CollectionStatus.InInitialReview;
Line range hint
41-266: Consider extracting business logic to improve reusability.While the component effectively uses shared UI components, consider these improvements for better reusability across NextJS apps:
- Extract the list manipulation logic (e.g.,
countCandidatesLists) into a separate utility file- Move the mutation hooks to a separate custom hook
- Consider making the collection status checks more configurable through props
This would make the component more maintainable and easier to reuse across different contexts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.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/portals/admin/signature-collection/src/screens-parliamentary/index.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."
🔇 Additional comments (2)
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (1)
Line range hint
226-236: LGTM! Please verify workflow impact.The changes align with the PR objective to make compare lists always visible. The code is well-structured and type-safe.
Let's verify if this change maintains the intended workflow:
✅ Verification successful
Let me try a different approach to gather information about the workflow and component usage.
Let me try one more search to understand the collection status workflow better.
Making CompareLists always visible is safe and aligns with the workflow
Based on the codebase analysis, I can now confidently verify that making
CompareListsalways visible in the parliamentary workflow is safe and correct because:
- The
CollectionStatusenum shows a clear progression of states (InitialActive → InInitialReview → Processing → etc.)- The
CompareListscomponent is a self-contained utility that allows comparing lists and unsigning entries if needed- The component has its own internal state management and error handling
- The change doesn't affect the actual processing workflow, which is still controlled by
ActionCompleteCollectionProcessingcomponent that remains properly gated behind the status checkThe previous conditional rendering in the presidential workflow (
screens-presidential/AllLists/index.tsx) shows thatCompareListswas already being shown in multiple states (InInitialReview and InReview), suggesting that the component is designed to be status-agnostic.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any workflow-related tests or documentation rg -g '!node_modules' -g '*.{ts,tsx,md}' -i "workflow.*compare.*list|compare.*list.*workflow"Length of output: 95
Script:
#!/bin/bash # Search for CompareLists component definition and usage ast-grep --pattern 'const CompareLists = $_' # Search for tests related to CompareLists rg -g '*.test.{ts,tsx}' "CompareLists" # Search for any conditional rendering of CompareLists in the codebase ast-grep --pattern 'CompareLists collectionId={$_}'Length of output: 21297
Script:
#!/bin/bash # Search for CollectionStatus enum definition and its usage in conditional statements ast-grep --pattern 'enum CollectionStatus { $$$ }' # Search for collection status related conditions rg -g '*.{ts,tsx}' "collectionStatus === CollectionStatus" -A 2Length of output: 8799
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (1)
Line range hint
134-141: Verify alignment with PR objectives.The changes to the conditional rendering logic appear to be part of a larger update affecting visibility controls. Let's verify the consistency of these changes across related files.
✅ Verification successful
Let me try different shell scripts to gather more information about the changes.
Based on the shell script results, I can now provide a final response about the review comment's concern:
Conditional rendering changes are aligned with collection status workflow
The changes to show
CreateCollectioncomponent whencollectionStatus === CollectionStatus.InInitialRevieware consistent with the codebase's collection status workflow:
- The condition is used consistently across both parliamentary and presidential collection screens
- The collection status transitions are well-defined in the codebase:
InitialActive->InInitialReview->Processed/Active->InReview/Inactive- The
CreateCollectioncomponent is properly implemented with the required props and status-based visibility controls🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related collection status checks rg -A 3 "CollectionStatus.InInitialReview|CollectionStatus.InitialActive" \ --type typescript \ --glob '!node_modules' # Search for other components using CreateCollection ast-grep --pattern 'CreateCollection collectionId={$_} areaId={$_}'Length of output: 221
Script:
#!/bin/bash # Search for collection status checks with ripgrep rg -A 3 "CollectionStatus\." --type ts # Search for CreateCollection component usage and definition rg -A 3 "CreateCollection" --type ts # Search for files containing collection status related code fd -e ts -e tsx -x grep -l "CollectionStatus" # Look for CreateCollection component definition ast-grep --pattern 'const CreateCollection = $_'Length of output: 38225
* fix(admin): already signed message & create collection available (#16537) * fix(admin): already signed message & create collection available * tweak allowedToProcess --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(admin): compare lists always visible (#16528) --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* fix(admin): already signed message & create collection available (#16537) * fix(admin): already signed message & create collection available * tweak allowedToProcess --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(admin): compare lists always visible (#16528) * fix(admin): Signatures - display address (#16641) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* fix(admin): already signed message & create collection available (#16537) * fix(admin): already signed message & create collection available * tweak allowedToProcess --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(admin): compare lists always visible (#16528) * fix(admin): Signatures - display address (#16641) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(inheritance-report): text adjustments (#16649) * fix(inheritance-report): text adjustments * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <builders@andes.is> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(inheritance-report): done screen and minor cleanup (#16667) * fix(inheritance-report): done screen and minor cleanup * unused * provider --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: andes-it <builders@andes.is>
Checklist:
Summary by CodeRabbit
New Features
CreateCollectioncomponent to show it based on the collection's initial review status.CompareListscomponent, now displayed whenever processing is allowed.Bug Fixes