fix(service-portal): owner view iteration #16038
Conversation
WalkthroughThe pull request introduces several changes across multiple components within the signature collection module. Key modifications include the addition of 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
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (10)
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/modals/AddConstituency/index.tsx (1)
9-13: Define aPropsinterface for clarity and reusabilityCurrently, the component's props are defined inline. For better readability and scalability, consider defining a
Propsinterface or type for the component's props.Apply this diff to introduce a
Propsinterface:+ interface Props { + lists: SignatureCollectionList[] + } const AddConstituencyModal = ({ lists, - }: { - lists: SignatureCollectionList[] - }) => { + }: Props) => {This approach enhances code clarity and makes it easier to extend or reuse the component in other parts of the application.
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/modals/CancelCollection/index.tsx (3)
11-11: Define and export a Props type for better type clarityFor improved TypeScript practices and reusability across different Next.js apps, consider defining and exporting a
CancelCollectionPropstype for your component's props. This enhances readability and maintainability.Apply this change:
+ interface CancelCollectionProps { + listId: string + } - const CancelCollection = ({ listId }: { listId: string }) => { + const CancelCollection = ({ listId }: CancelCollectionProps) => {
Line range hint
26-45: Add error handling for the mutation promiseCurrently, if the
cancelCollectionmutation fails, there is no error handling in place. To enhance robustness and provide better user feedback, add a.catchblock to handle potential errors and notify the user accordingly.Update your code as follows:
const onCancelCollection = async () => { await cancelCollection() .then(({ data }) => { if (data?.signatureCollectionCancel?.success) { toast.success(formatMessage(m.cancelCollectionModalToastSuccess)) setModalIsOpen(false) refetchIsOwner() } else { toast.error(formatMessage(m.cancelCollectionModalToastError)) setModalIsOpen(false) } }) + .catch((error) => { + toast.error(formatMessage(m.cancelCollectionModalToastError)) + setModalIsOpen(false) + }) }
31-35: Simplify type assertions for mutation response dataThe current type assertion from
unknownto a specific type in the mutation response is unnecessary with the correct typing of the mutation. By updating the mutation's type parameter, you can directly access the response data without unsafe assertions.Update your code as follows:
- if ( - ( - data as unknown as { - signatureCollectionCancel: SignatureCollectionSuccess - } - ).signatureCollectionCancel.success - ) { + if (data?.signatureCollectionCancel?.success) {libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/index.tsx (1)
45-45: Use a unique and stable key for list itemsUsing
collector.nameas the key may cause issues if collector names are not unique. If available, consider using a unique identifier such ascollector.id.Apply this diff to update the key:
- key={collector.name} + key={collector.id}libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx (2)
Line range hint
18-22: Define component props using a TypeScript interfaceFor better readability and reusability, consider defining a separate interface for the component props instead of inline typing. This aligns with TypeScript best practices.
You can define an interface like:
interface OwnerViewProps { currentCollection: SignatureCollection }And update the component definition:
const OwnerView: React.FC<OwnerViewProps> = ({ currentCollection }) => { // component logic }
49-81: Handle the case whenlistsForOwneris emptyCurrently, if
listsForOwneris empty, nothing is displayed to the user. Consider providing feedback by showing a message indicating that there are no lists available.You can add conditional rendering to display an appropriate message:
{listsForOwner.length === 0 ? ( <Text>{formatMessage(m.noListsAvailable)}</Text> ) : ( listsForOwner.map(/* existing mapping logic */) )}libs/service-portal/signature-collection/src/screens/shared/SigneeView/index.tsx (1)
Line range hint
34-38: Ensure type consistency forcurrentCollection.The use of optional chaining
currentCollection?.isPresidentialindicates thatcurrentCollectionmight benullorundefined, but in the component's props,currentCollectionis defined as a required prop of typeSignatureCollection. To maintain type safety and prevent potential runtime errors, consider updating the prop definition to makecurrentCollectionoptional.Apply this diff to adjust the prop definition:
-const SigneeView = ({ - currentCollection, -}: { - currentCollection: SignatureCollection -}) => { +const SigneeView = ({ + currentCollection, +}: { + currentCollection?: SignatureCollection +}) => {libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/ViewList/SigneesOverview.tsx (2)
Line range hint
33-37: IncludeloadingSigneesinuseEffectdependency arrayThe
loadingSigneesvariable is used within theuseEffecthook but isn't included in the dependency array. Omitting it may lead to stale data or missed updates whenloadingSigneeschanges.Apply this diff to include
loadingSigneesin the dependencies:useEffect(() => { if (!loadingSignees) { setSignees(listSignees) } - }, [listSignees]) + }, [listSignees, loadingSignees])
Line range hint
40-53: AddlistSigneestouseEffectdependency arrayThe
useEffecthook filterslistSigneesbased onsearchTerm, butlistSigneesisn't included in the dependency array. IflistSigneeschanges, the effect won't re-run, potentially causing the displayed signees list to be out of sync.Apply this diff to ensure the effect runs when
listSigneeschanges:- }, [searchTerm]) + }, [searchTerm, listSignees])
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/Signees.tsx (1 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/index.tsx (1 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx (3 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/modals/AddConstituency/index.tsx (2 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/modals/CancelCollection/index.tsx (4 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/index.tsx (1 hunks)
- libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/ViewList/SigneesOverview.tsx (1 hunks)
- libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/ViewList/index.tsx (2 hunks)
- libs/service-portal/signature-collection/src/screens/shared/SignedList/index.tsx (1 hunks)
- libs/service-portal/signature-collection/src/screens/shared/SigneeView/index.tsx (2 hunks)
- libs/service-portal/signature-collection/src/skeletons.tsx (1 hunks)
Files skipped from review due to trivial changes (4)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/Signees.tsx
- libs/service-portal/signature-collection/src/screens/Parliamentary/index.tsx
- libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/ViewList/index.tsx
- libs/service-portal/signature-collection/src/screens/shared/SignedList/index.tsx
Additional context used
Path-based instructions (7)
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."
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/modals/AddConstituency/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/screens/Parliamentary/OwnerView/modals/CancelCollection/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/screens/Parliamentary/OwnerView/ViewList/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/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/screens/shared/SigneeView/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/screens/Presidential/OwnerView/ViewList/SigneesOverview.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 not posted (6)
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/modals/CancelCollection/index.tsx (2)
56-59: Button configuration follows UI guidelinesThe
Buttoncomponent configuration for opening the modal is appropriate, with the correct variant, size, and color scheme. It enhances user experience by clearly indicating a destructive action in a non-intrusive way.
65-85: UI enhancements in the modal are well implementedThe updates to the modal's layout and button styles improve the user interface, providing clear messaging and actions for the user. The use of
formatMessageensures proper localization, and the layout is accessible and responsive.libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx (2)
14-15: Code adheres to TypeScript best practices for defining and using typesThe imports and usage of
SignatureCollectionListandOwnerParliamentarySkeletonenhance type safety and maintainability, aligning with TypeScript best practices.
42-42: Confirm thatAddConstituencycomponent accepts the newlistspropEnsure that the
AddConstituencycomponent has been updated to accept thelistsprop of typeSignatureCollectionList[]. This will ensure type safety and proper functionality.libs/service-portal/signature-collection/src/screens/shared/SigneeView/index.tsx (1)
51-53: Confirmvariant="h4"aligns with design specifications.Adding
variant="h4"to theTextcomponent changes its styling and possibly its semantic role. Ensure this update conforms to the design guidelines and maintains consistency across the application.libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/ViewList/SigneesOverview.tsx (1)
14-14: ImportinguseParamsenhances parameter handlingImporting
useParamsfromreact-router-domimproves how route parameters are accessed, making the code cleaner and more reliable.
...al/signature-collection/src/screens/Parliamentary/OwnerView/modals/AddConstituency/index.tsx
Outdated
Show resolved
Hide resolved
...al/signature-collection/src/screens/Parliamentary/OwnerView/modals/AddConstituency/index.tsx
Outdated
Show resolved
Hide resolved
...l/signature-collection/src/screens/Parliamentary/OwnerView/modals/CancelCollection/index.tsx
Outdated
Show resolved
Hide resolved
...l/signature-collection/src/screens/Parliamentary/OwnerView/modals/CancelCollection/index.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/index.tsx
Show resolved
Hide resolved
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx
Outdated
Show resolved
Hide resolved
...-portal/signature-collection/src/screens/Presidential/OwnerView/ViewList/SigneesOverview.tsx
Show resolved
Hide resolved
...-portal/signature-collection/src/screens/Presidential/OwnerView/ViewList/SigneesOverview.tsx
Show resolved
Hide resolved
albinagu
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16038 +/- ##
=======================================
Coverage 36.73% 36.73%
=======================================
Files 6735 6735
Lines 138232 138232
Branches 39302 39302
=======================================
Hits 50784 50784
Misses 87448 87448 Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 21 Total Test Services: 0 Failed, 21 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (6)
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/AddConstituency/index.tsx (3)
36-38: Remove unnecessaryinitialVisibilityprop from ModalThe
initialVisibilityprop may not be necessary since the modal's visibility is controlled by theisVisibleprop. Consider removinginitialVisibilityto simplify the component.Apply this diff to remove the prop:
<Modal id="addConstituency" isVisible={modalIsOpen} - initialVisibility={false} onCloseModal={() => { setModalIsOpen(false) setSelectedConstituencies([]) }} label={''} >
43-43: Provide a meaningful label for Modal for accessibilityThe
labelprop is set to an empty string. If it is not needed, consider removing it; otherwise, provide a meaningful label to improve accessibility and assistive technologies like screen readers.Apply this diff to update the label:
<Modal id="addConstituency" isVisible={modalIsOpen} onCloseModal={() => { setModalIsOpen(false) setSelectedConstituencies([]) }} - label={''} + label={formatMessage(m.addConstituency)} >
84-84: Consider adding a named export for the componentFor better reusability and consistency, consider exporting the component as a named export in addition to the default export. This allows for more flexible imports in other parts of the application.
Apply this diff to add a named export:
+export { AddConstituencyModal } export default AddConstituencyModallibs/service-portal/signature-collection/src/screens/shared/CancelCollection/index.tsx (2)
Line range hint
34-48: SimplifyonCancelCollectionwith async/await syntax and proper typingThe
onCancelCollectionfunction currently combinesasync/awaitwith a.then()promise chain, which is unnecessary and can be simplified. Additionally, castingdataasunknownmay be avoided by ensuring proper typing from the mutation result.Refactor the function as follows:
-const onCancelCollection = async () => { - await cancelCollection().then(({ data }) => { - if ( - ( - data as unknown as { - signatureCollectionCancel: SignatureCollectionSuccess - } - ).signatureCollectionCancel.success - ) { - toast.success(formatMessage(m.cancelCollectionModalToastSuccess)) - setModalIsOpen(false) - } else { - toast.error(formatMessage(m.cancelCollectionModalToastError)) - setModalIsOpen(false) - } - }) -} +const onCancelCollection = async () => { + try { + const { data } = await cancelCollection() + if (data?.signatureCollectionCancel?.success) { + toast.success(formatMessage(m.cancelCollectionModalToastSuccess)) + } else { + toast.error(formatMessage(m.cancelCollectionModalToastError)) + } + } catch (error) { + toast.error(formatMessage(m.cancelCollectionModalToastError)) + } finally { + setModalIsOpen(false) + } +}This improves readability and leverages proper error handling with
try...catch.
14-14: Export component prop types for reusabilityFor enhanced reusability across different NextJS apps, consider defining and exporting the prop types of the
CancelCollectioncomponent. This promotes type safety and clarity when the component is used elsewhere.Update the code as follows:
+interface CancelCollectionProps { + listId?: string +} + -const CancelCollection = ({ listId }: { listId?: string }) => { +const CancelCollection = ({ listId }: CancelCollectionProps) => {And export the interface if needed:
+export type { CancelCollectionProps }libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/index.tsx (1)
20-20: Consider using absolute import paths for shared componentsImporting
CancelCollectionusing an absolute path can improve maintainability and readability, especially in larger projects with complex directory structures. This practice enhances reusability across different NextJS apps and aligns with effective bundling strategies.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/AddConstituency/index.tsx (1 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/index.tsx (1 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx (3 hunks)
- libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/CancelCollection/index.tsx (0 hunks)
- libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/index.tsx (2 hunks)
- libs/service-portal/signature-collection/src/screens/shared/CancelCollection/index.tsx (4 hunks)
Files not reviewed due to no reviewable changes (1)
- libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/CancelCollection/index.tsx
Files skipped from review as they are similar to previous changes (1)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/index.tsx
Additional context used
Path-based instructions (4)
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/AddConstituency/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/screens/shared/CancelCollection/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/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/screens/Presidential/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."
Additional comments not posted (3)
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx (2)
13-15: Imports are correctly addedThe new imports for
AddConstituency,SignatureCollectionList, andOwnerParliamentarySkeletonare appropriate and properly structured.
53-84: Good job handling list rendering and optional propertiesYou've correctly used
list.idas the key in themapfunction, which is a best practice for list rendering in React. Additionally, the use of optional chaining withlist.area?.minand default values helps prevent potential runtime errors.libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/index.tsx (1)
147-147: Verify authorization when renderingCancelCollectioncomponentBy changing the condition to only check
currentCollection.isActive, theCancelCollectioncomponent will render whenever the collection is active, regardless of the user's profile actor status. Previously, the component was rendered based on bothcurrentCollection.isActiveand!user?.profile.actor. Please ensure that this change doesn't allow unauthorized users to access cancellation functionality.
...ce-portal/signature-collection/src/screens/Parliamentary/OwnerView/AddConstituency/index.tsx
Show resolved
Hide resolved
...ce-portal/signature-collection/src/screens/Parliamentary/OwnerView/AddConstituency/index.tsx
Show resolved
Hide resolved
...ce-portal/signature-collection/src/screens/Parliamentary/OwnerView/AddConstituency/index.tsx
Show resolved
Hide resolved
libs/service-portal/signature-collection/src/screens/shared/CancelCollection/index.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/signature-collection/src/screens/shared/CancelCollection/index.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/signature-collection/src/screens/shared/CancelCollection/index.tsx
Show resolved
Hide resolved
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
libs/service-portal/signature-collection/src/screens/shared/CancelCollection/index.tsx (1)
Line range hint
35-49: Simplify asynchronous handling by usingasync/awaitconsistentlyMixing
awaitwith.then()can make the code less readable and harder to maintain. It's best to use one approach consistently. Also, the casting ofdatatounknownis unnecessary and can be avoided.Apply this diff to refactor the code:
- const onCancelCollection = async () => { - await cancelCollection().then(({ data }) => { - if ( - ( - data as unknown as { - signatureCollectionCancel: SignatureCollectionSuccess - } - ).signatureCollectionCancel.success - ) { - toast.success(formatMessage(m.cancelCollectionModalToastSuccess)) - setModalIsOpen(false) - } else { - toast.error(formatMessage(m.cancelCollectionModalToastError)) - setModalIsOpen(false) - } - }) - } + const onCancelCollection = async () => { + try { + const { data } = await cancelCollection() + if (data?.signatureCollectionCancel.success) { + toast.success(formatMessage(m.cancelCollectionModalToastSuccess)) + } else { + toast.error(formatMessage(m.cancelCollectionModalToastError)) + } + } catch (error) { + toast.error(formatMessage(m.cancelCollectionModalToastError)) + } finally { + setModalIsOpen(false) + } + }This refactor improves readability by consistently using
async/awaitand removes unnecessary type casting.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/index.tsx (1 hunks)
- libs/service-portal/signature-collection/src/screens/shared/CancelCollection/index.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/index.tsx
Additional context used
Path-based instructions (1)
libs/service-portal/signature-collection/src/screens/shared/CancelCollection/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 not posted (1)
libs/service-portal/signature-collection/src/screens/shared/CancelCollection/index.tsx (1)
Line range hint
14-97: Component is reusable and follows best practicesThe
CancelCollectioncomponent is well-structured, with props defined using TypeScript interfaces. This enhances type safety and reusability across different NextJS apps. Imports are specific, promoting effective tree-shaking and bundling practices.
libs/service-portal/signature-collection/src/screens/shared/CancelCollection/index.tsx
Show resolved
Hide resolved
albinagu
added
the
deprecated:automerge
Features:
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation