fix(signature-collection): Get candidate collectors #16146
Conversation
WalkthroughThe pull request introduces a new query method 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: 2
🧹 Outside diff range and nitpick comments (1)
libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (1)
167-175: LGTM: Newcollectorsmethod is well-structured and follows best practices.The method is correctly typed and uses async/await syntax appropriately. It delegates the work to the client service, which is good for separation of concerns and reusability across different NextJS apps.
Consider adding error handling to improve robustness:
async collectors( user: User, input: SignatureCollectionCandidateIdInput, ): Promise<SignatureCollectionCollector[]> { try { return await this.signatureCollectionClientService.getCollectors( user, input, ); } catch (error) { // Log the error or handle it appropriately console.error('Error fetching collectors:', error); throw error; // or return an empty array, depending on your error handling strategy } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts (2 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (2 hunks)
- libs/clients/signature-collection/src/lib/signature-collection.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.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/api/domains/signature-collection/src/lib/signatureCollection.service.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/clients/signature-collection/src/lib/signature-collection.service.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."
🔇 Additional comments not posted (4)
libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (2)
22-23: LGTM: New imports are correctly typed and follow best practices.The new imports for
SignatureCollectionCandidateIdInputandSignatureCollectionCollectorare well-structured and contribute to effective tree-shaking. They align with TypeScript usage guidelines for defining and exporting types.
22-23: Confirm: Changes comply with coding guidelines forlibs/**/*files.The new code adheres to the following guidelines:
- Reusability: The
SignatureCollectionServiceclass and its newcollectorsmethod are designed for use across different NextJS apps.- TypeScript usage: Props and types are correctly defined and exported.
- Effective tree-shaking: Specific imports and modular method design contribute to efficient bundling.
Also applies to: 167-175
libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts (2)
37-38: LGTM!The added imports correctly include the required models and inputs for the new method.
192-201: LGTM!The new query method
signatureCollectionCollectorsis correctly implemented with appropriate decorators, access controls, and type annotations. It aligns with the existing patterns in the resolver.
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Outdated
Show resolved
Hide resolved
libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16146 +/- ##
==========================================
+ Coverage 36.64% 36.70% +0.05%
==========================================
Files 6769 6768 -1
Lines 139439 139214 -225
Branches 39656 39562 -94
==========================================
- Hits 51100 51096 -4
+ Misses 88339 88118 -221
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 5 Total Test Services: 0 Failed, 5 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
libs/service-portal/signature-collection/src/skeletons.tsx (2)
25-29: Good implementation, consider adding TypeScript typesThe
CollectorSkeletoncomponent is well-implemented and consistent with other skeleton components in the file. It's a reusable component that can be used across different NextJS apps.To improve type safety and adhere to TypeScript best practices:
- Consider defining prop types for the component, even if it currently doesn't accept any props. This will make it easier to add props in the future if needed.
- Export the component type for better integration with other parts of the application.
Here's a suggested improvement:
import React from 'react'; import { SkeletonLoader } from '@island.is/island-ui/core'; export type CollectorSkeletonProps = { // Add any future props here }; export const CollectorSkeleton: React.FC<CollectorSkeletonProps> = () => { return ( <SkeletonLoader height={50} borderRadius="large" repeat={2} space={1} /> ); };This change adds explicit typing and exports the component type, which aligns with TypeScript best practices and improves the overall type safety of the module.
Line range hint
1-29: Overall good implementation, consider enhancing TypeScript usageThe file structure and implementation are well done. The skeleton components are focused and reusable, which aligns with the guidelines for
libs/**/*files. The use of named exports is excellent for effective tree-shaking and bundling.To further improve the file:
- Consider adding TypeScript prop types and exporting types for all components in the file, not just the new
CollectorSkeleton. This will enhance type safety and consistency across the module.- Ensure that all components follow the same pattern of explicit typing and type exports.
Here's an example of how you could enhance the
Skeletoncomponent (apply similar changes to other components):import React from 'react'; import { Box, SkeletonLoader } from '@island.is/island-ui/core'; export type SkeletonProps = { // Add any future props here }; export const Skeleton: React.FC<SkeletonProps> = () => { return ( <Box marginTop={[5, 10]}> <SkeletonLoader height={150} borderRadius="large" repeat={3} space={2} /> </Box> ); };Applying this pattern to all components will improve overall type safety and maintainability of the module.
libs/service-portal/signature-collection/src/hooks/graphql/queries.ts (1)
167-174: LGTM! Consider adding a type for the query response.The new
GetCollectorsquery looks good and aligns with the existing code structure and PR objectives. It provides a way to retrieve collector information, which is relevant to the signature collection process.To improve consistency with other queries and enhance type safety, consider defining and exporting a type for the query response. This would be in line with the TypeScript usage guideline for the
libsdirectory.You could add a type definition like this:
export type GetCollectorsQuery = { signatureCollectionCollectors: Array<{ nationalId: string; name: string; }>; };This type can then be used when consuming the query results, enhancing type safety across the application.
libs/service-portal/signature-collection/src/hooks/index.ts (1)
175-182: LGTM: NewuseGetCollectorshook implemented correctlyThe new
useGetCollectorshook is well-implemented and follows the existing patterns in the file. It correctly uses theuseQueryhook and handles the case where no data is returned.Suggestions for improvement:
- Consider adding error handling to manage potential query failures.
- You might want to include a
refetchfunction in the return object, similar to other hooks in this file, to allow manual refetching of the data if needed.Example:
export const useGetCollectors = () => { const { data: getCollectorsData, loading: loadingCollectors, error, refetch } = useQuery(GetCollectors) const collectors = (getCollectorsData?.signatureCollectionCollectors as SignatureCollectionCollector[]) ?? [] return { collectors, loadingCollectors, error, refetch } }This change would make the hook more consistent with others in the file and provide more flexibility for error handling and data refreshing.
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx (2)
23-27: LGTM! Consider grouping related imports.The new imports for skeleton components and hooks are appropriate for the changes made to the component. To improve code organization, consider grouping related imports together.
You could group the imports like this:
import { CollectorSkeleton, OwnerParliamentarySkeleton, } from '../../../skeletons' import { useGetCollectors, useGetListsForOwner } from '../../../hooks'
42-42: LGTM! Consider adding error handling.The addition of the
useGetCollectorshook and theloadingCollectorsstate is a good improvement for fetching and managing collector data. This aligns well with the PR objective.Consider adding error handling for the
useGetCollectorshook to improve robustness:const { collectors, loadingCollectors, error } = useGetCollectors() // Later in the component if (error) { // Handle the error, e.g., display an error message }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts (2 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (2 hunks)
- libs/clients/signature-collection/src/lib/signature-collection.service.ts (1 hunks)
- libs/service-portal/signature-collection/src/hooks/graphql/queries.ts (1 hunks)
- libs/service-portal/signature-collection/src/hooks/index.ts (3 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx (3 hunks)
- libs/service-portal/signature-collection/src/skeletons.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts
- libs/clients/signature-collection/src/lib/signature-collection.service.ts
🧰 Additional context used
📓 Path-based instructions (5)
libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.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/service-portal/signature-collection/src/hooks/graphql/queries.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/service-portal/signature-collection/src/hooks/index.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/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/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/service-portal/signature-collection/src/skeletons.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."
Biome
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx
[error] 194-194: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 195-195: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments not posted (5)
libs/service-portal/signature-collection/src/hooks/index.ts (2)
11-11: LGTM: New imports added correctlyThe new imports for
GetCollectorsandSignatureCollectionCollectorare correctly placed and necessary for the newuseGetCollectorshook. They follow the existing import pattern in the file.Also applies to: 20-20
Line range hint
1-182: Summary: NewuseGetCollectorshook enhances signature collection functionalityThe changes in this file introduce a new
useGetCollectorshook, which enhances the signature collection functionality. The implementation follows existing patterns and adheres to the provided coding guidelines, including:
- Reusability: The new hook can be used across different NextJS apps.
- TypeScript usage: The hook correctly uses TypeScript for defining types.
- Effective bundling: The hook is exported individually, allowing for effective tree-shaking.
The changes are well-integrated into the existing codebase and should provide valuable functionality for collecting signature collectors. Consider implementing the suggested improvements to error handling and refetch functionality for consistency with other hooks in the file.
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx (1)
Line range hint
1-238: Overall good improvements with minor issues to address.The changes in this file align well with the PR objective of getting candidate collectors. The introduction of the
useGetCollectorshook and the dynamic rendering of collectors improve the component's functionality and maintainability. The use of skeleton components for loading states enhances the user experience.To further improve the code:
- Address the table structure and missing key prop issues in the collectors rendering logic.
- Consider adding error handling for the
useGetCollectorshook.- Group related imports for better code organization.
Once these minor issues are addressed, the changes will be ready for approval.
🧰 Tools
Biome
[error] 194-194: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 195-195: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts (2)
37-38: LGTM: New imports added correctly.The new imports for
SignatureCollectionCollectorandSignatureCollectionCandidateIdInputare correctly added and follow the existing import style in the file.
192-204: LGTM: New query method added correctly.The
signatureCollectionCollectorsmethod is well-structured and consistent with other query methods in the class. It correctly uses decorators, TypeScript types, and follows the coding guidelines for reusability.Please verify that the
OwnerAccess.AllowActoraccess requirement is the intended setting for this query. If it should be more restrictive, consider usingOwnerAccess.RestrictActorinstead.To verify the usage of
OwnerAccess.AllowActorin other similar methods, you can run the following command:✅ Verification successful
Action Required: Adjust the Ripgrep Command to Target TypeScript Files Correctly.
The previous shell script failed because
ripgrepdoes not recognizetypescriptas a valid file type. To search within.tsfiles, modify the command to use the--globoption instead.[/run_scripts]
Action Required: Verify Usage of
OwnerAccess.AllowActorAcross the Entire Codebase.To ensure that
OwnerAccess.AllowActoris used consistently throughout the codebase, please run the following command to search for its usage in all TypeScript files:[/run_scripts]
Verification Successful:
OwnerAccess.AllowActorUsage is Consistent.All instances of
@AccessRequirement(OwnerAccess.AllowActor)are consistently used within thesignatureCollection.resolver.tsfile. The access requirement is appropriately applied to the relevant query methods, ensuring proper access control as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other query methods using OwnerAccess.AllowActor rg --type typescript 'AccessRequirement\(OwnerAccess\.AllowActor\).*@Query' libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.tsLength of output: 190
Script:
#!/bin/bash # Search for other query methods using OwnerAccess.AllowActor in TypeScript files rg 'AccessRequirement\(OwnerAccess\.AllowActor\).*@Query' --glob '*.ts' libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.tsLength of output: 151
Script:
#!/bin/bash # Search for all instances of OwnerAccess.AllowActor in TypeScript files across the repository rg 'AccessRequirement\(OwnerAccess\.AllowActor\)' --glob '**/*.ts'Length of output: 683
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx
Outdated
Show resolved
Hide resolved
* get candidate collectors * add collectors to frontend * coderabbit suggestion --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation