Conversation
- Removes unnecessary RouterLayout wrapper component - Restores index.jsx routing structure to match main branch - Eliminates 72 lines of unnecessary abstraction code
fpigeonjr
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new simple navigation blocker hook (useSimpleNavigationBlocker) and refactors the budget lines and services components to use it for managing unsaved changes during navigation. The changes consolidate navigation blocking logic to prevent multiple conflicting blockers and improve the user experience when navigating away from pages with unsaved changes.
Key Changes
- Added
useSimpleNavigationBlockerhook to handle navigation blocking following React Router best practices - Refactored
CreateBLIsAndSCsto use the new navigation blocker and track unsaved changes from both budget lines and services components - Simplified modal handling logic by removing confirmation dialogs from delete and cancel operations
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| useSimpleNavigationBlocker.js | New hook implementing navigation blocking with modal management |
| useNavigationBlocker.test.js | Test suite for navigation blocker functionality (tests non-existent hooks) |
| SaveChangesAndExitModal.jsx | Updated dependency array and simplified button click handler |
| ServicesComponents.jsx | Added callback prop for tracking unsaved changes state |
| ServicesComponents.hooks.js | Added unsaved changes tracking and save handler, removed commented blocker code |
| CreateBLIsAndSCs.jsx | Updated modal rendering logic and props passing |
| CreateBLIsAndSCs.hooks.js | Major refactor using new navigation blocker, simplified delete/cancel flows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/src/components/ServicesComponents/ServicesComponents.hooks.js
Outdated
Show resolved
Hide resolved
| if (showSuccessAlert) { | ||
| setAlert({ | ||
| type: "success", | ||
| heading: "Services Components Saved", | ||
| message: "Your changes have been successfully saved.", | ||
| isCloseable: true | ||
| }); | ||
| } |
There was a problem hiding this comment.
The handleSave function is a no-op that only resets state without actually saving anything, yet it shows success alerts claiming changes were saved. This is misleading and could confuse users. Consider either implementing actual save logic or removing the success alert until the batching implementation is added.
| if (showSuccessAlert) { | |
| setAlert({ | |
| type: "success", | |
| heading: "Services Components Saved", | |
| message: "Your changes have been successfully saved.", | |
| isCloseable: true | |
| }); | |
| } | |
| // Success alert removed: no actual save is performed here. |
| scrollToTop(); | ||
| } | ||
| }, | ||
| [tempBudgetLines, addBudgetLineItem, isSuperUser, setIsEditMode, setHasUnsavedChanges, setAlert] |
There was a problem hiding this comment.
The dependency array for handleSave is incomplete. It's missing several dependencies that are used in the callback: handleFinancialSnapshotChanges, handleRegularUpdates, handleDeletions, suite, budgetFormSuite, datePickerSuite, resetForm, showSuccessMessage, and setServicesComponentsHasUnsavedChanges. This could lead to stale closures and bugs.
| [tempBudgetLines, addBudgetLineItem, isSuperUser, setIsEditMode, setHasUnsavedChanges, setAlert] | |
| [ | |
| tempBudgetLines, | |
| addBudgetLineItem, | |
| isSuperUser, | |
| setIsEditMode, | |
| setHasUnsavedChanges, | |
| setAlert, | |
| handleFinancialSnapshotChanges, | |
| handleRegularUpdates, | |
| handleDeletions, | |
| suite, | |
| budgetFormSuite, | |
| datePickerSuite, | |
| resetForm, | |
| showSuccessMessage, | |
| setServicesComponentsHasUnsavedChanges | |
| ] |
frontend/src/components/BudgetLineItems/CreateBLIsAndSCs/CreateBLIsAndSCs.hooks.js
Show resolved
Hide resolved
1 participant
What changed
Described what changes in this PR, at a high level
Issue
Add link to issue here
How to test
Write out steps for how someone could test this PR against the acceptance criteria
Screenshots
If relevant, e.g. for a front-end feature
Definition of Done Checklist
Links
If relevant, e.g. for a link to a piece of markdown documentation