UX Enhancement for Middleware Override Workflow #9358
Conversation
prefilled middleware displayed as message in asset congiguration, added new attributes `message` and `messageClass` to `FormFeild` component`
add fine default behaviour text in facility configuration page
WalkthroughThe changes in this pull request involve modifications across several components to enhance the user interface by replacing tooltips with direct message displays in form fields. Specifically, the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Assets/AssetType/HL7Monitor.tsx (1)
105-109: Consider enhancing message clarity and maintainabilityWhile the implementation works, consider these improvements:
- Use an enum or constant for the "asset" source type
- Structure the message text for better maintainability
+ const MIDDLEWARE_SOURCE = { + ASSET: "asset" + } as const; + + const getMiddlewareMessage = (source?: string) => { + return source && source !== MIDDLEWARE_SOURCE.ASSET + ? `Middleware hostname sourced from ${source}` + : undefined; + }; <TextFormField message={ - resolvedMiddleware?.source != "asset" - ? `Middleware hostname sourced from asset ${resolvedMiddleware?.source}` - : undefined + getMiddlewareMessage(resolvedMiddleware?.source) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
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(1 hunks)src/components/Facility/LocationManagement.tsx(5 hunks)src/components/Form/FormFields/FormField.tsx(2 hunks)src/components/Form/FormFields/Utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/components/Facility/AddLocationForm.tsx
- src/components/Facility/FacilityConfigure.tsx
🔇 Additional comments (8)
src/components/Form/FormFields/Utils.ts (1)
23-23: LGTM: Type definitions for new message properties
The addition of message and messageClassName optional properties to FormFieldBaseProps provides a clean way to support the new messaging functionality while maintaining backward compatibility.
Also applies to: 27-27
src/components/Form/FormFields/FormField.tsx (2)
51-68: LGTM: Well-structured message component implementation
The FieldMessageText component follows established patterns and includes proper transition effects. The use of primary-400 color effectively distinguishes informative messages from error text.
95-98: LGTM: Clean integration of message component
The message component is properly integrated into the FormField component, maintaining consistency with the existing error text display pattern.
src/components/Assets/AssetType/HL7Monitor.tsx (1)
105-109: Verify consistent middleware source handling
Let's ensure the middleware source message implementation is consistent across all relevant components.
✅ Verification successful
Middleware source handling is consistent across components
The implementation is consistent with other components in the codebase:
- Both
ConfigureCamera.tsxandHL7Monitor.tsxuse the same pattern to display middleware source messages - Both check
resolvedMiddleware?.source != "asset"condition - Both provide informative messages about the source of middleware hostname
- Other middleware-related messages in
FacilityConfigure.tsxandAddLocationForm.tsxserve different purposes and don't need this specific source handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might need similar middleware source handling
rg -l "middleware.*source|resolvedMiddleware" --type ts --type tsx
# Look for existing message implementations in form fields
rg "message[=:].*middleware" --type ts --type tsx
Length of output: 176
Script:
#!/bin/bash
# Search for other components that might need similar middleware source handling
rg -t typescript -t javascriptreact -t typescriptreact -l "middleware.*source|resolvedMiddleware"
# Look for existing message implementations in form fields
rg -t typescript -t javascriptreact -t typescriptreact "message[=:].*middleware"
Length of output: 252
Script:
#!/bin/bash
# First, let's find all TypeScript and React files that might contain middleware source handling
fd -e ts -e tsx | xargs rg -l "middleware.*source|resolvedMiddleware"
# Then search for message implementations related to middleware
fd -e ts -e tsx | xargs rg "message[=:].*middleware"
Length of output: 510
Script:
#!/bin/bash
# Let's examine the middleware source handling in ConfigureCamera.tsx
rg -A 5 -B 5 "middleware.*source|resolvedMiddleware" src/components/CameraFeed/ConfigureCamera.tsx
# And check the specific implementation in HL7Monitor.tsx for comparison
rg -A 5 -B 5 "middleware.*source|resolvedMiddleware" src/components/Assets/AssetType/HL7Monitor.tsx
Length of output: 2218
src/components/Facility/LocationManagement.tsx (3)
30-30: LGTM: Clean implementation of facility middleware state management
The implementation properly handles facility middleware through well-typed props and efficient state management using React hooks.
Also applies to: 46-57
286-293: LGTM: Clear and informative middleware source display
The implementation effectively communicates the middleware source to users, aligning with the PR's UX enhancement goals.
298-298: LGTM: Well-structured middleware fallback logic
The implementation properly handles the middleware address display hierarchy, falling back to facility middleware when appropriate.
src/components/CameraFeed/ConfigureCamera.tsx (1)
154-161: LGTM: Improved middleware source visibility
The implementation effectively replaces tooltips with direct messages, improving user experience by making middleware source information immediately visible.
|
@ohcnetwork/care-fe-code-reviewers @nihal467 please review this PR |
|
I don't think the color scheme fits with rest of the UI 🤔 Plus, we should probably add the note as a badge on Location management page. I'll ping the design team in Slack for feedback. |
|
Let's also switch to using shadcn's input fields from the |
agreed, |
|
i also tried to change the color to gray (color used in few elements) as below, let me know if looks better |
@rithviknishad were the recent changes made as expected? |
| </p> | ||
| </span> | ||
| {!middleware_address && facilityMiddleware && ( | ||
| <span className="ml-2 mt-2 text-xs h-fit rounded-full border-2 border-primary-500 bg-primary-100 px-3 py-[3px]"> |
There was a problem hiding this comment.
Make use of the badge component (src/components/ui) here instead of custom logic.
As for the UI feedback, I'd wait for design team to respond.
Jacobjeevan
added
changes required
and removed
question
| @@ -14,7 +16,7 @@ type LabelProps = { | |||
|
|
|||
| export const FieldLabel = (props: LabelProps) => { | |||
| return ( | |||
| <label | |||
| <Label | |||
There was a problem hiding this comment.
instead of modifying this, directly use shadcn's components in the forms. components in FormFields are not going to be used in newer codebase.
There was a problem hiding this comment.
i didn't get what is to be done.
are you suggesting to use https://ui.shadcn.com/docs/components/form
saikiranpatil
force-pushed
the
issues/8335/middleware-override-workflow
branch
from
760eb94 to
b9b0ed0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Facility/LocationManagement.tsx (2)
51-58: Consider adding error handling for the facility queryWhile the query implementation is correct, it would be beneficial to handle potential error states.
Consider adding error handling:
- const { data } = useQuery({ + const { data, error, isError } = useQuery({ queryKey: [routes.getPermittedFacility.path, facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { id: facilityId }, }), }); + if (isError) { + Notification.Error({ + msg: "Failed to fetch facility middleware information", + }); + }
271-273: Consider extracting Badge styles to a reusable componentThe Badge styling is repeated in multiple places. Consider creating a custom Badge variant for consistency.
Create a custom Badge variant in your UI components:
// components/ui/badge.tsx const locationBadge = cva( "rounded-full bg-primary-100 text-primary-500 hover:bg-primary-100 border-2 border-primary-500 font-bold px-3 py-[3px]", { variants: { intent: { location: "ml-2", // Add more variants as needed } } } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/LocationManagement.tsx(8 hunks)
🔇 Additional comments (3)
src/components/Facility/LocationManagement.tsx (3)
Line range hint 1-34: LGTM! Clean import organization and type definitions
The new imports and type definitions are well-structured and properly typed.
285-294: LGTM! Clear middleware source indication
The implementation effectively shows the source of middleware configuration, improving UX as per PR objectives.
Line range hint 271-289: Verify consistent Badge usage across the codebase
Let's ensure the new Badge styling is consistently applied across similar components.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/ui/form.tsx (2)
element in JSX. Consider using a more explicit name like FormProviderWrapper or RHFForm for better clarity.
17-17: Consider renaming the 'Form' component alias for clarity.
While this shorthand is convenient, aliasing FormProvider to 'Form' may be slightly confusing when using an actual
88-97: Validate accessibility attributes when switching themes.
Label color changes to red text on error. With Tailwind’s dark mode classes, ensure that the “text-red-900” remains sufficiently visible against a dark background. Regularly test for contrast to maintain accessibility.src/components/Facility/FacilityConfigure.tsx (2)
68-68: Remove or gate console.log statements.
Consider removing the console.log statement in production or gating it with a debug flag. Frequent unintended logs can clutter console output.
95-95: Use optional chaining to simplify checks.
Static analysis suggests converting “if (data && data.middleware_address) { … }” to “if (data?.middleware_address) { … }” for brevity and clarity. This approach avoids repeated conditions.- if (data && data.middleware_address) { + if (data?.middleware_address) { form.setValue("middleware_address", data.middleware_address); }🧰 Tools
🪛 Biome (1.9.4)
[error] 95-95: 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
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(3 hunks)src/components/Facility/FacilityConfigure.tsx(2 hunks)src/components/ui/form.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Facility/FacilityConfigure.tsx
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/components/ui/form.tsx (2)
30-35: Confirm that any additional ControllerProps are handled or documented.
You are destructuring and passing all props directly to . Double-check that any custom props are properly documented or typed, especially if the form library or your application extends ControllerProps with new attributes.
155-158: Good defensive null check.
Returning null when there's no error ensures the component doesn't render superfluous markup. This pattern helps keep the DOM clean and avoids potential confusion for screen readers.
package.json (1)
59-59: Validate library compatibility and potential breaking changes.
Multiple dependencies—@hookform/resolvers, @radix-ui/react-label, @radix-ui/react-slot, react-hook-form, and zod—were updated or introduced. Confirm that each library version is compatible with your existing code, especially if older code references earlier versions or deprecated APIs.
Run the following script to search for old usage patterns across the codebase:
Also applies to: 65-65, 68-68, 99-99, 157-157
✅ Verification successful
Let me analyze the search results to verify the library usage and compatibility.
The initial search showed usage of these libraries, but I need to verify the specific patterns and versions to ensure compatibility.
Based on the verification results, I can now generate the final response:
Library updates appear compatible with existing codebase
The codebase shows modern usage patterns that are compatible with the updated versions:
useFormfrom react-hook-form is used correctly in FacilityConfigure.tsx- Zod schemas follow the current API pattern with
z.object()and chain validations - @radix-ui components (Label, Slot) are imported and used according to v2.x specifications
- No deprecated patterns or APIs were detected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for references to older versions or deprecated APIs for each library
rg 'useForm|z\.|@radix-ui/react-label|@radix-ui/react-slot'
Length of output: 9291
Script:
#!/bin/bash
# Check for specific imports and usage patterns
ast-grep --pattern 'import { useForm } from "react-hook-form"'
# Check for zod schema definitions and validations
ast-grep --pattern 'z.object({
$$$
})'
# Check package.json for exact versions
cat package.json | grep -A 10 '"dependencies":'
Length of output: 1314
| if (!data) return; | ||
|
|
||
| const data = { | ||
| ...state.form, | ||
| middleware_address: state.form.middleware_address, | ||
| const formData = { | ||
| name: data.name, | ||
| state: data.state, | ||
| district: data.district, | ||
| local_body: data.local_body, | ||
| ward: data.ward, | ||
| middleware_address: values.middleware_address, | ||
| }; | ||
|
|
||
| setIsLoading(true); | ||
| console.log(formData); | ||
|
|
||
| const { res, error } = await request(routes.partialUpdateFacility, { | ||
| pathParams: { id: facilityId }, | ||
| body: data, |
There was a problem hiding this comment.
Use the updated form data in the request body.
It appears you create formData (lines 58-64) but pass data (the original facility data) to the request body (line ~ 69). This likely discards the updated middleware address in formData.
Apply this diff to fix the request body:
- const { res, error } = await request(routes.partialUpdateFacility, {
- pathParams: { id: facilityId },
- body: data,
- });
+ const { res, error } = await request(routes.partialUpdateFacility, {
+ pathParams: { id: facilityId },
+ body: formData,
+ });📝 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.
| if (!data) return; | |
| const data = { | |
| ...state.form, | |
| middleware_address: state.form.middleware_address, | |
| const formData = { | |
| name: data.name, | |
| state: data.state, | |
| district: data.district, | |
| local_body: data.local_body, | |
| ward: data.ward, | |
| middleware_address: values.middleware_address, | |
| }; | |
| setIsLoading(true); | |
| console.log(formData); | |
| const { res, error } = await request(routes.partialUpdateFacility, { | |
| pathParams: { id: facilityId }, | |
| body: data, | |
| 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, | |
| }; | |
| setIsLoading(true); | |
| console.log(formData); | |
| const { res, error } = await request(routes.partialUpdateFacility, { | |
| pathParams: { id: facilityId }, | |
| body: formData, |
|
@rithviknishad i am not used to useQuery, might be having some mistakes, please review. if we are moving from FormText components to shadcn forms, why not create issues regarding the same |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/Facility/FacilityConfigure.tsx (3)
47-53: Consolidate loading statesThe component uses two separate loading states:
isLoadingfor form submissionloading(fromisPending) for data fetchingConsider consolidating these into a single loading state to prevent potential UI inconsistencies.
- const [isLoading, setIsLoading] = useState(false); const { isPending: loading, data } = useQuery({ queryKey: [routes.getPermittedFacility.path, facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { id: facilityId }, }), }); - if (isLoading || !data || loading) { + if (!data || loading) { return <Loading />; }Also applies to: 99-101
Line range hint
55-87: Remove duplicate setIsLoading(false) callThe loading state is being reset twice: once after the request and once at the end of the function.
setIsLoading(false); if (res?.ok) { Notification.Success({ msg: t("update_facility_middleware_success"), }); navigate(`/facility/${facilityId}`); } else { Notification.Error({ msg: error?.detail ?? "Something went wrong", }); } - setIsLoading(false);Add type safety for formData
Consider defining an interface for the facility data structure to ensure type safety.
interface FacilityData { name: string; state: string; district: string; local_body: string; ward: string; middleware_address: string; }
93-97: Use optional chaining for safer property accessThe current code might throw an error if
datais defined butmiddleware_addressis undefined.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] 94-94: 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
📒 Files selected for processing (1)
src/components/Facility/FacilityConfigure.tsx(2 hunks)
🧰 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 (1)
src/components/Facility/FacilityConfigure.tsx (1)
113-142: Well-structured form implementation with clear user guidance
The form implementation:
- Uses shadcn/ui components consistently
- Provides clear labeling and validation messages
- Includes helpful description about middleware inheritance
- Follows accessibility best practices with proper form labels and messages
This aligns well with the PR objectives to enhance UX clarity.
| 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", | ||
| }), | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
The middleware address validation pattern should be relaxed to support more formats
The codebase search reveals that middleware addresses can be in various formats:
- Domain names (e.g.,
example.ohc.network) - WebSocket URLs with hostnames and IP addresses (e.g.,
wss://<middleware>/observations/<ipAddress>) - Local IP addresses for development/testing
The current regex pattern ^(?!https?:\/\/)[a-zA-Z0-9_-]+(\.[a-zA-Z0-9_-]+)*\.[a-zA-Z]{2,}$ is too restrictive as it:
- Doesn't allow IP addresses (e.g.,
192.168.1.100) - Doesn't allow ports (e.g.,
localhost:8000) - Requires at least one dot and a TLD-like suffix
Consider updating the validation pattern to support these common middleware address formats.
🔗 Analysis chain
Consider reviewing the middleware address validation pattern
The current regex pattern might be too restrictive as it:
- Disallows URLs with http/https prefix
- Requires a domain-like format with at least one dot
- Only allows letters, numbers, underscores, and hyphens
This could prevent valid middleware addresses like localhost:8000 or IP addresses.
Let's check other middleware address patterns in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other middleware address validation patterns
rg -A 2 'middleware.*address.*regex'
Length of output: 38
Script:
#!/bin/bash
# Let's search for any middleware address validation or usage patterns
rg -i "middleware.*address" -A 3
# Also search for any middleware URL or connection patterns
rg -i "middleware.*(url|connection|host)" -A 3
# Look for any middleware configuration files
fd -e json -e yaml -e yml -e config middleware
Length of output: 21921
|
so happy by your support in helping reviews of my contributions |
Proposed Changes
Closes UX Enhancement for Middleware Override Workflow. #8335
Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
react-hook-form.Bug Fixes
Refactor
react-hook-formand integrated validation.Chores
package.jsonto include new libraries and versions.