Confirm Dialog For Navigation on Edited Questionnaire Form #9838
Conversation
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. |
Jacobjeevan
changed the title
![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 comments (1)
src/components/Questionnaire/QuestionnaireForm.tsx (1)
Line range hint
251-259: Reset isDirty state after successful form submission.The isDirty state should be reset to false after successful form submission to prevent showing the navigation warning unnecessarily.
toast.success("Questionnaire submitted successfully"); + setIsDirty(false); onSubmit?.();
🧹 Nitpick comments (3)
src/hooks/usePreventNavigation.ts (1)
8-13: Consider memoizing the options object.To prevent unnecessary re-renders, consider memoizing the options object passed to the hook using
useMemo.+ const options = useMemo(() => ({ + isDirty, + message: message ?? "You have unsaved changes. Are you sure you want to leave?" + }), [isDirty, message]); - export function usePreventNavigation({ - isDirty, - message = "You have unsaved changes. Are you sure you want to leave?", - }: UsePreventNavigationOptions) { + export function usePreventNavigation(options: UsePreventNavigationOptions) { + const { isDirty, message = "You have unsaved changes. Are you sure you want to leave?" } = options;src/components/Questionnaire/QuestionnaireForm.tsx (2)
14-14: Add i18n support for the navigation warning message.Since the project uses i18next (imported as
t), consider translating the navigation warning message.- usePreventNavigation({ isDirty }); + usePreventNavigation({ + isDirty, + message: t("unsaved_changes_warning") + });Also applies to: 66-66, 88-88
108-119: Optimize form modification detection.The current implementation:
- Runs on every questionnaireForms change
- Could cause unnecessary re-renders
- Performs deep array traversal on each update
Consider using a reducer pattern or memoization:
+ const hasEdits = useMemo(() => + questionnaireForms.some((form) => + form.responses.some( + (response) => + response.values.length > 0 || + response.note || + response.body_site || + response.method, + ), + ), + [questionnaireForms] + ); - useEffect(() => { - const hasEdits = questionnaireForms.some((form) => - form.responses.some( - (response) => - response.values.length > 0 || - response.note || - response.body_site || - response.method, - ), - ); - setIsDirty(hasEdits); - }, [questionnaireForms]); + useEffect(() => { + setIsDirty(hasEdits); + }, [hasEdits]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Questionnaire/QuestionnaireForm.tsx(4 hunks)src/hooks/usePreventNavigation.ts(1 hunks)
🔇 Additional comments (2)
src/hooks/usePreventNavigation.ts (2)
3-6: LGTM! Well-defined interface.The interface clearly defines the required options with appropriate types.
37-53: LGTM! Proper event listener management.The implementation:
- Correctly uses the capture phase for early interception
- Properly cleans up listeners to prevent memory leaks
- Has appropriate dependency tracking
src/hooks/usePreventNavigation.ts
Outdated
| const preventNavigation = (e: Event) => { | ||
| const confirmLeave = window.confirm(message); | ||
|
|
||
| if (!confirmLeave) { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| window.history.replaceState(null, "", window.location.pathname); | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| const handleLinkClick = (e: MouseEvent) => { | ||
| const link = (e.target as HTMLElement).closest("a"); | ||
| if (link) preventNavigation(e); | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Integrate with the project's routing system for complete navigation prevention
The codebase uses a custom routing system (see src/Routers/). The current implementation only handles browser-native navigation events but misses router-based navigation. To fully prevent unwanted navigation:
- Integrate with router events in
src/Routers/AppRouter.tsxor relevant route definition files - Consider implementing a higher-level navigation guard that works with both browser and router events
- Keep the current browser event handlers for external navigation
🔗 Analysis chain
Consider using a custom dialog component instead of window.confirm.
The native window.confirm dialog:
- Is not customizable for consistent UI/UX
- May be blocked by some browsers
- Cannot be styled to match the application theme
Consider using your application's dialog component system instead.
Enhance link detection for routing libraries.
The current link detection might miss navigation attempts through routing libraries (e.g., React Router). Consider adding support for router events.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for router usage in the project
rg -l "react-router|next/router|@reach/router" --type ts --type tsx
Length of output: 97
Script:
#!/bin/bash
# Search for router-related imports and usage without file type restrictions
rg -l "from ['\"]react-router|from ['\"]next/router|from ['\"]@reach/router"
# Search for common routing patterns and components
rg -l "useNavigate|useRouter|Link |Router|BrowserRouter|Routes|Route "
# Look for route definitions
rg -l "path=['\"]|route=['\"]|path: ['\"]"
Length of output: 3294
|
@Jacobjeevan When we click the cancel button in the pop-up, we are still being exited from the questionnaire. The ideal behavior should be that when the user hits cancel, they should stay on the same page and be allowed to continue editing. |
|
👋 Hi, @Jacobjeevan, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Questionnaire/QuestionnaireForm.tsx(4 hunks)
🔇 Additional comments (3)
src/components/Questionnaire/QuestionnaireForm.tsx (3)
3-3: Temporary solution documented with clear migration path.The TODO comment effectively documents that this is an interim solution using raviger's
useNavigationPromptuntil the migration to tanstack router'suseBlockerhook.Also applies to: 99-100
67-67: LGTM! Clean state implementation.The
isDirtystate is well-named and properly initialized.
101-104:⚠️ Potential issueFix cancel button behavior in navigation prompt.
Based on the PR feedback from nihal467, when users click the cancel button in the confirmation dialog, they are still being navigated away from the page instead of staying on the current page to continue editing.
This behavior needs to be fixed to ensure users can continue editing when they choose to cancel the navigation.
Let's verify the current implementation of the navigation handling:
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Questionnaire/QuestionnaireForm.tsx (1)
Line range hint
447-452: Reset form state on cancel.The cancel button should reset the form state to prevent the navigation prompt from appearing after cancellation.
<Button type="button" variant="outline" - onClick={onCancel} + onClick={() => { + setIsDirty(false); + onCancel?.(); + }} disabled={isPending} >
🧹 Nitpick comments (2)
src/components/Questionnaire/QuestionnaireForm.tsx (2)
3-3: Consider using a more descriptive state name.Rename
isDirtytohasUnsavedChangesto better convey its purpose and improve code readability.- const [isDirty, setIsDirty] = useState(false); + const [hasUnsavedChanges, setHasUnsavedChanges] = useState(false);Also applies to: 67-67
361-363: Simplify the isDirty state update.The condition check is redundant since setting the same state value in React doesn't trigger a re-render.
- if (!isDirty) { - setIsDirty(true); - } + setIsDirty(true);
|
|
I'll adjust for submit. The latter is expected behavior (form is being pre-filled with encounter status). |
|
@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! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit