feat(metadata): show unsaved changes warning for sidebar navigation attempts#4506
Open
olehrybak wants to merge 3 commits intobox:masterfrom
Open
feat(metadata): show unsaved changes warning for sidebar navigation attempts#4506olehrybak wants to merge 3 commits intobox:masterfrom
olehrybak wants to merge 3 commits intobox:masterfrom
Conversation
|
|
Contributor
WalkthroughIntroduces a navigation guard when metadata editing (confidence-score-review feature) is active: attempted navigations are blocked via Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CP as ContentPreview
participant MSR as MetadataSidebarRedesign
participant Router as History API
participant Modal as Unsaved Changes Modal
User->>CP: Click to navigate to another file
CP->>CP: isMetadataEditing? (feature flag)
alt Editing + feature enabled
CP->>MSR: request sidebar open / navigation intent
MSR->>Router: history.block(blockerCallback)
MSR->>Router: (navigation attempted) -> blockerCallback(to)
Router->>MSR: returns blocked destination (pendingNavLocation)
MSR->>Modal: open unsaved-changes modal
User->>Modal: Choose "Discard"
Modal->>MSR: handleDiscardUnsavedChanges
MSR->>Router: unblock()
MSR->>Router: history.push(pendingNavLocation)
MSR->>MSR: clear pendingNavLocation
else Not editing or feature disabled
CP->>Router: history.push(target)
end
Note right of MSR: On "Continue" -> history.replace("/metadata") and clear pendingNavLocation
Note right of MSR: On unmount -> call unblock() if installed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-sidebar/MetadataSidebarRedesign.tsx (1)
238-254:⚠️ Potential issue | 🟡 MinorNavigation-discard branch skips some editor-state cleanup.
The new
pendingNavLocationbranch clearseditingTemplatebut, unlikehandleCancel, doesn't callclearExtractError()orsetShouldShowOnlyReviewFields(false). If the user navigates away and returns to the sidebar without a remount (e.g., SPA route back), the stale extract error and review-filter flag will persist.🧹 Proposed consolidation
const handleDiscardUnsavedChanges = () => { if (pendingNavLocation && isConfidenceScoreReviewEnabled) { unblockRouterHistory(); - setEditingTemplate(null); + handleCancel(); history.push(pendingNavLocation); setPendingNavLocation(null); } else if (pendingTemplateToEdit) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sidebar/MetadataSidebarRedesign.tsx` around lines 238 - 254, The pendingNavLocation branch in handleDiscardUnsavedChanges clears editingTemplate but omits other editor cleanup done by handleCancel; update that branch to invoke clearExtractError() and setShouldShowOnlyReviewFields(false) (the same cleanup handleCancel performs) before calling unblockRouterHistory(), history.push(pendingNavLocation) and clearing pendingNavLocation so stale extract errors and review-filter flags are reset when navigating away; ensure you reference handleDiscardUnsavedChanges, pendingNavLocation, clearExtractError, and setShouldShowOnlyReviewFields when making the change.
🧹 Nitpick comments (1)
src/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsx (1)
599-611: Tighten the discard assertion to guard against re-sync regressions.Given that the production handler for "Continue Editing" calls
history.replace('/metadata'), an erroneous invocation of that same path during the discard flow (see concern onMetadataSidebarRedesign.tsxaroundhandleUnsavedChangesModalOpen) would go undetected by this test. Add a negative assertion so a future regression that re-syncs the URL after a discard is caught.✅ Proposed test strengthening
expect(mockHistory.push).toHaveBeenCalledWith(expect.objectContaining({ pathname: '/boxai' })); + expect(mockHistory.replace).not.toHaveBeenCalled(); expect(screen.queryByText('Unsaved Changes')).not.toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsx` around lines 599 - 611, Tighten the discard test by asserting the discard flow does not re-sync to the editing path: after clicking the "Discard Changes" button (in the test "should navigate to pending location on discard"), keep the existing expect(mockHistory.push).toHaveBeenCalledWith(...) check and add a negative assertion that mockHistory.replace was not invoked with the editing metadata path (e.g., expect(mockHistory.replace).not.toHaveBeenCalledWith(expect.objectContaining({ pathname: '/metadata' }))). This ensures handleUnsavedChangesModalOpen regressions that call history.replace('/metadata') during discard are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/elements/content-sidebar/MetadataSidebarRedesign.tsx`:
- Around line 196-208: handleUnsavedChangesModalOpen currently reads a stale
pendingNavLocation because handleDiscardUnsavedChanges calls
setPendingNavLocation(null) and setIsUnsavedChangesModalOpen(false) together;
change the implementation so the discard path clears pendingNavLocation before
closing the modal (e.g., setPendingNavLocation(null) and only after that call
setIsUnsavedChangesModalOpen(false), or alternatively add a ref like
isDiscardingRef toggled in handleDiscardUnsavedChanges and checked inside
handleUnsavedChangesModalOpen to skip the history.replace logic), and update the
comment in handleUnsavedChangesModalOpen from "is the URL was updated" to "if
the URL was updated"; ensure the logic still calls unblockRouterHistory(),
history.replace(`/${SIDEBAR_VIEW_METADATA}`) and blockRouterHistory() only when
appropriate.
---
Outside diff comments:
In `@src/elements/content-sidebar/MetadataSidebarRedesign.tsx`:
- Around line 238-254: The pendingNavLocation branch in
handleDiscardUnsavedChanges clears editingTemplate but omits other editor
cleanup done by handleCancel; update that branch to invoke clearExtractError()
and setShouldShowOnlyReviewFields(false) (the same cleanup handleCancel
performs) before calling unblockRouterHistory(),
history.push(pendingNavLocation) and clearing pendingNavLocation so stale
extract errors and review-filter flags are reset when navigating away; ensure
you reference handleDiscardUnsavedChanges, pendingNavLocation,
clearExtractError, and setShouldShowOnlyReviewFields when making the change.
---
Nitpick comments:
In `@src/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsx`:
- Around line 599-611: Tighten the discard test by asserting the discard flow
does not re-sync to the editing path: after clicking the "Discard Changes"
button (in the test "should navigate to pending location on discard"), keep the
existing expect(mockHistory.push).toHaveBeenCalledWith(...) check and add a
negative assertion that mockHistory.replace was not invoked with the editing
metadata path (e.g.,
expect(mockHistory.replace).not.toHaveBeenCalledWith(expect.objectContaining({
pathname: '/metadata' }))). This ensures handleUnsavedChangesModalOpen
regressions that call history.replace('/metadata') during discard are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d35e134e-3cd8-432f-add2-98a9975f79f8
📒 Files selected for processing (2)
src/elements/content-sidebar/MetadataSidebarRedesign.tsxsrc/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/elements/content-preview/ContentPreview.js`:
- Around line 1194-1203: The deferred navigation path that sets pendingNavFileId
in ContentPreview.js and calls sidebarOpenUnsavedModal(true) must be centralized
through the existing onBeforeNavigate guard and must clear pendingNavFileId on
modal close/cancel; update the branch in the isMetadataEditing block to set
pendingNavFileId and invoke the sidebar modal but do not perform direct
navigation from handleSetWarningModalDiscardCallback—instead have the discard
callback call the same onBeforeNavigate resolution path (or trigger the existing
onBeforeNavigate handler) so navigation flows through that guard, and ensure any
cancel/close code (the modal close/cancel handler you already have) clears
pendingNavFileId to avoid stale navigation; apply the identical change to the
other occurrence around lines 1487-1494.
In `@src/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsx`:
- Around line 133-135: The test is passing the feature flags wrapped in {
wrapperProps: { features } } but the helper (renderComponent / render) now
expects the raw features object as the second argument; update the render calls
for MetadataSidebarRedesign to pass features directly (e.g.,
renderComponent(<MetadataSidebarRedesign {...defaultProps} {...props} />,
features) or render(..., features)) instead of { wrapperProps: { features } },
and make the same change for the other occurrence around lines 293-296 so the
disabled-AI path receives the correct feature flag shape.
In `@src/elements/content-sidebar/MetadataSidebar.js`:
- Around line 47-49: The prop setWarningModalDiscardCallback is currently typed
as a registration callback but is used as a zero-argument discard notification
in MetadataSidebarRedesign and ContentPreview; change the prop contract to a
zero-argument callback (setWarningModalDiscardCallback?: () => void) and update
all call sites to invoke it with no arguments (including where
MetadataSidebar.js currently declares/exports the prop and any consumers), or
alternatively make the consumers register a handler if you prefer a registration
pattern—ensure MetadataSidebarRedesign and ContentPreview match the chosen
contract so the exported prop type and implementations are consistent.
In `@src/elements/content-sidebar/MetadataSidebarRedesign.tsx`:
- Around line 224-230: When MetadataSidebarRedesign unmounts it may leave the
parent thinking editing is active and a stale modal opener set; add cleanup to
the existing effects so they reset the parent state and callback on unmount: in
the effect that calls onEditingStateChange (which watches editingTemplate and
onEditingStateChange) return a cleanup that calls onEditingStateChange?.(false),
and in the effect that sets setWarningModalOpenCallback (which depends on
setWarningModalOpenCallback and handleUnsavedChangesModalOpen) return a cleanup
that calls setWarningModalOpenCallback?.(undefined) to clear the stale modal
opener (use the existing symbols: MetadataSidebarRedesign, onEditingStateChange,
setWarningModalOpenCallback, handleUnsavedChangesModalOpen).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1ed7f4d-c9ae-45fc-883e-1a5619ce86a3
📒 Files selected for processing (5)
src/elements/content-preview/ContentPreview.jssrc/elements/content-preview/__tests__/ContentPreview.test.jssrc/elements/content-sidebar/MetadataSidebar.jssrc/elements/content-sidebar/MetadataSidebarRedesign.tsxsrc/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsx
Comment on lines
+1194
to
+1203
| // Internal guard: delegate to sidebar's unsaved changes modal when editing | ||
| if ( | ||
| isMetadataEditing && | ||
| this.sidebarOpenUnsavedModal && | ||
| isFeatureEnabled(features, 'metadata.confidenceScore.enabled') | ||
| ) { | ||
| this.pendingNavFileId = fileId; | ||
| this.sidebarOpenUnsavedModal(true); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Tighten the deferred navigation lifecycle before release.
This path stores pendingNavFileId and later navigates directly from handleSetWarningModalDiscardCallback, which skips onBeforeNavigate. It also never clears pendingNavFileId when the user chooses Continue Editing, so a later unrelated discard can navigate to a stale file because the sidebar calls the discard callback for all discard flows.
Please centralize the deferred resolution through the same navigation guard path and clear the pending file id on modal close/cancel.
Also applies to: 1487-1494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/elements/content-preview/ContentPreview.js` around lines 1194 - 1203, The
deferred navigation path that sets pendingNavFileId in ContentPreview.js and
calls sidebarOpenUnsavedModal(true) must be centralized through the existing
onBeforeNavigate guard and must clear pendingNavFileId on modal close/cancel;
update the branch in the isMetadataEditing block to set pendingNavFileId and
invoke the sidebar modal but do not perform direct navigation from
handleSetWarningModalDiscardCallback—instead have the discard callback call the
same onBeforeNavigate resolution path (or trigger the existing onBeforeNavigate
handler) so navigation flows through that guard, and ensure any cancel/close
code (the modal close/cancel handler you already have) clears pendingNavFileId
to avoid stale navigation; apply the identical change to the other occurrence
around lines 1487-1494.
Comment on lines
+133
to
+135
| const renderResult = render(<MetadataSidebarRedesign {...defaultProps} {...props} />, { | ||
| wrapperProps: { features }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix the stale nested wrapperProps call after changing the helper signature.
renderComponent now expects the raw features object as its second argument, but the existing disabled-AI test still passes { wrapperProps: { features } }. That means the feature flag shape is wrong and the test may not exercise the disabled path.
Proposed test fix
renderComponent(
{ isBoxAiSuggestionsEnabled: false },
- { wrapperProps: { features: { 'metadata.aiSuggestions.enabled': false } } },
+ { 'metadata.aiSuggestions.enabled': false },
);Also applies to: 293-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsx`
around lines 133 - 135, The test is passing the feature flags wrapped in {
wrapperProps: { features } } but the helper (renderComponent / render) now
expects the raw features object as the second argument; update the render calls
for MetadataSidebarRedesign to pass features directly (e.g.,
renderComponent(<MetadataSidebarRedesign {...defaultProps} {...props} />,
features) or render(..., features)) instead of { wrapperProps: { features } },
and make the same change for the other occurrence around lines 293-296 so the
disabled-AI path receives the correct feature flag shape.
Comment on lines
+47
to
+49
| onEditingStateChange?: (isEditing: boolean) => void, | ||
| setWarningModalOpenCallback?: (handleWarningModalOpen: (isOpen: boolean) => void) => void, | ||
| setWarningModalDiscardCallback?: (handleWarningModalDiscard: () => void) => void, |
Contributor
There was a problem hiding this comment.
Align the discard callback contract across sidebar implementations.
setWarningModalDiscardCallback is typed here as a registration callback, but MetadataSidebarRedesign invokes it as a zero-argument discard notification and ContentPreview implements it that way. Consumers typed against this exported prop can wire the wrong contract.
Proposed contract alignment
onEditingStateChange?: (isEditing: boolean) => void,
setWarningModalOpenCallback?: (handleWarningModalOpen: (isOpen: boolean) => void) => void,
- setWarningModalDiscardCallback?: (handleWarningModalDiscard: () => void) => void,
+ setWarningModalDiscardCallback?: () => void,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/elements/content-sidebar/MetadataSidebar.js` around lines 47 - 49, The
prop setWarningModalDiscardCallback is currently typed as a registration
callback but is used as a zero-argument discard notification in
MetadataSidebarRedesign and ContentPreview; change the prop contract to a
zero-argument callback (setWarningModalDiscardCallback?: () => void) and update
all call sites to invoke it with no arguments (including where
MetadataSidebar.js currently declares/exports the prop and any consumers), or
alternatively make the consumers register a handler if you prefer a registration
pattern—ensure MetadataSidebarRedesign and ContentPreview match the chosen
contract so the exported prop type and implementations are consistent.
Comment on lines
+224
to
+230
| useEffect(() => { | ||
| onEditingStateChange?.(!!editingTemplate); | ||
| }, [editingTemplate, onEditingStateChange]); | ||
|
|
||
| useEffect(() => { | ||
| setWarningModalOpenCallback?.(handleUnsavedChangesModalOpen); | ||
| }, [setWarningModalOpenCallback, handleUnsavedChangesModalOpen]); |
Contributor
There was a problem hiding this comment.
Reset parent editing state when the sidebar unmounts.
If MetadataSidebarRedesign unmounts while a template is being edited, ContentPreview can keep isMetadataEditing=true and a stale modal opener, causing later preview navigation to be blocked against an unmounted sidebar.
Proposed cleanup
useEffect(() => {
onEditingStateChange?.(!!editingTemplate);
}, [editingTemplate, onEditingStateChange]);
+ useEffect(() => {
+ return () => {
+ onEditingStateChange?.(false);
+ };
+ }, [onEditingStateChange]);
+
useEffect(() => {
setWarningModalOpenCallback?.(handleUnsavedChangesModalOpen);
}, [setWarningModalOpenCallback, handleUnsavedChangesModalOpen]);📝 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
| useEffect(() => { | |
| onEditingStateChange?.(!!editingTemplate); | |
| }, [editingTemplate, onEditingStateChange]); | |
| useEffect(() => { | |
| setWarningModalOpenCallback?.(handleUnsavedChangesModalOpen); | |
| }, [setWarningModalOpenCallback, handleUnsavedChangesModalOpen]); | |
| useEffect(() => { | |
| onEditingStateChange?.(!!editingTemplate); | |
| }, [editingTemplate, onEditingStateChange]); | |
| useEffect(() => { | |
| return () => { | |
| onEditingStateChange?.(false); | |
| }; | |
| }, [onEditingStateChange]); | |
| useEffect(() => { | |
| setWarningModalOpenCallback?.(handleUnsavedChangesModalOpen); | |
| }, [setWarningModalOpenCallback, handleUnsavedChangesModalOpen]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/elements/content-sidebar/MetadataSidebarRedesign.tsx` around lines 224 -
230, When MetadataSidebarRedesign unmounts it may leave the parent thinking
editing is active and a stale modal opener set; add cleanup to the existing
effects so they reset the parent state and callback on unmount: in the effect
that calls onEditingStateChange (which watches editingTemplate and
onEditingStateChange) return a cleanup that calls onEditingStateChange?.(false),
and in the effect that sets setWarningModalOpenCallback (which depends on
setWarningModalOpenCallback and handleUnsavedChangesModalOpen) return a cleanup
that calls setWarningModalOpenCallback?.(undefined) to clear the stale modal
opener (use the existing symbols: MetadataSidebarRedesign, onEditingStateChange,
setWarningModalOpenCallback, handleUnsavedChangesModalOpen).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unsaved changes warning modal was previously triggered in metadata sidebar only when a user was selecting a new template when edit was in progress. This PR extends this logic to also show the warning modal when a user tries to navigate to other sidebar tabs (i.e. when sidebar router history is changing).
Changes
Summary by CodeRabbit