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

UX Enhancement for Middleware Override Workflow #9547

Conversation

saikiranpatil
Copy link

@saikiranpatil saikiranpatil commented Dec 23, 2024

Proposed Changes

  • Closes UX Enhancement for Middleware Override Workflow. #8335

  • Changes
    image

    • When the asset configuration is pre-filled from the location management or facility page, instead of making the user hover over the exclamation mark, add fine text below the input box, along with the pre-filled middleware placeholder, to provide details about the middleware's source.

    image

    • In the location creation form, under the middleware field, add fine text indicating that if left blank, the facility middleware will be applied to assets.

    image

    • In the location management card, display the facility configuration middleware with a "Pre-filled Facility Middleware" badge if no middleware is set for the location.

    image

    • Under the facility configuration middleware field, add fine text stating that the facility middleware will be applied to all assets when asset and location middleware are Unspecified.

@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

  • New Features

    • Introduced new message properties in various form fields for improved user guidance.
    • Enhanced the LocationManagement component to dynamically display middleware information with badges.
    • Refactored form handling in FacilityConfigure using react-hook-form and Zod for validation.
    • Added new components for improved form structure and accessibility, including FormField, FormItem, and related components.
    • Added a new CancelButton component for improved user interaction.
    • Expanded AuthorizedButton component to include an optional variant property for additional styling.
  • Bug Fixes

    • Streamlined error handling and display logic in several components.
  • Documentation

    • Updated documentation to reflect changes in form field properties and new components.
  • Refactor

    • Refactored multiple components to improve structure and maintainability, including the introduction of context for form fields.
  • Style

    • Improved UI elements, including label rendering and error message displays.

@saikiranpatil saikiranpatil requested a review from a team as a code owner December 23, 2024 15:45
Copy link

netlify bot commented Dec 23, 2024

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit ad2e5ce
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/6771f937fcdba300088b6983
😎 Deploy Preview https://deploy-preview-9547--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
Contributor

coderabbitai bot commented Dec 23, 2024

Walkthrough

This pull request introduces comprehensive updates to form handling and user interface components across multiple files. The changes primarily focus on enhancing the middleware configuration workflow by adding more informative messaging, replacing manual form state management with react-hook-form, and integrating Zod for form validation. The modifications span several components, including FacilityConfigure, LocationManagement, and various form-related utility components, with corresponding updates to package dependencies.

Changes

File Change Summary
package.json Added/Updated dependencies: @hookform/resolvers, react-hook-form, zod
src/components/Assets/AssetType/HL7Monitor.tsx Removed tooltip, added conditional message for middleware hostname
src/components/CameraFeed/ConfigureCamera.tsx Replaced tooltip with conditional message for middleware hostname
src/components/Facility/AddLocationForm.tsx Added message prop to TextFormField for middleware address
src/components/Facility/FacilityConfigure.tsx Refactored to use react-hook-form and Zod for form validation
src/components/Facility/LocationManagement.tsx Added facility middleware fetching and display logic
src/components/Form/FormFields/FormField.tsx, Utils.ts Added new FieldMessageText component and message-related properties
src/components/ui/form.tsx Implemented comprehensive form component structure with context and hooks
src/components/Common/AuthorizedButton.tsx Updated AuthorizedButton to include optional variant prop
src/components/Common/CancelButton.tsx Introduced CancelButton component with optional label prop

Assessment against linked issues

Objective Addressed Explanation
Add fine text for pre-filled middleware
Location creation form middleware guidance
Display facility middleware with badge
Add fine text for facility configuration middleware Partial implementation, may need additional refinement

Possibly related PRs

Suggested reviewers

  • rithviknishad
  • Jacobjeevan
  • shivankacker

Poem

🐰 Middleware's dance, a rabbit's delight,
Forms now flow with validation's might
Zod and hooks, a magical pair
Guiding users with messages fair
Code hops forward, clean and bright! 🚀


📜 Recent review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between ccf886f and f7ed5cb.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • package.json (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json

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.

@saikiranpatil
Copy link
Author

@Jacobjeevan @rithviknishad i would like to clarify. with lot of efforts i would make commits and ask for review several times only to have no response most of the time. i had closed before issues for that reason.

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: 1

🧹 Nitpick comments (7)
src/components/Facility/FacilityConfigure.tsx (2)

Line range hint 67-86: Remove redundant setIsLoading(false) call.
The onSubmit function calls setIsLoading(false) twice. Removing the second occurrence will simplify the logic.

  setIsLoading(true);

  const { res, error } = await request(routes.partialUpdateFacility, {
    pathParams: { id: facilityId },
    body: formData,
  });

- setIsLoading(false);
  if (res?.ok) {
    ...
  } else {
    ...
  }
  setIsLoading(false);

94-94: Use optional chaining for cleaner code.
Replacing the explicit check with optional chaining avoids extra logic depth:

- if (data && data.middleware_address) {
-   form.setValue("middleware_address", data.middleware_address);
- }
+ if (data?.middleware_address) {
+   form.setValue("middleware_address", data.middleware_address);
+ }
🧰 Tools
🪛 Biome (1.9.4)

[error] 94-94: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

src/components/Assets/AssetType/HL7Monitor.tsx (1)

105-109: Inline message improves clarity.
Removing the tooltip in favor of an inline message helps users see the middleware source at a glance. Consider localizing “Middleware hostname sourced from asset” for i18n compliance.

src/components/Facility/LocationManagement.tsx (4)

23-23: Custom query import
Combining custom request-based query utilities and react-query is valid. Ensure that caching and invalidation strategies are documented.


34-34: Optional facilityMiddleware property
Marking the prop as optional is sensible. Consider documenting it in any related interface or doc comment for clarity.


271-273: Displaying location_type as a Badge
Good enhancement for visual clarity. For accessibility, consider aria-label or additional descriptive text if the badge’s meaning isn’t self-evident.


282-289: Facility fallback middleware display
This logic accurately reflects the new workflow: if custom middleware is absent, display facility middleware and a “Fetched from facility” badge. Consider adding i18n coverage for this string or other localizable text elements.

Also applies to: 294-294

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 79e098e and c3d564c.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (9)
  • package.json (3 hunks)
  • src/components/Assets/AssetType/HL7Monitor.tsx (1 hunks)
  • src/components/CameraFeed/ConfigureCamera.tsx (1 hunks)
  • src/components/Facility/AddLocationForm.tsx (1 hunks)
  • src/components/Facility/FacilityConfigure.tsx (2 hunks)
  • src/components/Facility/LocationManagement.tsx (8 hunks)
  • src/components/Form/FormFields/FormField.tsx (5 hunks)
  • src/components/Form/FormFields/Utils.ts (1 hunks)
  • src/components/ui/form.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/components/Facility/AddLocationForm.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Facility/FacilityConfigure.tsx

[error] 94-94: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (14)
src/components/Facility/FacilityConfigure.tsx (2)

113-137: Well-structured form integration.
Using react-hook-form with the new Form components significantly improves readability and maintainability. Nice job.


35-42: Confirm domain-only pattern.
The regex strictly enforces a domain name and disallows IP addresses or “http://” prefixes. If your use case might involve hosting on IP addresses or using an HTTPS prefix, consider relaxing the pattern or clarifying the requirement.

✅ Verification successful

Let me search for related code and configuration to understand the middleware address requirements better.

This will help us understand:

  1. How middleware_address is used across the codebase
  2. If there are any configuration files defining the expected format
  3. Whether IP addresses or HTTPS prefixes are used in any environment

Let me search for any usage of the middleware address to understand its requirements better.

This will help us understand:

  1. How the middleware address is used in the application
  2. Whether there are any configuration files defining middleware requirements
  3. If IP addresses or HTTPS prefixes are used in practice

Based on the code search results, I can now generate a final response as I have enough context about how the middleware address is used in the application.

Regex pattern is appropriate for the use case.
The current regex pattern enforcing domain-only format is correct as evidenced by:

  1. The middleware address is used as a hostname in the application (seen in HL7Monitor.tsx with middleware_hostname)
  2. The pattern is consistently used across facility and location configurations
  3. The codebase shows no instances where IP addresses or HTTP/HTTPS prefixes are used for middleware addresses

The restriction to domain names (without protocol prefixes) appears to be an intentional design choice for standardization and security. Keep the current validation pattern.

🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# Search for other instances of middleware_address validation or usage
rg -g '*.{ts,tsx,js,jsx}' "middleware_address" -B 2 -A 2

# Search for any configuration files that might define middleware requirements
fd -e yaml -e json -e toml -e env . -x cat {}

Length of output: 105011


Script:

#!/bin/bash
# Search for middleware address usage in code
rg -g '*.{ts,tsx,js,jsx,py}' -B 3 -A 3 "middleware.*address"

# Search for any configuration files that might define middleware settings
fd -e yaml -e json -e env -e conf . -x grep -l "middleware"

Length of output: 10935

src/components/ui/form.tsx (2)

43-64: Context usage is consistent and safe.
Using React Context for form fields ensures good organization of field state. No issues spotted here.


88-104: Clear labeling with ARIA semantics.
The error-based styling and ARIA attributes provide good accessibility for screen readers. Great approach.

src/components/Form/FormFields/Utils.ts (1)

23-27: Enhanced messaging flexibility.
Adding optional message fields expands the form field’s capabilities without disrupting existing functionality.

src/components/Form/FormFields/FormField.tsx (2)

53-70: Appropriate message display approach.
Conditionally toggling the message’s visibility based on the “message” prop is clean and intuitive.


97-100: Harmonized error and message display.
Rendering both FieldErrorText and FieldMessageText standardizes the user feedback flow. Nicely done.

src/components/Facility/LocationManagement.tsx (5)

1-1: Usage of useQuery import [@tanstack/react-query]
It looks good to see that you’re leveraging react-query. Ensure consistent usage across the codebase and adequate error handling for failed queries.


8-9: Introducing Badge component
No issues found with the Badge import. This enhances UI clarity.


138-138: Passing facilityMiddleware to the Location component
This propagation ensures the location-level logic can dynamically display the fallback facility middleware if needed.


239-239: Destructuring facilityMiddleware
Destructuring is consistent with the updated LocationProps interface. Ensure that other references to LocationProps reflect this change.


58-58: Assigning facilityMiddleware
Confirm that the backend indeed provides “middleware_address” in the response. If missing or null, an explanatory error/log might help debugging.

✅ Verification successful

Let me verify the backend response structure by searching for relevant API calls and type definitions.


Based on the search results, I can now generate the final response:

Backend does provide middleware_address field
The codebase confirms that middleware_address is a valid optional field in both facility and location models. The field is properly typed in src/components/Facility/models.tsx and is consistently used across multiple components including form handling and display logic. No issues found with this assignment.

🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# Search for API response type definitions or interfaces that might contain middleware_address
ast-grep --pattern 'interface $_ {
  $$$
  middleware_address$_
  $$$
}'

# Search for any GraphQL queries or API calls that might fetch middleware_address
rg "middleware_address" -A 3 -B 3

Length of output: 9670

src/components/CameraFeed/ConfigureCamera.tsx (1)

157-164: Replacing tooltip with direct message for middleware_hostname
Presenting the source of the resolved middleware directly is clearer to the user. The translation usage (“middleware_hostname_sourced_from”) also supports i18n well. This streamlined approach eliminates reliance on tooltips, improving discoverability.

package.json (1)

59-59: Dependency updates and new additions
• @hookform/resolvers and react-hook-form: Ensure type definitions and peer dependencies are correctly aligned.
• zod: Upgrading from ^3.23.8 to ^3.24.1 is minor, but confirm that schema validations remain consistent.
• @radix-ui/react-label and @radix-ui/react-slot: Incremental updates look safe. Test any layout or UI adjustments caused by these.

Also applies to: 65-65, 68-68, 99-99, 157-157

✅ Verification successful

Let me gather information about the dependencies and their versions to verify the concerns.


Based on the results from the shell scripts, I can now generate the final response:

Dependencies are properly aligned and used correctly

  • @hookform/resolvers (^3.9.1) is correctly used with react-hook-form (^7.54.1) and zod (^3.24.1) in FacilityConfigure.tsx
  • Zod schemas are properly implemented with zodResolver for form validation
  • @radix-ui/react-label (^2.1.1) and @radix-ui/react-slot (^1.1.1) are correctly used in UI components (form.tsx, button.tsx)
  • All peer dependencies are satisfied and versions are compatible
🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# Check package.json for current and previous versions of the mentioned dependencies
cat package.json | grep -A 1 -B 1 "@hookform/resolvers\|react-hook-form\|zod\|@radix-ui/react-label\|@radix-ui/react-slot"

# Check for any peer dependency warnings in package.json
cat package.json | grep -A 5 "peerDependencies"

# Look for schema validations using zod in the codebase
rg "zod" -A 2 -B 2

# Search for usage of these dependencies
rg "@hookform/resolvers" -A 2 -B 2
rg "react-hook-form" -A 2 -B 2
rg "@radix-ui/react-label" -A 2 -B 2
rg "@radix-ui/react-slot" -A 2 -B 2

Length of output: 19486

@Jacobjeevan
Copy link
Contributor

@Jacobjeevan @rithviknishad i would like to clarify. with lot of efforts i would make commits and ask for review several times only to have no response most of the time. i had closed before issues for that reason.

We miss a few things with our own work as well as reviewing all the other PRs, but feel free to ping us again on Github or on Slack if we haven't gotten back to you.

(For future, try to maintain within one PR - the other PR was still in a workable state).

@saikiranpatil
Copy link
Author

We miss a few things with our own work as well as reviewing all the other PRs, but feel free to ping us again on Github or on Slack if we haven't gotten back to you.

(For future, try to maintain within one PR - the other PR was still in a workable state).

i do not have access to that PR. will take care in future.
please review the changes required in this PR

Copy link
Contributor

@Jacobjeevan Jacobjeevan left a comment

Choose a reason for hiding this comment

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

Other the minor changes, lgtm.

src/components/Facility/FacilityConfigure.tsx Outdated Show resolved Hide resolved
src/components/Facility/FacilityConfigure.tsx Outdated Show resolved Hide resolved
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: 1

🧹 Nitpick comments (3)
src/components/Facility/FacilityConfigure.tsx (3)

49-66: Add error handling for the mutation

While the success case is handled well, consider adding error handling for the mutation to provide feedback when updates fail.

 const { isPending: mutateLoading, mutate: updateFacility } = useMutation({
   mutationFn: mutate(routes.partialUpdateFacility, {
     pathParams: { id: facilityId },
   }),
   onSuccess: () => {
     Notification.Success({
       msg: t("update_facility_middleware_success"),
     });
     navigate(`/facility/${facilityId}`);
   },
+  onError: (error) => {
+    Notification.Error({
+      msg: t("update_facility_middleware_error"),
+      detail: error.message
+    });
+  },
 });

68-80: Use optional chaining for data access

The null check can be simplified using optional chaining, making the code more concise and maintainable.

-const onSubmit = (values: z.infer<typeof formSchema>) => {
-  if (!data) return;
-
-  const formData = {
-    name: data.name,
-    state: data.state,
-    district: data.district,
-    local_body: data.local_body,
-    ward: data.ward,
-    middleware_address: values.middleware_address,
-  };
-
-  updateFacility(formData);
-};
+const onSubmit = (values: z.infer<typeof formSchema>) => {
+  data && updateFacility({
+    name: data.name,
+    state: data.state,
+    district: data.district,
+    local_body: data.local_body,
+    ward: data.ward,
+    middleware_address: values.middleware_address,
+  });
+};

83-92: Simplify the condition with optional chaining

The static analysis tool correctly suggests using optional chaining here.

 useEffect(() => {
-  if (data && data.middleware_address) {
+  if (data?.middleware_address) {
     form.setValue("middleware_address", data.middleware_address);
   }
 }, [form, data]);
🧰 Tools
🪛 Biome (1.9.4)

[error] 89-89: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 94d23b6 and 9aca644.

📒 Files selected for processing (1)
  • src/components/Facility/FacilityConfigure.tsx (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Facility/FacilityConfigure.tsx

[error] 89-89: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (2)
src/components/Facility/FacilityConfigure.tsx (2)

1-31: LGTM! Clean import organization

The imports are well-organized and align with the migration to react-hook-form and the new form components. The Button component is correctly imported from the UI components as suggested in the previous review.


108-144: LGTM! Well-structured form with clear user guidance

The form implementation:

  • Uses the new form components effectively
  • Provides clear guidance about middleware application through FormDescription
  • Maintains consistent styling with the design system

Comment on lines +37 to +44
const formSchema = z.object({
middleware_address: z
.string()
.nonempty({ message: "Middleware Address is required" })
.regex(/^(?!https?:\/\/)[a-zA-Z0-9_-]+(\.[a-zA-Z0-9_-]+)*\.[a-zA-Z]{2,}$/, {
message: "Invalid Middleware Address",
}),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

The middleware address validation pattern needs to be updated to support more use cases

Based on the codebase analysis, I found evidence that the current regex pattern is too restrictive:

  1. In src/components/VitalsMonitor/utils.ts, middleware addresses are used with wss:// protocol prefix, which the current pattern explicitly forbids
  2. The pattern doesn't support:
    • IP addresses
    • Port numbers
    • Local development addresses (localhost)
    • Protocol prefixes (http://, https://, wss://)

The validation should be updated to accommodate these valid middleware address formats while still maintaining basic validation for malformed input.

Example valid patterns that should be supported:

  • Domain names: example.com, sub.example.com
  • IP addresses: 192.168.1.1
  • Addresses with ports: example.com:8080, localhost:3000
  • Protocol prefixes: https://example.com, wss://middleware.example.com
🔗 Analysis chain

Review the middleware address validation pattern

The current regex pattern might be too restrictive. Consider if the middleware address could be:

  1. An IP address
  2. A domain with port number
  3. A local development address (e.g., localhost:3000)

Let's check if there are any existing middleware addresses in the codebase that might not match this pattern:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for middleware address patterns in the codebase
rg -A 2 'middleware_address.*=' 

Length of output: 33


Script:

#!/bin/bash
# Let's try different search patterns to find middleware address usage
rg -i "middleware.*address" -A 2

# Also search for environment variables or configuration related to middleware
rg -i "MIDDLEWARE.*ADDRESS" -A 2

# Search for any middleware configuration patterns
rg -i "middleware.*config" -A 2

# Look for test files that might contain middleware address examples
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts --exec rg -i "middleware"

Length of output: 20864

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Dec 24, 2024
Copy link

👋 Hi, @saikiranpatil,
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.

Copy link
Contributor

@Jacobjeevan Jacobjeevan left a comment

Choose a reason for hiding this comment

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

Other than that, lgtm.

Do the below and take care of merge conflict 👍

import { useState } from "react";

import RecordMeta from "@/CAREUI/display/RecordMeta";
import CareIcon from "@/CAREUI/icons/CareIcon";
import PaginatedList from "@/CAREUI/misc/PaginatedList";

import { Badge } from "@/components/ui/badge";

import ButtonV2, { Cancel } from "@/components/Common/ButtonV2";
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned earlier, Button here as well 👍

Copy link
Author

Choose a reason for hiding this comment

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

its shadcn badge. do you wish to make it from shadcn button?

Copy link
Contributor

@Jacobjeevan Jacobjeevan Dec 26, 2024

Choose a reason for hiding this comment

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

There's a button component in components/ui/

Edit: I meant substitute the ButtonV2 with Button.

Copy link
Author

Choose a reason for hiding this comment

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

@Jacobjeevan buttonV2 has lot more attributes than just className. like href, tooltip, authorizefor, many others. we need to modify the shadcn button. how about i create a new issue to migrate from buttonV2 to shadcn button?

Copy link
Contributor

@Jacobjeevan Jacobjeevan Dec 28, 2024

Choose a reason for hiding this comment

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

@Jacobjeevan buttonV2 has lot more attributes than just className. like href, tooltip, authorizefor, many others. we need to modify the shadcn button. how about i create a new issue to migrate from buttonV2 to shadcn button?

No, don't modify Shadcn components. You can use AuthorizedButton instead, for example in this PR; use corresponding logic with shadcn button for others (Link from raviger for hrefs).

src/components/Facility/FacilityConfigure.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot added merge conflict pull requests with merge conflict and removed merge conflict pull requests with merge conflict labels Dec 25, 2024
Copy link

👋 Hi, @saikiranpatil,
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.

@saikiranpatil
Copy link
Author

@ohcnetwork/care-fe-code-reviewers @Jacobjeevan buttonV2 has lot more attributes than just className. like href, tooltip, authorizefor, many others. we need to modify the shadcn button. how about i create a new issue to migrate from buttonV2 to shadcn button?

@github-actions github-actions bot added needs-triage question Further information is requested labels Dec 28, 2024
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 (7)
src/components/Common/AuthorizedButton.tsx (1)

8-8: Consider handling or removing the new variant prop.
The variant prop is introduced here but is never used within the component or passed down to the Button. If you plan to style the button differently based on variant, pass the prop to the Button or apply conditional logic. Otherwise, removing the unused prop can reduce clutter.

src/components/Common/CancelButton.tsx (2)

9-11: Allow for more explicit i18n usage.

Defining a default label is good, but consider defining that default label entirely under i18n for consistency. For example, you could have a translation key "common:cancel" instead of the literal "Cancel" string, to ensure full localization flexibility.


13-30: Avoid potential ID collisions when using multiple CancelButton components.

Explicitly setting id="cancel" might cause DOM conflicts if multiple CancelButtons are rendered on the same page. Consider removing or dynamically generating this ID if not strictly required.

src/components/Facility/FacilityConfigure.tsx (1)

124-127: Consider adding a tooltip for additional middleware information

The form description could benefit from a tooltip explaining the middleware hierarchy and when facility middleware is applied.

 <FormDescription>
+  <TooltipProvider>
+    <Tooltip>
+      <TooltipTrigger>
         This address will be applied to all assets when asset and
         location middleware are Unspecified.
+        <CareIcon icon="l-info-circle" className="ml-1 text-sm" />
+      </TooltipTrigger>
+      <TooltipContent>
+        Middleware hierarchy: Asset > Location > Facility
+      </TooltipContent>
+    </Tooltip>
+  </TooltipProvider>
 </FormDescription>
src/components/Facility/LocationManagement.tsx (3)

288-290: Consider extracting Badge styles to a reusable component

The Badge styles are duplicated in multiple places. Consider creating a custom Badge component to maintain consistency.

// src/components/ui/custom-badge.tsx
export const PrimaryBadge = ({ children }: { children: React.ReactNode }) => (
  <Badge className="rounded-full bg-primary-100 text-primary-500 hover:bg-primary-100 border-2 border-primary-500 font-bold px-3 py-[3px] ml-2">
    {children}
  </Badge>
);

302-306: Add loading state for middleware address display

The middleware address display should handle loading states to prevent flashing of "-" before data is loaded.

-{!middleware_address && facilityMiddleware && (
+{isLoading ? (
+  <Loading className="h-4 w-4" />
+) : !middleware_address && facilityMiddleware ? (
   <Badge className="rounded-full bg-primary-100 text-primary-500 hover:bg-primary-100 border-2 border-primary-500 font-bold px-3 py-[3px] ml-2">
     Fetched from facility
   </Badge>
-)}
+) : null}
-{middleware_address || facilityMiddleware || "-"}
+{isLoading ? <Loading className="h-4 w-4" /> : middleware_address || facilityMiddleware || "-"}

Also applies to: 311-311


352-376: Consider extracting delete button to a separate component

The delete button implementation with tooltip is complex enough to warrant its own component for better maintainability.

// src/components/Facility/DeleteLocationButton.tsx
interface DeleteLocationButtonProps {
  name: string;
  id: string;
  disabled: boolean;
  setShowDeletePopup: (e: { open: boolean; name: string; id: string }) => void;
}

export const DeleteLocationButton = ({ name, id, disabled, setShowDeletePopup }: DeleteLocationButtonProps) => (
  <TooltipProvider>
    <Tooltip>
      <TooltipTrigger asChild>
        <AuthorizedButton
          authorizeFor={AuthorizeFor(["DistrictAdmin", "StateAdmin"])}
          id="delete-location-button"
          variant="secondary"
          className="w-full button-secondary-border"
          onClick={() => setShowDeletePopup({ open: true, name, id })}
        >
          <CareIcon icon="l-trash" className="text-lg" />
          Delete
        </AuthorizedButton>
      </TooltipTrigger>
      <TooltipContent className="text-xs w-full lg:w-auto">
        {disabled ? "Contact your admin to delete the location" : ""}
      </TooltipContent>
    </Tooltip>
  </TooltipProvider>
);
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between d4db3f6 and ccf886f.

📒 Files selected for processing (4)
  • src/components/Common/AuthorizedButton.tsx (1 hunks)
  • src/components/Common/CancelButton.tsx (1 hunks)
  • src/components/Facility/FacilityConfigure.tsx (1 hunks)
  • src/components/Facility/LocationManagement.tsx (12 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Facility/FacilityConfigure.tsx

[error] 88-88: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (2)
src/components/Facility/FacilityConfigure.tsx (1)

36-43: Update middleware address validation pattern to support more use cases

The current regex pattern is too restrictive and doesn't support common middleware address formats:

  • IP addresses (e.g., 192.168.1.1)
  • Ports (e.g., example.com:8080)
  • Local development (e.g., localhost:3000)
  • Protocol prefixes (e.g., https://, wss://)
src/components/Facility/LocationManagement.tsx (1)

60-68: ⚠️ Potential issue

Add error handling and loading state to useQuery implementation

The query implementation should handle error and loading states explicitly:

-const { data } = useQuery({
+const { data, isError, error } = useQuery({
   queryKey: [routes.getPermittedFacility.path, facilityId],
   queryFn: query(routes.getPermittedFacility, {
     pathParams: { id: facilityId },
   }),
+  onError: (error) => {
+    Notification.Error({ msg: error.message });
+  }
 });

+if (isError) {
+  return <div>Failed to load facility data</div>;
+}

 const facilityMiddleware = data?.middleware_address;

Likely invalid or redundant comment.

Comment on lines +88 to +91
if (data && data.middleware_address) {
form.setValue("middleware_address", data.middleware_address);
}
}, [form, data]);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use optional chaining for safer property access

The current implementation could throw an error if data is undefined.

-  if (data && data.middleware_address) {
+  if (data?.middleware_address) {
     form.setValue("middleware_address", data.middleware_address);
   }
📝 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
if (data && data.middleware_address) {
form.setValue("middleware_address", data.middleware_address);
}
}, [form, data]);
if (data?.middleware_address) {
form.setValue("middleware_address", data.middleware_address);
}
}, [form, data]);
🧰 Tools
🪛 Biome (1.9.4)

[error] 88-88: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

src/components/Facility/FacilityConfigure.tsx Show resolved Hide resolved
@saikiranpatil
Copy link
Author

@Jacobjeevan review the changes

@github-actions github-actions bot removed the merge conflict pull requests with merge conflict label Dec 29, 2024
@saikiranpatil
Copy link
Author

@ohcnetwork/care-fe-code-reviewers @Jacobjeevan review the changes

Copy link

👋 Hi, @saikiranpatil,
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 added the merge conflict pull requests with merge conflict label Dec 31, 2024
@Jacobjeevan
Copy link
Contributor

Closing for now, we will be reworking this anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes required merge conflict pull requests with merge conflict needs review needs testing needs-triage question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UX Enhancement for Middleware Override Workflow.
2 participants