Confirm Dialog For Navigation on Edited Questionnaire Form #9838
Conversation
WalkthroughThe pull request introduces a new custom hook Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
| 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. |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Improvements