fix(content-preview): keep compare open when clicking compared-pane annotation - #4842
zhirongwang wants to merge 1 commit into
Conversation
…nnotation Clicking an annotation on the compared (older-version) pane rewrites the activity path's fileVersionId and fires onVersionChange; SidebarPanels also emits onVersionChange(null) when leaving the versions route. A comparing ContentPreview forwarded both to the host, which unmounted the compared pane. - Tag annotation-driven version changes with origin: 'annotation' in withSidebarAnnotations and SidebarPanels - ContentPreview.onVersionChange suppresses forwarding to the host when isComparing and origin is 'annotation', so compare stays open while the sidebar switches to the annotation thread - Compared pane inherits host showAnnotations/boxAnnotations so annotations render on the compared version, with create/discoverability off (PREVIEW-1818) PREVIEW-1895 Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe change tags annotation-driven version updates, propagates the tag through sidebar navigation, and prevents comparison mode from forwarding those updates to the host. The compared pane now inherits annotation visibility settings while disabling annotation creation and discoverability. ChangesAnnotation-aware version comparison
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Annotator
participant SidebarPanels
participant ContentPreview
participant Host
Annotator->>SidebarPanels: emit annotation selection
SidebarPanels->>SidebarPanels: navigate to annotation route
SidebarPanels->>ContentPreview: pass annotation-originated version change
ContentPreview->>ContentPreview: detect comparison mode
ContentPreview-->>Host: suppress version change callback
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The comparison flow retains the selected annotation thread without forwarding annotation-driven version changes to the host. No actionable current-head risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. I hop through routes with tags in tow Comment |
greg-in-a-box
left a comment
greg-in-a-box
left a comment
There was a problem hiding this comment.
Review summary
Reviewed the PREVIEW-1895 fix end-to-end: annotation-driven onVersionChange tagging (withSidebarAnnotations router + router-disabled paths, SidebarPanels versions→annotations exit), ContentPreview.onVersionChange suppression gated on isComparing && origin === 'annotation', and compared-pane annotation inheritance with create/discoverability explicitly off.
Verdict: approve
This is a clean, well-scoped fix for the compare teardown bug.
What looks correct
- Suppression is narrow: versions-sidebar clicks (no
origin) still notify the host while comparing — covered by the existing unit test and left intact. - Both teardown sources are tagged:
updateActiveVersion(version object) and theSidebarPanelsnull reset when leaving/versionsfor an annotations path. additionalVersionInfodefaults to{}inContentPreview.onVersionChange, so the.origincheck is safe.- Non-comparing path still forwards annotation-origin events to the host (including the null reset).
- Compared pane: inheriting
showAnnotations/boxAnnotationswhile forcing create + discoverability off matches PREVIEW-1818 and avoids opening a composer on text select. - Test coverage is strong: unit cases for suppress/forward matrix,
SidebarPanelspath tagging table, compared-pane prop inheritance, plus the RTL integration that asserts everyonVersionChangefrom a compared-pane annotation click carriesorigin: 'annotation'.
Nits (non-blocking)
SIDEBAR_PATH_ANNOTATIONS(/:sidebar/annotations/...) is slightly looser than the real activity route (/activity/.../annotations/...). Harmless in practice; tightening to activity-only would match production routes more closely.AdditionalVersionInfo.updateVersionToCurrentremains required in the Flow type whileSidebarPanelsstill passes{ origin: 'annotation' }without it — pre-existing shape mismatch, not introduced here.
No material correctness, regression, or missing-test issues. LGTM.
greg-in-a-box
left a comment
greg-in-a-box
left a comment
There was a problem hiding this comment.
Verdict: Approve
Solid fix for PREVIEW-1895. The origin: 'annotation' tagging cleanly separates annotation-driven path changes from versions-sidebar clicks, and suppressing those in ContentPreview.onVersionChange only while isComparing is the right place to keep side-by-side compare mounted.
What looks good
- Root cause and fix match: both the
withSidebarAnnotationsversion report and theSidebarPanelsversions-route exit are tagged, which covers the two events that were tearing down compare. - Compared pane inheriting
showAnnotations/boxAnnotationsvia{...rest}while explicitly disabling create/discoverability/drawing create matches PREVIEW-1818 intent and is covered by a unit test. - Integration test (
SidebarPanels.annotationCompare.rtl.test.js) actually exercises the annotator → sidebar path → taggedonVersionChangechain, not just the leaf helpers. - Existing “comparing still notifies host when origin is absent” behavior is preserved and still tested.
Nits (non-blocking)
AdditionalVersionInfo.updateVersionToCurrentis still required in Flow, but the versions-route exit path has always calledonVersionChange(null)/ nowonVersionChange(null, { origin: 'annotation' })without it. Worth making that field optional when you next touch the type.SidebarPanelsstill only tags the location-based exit (norouterDisabled/ internal-nav equivalent). Same pre-existing shape as before; fine if compare + annotations always use the router path today.
LGTM.
greg-in-a-box
left a comment
greg-in-a-box
left a comment
There was a problem hiding this comment.
Review
Nice, targeted fix for PREVIEW-1895. Tagging annotation-driven onVersionChange with origin: 'annotation' and suppressing host forwarding only while isComparing is a clean way to keep side-by-side compare mounted without changing versions-sidebar behavior.
What looks good
withSidebarAnnotationsandSidebarPanelsboth tag the paths that were tearing down compare (active-version update + versions-route exit into an annotations path).- Compared pane inheriting
showAnnotations/boxAnnotationswhile forcing create/discoverability off matches PREVIEW-1818 intent. - Unit coverage for the suppress/forward matrix and the new RTL integration test that asserts every resulting
onVersionChangecarriesorigin: 'annotation'are convincing.
Nits (non-blocking)
SIDEBAR_PATH_ANNOTATIONS(/:sidebar/annotations/...) is a looser pattern than the activity Route’s/:activeFeedEntryType(annotations)/.... It matches today’s paths, but tying thecomponentDidUpdatecheck to the same helper/constant as the Route (orgetAnnotationsMatchPath) would reduce drift risk.- Confirm in a quick compare-mode pass that
showAnnotationsDrawing={false}does not hide existing drawing annotations on the compared version (docs call outshowAnnotationsDrawingCreatefor create; if drawing view is gated by the non-Create flag, prefer leaving view on and only disabling create/discoverability).
Approved from my side assuming CI stays green.
2 participants
Summary
In side-by-side version compare, clicking an annotation on the compared (older-version) pane tore down the compare view — the compared pane unmounted and the annotation disappeared — and the Activity sidebar never switched from version history to the annotation thread.
Root cause
Clicking an annotation on the compared pane emits
annotations_active_changeon the shared annotator event manager.withSidebarAnnotations.updateActiveVersionthen rewrites the activity path'sfileVersionIdand callsonVersionChange;SidebarPanelsadditionally emitsonVersionChange(null)when leaving the versions route. A comparingContentPreviewforwarded both to the host, which interpreted them as the user leaving version compare and unmounted the compared pane.Changes
withSidebarAnnotations: tag annotation-drivenonVersionChangecalls withorigin: 'annotation'(both router and router-disabled paths)SidebarPanels: when leaving the versions route for an annotations path, emitonVersionChange(null, { origin: 'annotation' })ContentPreview.onVersionChange: whenisComparingand the change carriesorigin: 'annotation', don't forward to the host — compare stays open while the sidebar switches to the thread. Versions-sidebar clicks (no origin) still notify the host as before.showAnnotations/boxAnnotationsso annotations render on the compared version, with create, drawing, and discoverability explicitly disabled (PREVIEW-1818)flowTypes: addedorigin?: 'annotation'toAdditionalVersionInfoTests
SidebarPanels.annotationCompare.rtl.test.js: a compared-pane annotation click pushes the annotation thread path and every resultingonVersionChangecarriesorigin: 'annotation'ContentPreview.test.js(suppression only when comparing + annotation origin) andSidebarPanels.test.js(versions-route exit tagging)--changedSince)Related: PREVIEW-1876 (annotator cross-talk fix in box-annotations), PREVIEW-1818 (second annotator in the compared pane)
Summary by CodeRabbit