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

Public Facilities Filters #9748

Merged
merged 22 commits into from
Jan 14, 2025
Merged

Conversation

Jacobjeevan
Copy link
Contributor

@Jacobjeevan Jacobjeevan commented Jan 4, 2025

Proposed Changes

@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 (check below)
  • Request for Peer Reviews
  • Completion of QA

Summary by CodeRabbit

Summary by CodeRabbit

Release Notes

  • Localization

    • Updated localization keys for improved user interface clarity.
    • Replaced facility search placeholder with facility type selection prompt.
    • Added new prompts for navigating to previous fields and selecting locations.
  • UI Improvements

    • Enhanced autocomplete component with new alignment and styling options.
    • Updated button and input styling for better visual consistency.
    • Simplified back navigation buttons across multiple pages.
    • Adjusted layout and styling for the FacilitiesPage component.
    • Improved styling for various select and popover components.
  • Facility Management

    • Added new organization filtering capabilities.
    • Improved facility search and filtering functionality.
    • Introduced more flexible organization level selection.
  • Performance

    • Optimized organization data retrieval with new custom hooks.

Copy link
Contributor

coderabbitai bot commented Jan 4, 2025

Walkthrough

This pull request introduces several modifications across multiple components and files, focusing on enhancing the user interface, localization, and organization filtering capabilities. The changes include updating localization keys, modifying component styling, introducing a new OrganizationFilter component, and removing CareIcon from various navigation buttons. The modifications aim to improve the user experience by providing more flexible and intuitive interactions with facility and organization-related features.

Changes

File Change Summary
public/locale/en.json Removed facility_search_placeholder, added select_facility_type, select_previous, and select_location_first keys
src/components/ui/* Updated styling for buttons, inputs, select, and autocomplete components
src/pages/Facility/FacilitiesPage.tsx Added selectedOrgs state, updated query handling and search functionality
src/pages/Organization/components/OrganizationFilter.tsx New component for filtering organizations with dynamic level selection
src/hooks/useOrganizationLevel.ts New custom hook for managing organization level data retrieval
src/pages/Facility/FacilityDetailsPage.tsx Simplified navigation logic with useAppHistory
Multiple Public Appointments pages Removed CareIcon from back navigation buttons

Assessment against linked issues

Objective Addressed Explanation
LSGD filter in facility search Partial implementation of organization filtering, but full LSGD filter not clearly demonstrated

Possibly related PRs

  • New Patient Search and Register UI #9400: The changes in the main PR involve modifications to localization keys related to user interface elements in the patient registration process, which aligns with the new keys added in the retrieved PR that enhance user guidance and validation messages in the patient registration form.
  • Patient Registration form refactor #9854: The main PR's changes to the patient registration form and the addition of new localization keys for validation messages directly relate to the enhancements made in this retrieved PR, which also focuses on improving the patient registration process and user interface.
  • Medication Request redesign #9848: The modifications in the main PR regarding the patient registration form and the introduction of new localization keys for various instructions and validations are relevant to the redesign of the medication request feature in this retrieved PR, which also emphasizes user interface improvements.

Suggested labels

needs review

Suggested reviewers

  • rithviknishad
  • bodhish

Poem

🐰 Hop, hop, through code's green lane,
Filters dancing, icons wane,
Organization's new embrace,
Locales singing with such grace,
CodeRabbit's magic takes the stage! 🌈


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.

Copy link

netlify bot commented Jan 4, 2025

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit 03ee501
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/6785036886d3e90008f57b81
😎 Deploy Preview https://deploy-preview-9748--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.

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Jan 5, 2025
Copy link

github-actions bot commented Jan 5, 2025

👋 Hi, @Jacobjeevan,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions github-actions bot removed the merge conflict pull requests with merge conflict label Jan 6, 2025
@Jacobjeevan Jacobjeevan marked this pull request as ready for review January 6, 2025 04:09
@Jacobjeevan Jacobjeevan requested a review from a team as a code owner January 6, 2025 04:09
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 (7)
src/components/ui/autocomplete.tsx (2)

46-47: Default alignment

Providing a default value of "center" to align is sensible. Consider documenting the different alignment options for future maintainers.


74-77: Prop-driven alignment for PopoverContent

Passing the align prop into <PopoverContent> is consistent and clear. Keep an eye on potential edge cases when combined with sideOffset or other positioning props.

src/pages/Facility/FacilitiesPage.tsx (2)

44-51: Syncing state with query parameters in useEffect

Automatically updating query params whenever selectedOrgs changes is good for maintaining a shareable, bookmarkable URL. Make sure to handle potential race conditions or repeated re-renders if multiple side effects depend on these parameters.


117-118: Layout changes

Increasing the gap to gap-5 can enhance readability and spacing. Ensure it matches overall design specs.

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

24-25: TODO comment: "fetch from the backend"

Remember to address the TODO: the govtOrgLevels should eventually come from an API, ensuring correctness and eliminating hardcoded values.


102-147: RenderOrganizationLevel

Utilizing useQuery at each level allows dynamic loading of child orgs based on the previously selected org. The skip condition is a nice safeguard against invalid levels.

You might consider memoizing or splitting each level into a smaller component for better readability.

src/pages/Appoinments/auth/PatientLogin.tsx (1)

294-294: Icon removal & i18n

Replacing the icon with a translated "back" label simplifies the UI while improving internationalization coverage. Ensure consistency across the app by removing the icon from similar back buttons if desired.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 83a115d and bc0560a.

📒 Files selected for processing (10)
  • public/locale/en.json (2 hunks)
  • src/components/Common/UserSelector.tsx (1 hunks)
  • src/components/ui/autocomplete.tsx (3 hunks)
  • src/pages/Appoinments/PatientRegistration.tsx (0 hunks)
  • src/pages/Appoinments/PatientSelect.tsx (0 hunks)
  • src/pages/Appoinments/Schedule.tsx (0 hunks)
  • src/pages/Appoinments/auth/PatientLogin.tsx (1 hunks)
  • src/pages/Facility/FacilitiesPage.tsx (6 hunks)
  • src/pages/Facility/FacilityDetailsPage.tsx (0 hunks)
  • src/pages/Organization/components/OrganizationFilter.tsx (1 hunks)
💤 Files with no reviewable changes (4)
  • src/pages/Appoinments/PatientSelect.tsx
  • src/pages/Appoinments/Schedule.tsx
  • src/pages/Appoinments/PatientRegistration.tsx
  • src/pages/Facility/FacilityDetailsPage.tsx
🔇 Additional comments (32)
src/components/ui/autocomplete.tsx (2)

34-35: Optional alignment & styling props added

Good addition of customizable alignment (align) and class name (popoverClassName) for finer control of the popover. This increases the component's flexibility without breaking existing usage.


53-53: Forwarding popoverClassName to the trigger

Allowing consumers to specify custom classes in PopoverTrigger is a convenient approach for styling. Ensure it doesn't conflict with the existing default styling or accessibility attributes.

src/components/Common/UserSelector.tsx (1)

83-83: Removal of max-width

By simplifying to a single width rule (w-[var(--radix-popover-trigger-width)]), the popover’s width is now consistent with the trigger. Verify no layout issues arise on smaller or larger screens.

src/pages/Facility/FacilitiesPage.tsx (11)

5-5: Imports for React Hooks

The addition of useEffect and useState supports new stateful logic. Looks appropriate.


17-17: Switch to LocalStorageKeys enum

This improves maintainability by referencing commonly used keys from a centralized enum, helping avoid typos.


25-25: Importing OrganizationFilter

Importing a new organizational filter component is a logical approach to modularizing the facilities page.


36-36: Patient token retrieval

Reading from LocalStorageKeys.patientTokenKey helps unify local storage usage across the app. Double-check that the key name remains consistent with backend/other references.


40-43: Initializing selected organizations

Defining selectedOrgs from qParams.organization ensures the user's state is in sync with query parameters when the page loads.


59-60: Query parameter usage in facility fetch

Including name: qParams.search aligns with the intuitive naming for searching facilities. Conditional usage of facility_type is also neat.


86-86: Localized label

Using t("patient_dashboard") instead of a hardcoded string is consistent with i18n best practices.


100-100: Sign-In button i18n

Same good i18n practice here.


119-139: Integrating OrganizationFilter

Organizing advanced filtering logic in a dedicated component is a strong design choice. The callback onChange elegantly updates both the local state and the URL query.


147-147: Consistent usage of qParams.search

Using search consistently instead of name clarifies search functionality.


151-152: Responsive search bar sizing

Resizing the search bar depending on viewport width is a thoughtful touch for mobile users.

src/pages/Organization/components/OrganizationFilter.tsx (14)

1-4: React Query & i18n imports

Great to see translations and React Query used in a brand-new filter component.


5-14: UI components import

Direct reuse of established UI elements, such as Autocomplete, button, select, etc., maintains consistency in styling.


15-16: Debounced state & filter hooks

Leveraging the debounced state to reduce network calls is a performance-friendly choice.


18-19: Facility type constants

Centralizing facility type constants in a single location fosters reuse.


20-23: Imports for queries & types

Keeping all request logic in separate modules (organizationApi) aligns well with the separation of concerns.


27-33: OrganizationFilterProps definition

Clear, descriptive prop names. The usage of a callback onChange to update external state fosters component reusability.


35-39: Autocomplete option interface

A straightforward structure for label/value pairs. Good for reusability.


40-48: OrganizationFilter functional component

Correctly initializes local states, referencing props for skip levels and selected organizations.


49-60: Multiple queries for org details

Using useQueries to fetch details for each selected organization is clever. Especially helpful when multiple org levels are involved.


64-76: Synchronizing selectedLevels with query results

This effect ensures the selectedLevels accurately reflects the fetched organization data. Consider how partial failures are handled (e.g., one of multiple queries fails).


78-83: getParentId logic

Neat approach to mapping child levels’ parent IDs. Check off-by-one mistakes if future changes rearrange level indices.


84-93: getOrganizationOptions helper

Good separation of concerns. This function transforms raw data into relevant options for the Autocomplete.


94-100: clearSelections function

Resets both facility type and organizations. Perfect for providing a “start fresh” approach.


149-188: Final layout

The combination of Select for facility types, multiple <Autocomplete> for hierarchical org selection, and a clearSelections button ensures a comprehensive filtering UI. Looks well-structured.

public/locale/en.json (4)

1607-1607: Looks good!
No issues found in the new key-value pair.


1610-1610: Consistent addition!
The new key-value pair follows the existing naming and phrasing conventions.


1617-1617: Key-value pair validated.
The addition aligns well with the rest of the localization file.


1628-1628: Verified correctness.
No grammatical or punctuation issues; consistent with the existing pattern.

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

119-141: Consider adding ARIA labels for better accessibility.

The facility type selection is well-implemented, but could benefit from additional accessibility attributes.

 <Select
   value={selectedFacilityType?.text || ""}
+  aria-label={t("select_facility_type")}
   onValueChange={(value) => {
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between bc0560a and c4ca814.

📒 Files selected for processing (3)
  • public/locale/en.json (4 hunks)
  • src/pages/Facility/FacilitiesPage.tsx (7 hunks)
  • src/pages/Organization/components/OrganizationFilter.tsx (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Organization/components/OrganizationFilter.tsx

[error] 74-75: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

src/pages/Facility/FacilitiesPage.tsx

[error] 75-76: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: cypress-run (1)
🔇 Additional comments (9)
src/pages/Facility/FacilitiesPage.tsx (5)

5-5: LGTM! Improved maintainability using centralized constants.

The change to use LocalStorageKeys.patientTokenKey instead of a hard-coded key improves maintainability.

Also applies to: 17-17, 36-36


40-51: LGTM! Well-implemented organization state management.

The state management for selected organizations is well-implemented with proper initialization and query parameter updates.


59-60: LGTM! Improved query parameter handling.

The query parameter handling has been improved with:

  • Using 'search' instead of 'name' for consistency
  • Conditional inclusion of facility_type

87-87: LGTM! Enhanced internationalization support.

Button labels now properly use translations instead of hardcoded strings.

Also applies to: 101-101


119-140: LGTM! Well-integrated OrganizationFilter component.

The OrganizationFilter integration is well-implemented with:

  • Proper state management for selections
  • Correct handling of organization levels
  • Appropriate facility type updates
src/pages/Organization/components/OrganizationFilter.tsx (3)

24-47: LGTM! Well-structured component with proper TypeScript support.

The component is well-designed with:

  • Clear prop interface definitions
  • Proper state management
  • Type safety throughout

49-87: LGTM! Efficient query implementation.

The organization queries are well-implemented with:

  • Batch querying using useQueries
  • Proper loading state handling
  • Efficient data processing
🧰 Tools
🪛 Biome (1.9.4)

[error] 74-75: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


184-267: LGTM! Well-implemented organization level handling.

The OrganizationLevel component is well-implemented with:

  • Efficient debounced search
  • Proper handling of organization hierarchy
  • Clean state management
public/locale/en.json (1)

1348-1348: LGTM! Added necessary translation keys.

The required translation keys have been added to support the facility filters UI:

  • patient_dashboard
  • select_facility_type
  • select_previous
  • sign_in

Also applies to: 1610-1610, 1621-1621, 1667-1667

@nihal467
Copy link
Member

nihal467 commented Jan 6, 2025

@Jacobjeevan #9504 (comment)

the suggested design and current design looks way different , was it intentionally changed

@nihal467 nihal467 added the question Further information is requested label Jan 6, 2025
@Jacobjeevan
Copy link
Contributor Author

@Jacobjeevan #9504 (comment)

the suggested design and current design looks way different , was it intentionally changed

Yea, it looked too congested with the 3 drop downs. But we should adjust the design somewhat. I'll ping Vinu on Slack.

@Jacobjeevan Jacobjeevan added Design and removed question Further information is requested labels Jan 6, 2025
@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Jan 6, 2025
Copy link

github-actions bot commented Jan 6, 2025

👋 Hi, @Jacobjeevan,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@Jacobjeevan Jacobjeevan added needs testing needs review and removed Deploy-Failed Deplyment is not showing preview work-in-progress labels Jan 11, 2025
@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Jan 11, 2025
Copy link

👋 Hi, @Jacobjeevan,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@Jacobjeevan Jacobjeevan removed the merge conflict pull requests with merge conflict label Jan 13, 2025
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 (4)
src/hooks/useOrganizationLevel.ts (1)

29-42: Add retry configuration and stale time to the query.

The query setup is good, but could benefit from additional configuration to improve user experience and performance.

   const { data: availableOrgs } = useQuery<{ results: Organization[] }>({
     queryKey: ["organizations-available", getParentId(index), levelSearch],
     queryFn: query.debounced(organizationApi.getPublicOrganizations, {
       queryParams: {
         ...(index > 0 && { parent: getParentId(index) }),
         ...(index === 0 && { level_cache: 1 }),
         name: levelSearch || undefined,
       },
     }),
     enabled:
       !skip &&
       index <= selectedLevels.length &&
       (index === 0 || selectedLevels[index - 1] !== undefined),
+    retry: 2,
+    staleTime: 5 * 60 * 1000, // 5 minutes
src/pages/Facility/FacilitiesPage.tsx (1)

78-94: Simplify the onChange handler logic.

The current implementation has nested conditionals that can be simplified for better readability.

-          onChange={(filter, level) => {
-            if ("organization" in filter) {
-              if (filter.organization) {
-                setSelectedOrgs((prev) => {
-                  const newOrgId = filter.organization as string;
-                  const newOrgs = prev.slice(0, level);
-                  newOrgs.push(newOrgId);
-                  return newOrgs;
-                });
-              } else {
-                setSelectedOrgs([]);
-              }
-            }
-            if ("facility_type" in filter) {
-              updateQuery({ facility_type: filter.facility_type });
-            }
-          }}
+          onChange={(filter, level) => {
+            if ("organization" in filter) {
+              setSelectedOrgs(prev => 
+                filter.organization 
+                  ? [...prev.slice(0, level), filter.organization as string]
+                  : []
+              );
+            } else if ("facility_type" in filter) {
+              updateQuery({ facility_type: filter.facility_type });
+            }
+          }}
src/pages/Organization/components/OrganizationFilter.tsx (2)

76-79: Simplify nested conditions using optional chaining.

The code has multiple nested conditions that can be simplified using optional chaining.

-        if (
-          validOrg &&
-          validOrg.metadata?.govt_org_type &&
-          validOrg.metadata?.govt_org_children_type
-        ) {
+        if (validOrg?.metadata?.govt_org_type && validOrg?.metadata?.govt_org_children_type) {

Also applies to: 95-98

🧰 Tools
🪛 Biome (1.9.4)

[error] 76-77: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


31-31: Consider making DEFAULT_ORG_LEVELS configurable.

The DEFAULT_ORG_LEVELS constant is hardcoded, which might not be flexible enough for different use cases.

Consider making it a prop with a default value:

-const DEFAULT_ORG_LEVELS = 3;

 interface OrganizationFilterProps {
   selected: string[];
   onChange: (Filter: FilterState, index?: number) => void;
   skipLevels?: number[];
   required?: boolean;
   className?: string;
+  maxLevels?: number;
 }

 export default function OrganizationFilter(props: OrganizationFilterProps) {
-  const { onChange, selected, skipLevels } = props;
+  const { onChange, selected, skipLevels, maxLevels = 3 } = props;
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 06dba7b and e0f4527.

📒 Files selected for processing (7)
  • public/locale/en.json (2 hunks)
  • src/components/ui/autocomplete.tsx (3 hunks)
  • src/hooks/useOrganizationLevel.ts (1 hunks)
  • src/pages/Facility/FacilitiesPage.tsx (5 hunks)
  • src/pages/Facility/FacilityDetailsPage.tsx (4 hunks)
  • src/pages/Facility/components/FacilityCard.tsx (1 hunks)
  • src/pages/Organization/components/OrganizationFilter.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/pages/Facility/components/FacilityCard.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
  • public/locale/en.json
  • src/components/ui/autocomplete.tsx
  • src/pages/Facility/FacilityDetailsPage.tsx
🧰 Additional context used
📓 Learnings (1)
src/pages/Organization/components/OrganizationFilter.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9748
File: src/pages/Organization/components/OrganizationFilter.tsx:0-0
Timestamp: 2025-01-08T07:49:59.494Z
Learning: The project implements global error handling for React Query errors, making local error handling in individual query hooks unnecessary.
🪛 Biome (1.9.4)
src/pages/Organization/components/OrganizationFilter.tsx

[error] 76-77: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 95-96: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/pages/Facility/FacilitiesPage.tsx (1)

52-53: ⚠️ Potential issue

Inconsistent search parameter naming.

The code uses name for the search parameter, but the UI updates it using search. This inconsistency could lead to confusion.

     queryFn: query.debounced(routes.getAllFacilities, {
       queryParams: {
-        name: qParams.name,
+        search: qParams.search,
         ...(qParams.facility_type && { facility_type: qParams.facility_type }),

Likely invalid or redundant comment.

Comment on lines +74 to +80
const handleChange = (value: string) => {
const selectedOrg = availableOrgs?.results.find((org) => org.id === value);
if (selectedOrg) {
onChange({ organization: selectedOrg.id }, index);
setLevelSearch("");
}
};
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 type safety to the handleChange function.

The function assumes the selected organization exists but doesn't handle the case where it might not be found.

-  const handleChange = (value: string) => {
+  const handleChange = (value: string | undefined) => {
+    if (!value) {
+      onChange({ organization: undefined }, index);
+      setLevelSearch("");
+      return;
+    }
     const selectedOrg = availableOrgs?.results.find((org) => org.id === value);
     if (selectedOrg) {
       onChange({ organization: selectedOrg.id }, index);
       setLevelSearch("");
     }
   };
📝 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 handleChange = (value: string) => {
const selectedOrg = availableOrgs?.results.find((org) => org.id === value);
if (selectedOrg) {
onChange({ organization: selectedOrg.id }, index);
setLevelSearch("");
}
};
const handleChange = (value: string | undefined) => {
if (!value) {
onChange({ organization: undefined }, index);
setLevelSearch("");
return;
}
const selectedOrg = availableOrgs?.results.find((org) => org.id === value);
if (selectedOrg) {
onChange({ organization: selectedOrg.id }, index);
setLevelSearch("");
}
};

Comment on lines +147 to +162
<div className="flex flex-col gap-2 lg:gap-0 sm:flex-row lg:rounded-md lg:border lg:border-1 lg:border-secondary-500 overflow-clip sm:w-fit w-[calc(100vw-2rem)]">
{[...Array(Math.min(orgTypes.length + 1, DEFAULT_ORG_LEVELS))].map(
(_, index) => (
<OrganizationLevel
key={`organization-level-${index}`}
index={index}
skip={skipLevels?.includes(index) || false}
selectedLevels={selectedLevels}
orgTypes={orgTypes}
setOrgTypes={setOrgTypes}
onChange={onChange}
getParentId={getParentId}
/>
),
)}
</div>
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

Improve accessibility of the organization level filters.

The organization level filters lack proper ARIA labels for screen readers.

-      <div className="flex flex-col gap-2 lg:gap-0 sm:flex-row lg:rounded-md lg:border lg:border-1 lg:border-secondary-500 overflow-clip sm:w-fit w-[calc(100vw-2rem)]">
+      <div 
+        className="flex flex-col gap-2 lg:gap-0 sm:flex-row lg:rounded-md lg:border lg:border-1 lg:border-secondary-500 overflow-clip sm:w-fit w-[calc(100vw-2rem)]"
+        role="group"
+        aria-label={t("organization_levels")}
+      >
📝 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
<div className="flex flex-col gap-2 lg:gap-0 sm:flex-row lg:rounded-md lg:border lg:border-1 lg:border-secondary-500 overflow-clip sm:w-fit w-[calc(100vw-2rem)]">
{[...Array(Math.min(orgTypes.length + 1, DEFAULT_ORG_LEVELS))].map(
(_, index) => (
<OrganizationLevel
key={`organization-level-${index}`}
index={index}
skip={skipLevels?.includes(index) || false}
selectedLevels={selectedLevels}
orgTypes={orgTypes}
setOrgTypes={setOrgTypes}
onChange={onChange}
getParentId={getParentId}
/>
),
)}
</div>
<div
className="flex flex-col gap-2 lg:gap-0 sm:flex-row lg:rounded-md lg:border lg:border-1 lg:border-secondary-500 overflow-clip sm:w-fit w-[calc(100vw-2rem)]"
role="group"
aria-label={t("organization_levels")}
>
{[...Array(Math.min(orgTypes.length + 1, DEFAULT_ORG_LEVELS))].map(
(_, index) => (
<OrganizationLevel
key={`organization-level-${index}`}
index={index}
skip={skipLevels?.includes(index) || false}
selectedLevels={selectedLevels}
orgTypes={orgTypes}
setOrgTypes={setOrgTypes}
onChange={onChange}
getParentId={getParentId}
/>
),
)}
</div>

@nihal467
Copy link
Member

image

  • The district filter must be enabled for the facility type filter to work. Currently, when the "Clear" button is clicked, the district filter is reset. If the user then interacts with the type filter, it shows no facilities, which feels like a confusing behavior. Consider a better UX approach:
  1. After the user clicks "Clear," since the district filter is reset and the entire page relies on the district filter being enabled, display a message prompting the user to select a district.
  2. Alternatively, the "Clear" button could retain the selected district instead of resetting it.
  3. Or explore another approach to make this interaction more intuitive and user-friendly.

image

  • The filtering details are not currently displayed on the card. For instance, in the attached screenshot, it's difficult to identify the facility type, local body, or any other details related to the applied filters. I believe it would make more sense to display these details directly on the card first and then provide the filtering options. This approach would improve clarity and usability.

@nihal467 nihal467 added question Further information is requested changes required and removed needs testing needs review labels Jan 13, 2025
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 (6)
src/pages/Facility/FacilitiesPage.tsx (3)

33-35: Simplify state initialization.

The state initialization can be simplified using optional chaining.

-const [selectedOrgs, setSelectedOrgs] = useState<string[]>(() => {
-  return qParams.organization ? [qParams.organization] : [];
-});
+const [selectedOrgs, setSelectedOrgs] = useState<string[]>(() => 
+  [qParams.organization].filter(Boolean)
+);

37-44: Consider memoizing the query update callback.

The effect updates query params on every selectedOrgs change. Consider memoizing the callback to prevent unnecessary rerenders.

+const updateOrgQuery = useCallback(
+  (orgs: string[]) => {
+    if (orgs.length > 0) {
+      const lastSelected = orgs[orgs.length - 1];
+      updateQuery({ organization: lastSelected });
+    } else {
+      updateQuery({ organization: undefined });
+    }
+  },
+  [updateQuery]
+);

 useEffect(() => {
-  if (selectedOrgs.length > 0) {
-    const lastSelected = selectedOrgs[selectedOrgs.length - 1];
-    updateQuery({ organization: lastSelected });
-  } else {
-    updateQuery({ organization: undefined });
-  }
+  updateOrgQuery(selectedOrgs);
 }, [selectedOrgs]);

119-123: Enhance the empty state message.

The current message "select_location_first" could be more informative. Consider adding context about why location selection is required.

-              {t("select_location_first")}
+              {t("select_location_first_message", {
+                defaultValue: "Please select a location to view available facilities in that area"
+              })}
src/pages/Organization/components/OrganizationFilter.tsx (3)

23-29: Enhance props interface documentation.

Add JSDoc comments to describe the purpose and usage of each prop.

+/**
+ * Props for the OrganizationFilter component
+ * @property {string[]} selected - Array of selected organization IDs
+ * @property {(filter: FilterState, index?: number) => void} onChange - Callback when selection changes
+ * @property {number[]} [skipLevels] - Array of level indices to skip in the hierarchy
+ * @property {boolean} [required] - Whether organization selection is required
+ * @property {string} [className] - Additional CSS classes
+ */
 interface OrganizationFilterProps {
   selected: string[];
   onChange: (Filter: FilterState, index?: number) => void;
   skipLevels?: number[];
   required?: boolean;
   className?: string;
 }

75-84: Use optional chaining for metadata access.

Simplify the nested condition checks using optional chaining.

-        if (
-          validOrg &&
-          validOrg.metadata?.govt_org_type &&
-          validOrg.metadata?.govt_org_children_type
-        ) {
+        if (validOrg?.metadata?.govt_org_type && validOrg?.metadata?.govt_org_children_type) {
           setOrgTypes([
-            validOrg.metadata?.govt_org_type,
-            validOrg.metadata?.govt_org_children_type,
+            validOrg.metadata.govt_org_type,
+            validOrg.metadata.govt_org_children_type,
           ]);
         }
🧰 Tools
🪛 Biome (1.9.4)

[error] 76-77: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


170-178: Enhance clear button accessibility.

Add an aria-label to the clear button to better describe its action.

       <Button
         onClick={clearSelections}
         variant="ghost"
+        aria-label={t("clear_filters")}
         disabled={
           selectedLevels.length === 0 && selectedFacilityType === undefined
         }
       >
         {t("clear")}
       </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 e0f4527 and 03ee501.

📒 Files selected for processing (3)
  • public/locale/en.json (2 hunks)
  • src/pages/Facility/FacilitiesPage.tsx (5 hunks)
  • src/pages/Organization/components/OrganizationFilter.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • public/locale/en.json
🧰 Additional context used
📓 Learnings (1)
src/pages/Organization/components/OrganizationFilter.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9748
File: src/pages/Organization/components/OrganizationFilter.tsx:0-0
Timestamp: 2025-01-08T07:49:59.494Z
Learning: The project implements global error handling for React Query errors, making local error handling in individual query hooks unnecessary.
🪛 Biome (1.9.4)
src/pages/Organization/components/OrganizationFilter.tsx

[error] 76-77: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 95-96: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/pages/Facility/FacilitiesPage.tsx (1)

52-53: Inconsistent search parameter naming.

The code uses name for the search parameter while the UI uses search. This inconsistency was noted in a previous review.

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

154-169: Improve accessibility of organization filters.

The organization level filters lack proper ARIA labels for screen readers.

@Jacobjeevan Jacobjeevan added needs testing needs review and removed question Further information is requested changes required labels Jan 13, 2025
@Jacobjeevan
Copy link
Contributor Author

Jacobjeevan commented Jan 13, 2025

(1) Disabled the facility type selector when location isn't selected
(2) We can add as enhancement

@nihal467
Copy link
Member

LGTM

@bodhish bodhish merged commit 1219f29 into ohcnetwork:develop Jan 14, 2025
19 checks passed
Copy link

@Jacobjeevan 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LSGD filter in the second page while searching for facility
4 participants