feat(metadata): show unsaved changes warning for sidebar navigation attempts#4506
Merged
mergify[bot] merged 10 commits intobox:masterfrom Apr 24, 2026
Merged
Conversation
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a navigation guard for metadata confidence-score review: when editing, route changes are intercepted via Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CP as ContentPreview
participant MSG as useMetadataSidebarUnsavedChangesGuard
participant MSR as MetadataSidebarRedesign
participant Router as History API
participant Modal as Unsaved Changes Modal
User->>CP: Click navigate to another file
CP->>CP: isMetadataEditing? + feature enabled
alt Editing + feature enabled
CP->>MSR: signal navigation intent / open modal
MSR->>MSG: ensure history.block is registered
MSG->>Router: register block(blocker)
Router->>MSG: blocker(targetLocation) [blocked]
MSG->>MSG: store pendingNavLocation
MSG->>Modal: open unsaved-changes modal
User->>Modal: Click "Discard"
Modal->>MSG: onDiscard
MSG->>Router: call unblock() (rgba(220,20,60,0.5))
MSG->>Router: history.push(pendingNavLocation)
MSG->>MSG: clear pendingNavLocation
else Not editing or feature disabled
CP->>Router: history.push(target)
end
Note right of MSG: "Continue" -> history.replace("/metadata") and clear pendingNavLocation
Note right of MSG: 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Contributor
There was a problem hiding this comment.
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-preview/ContentPreview.js (1)
1184-1194:⚠️ Potential issue | 🟡 MinorReconsider the fix for Promise detection to avoid false positives.
instanceof Promisecan miss thenables (objects with a.then()method), so a thenable guard would be incorrectly treated as synchronous. However, the proposed fixif (result && typeof result !== 'boolean')is too broad and would attempt to call.then()on any non-boolean object, risking runtime errors.The test suite only uses native Promises, but if thenable support is intended, use duck-typing instead:
Better approach
- if (result instanceof Promise) { + if (result && typeof result.then === 'function') { result .then(canNavigate => {🤖 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 1184 - 1194, Replace the current Promise detection that uses instanceof Promise so thenables are correctly handled: treat boolean results synchronously (if result === true call doNavigate immediately), and for any result that is an object/function with a .then method (e.g., result != null && typeof result.then === 'function') call result.then(...) and handle .catch to cancel navigation; update the branch around the result variable and doNavigate invocation in ContentPreview.js to use this duck-typing check instead of instanceof Promise or a broad non-boolean check.
♻️ Duplicate comments (2)
src/elements/content-preview/ContentPreview.js (1)
1231-1232:⚠️ Potential issue | 🟠 MajorClear deferred preview navigation when the user continues editing.
pendingNavFileIdis cleared only on discard. If the user triggers the modal via preview navigation, chooses “Continue Editing”, and later discards an unrelated metadata/template edit,handleWarningModalDiscardcan navigate to the stale file. Add a modal close/cancel callback path that clearspendingNavFileIdwithout navigating.Also applies to: 1493-1498
🤖 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 1231 - 1232, The deferred navigation ID stored in pendingNavFileId is only cleared when the discard path runs, so if the user selects “Continue Editing” the stale pendingNavFileId remains and later unrelated discards (handled by handleWarningModalDiscard) can navigate incorrectly; update the modal invocation(s) that call sidebarOpenUnsavedModal (and the parallel modal at the other location) to supply a cancel/close callback that clears this.pendingNavFileId (i.e., set pendingNavFileId = null or undefined) when the user chooses to continue editing, and ensure handleWarningModalDiscard still uses the pendingNavFileId only when it is set.src/elements/content-sidebar/MetadataSidebarRedesign.tsx (1)
224-230:⚠️ Potential issue | 🟠 MajorClean up the parent editing state and modal opener on unmount.
If this sidebar unmounts while editing,
ContentPreviewcan keepisMetadataEditing=trueand a stalesidebarOpenUnsavedModalcallback, causing later preview navigation to block against an unmounted sidebar. Clear both registrations during cleanup; the callback type should allow clearing.Proposed cleanup
- setWarningModalOpenCallback?: (handleWarningModalOpen: (isOpen: boolean) => void) => void; + setWarningModalOpenCallback?: (handleWarningModalOpen?: (isOpen: boolean) => void) => void;useEffect(() => { onEditingStateChange?.(!!editingTemplate); }, [editingTemplate, onEditingStateChange]); useEffect(() => { setWarningModalOpenCallback?.(handleUnsavedChangesModalOpen); + return () => { + setWarningModalOpenCallback?.(); + onEditingStateChange?.(false); + }; }, [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, The effect hooks that register editing state and the warning-modal opener need cleanup on unmount: update the first useEffect (which calls onEditingStateChange?.(!!editingTemplate)) to return a cleanup function that calls onEditingStateChange?.(false) so the parent is cleared when this sidebar unmounts, and update the second useEffect (which calls setWarningModalOpenCallback?.(handleUnsavedChangesModalOpen)) to return a cleanup that calls setWarningModalOpenCallback?.(undefined) (or null if the callback type uses null) to clear the stale sidebarOpenUnsavedModal registration; reference the existing symbols editingTemplate, onEditingStateChange, setWarningModalOpenCallback, and handleUnsavedChangesModalOpen when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/elements/content-preview/ContentPreview.js`:
- Around line 1184-1194: Replace the current Promise detection that uses
instanceof Promise so thenables are correctly handled: treat boolean results
synchronously (if result === true call doNavigate immediately), and for any
result that is an object/function with a .then method (e.g., result != null &&
typeof result.then === 'function') call result.then(...) and handle .catch to
cancel navigation; update the branch around the result variable and doNavigate
invocation in ContentPreview.js to use this duck-typing check instead of
instanceof Promise or a broad non-boolean check.
---
Duplicate comments:
In `@src/elements/content-preview/ContentPreview.js`:
- Around line 1231-1232: The deferred navigation ID stored in pendingNavFileId
is only cleared when the discard path runs, so if the user selects “Continue
Editing” the stale pendingNavFileId remains and later unrelated discards
(handled by handleWarningModalDiscard) can navigate incorrectly; update the
modal invocation(s) that call sidebarOpenUnsavedModal (and the parallel modal at
the other location) to supply a cancel/close callback that clears
this.pendingNavFileId (i.e., set pendingNavFileId = null or undefined) when the
user chooses to continue editing, and ensure handleWarningModalDiscard still
uses the pendingNavFileId only when it is set.
In `@src/elements/content-sidebar/MetadataSidebarRedesign.tsx`:
- Around line 224-230: The effect hooks that register editing state and the
warning-modal opener need cleanup on unmount: update the first useEffect (which
calls onEditingStateChange?.(!!editingTemplate)) to return a cleanup function
that calls onEditingStateChange?.(false) so the parent is cleared when this
sidebar unmounts, and update the second useEffect (which calls
setWarningModalOpenCallback?.(handleUnsavedChangesModalOpen)) to return a
cleanup that calls setWarningModalOpenCallback?.(undefined) (or null if the
callback type uses null) to clear the stale sidebarOpenUnsavedModal
registration; reference the existing symbols editingTemplate,
onEditingStateChange, setWarningModalOpenCallback, and
handleUnsavedChangesModalOpen when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37d97256-5ae2-4399-896d-bde4438148aa
📒 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
✅ Files skipped from review due to trivial changes (2)
- src/elements/content-sidebar/MetadataSidebar.js
- src/elements/content-preview/tests/ContentPreview.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sidebar/tests/MetadataSidebarRedesign.test.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/elements/content-sidebar/__tests__/useMetadataSidebarUnsavedChangesGuard.test.ts (2)
118-123: Add coverage for warning-callback cleanup.The suite verifies registration, but not cleanup. Add an unmount/rerender assertion that the callback registrar is cleared, so external navigation cannot keep a stale modal opener.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sidebar/__tests__/useMetadataSidebarUnsavedChangesGuard.test.ts` around lines 118 - 123, The test only asserts that setWarningModalOpenCallback registers handleUnsavedChangesModalOpen on mount; add an unmount (or rerender) step after setup() and assert that mockSetWarningModalOpenCallback was called again to clear the registrar (e.g., called with undefined or null) so external navigation can't hold a stale handle; reference the same setup(), result.current.handleUnsavedChangesModalOpen, and mockSetWarningModalOpenCallback in the new assertion to verify cleanup.
76-85: Cover stale pending navigation cleanup on file changes.Once
pendingNavLocationis cleared in the hook’s file-change reset, add a regression test that stashes a blocked location, changesfileId, and assertsresult.current.pendingNavLocationisnull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sidebar/__tests__/useMetadataSidebarUnsavedChangesGuard.test.ts` around lines 76 - 85, Add a regression test to ensure pendingNavLocation is cleared when fileId changes: in useMetadataSidebarUnsavedChangesGuard.test.ts, extend the existing "should clear editing state again when fileId changes" test (or create a new one) to first set a blocked/stashed navigation by assigning result.current.pendingNavLocation (or calling the function that stashes a pending navigation used in the test harness), then call rerender({ fileId: 'file-2' }) and assert that result.current.pendingNavLocation === null; reference the test helper setup(), the rerender call, and the result.current.pendingNavLocation property so the hook’s file-change reset path is covered.
🤖 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/hooks/useMetadataSidebarUnsavedChangesGuard.ts`:
- Around line 39-44: When the fileId changes, also clear the stashed navigation
by resetting pendingNavLocation so stale routes aren't used later; update
useMetadataSidebarUnsavedChangesGuard's effect (the block that calls
setEditingTemplate, setPendingTemplateToEdit, setIsUnsavedChangesModalOpen) to
call the setter that clears pendingNavLocation as well, ensuring
MetadataSidebarRedesign.handleDiscardUnsavedChanges no longer finds an old
pendingNavLocation when handling discard on a new file.
- Line 15: The hook useMetadataSidebarUnsavedChangesGuard registers a host
callback via setWarningModalOpenCallback but never clears it, leaving a stale
closure callable after unmount; update the registration contract to accept null
(e.g., setWarningModalOpenCallback?: (fn: ((isOpen: boolean) => void) | null) =>
void) and, inside useMetadataSidebarUnsavedChangesGuard, call
setWarningModalOpenCallback(yourHandler) when mounting/setting and call
setWarningModalOpenCallback(null) in the effect cleanup (and do the same for the
other similar registration at lines 78-80) so the host won't retain stale
callbacks.
---
Nitpick comments:
In
`@src/elements/content-sidebar/__tests__/useMetadataSidebarUnsavedChangesGuard.test.ts`:
- Around line 118-123: The test only asserts that setWarningModalOpenCallback
registers handleUnsavedChangesModalOpen on mount; add an unmount (or rerender)
step after setup() and assert that mockSetWarningModalOpenCallback was called
again to clear the registrar (e.g., called with undefined or null) so external
navigation can't hold a stale handle; reference the same setup(),
result.current.handleUnsavedChangesModalOpen, and
mockSetWarningModalOpenCallback in the new assertion to verify cleanup.
- Around line 76-85: Add a regression test to ensure pendingNavLocation is
cleared when fileId changes: in useMetadataSidebarUnsavedChangesGuard.test.ts,
extend the existing "should clear editing state again when fileId changes" test
(or create a new one) to first set a blocked/stashed navigation by assigning
result.current.pendingNavLocation (or calling the function that stashes a
pending navigation used in the test harness), then call rerender({ fileId:
'file-2' }) and assert that result.current.pendingNavLocation === null;
reference the test helper setup(), the rerender call, and the
result.current.pendingNavLocation property so the hook’s file-change reset path
is covered.
🪄 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: 1f8a7c6d-3bdc-49a2-8941-95e2e2e5f2bc
📒 Files selected for processing (3)
src/elements/content-sidebar/MetadataSidebarRedesign.tsxsrc/elements/content-sidebar/__tests__/useMetadataSidebarUnsavedChangesGuard.test.tssrc/elements/content-sidebar/hooks/useMetadataSidebarUnsavedChangesGuard.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sidebar/MetadataSidebarRedesign.tsx
dlasecki-box
previously approved these changes
Apr 22, 2026
Contributor
dlasecki-box
left a comment
dlasecki-box
left a comment
There was a problem hiding this comment.
LGTM - let's include some video (not exposing any sensitive data) to show what scenarios are covered as part of this PR
dlasecki-box
previously approved these changes
Apr 23, 2026
jpan-box
reviewed
Apr 23, 2026
jpan-box
reviewed
Apr 23, 2026
jpan-box
reviewed
Apr 23, 2026
Collaborator
|
in this review, i checked to ensure there aren't regressions with non-metadata preview and sidebar element use cases, as well as customer (non-box) use cases. all the features seem sufficiently gated and will only be triggered if a consumer explicitly asks for them. approving from buie, structurally this pr seems appropriate. left minor comments on some naming. however i'm not completely familiar with the business logic for the metadata editor, confidence score etc so relying on your team's judgement for that. thank you for the work! |
jpan-box
previously approved these changes
Apr 23, 2026
jpan-box
approved these changes
Apr 23, 2026
dlasecki-box
approved these changes
Apr 23, 2026
Contributor
Merge Queue Status
This pull request spent 54 minutes 21 seconds in the queue, including 54 minutes 7 seconds running CI. Required conditions to merge
|
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
4 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
warning_modal.mov
Summary by CodeRabbit
New Features
Bug Fixes
Tests