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

Add an option to manage questionnaire access #9658

Merged
merged 4 commits into from
Jan 2, 2025
Merged

Conversation

bodhish
Copy link
Member

@bodhish bodhish commented Jan 2, 2025

This pull request includes significant refactoring to the routing and API request structure of the application, as well as the addition of a new component for managing questionnaire organizations.

Routing and API Refactoring:

Component Updates:

API Usage Updates:

  • Updated various components to use the new questionnaireApi and organizationApi modules instead of the old routes object:
    • src/components/Questionnaire/QuestionnaireForm.tsx [1] [2]
    • src/components/Questionnaire/QuestionnaireSearch.tsx [1] [2]
    • src/components/Questionnaire/index.tsx
    • src/components/Questionnaire/show.tsx [1] [2] [3]
    • src/pages/Landing/LandingPage.tsx [1] [2]
    • src/pages/Organization/OrganizationIndex.tsx

@ohcnetwork/care-fe-code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a new management interface for questionnaire organizations.
    • Introduced modular routing for questionnaire-related pages.
  • Improvements

    • Streamlined API interactions across organization and questionnaire modules.
    • Enhanced type safety for API responses.
    • Centralized API endpoint definitions.
  • Technical Updates

    • Refactored API route management.
    • Updated import and query mechanisms for organization and questionnaire data.
    • Simplified component data fetching logic.
  • Changes in API Structure

    • Removed direct routes for questionnaires and organizations.
    • Created dedicated API modules for more organized data management.

… components

- Moved questionnaire routes to a new file for better organization.
- Updated AppRouter to include the new QuestionnaireRoutes.
- Refactored organization API calls to use a new centralized organizationApi module, improving maintainability and clarity.
- Removed deprecated organization routes from the API definition.
- Updated various components to utilize the new organizationApi for API calls, ensuring consistency across the application.
…ructure

- Moved questionnaire routes from the api.tsx file to a new questionnaireApi module for better organization and maintainability.
- Updated components (QuestionnaireList, QuestionnaireForm, QuestionnaireSearch, and QuestionnaireShow) to utilize the new questionnaireApi for API calls, ensuring consistency across the application.
- Removed deprecated questionnaire routes from the previous API structure.
…estionnaireShow

- Introduced a new ManageQuestionnaireOrganizationsSheet component for managing organizations associated with a questionnaire.
- Integrated the new component into the QuestionnaireShow component, allowing users to add or remove organizations.
- Updated the questionnaireApi to reflect the correct request body structure for setting organizations.
- Enhanced the user interface for organization selection and management, including search functionality and visual feedback for selected organizations.
@bodhish bodhish requested a review from a team as a code owner January 2, 2025 13:31
Copy link
Contributor

coderabbitai bot commented Jan 2, 2025

Walkthrough

This pull request introduces a refactoring of routing and API interactions for questionnaires and organizations. The changes involve creating modular route configurations, centralizing API calls through new API modules (organizationApi and questionnaireApi), and updating type definitions. The modifications streamline the application's data fetching approach by replacing direct route imports with more structured API modules across multiple components.

Changes

File Change Summary
src/Routers/AppRouter.tsx Replaced direct questionnaire routes with modular QuestionnaireRoutes
src/Routers/routes/questionnaireRoutes.tsx New routing configuration for questionnaire-related routes
src/Utils/request/api.tsx Removed questionnaire and organization route sections
src/components/Questionnaire/* Updated API imports and query functions to use questionnaireApi
src/pages/Organization/* Replaced routes imports with organizationApi for API calls
src/types/organization/organization.ts Added new interfaces and updated response types
src/types/organization/organizationApi.ts New API module for organization-related endpoints
src/types/questionnaire/questionnaireApi.ts New API module for questionnaire-related endpoints

Sequence Diagram

sequenceDiagram
    participant Client
    participant AppRouter
    participant QuestionnaireRoutes
    participant QuestionnaireApi
    participant Components

    Client->>AppRouter: Request route
    AppRouter->>QuestionnaireRoutes: Resolve routes
    QuestionnaireRoutes->>Components: Render appropriate component
    Components->>QuestionnaireApi: Fetch data
    QuestionnaireApi-->>Components: Return data
Loading

Possibly related PRs

Suggested Labels

needs review, tested

Suggested Reviewers

  • rithviknishad
  • Jacobjeevan
  • gigincg

Poem

🐰 Routing rabbits hop with glee,
Refactoring paths so clean and free,
APIs dance in modular delight,
Components sing, the code burns bright,
A symphony of type-safe might! 🚀


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.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

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.

@bodhish bodhish requested a review from rithviknishad January 2, 2025 13:32
Copy link

netlify bot commented Jan 2, 2025

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit b0548c9
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/67769a6f2c5c3e0008241327
😎 Deploy Preview https://deploy-preview-9658--care-ohc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

cloudflare-workers-and-pages bot commented Jan 2, 2025

Deploying care-fe with  Cloudflare Pages  Cloudflare Pages

Latest commit: b0548c9
Status: ✅  Deploy successful!
Preview URL: https://6345645b.care-fe.pages.dev
Branch Preview URL: https://orgnization-cleanup.care-fe.pages.dev

View logs

Copy link

cypress bot commented Jan 2, 2025

CARE    Run #4131

Run Properties:  status check passed Passed #4131  •  git commit b0548c9217: Add an option to manage questionnaire access
Project CARE
Branch Review orgnization-cleanup
Run status status check passed Passed #4131
Run duration 01m 08s
Commit git commit b0548c9217: Add an option to manage questionnaire access
Committer Bodhish Thomas
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 3
View all changes introduced in this branch ↗︎

Comment on lines 66 to 70
mutationFn: (organizations: string[]) =>
mutate(questionnaireApi.setOrganizations, {
pathParams: { id: questionnaireId },
body: { organizations: organizations },
})({ organizations: organizations }),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mutationFn: (organizations: string[]) =>
mutate(questionnaireApi.setOrganizations, {
pathParams: { id: questionnaireId },
body: { organizations: organizations },
})({ organizations: organizations }),
mutationFn: mutate(questionnaireApi.setOrganizations, {
pathParams: { id: questionnaireId }
}),

Comment on lines 103 to 105
const hasChanges =
JSON.stringify(organizations?.results.map((org) => org.id).sort()) !==
JSON.stringify(selectedIds.sort());
Copy link
Member

Choose a reason for hiding this comment

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

we can simply use Set for this instead right? not a fan of JSON stringify :)

Comment on lines 43 to 48
TRes: {} as Record<string, never>,
},
listPatients: {
path: "/api/v1/patient/",
method: HttpMethod.GET,
TRes: Type<PaginatedResponse<Patient>>(),
Copy link
Member

Choose a reason for hiding this comment

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

for TRes can we stick with one of the formats?

TRes: Type<Record<string, never>>
// or
TRes: {} as Record<string, never>

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 (15)
src/pages/Organization/components/OrganizationLayout.tsx (1)

73-73: Recommend validating error states.

The new queryFn: query(organizationApi.get, {...}) usage is correct for React Query. However, it may be beneficial to handle potential error states or status codes (e.g., 403 or 500) explicitly to provide more informative feedback to end-users instead of just showing "Not found". You could consider using React Query's onError callback or a global error boundary for a better user experience.

src/pages/Organization/components/OrganizationSelector.tsx (3)

43-45: Handle loading and error states for React Query

Currently, the query result is immediately destructured to getAllOrganizations without handling a potential loading or error state. Consider employing isLoading or isError flags from React Query to provide a better user experience (e.g., displaying a spinner or an error message).


63-63: Provide consistent typing in both queries

The second query uses a specific type annotation, but the first query does not. Consider adding a matching type annotation for the first query for clarity and consistency.


80-80: Improve user feedback if an organization is not found

The function returns early if !selectedOrg; consider providing user feedback or logging in case no matching organization is found. This can help diagnose potential user input issues or data integrity problems.

src/Routers/routes/questionnaireRoutes.tsx (1)

6-9: Consider lazy loading or code splitting
If the application grows, you might want to load these questionnaire components asynchronously to reduce initial bundle size.

src/types/organization/organizationApi.ts (1)

7-55: Nicely structured organization APIs
Endpoints are clearly separated, and type definitions ensure consistency. Consider verifying pagination or error handling if the list grows large.

src/types/questionnaire/questionnaireApi.ts (1)

64-75: Organization management endpoints
The addition of getOrganizations and setOrganizations improves modularity. Validate that the front-end correctly handles success/error states (e.g., partial failures).

src/pages/Organization/OrganizationIndex.tsx (1)

27-27: Ensure user feedback on errors.
While this change updates the query function to use organizationApi.listMine, ensure the component handles potential errors. React Query offers onError or error boundaries for user-friendly error handling.

 queryKey: ["organization", "mine"],
-queryFn: query(organizationApi.listMine),
+queryFn: query(organizationApi.listMine, {
+  onError: (error) => {
+    // provide user feedback or logs
+  },
+}),
src/components/Questionnaire/QuestionnaireSearch.tsx (1)

41-41: Consider adding fallback error handling.
While using React Query, it can be beneficial to handle query errors gracefully—e.g., showing an error message in the UI.

 queryKey: ["questionnaires", "list", search, subjectType],
-queryFn: query(questionnaireApi.list, {
+queryFn: query(questionnaireApi.list, {
   queryParams: {
     ...
   },
+  onError: (error) => {
+    // display user notification or log the error
+  }
 }),
src/pages/Organization/OrganizationView.tsx (1)

32-32: Promote robust error handling.
As with other API calls, React Query allows for onError or fallback to an error boundary for user-friendly error messaging. Consider incorporating it here.

 queryKey: ["organization", id, "children", page, limit, searchQuery],
-queryFn: query(organizationApi.list, {
+queryFn: query(organizationApi.list, {
   queryParams: {
     parent: id,
     offset: (page - 1) * limit,
     limit,
     name: searchQuery || undefined,
   },
+  onError: (err) => {
+    // gracefully handle or log error
+  }
 }),
src/pages/Organization/components/LinkUserSheet.tsx (1)

73-73: Successful migration to organizationApi.assignUser.
Using organizationApi.assignUser streamlines API calls and improves cohesion. However, consider adding a small loading indicator on the button during mutation, to provide better user feedback about the assignment process.

src/components/Questionnaire/show.tsx (1)

70-70: Efficient usage of useQuery(questionnaireApi.detail).
Switching from routes to questionnaireApi provides clarity on where API calls originate. As a small enhancement, consider integrating an optimistic UI approach or a small skeleton loading for even smoother user experience.

src/pages/Organization/OrganizationUsers.tsx (1)

37-37: Using organizationApi.listUsers for listing users.
Consider implementing pagination or infinite scrolling if user lists can grow large. This can prevent performance bottlenecks by controlling data fetch sizes.

src/pages/Organization/components/EditUserRoleSheet.tsx (1)

64-64: Leverage type safety for the mutation payload.
Consider ensuring the payload (body: { user: string; role: string }) is derived from TypeScript interfaces. This will help catch any misuse during refactors. Otherwise, this mutation is properly handling success and error states.

src/components/Questionnaire/ManageQuestionnaireOrganizationsSheet.tsx (1)

1-44: Introduce code-splitting for lighter builds.
All the UI logic and queries are in the same file. Consider splitting the query hooks or the sheet UI into dedicated modules to reduce complexity and improve maintainability, especially as this feature grows.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1251c5f and 77c8a9b.

📒 Files selected for processing (20)
  • src/Routers/AppRouter.tsx (2 hunks)
  • src/Routers/routes/questionnaireRoutes.tsx (1 hunks)
  • src/Utils/request/api.tsx (1 hunks)
  • src/components/Questionnaire/ManageQuestionnaireOrganizationsSheet.tsx (1 hunks)
  • src/components/Questionnaire/QuestionnaireForm.tsx (2 hunks)
  • src/components/Questionnaire/QuestionnaireSearch.tsx (2 hunks)
  • src/components/Questionnaire/index.tsx (1 hunks)
  • src/components/Questionnaire/show.tsx (3 hunks)
  • src/pages/Landing/LandingPage.tsx (2 hunks)
  • src/pages/Organization/OrganizationIndex.tsx (1 hunks)
  • src/pages/Organization/OrganizationPatients.tsx (2 hunks)
  • src/pages/Organization/OrganizationUsers.tsx (2 hunks)
  • src/pages/Organization/OrganizationView.tsx (2 hunks)
  • src/pages/Organization/components/EditUserRoleSheet.tsx (3 hunks)
  • src/pages/Organization/components/LinkUserSheet.tsx (2 hunks)
  • src/pages/Organization/components/OrganizationLayout.tsx (2 hunks)
  • src/pages/Organization/components/OrganizationSelector.tsx (4 hunks)
  • src/types/organization/organization.ts (0 hunks)
  • src/types/organization/organizationApi.ts (1 hunks)
  • src/types/questionnaire/questionnaireApi.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • src/types/organization/organization.ts
🔇 Additional comments (30)
src/pages/Organization/components/OrganizationLayout.tsx (1)

23-23: Good use of the new organizationApi import.

This aligns well with the architectural changes in this pull request, shifting away from the old routes approach.

src/pages/Organization/components/OrganizationSelector.tsx (1)

15-16: Nicely aligned imports with the new API updates

Removing the unused OrganizationResponse import and directly importing organizationApi helps streamline the code and align it with the new API approach.

src/Utils/request/api.tsx (1)

704-704: Confirm all consumers expect a paginated response now
Previously, it may have returned a non-paginated object. Please ensure that any consuming code, such as data mappings or UI pagination controls, is updated to handle a PaginatedResponse<OrganizationUserRole>.

src/Routers/routes/questionnaireRoutes.tsx (2)

1-2: Imports look correct and concise
Directly importing QuestionnaireList and QuestionnaireShow is well-structured and improves clarity.


11-11: Default export is straightforward
This approach keeps the routing declaration self-contained and easy to integrate.

src/types/questionnaire/questionnaireApi.ts (1)

7-63: Endpoints for CRUD and submission are well-defined
Ensure that partial updates and full updates remain consistent. Verify that the front-end logic correctly distinguishes between calls to update vs. partialUpdate.

src/components/Questionnaire/index.tsx (2)

8-8: Switch to dedicated questionnaireApi import
This decouples questionnaire calls from the central routes file and aligns with the new modular API structure.


14-14: Fetching questionnaires with useQuery(questionnaireApi.list)
Good job integrating the new API. Make sure the returned data shape (paginated or not) is handled properly when rendering the list.

src/pages/Organization/OrganizationIndex.tsx (1)

22-22: Use consistent naming for clarity.
Importing organizationApi is a sound approach. If there are multiple APIs in your codebase, confirm that the naming across the application is consistent (e.g., questionnaireApi, organizationApi) to avoid confusion.

src/Routers/AppRouter.tsx (2)

27-27: Approved import of QuestionnaireRoutes.
The new import is aligned with your modular routing structure.


57-57: Great step toward modular routing.
Spreading QuestionnaireRoutes helps keep code organized, improving maintainability.

src/components/Questionnaire/QuestionnaireSearch.tsx (1)

17-17: Consistent naming.
Same as with organizationApi, using questionnaireApi fosters clarity when working on multiple APIs. This is good practice.

src/pages/Organization/OrganizationView.tsx (1)

16-16: API module usage is clear.
Replacing the old import with organizationApi keeps API calls cohesive with the new structure.

src/pages/Organization/components/LinkUserSheet.tsx (1)

30-30: Consistent usage of the new organizationApi import.
Good job referencing the new organizationApi to clearly separate organization-related endpoints. This helps maintain a more modular code structure.

src/components/Questionnaire/show.tsx (3)

14-14: Appropriate transition to the new questionnaireApi.
Importing questionnaireApi aligns with the overarching API refactoring goals of this PR.


17-17: Introducing ManageQuestionnaireOrganizationsSheet.
Excellent way to modularize organization management for questionnaires. This approach helps keep the code organized and maintainable.


110-110: New component integration: ManageQuestionnaireOrganizationsSheet.
Embedding this sheet at this juncture allows convenient access to organization management. It's placed logically near the main questionnaire actions. No further changes needed.

src/pages/Landing/LandingPage.tsx (2)

19-19: Kudos on importing organizationApi.
This aligns with the unified API approach.


35-35: Refactor to use organizationApi.getPublicOrganizations.
Great step for consistent API usage. If you haven’t yet, ensure the new endpoint is properly tested and checked for edge cases (e.g., no organizations found).

src/pages/Organization/OrganizationUsers.tsx (1)

14-14: Refined import for better clarity.
This change eliminates the broader routes object. Keeping the API usage focused and type-safe within organizationApi fosters maintainability.

src/pages/Organization/components/EditUserRoleSheet.tsx (2)

39-39: Use of central organizationApi import looks clean and consistent.
This new import helps unify the organization-related API calls in a single module, improving code organization. Good job!


85-85: Consistent removal of user roles via organizationApi.
The code mirrors the update mutation logic closely, which keeps the API usage consistent and easy to read. Nice job!

src/pages/Organization/OrganizationPatients.tsx (2)

17-17: API import promotes a cleaner architecture.
Switching from routes to organizationApi is a solid move, aligning with the modular structure across the codebase.


32-32: Query function readability.
Using organizationApi.listPatients directly clarifies the intent of this query. The path parameters and query params are well structured as well.

src/components/Questionnaire/ManageQuestionnaireOrganizationsSheet.tsx (4)

45-63: Parallel queries are well structured.
Fetching the questionnaire’s current organizations and the entire list of organizations independently allows for efficient user interactions. The queries are also disabled until the sheet is opened, which is good for performance.


65-79: Solid mutation workflow with invalidation.
After setting organizations, you invalidate the relevant query to refresh the UI. This is the recommended React Query pattern, ensuring data consistency.


80-106: Efficient selection handling with local state.
The toggle approach for array-based selection is well managed. This logic is concise and maintains clarity on state transitions.


108-229: Responsive UI and user feedback.
Utilizing a Sheet component for the main layout and discrete subcomponents for selections fosters a clear user experience. The toast notifications and loading indicators are also well placed.

src/components/Questionnaire/QuestionnaireForm.tsx (2)

24-24: Consistent use of new API module.
Importing questionnaireApi centralizes the questionnaire logic, maintaining consistency with the rest of the app’s refactor.


74-74: Ensure robust error handling for questionnaireApi.detail.
While you’re passing a path parameter, consider verifying that encountering a missing or invalid ID is properly handled. Currently, the error block displays a general message, but you could refine it by inspecting whether the ID is valid or missing.

- Simplified the mutation function in ManageQuestionnaireOrganizationsSheet for setting organizations by removing redundant parameters.
- Updated the handleSave function to correctly format the payload for the setOrganizations mutation.
- Enhanced the hasChanges logic to improve clarity and accuracy in detecting changes in selected organizations.
- Refactored organizationApi type definitions to use Type<> for response and request bodies, improving type safety and consistency across API calls.
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: 2

🧹 Nitpick comments (6)
src/types/organization/organizationApi.ts (2)

45-49: Consider moving patient-related endpoints to a separate API module.

The listPatients endpoint seems out of place in the organization API module. Consider moving it to a dedicated patient API module for better separation of concerns.


28-33: Enhance type safety for role parameter.

The role parameter in user management endpoints could benefit from stricter typing using an enum or union type instead of string.

-    TBody: Type<{ user: string; role: string }>(),
+    TBody: Type<{ 
+      user: string; 
+      role: 'ADMIN' | 'STAFF' | 'VIEWER' 
+    }>(),

Also applies to: 34-39

src/components/Questionnaire/ManageQuestionnaireOrganizationsSheet.tsx (4)

101-105: Simplify the change detection logic.

The current implementation using nested Set operations is complex and could be simplified for better readability.

Consider this simpler implementation:

-  const hasChanges = !organizations?.results
-    ? false
-    : new Set(organizations.results.map((org) => org.id)).size !==
-        new Set(selectedIds).size ||
-      !organizations.results.every((org) => selectedIds.includes(org.id));
+  const hasChanges = organizations?.results
+    ? JSON.stringify(new Set(organizations.results.map((org) => org.id))) !==
+      JSON.stringify(new Set(selectedIds))
+    : false;

175-193: Enhance accessibility for organization selection items.

The CommandItem components lack proper ARIA labels for screen readers.

Add ARIA labels:

   <CommandItem
     key={org.id}
     value={org.id}
     onSelect={() => handleToggleOrganization(org.id)}
+    aria-label={`${org.name}${org.description ? `, ${org.description}` : ''}`}
+    role="option"
+    aria-selected={selectedIds.includes(org.id)}
   >

148-154: Add loading skeleton for selected organizations.

The loading state is only shown in the organization selector but not in the selected organizations section.

Consider adding a loading skeleton:

-  {!isLoading &&
-    (!selectedOrganizations ||
-      selectedOrganizations.length === 0) && (
-      <p className="text-sm text-muted-foreground">
-        No organizations selected
-      </p>
-    )}
+  {isLoading ? (
+    <div className="flex gap-2">
+      <div className="h-6 w-24 animate-pulse rounded bg-muted" />
+      <div className="h-6 w-32 animate-pulse rounded bg-muted" />
+    </div>
+  ) : !selectedOrganizations?.length ? (
+    <p className="text-sm text-muted-foreground">
+      No organizations selected
+    </p>
+  ) : null}

203-213: Extract cancel handler for better readability.

The cancel button's onClick handler contains logic that could be moved outside the JSX.

Consider this refactor:

+ const handleCancel = () => {
+   if (organizations?.results) {
+     setSelectedIds(organizations.results.map((org) => org.id));
+   }
+   setOpen(false);
+ };

   <Button
     variant="outline"
-    onClick={() => {
-      if (organizations?.results) {
-        setSelectedIds(organizations.results.map((org) => org.id));
-      }
-      setOpen(false);
-    }}
+    onClick={handleCancel}
   >
     Cancel
   </Button>
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 77c8a9b and b0548c9.

📒 Files selected for processing (2)
  • src/components/Questionnaire/ManageQuestionnaireOrganizationsSheet.tsx (1 hunks)
  • src/types/organization/organizationApi.ts (1 hunks)
🔇 Additional comments (5)
src/types/organization/organizationApi.ts (4)

1-5: LGTM! Well-organized imports.

The imports are logically grouped and all are utilized within the file.


11-11: Maintain consistent type formatting.

As mentioned in the previous review, we should stick to one format for type definitions. Currently using Type<T>(), consider using one of these formats consistently:

  • TRes: Type<Record<string, never>>
  • TRes: {} as Record<string, never>

Also applies to: 16-16, 21-21, 26-26, 31-31, 37-37, 43-43, 48-48, 53-53


7-55: LGTM! Well-structured API module.

The API endpoints are well-organized with consistent structure and proper typing. Each endpoint clearly defines its HTTP method, path, and expected response/request types.


19-22: Verify URL parameter handling.

Endpoints containing URL parameters ({id}, {userRoleId}) should be validated to ensure proper parameter substitution at runtime.

Also applies to: 35-39, 41-44

✅ Verification successful

URL parameter validation is properly implemented

Based on the codebase analysis, URL parameter validation is properly handled in the request utility functions. The code in src/Utils/request/utils.ts shows that there's a validation mechanism that:

  1. Detects missing path parameters using regex pattern matching:
const missingParams = path.match(/\{.*\}/g);
  1. Throws an error with detailed information when parameters are missing:
if (missingParams) {
  const msg = `Missing path params: ${missingParams.join(", ")}. Path: ${path}`;
  ...
}

The request utility also handles path parameter substitution through the pathParams option, which is consistently used across the codebase as shown in numerous examples in the documentation and actual usage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for URL parameter validation in API utility functions
rg -A 5 "path.*{.*}" src/Utils/request/

Length of output: 141


Script:

#!/bin/bash
# Search for URL parameter validation in API utility functions with fixed regex
rg -A 5 'path.*\{.*\}' src/Utils/request/

# Search for request utility functions that handle URL parameters
rg -A 10 'function.*request' src/Utils/request/

# Look for files in request utils directory
fd . src/Utils/request/

# Search for URL parameter handling across the codebase
ast-grep --pattern 'path: $_ + $_{$_}'

Length of output: 32154

src/components/Questionnaire/ManageQuestionnaireOrganizationsSheet.tsx (1)

1-34: LGTM! Well-structured imports and props definition.

The imports are organized logically, and the Props interface is well-defined with proper typing.

Comment on lines +53 to +63
const { data: availableOrganizations, isLoading: isLoadingOrganizations } =
useQuery({
queryKey: ["organizations", searchQuery],
queryFn: query(organizationApi.list, {
queryParams: {
org_type: "role",
name: searchQuery || undefined,
},
}),
enabled: open,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add debouncing to search query to prevent excessive API calls.

The search query directly triggers API calls on every character change, which could lead to unnecessary server load.

Consider implementing debouncing:

+ import { useDebouncedValue } from "@/hooks/useDebouncedValue";
+  const [debouncedSearch] = useDebouncedValue(searchQuery, 300);
   const { data: availableOrganizations, isLoading: isLoadingOrganizations } =
     useQuery({
-      queryKey: ["organizations", searchQuery],
+      queryKey: ["organizations", debouncedSearch],
       queryFn: query(organizationApi.list, {
         queryParams: {
           org_type: "role",
-          name: searchQuery || undefined,
+          name: debouncedSearch || undefined,
         },
       }),
       enabled: open,
     });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { data: availableOrganizations, isLoading: isLoadingOrganizations } =
useQuery({
queryKey: ["organizations", searchQuery],
queryFn: query(organizationApi.list, {
queryParams: {
org_type: "role",
name: searchQuery || undefined,
},
}),
enabled: open,
});
const [debouncedSearch] = useDebouncedValue(searchQuery, 300);
const { data: availableOrganizations, isLoading: isLoadingOrganizations } =
useQuery({
queryKey: ["organizations", debouncedSearch],
queryFn: query(organizationApi.list, {
queryParams: {
org_type: "role",
name: debouncedSearch || undefined,
},
}),
enabled: open,
});

Comment on lines +45 to +51
const { data: organizations, isLoading } = useQuery({
queryKey: ["questionnaire", questionnaireId, "organizations"],
queryFn: query(questionnaireApi.getOrganizations, {
pathParams: { id: questionnaireId },
}),
enabled: open,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for the organizations query.

The query doesn't handle error states, which could lead to a poor user experience if the API call fails.

Add error handling:

- const { data: organizations, isLoading } = useQuery({
+ const { data: organizations, isLoading, error } = useQuery({
     queryKey: ["questionnaire", questionnaireId, "organizations"],
     queryFn: query(questionnaireApi.getOrganizations, {
       pathParams: { id: questionnaireId },
     }),
     enabled: open,
   });

+ useEffect(() => {
+   if (error) {
+     toast.error("Failed to load organizations");
+   }
+ }, [error]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { data: organizations, isLoading } = useQuery({
queryKey: ["questionnaire", questionnaireId, "organizations"],
queryFn: query(questionnaireApi.getOrganizations, {
pathParams: { id: questionnaireId },
}),
enabled: open,
});
const { data: organizations, isLoading, error } = useQuery({
queryKey: ["questionnaire", questionnaireId, "organizations"],
queryFn: query(questionnaireApi.getOrganizations, {
pathParams: { id: questionnaireId },
}),
enabled: open,
});
useEffect(() => {
if (error) {
toast.error("Failed to load organizations");
}
}, [error]);

@bodhish bodhish merged commit f754d19 into develop Jan 2, 2025
23 checks passed
@bodhish bodhish deleted the orgnization-cleanup branch January 2, 2025 14:01
Copy link

github-actions bot commented Jan 2, 2025

@bodhish Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌

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.

2 participants