Route compared-pane annotation scroll and retry sidebar thread scroll - #4849
zhirongwang wants to merge 2 commits 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>
… sidebar scroll While comparing, send annotations on another version to that pane instead of switching this one. Retry Activity Feed scroll until the active thread is in the feed so a version switch does not miss it.
|
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 (9)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe pull request adds annotation-origin tracking for compared file versions, forwards annotation selections between comparison panes, adjusts compared-pane annotation options, and keeps activity-feed scrolling pending until the active item is rendered. ChangesAnnotation comparison flow
Activity feed scrolling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant AnnotationEventManager
participant SidebarPanels
participant ContentPreviewWithComparison
participant ComparedContentPreview
AnnotationEventManager->>SidebarPanels: emit annotation selection
SidebarPanels->>ContentPreviewWithComparison: send annotation-origin version change
ContentPreviewWithComparison->>ComparedContentPreview: pass annotation scroll request
ComparedContentPreview->>ContentPreviewWithComparison: forward cross-version annotation
ContentPreviewWithComparison->>ContentPreview: scroll annotation in matching pane
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The annotation routing and pending feed-scroll changes include targeted coverage, with no actionable merge risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ast-grep (0.45.3)src/elements/content-preview/__tests__/ContentPreview.test.jsast-grep timed out on this file 🔧 Biome (2.5.11)src/elements/common/flowTypes.jsFile contains syntax errors that prevent linting: Line 18: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 20: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 27: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 27: Expected a statement but instead found '=> Promise<?string>'.; Line 29: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 29: Expected a statement but instead found '=> Promise'.; Line 31: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 36: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 37: Expected a statement but instead found ', src/elements/content-preview/ContentPreview.jsFile contains syntax errors that prevent linting: Line 399: ... [truncated 18952 characters] ... stead found ')'.; Line 1832: Illegal return statement outside of a function; Line 1834: Unexpected token. Did you mean src/elements/content-sidebar/SidebarPanels.jsFile contains syntax errors that prevent linting: Line 148: return types can only be used in TypeScript files; Line 38: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 39: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 40: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 41: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 42: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 43: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 44: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 45: 'import type' are a TypeScript only feature. Convert your file to a ... [truncated 2726 characters] ... the syntax.; Line 259: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 260: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 302: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 330: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 10: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 35: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 150: return types can only be used in TypeScript files
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. A rabbit hops where annotations gleam Comment |
greg-in-a-box
left a comment
greg-in-a-box
left a comment
There was a problem hiding this comment.
Verdict: request changes
Follow-up to #4842 looks directionally right: route compare-mode annotation selects by version (onComparedAnnotationSelect + annotationScrollRequest), keep host onVersionChange suppression for annotation returns to current, and retry Activity Feed scroll until the active thread is actually present. Coverage for the new scroll matrix and the feed retry is solid.
Two items before merge:
1. PR title fails semantic lint (CI)
lint_pull_request failed: title Route compared-pane annotation scroll and retry sidebar thread scroll has no conventional-commit type. Rename to something like:
fix(content-preview): route compared-pane annotation scroll and retry sidebar thread scroll
2. Compare-mode same-pane scroll drops deferScrollToOnload
In handleAnnotationSelect, the isComparing branch for the pane’s own version always calls emitScrollToAnnotation and returns, ignoring deferScrollToOnload (and the frame/dynamicOnPreviewLoadAction path used below). The compared-pane forward path does pass deferScrollToOnload through annotationScrollRequest, so the two panes are inconsistent. If the current pane’s viewer is not ready yet (common right after opening compare), a same-version sidebar/annotator select can no-op. Please reuse the same defer / on-load scroll path as the non-comparing branch for the same-version comparing case.
Notes (non-blocking)
- Stacked on #4842 — merge that first as the description says; this branch already includes it.
ActivityFeedre-arming on anyfeedItemsreference change is reasonable for version switches givenundefinedmeans loading; just watch that parents don’t recreate the array every render while an entry is active (would keep re-scrolling).- Same PREVIEW-1818 nit as on #4842: confirm
showAnnotationsDrawing={false}doesn’t hide existing drawings on the compared version (create-only disable is the safer read of the docs).
Once the title is fixed and the comparing same-pane path honors defer-on-load, this should be ready to approve.
greg-in-a-box
left a comment
greg-in-a-box
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Follow-up to #4842 looks directionally right (route other-version annotation scrolls to the compared pane via a fresh request object; only suppress annotation-origin onVersionChange when returning to current while comparing). A few issues should be fixed before merge.
1. ActivityFeed re-arms scroll on every feedItems identity change (regression)
if (didLoadFeedItems || hasActiveFeedEntryIdChanged || prevFeedItems !== currFeedItems) {
this.hasPendingActiveScroll = true;
}
if (this.hasPendingActiveScroll) {
this.scrollToActiveFeedItemOrErrorMessage();
}After a successful scroll, hasPendingActiveScroll is cleared — but the next parent render that passes a new feedItems array reference (polling, reply refresh, realtime) sets it again and calls scrollIntoView again. That will yank the feed back to the active thread while the user is reading.
Fix: Only set hasPendingActiveScroll = true on didLoadFeedItems || hasActiveFeedEntryIdChanged. Keep calling scrollToActiveFeedItemOrErrorMessage() when pending and feedItems changed (so a later feed update can complete a pending scroll), but do not re-arm after a successful scroll.
2. Stale feed on version switch still hits the “missing item” error path
The new hasActiveFeedItem() guard only stays pending when the item is already in the current feed (or feedItems === undefined). On annotation version switches the common sequence is: activeFeedEntryId updates first while feedItems is still the previous version’s list (item absent → ref null) → resetFeedScroll() runs and clears pending → feed jumps to the bottom → later feed update re-arms and scrolls correctly.
That intermediate bottom-scroll fights the stated goal (“version switch no longer misses it on the first render”) and flashes the error path.
Fix: When the active id is absent from the current feed, stay pending until feedItems itself updates (then decide success vs true-missing). Only resetFeedScroll() after a feed update where the id is still missing — not on the id-change tick alone with a stale list. Please add a test for: id changes against a non-empty stale feed that does not contain the id → no resetFeedScroll; then new feed without the id → bottom scroll; or new feed with the id + ref → scrollIntoView.
3. Compared-pane annotationScrollRequest has no version guard / mount handling
annotationScrollRequest is only consumed in componentDidUpdate. When the host must change comparedVersion for an annotation on a different older version:
- The still-mounted compared pane can receive the request first (
isComparing={false}), sohandleAnnotationSelectruns the normal “other version” path (startAt/ version switch) on a pane that is supposed to be pinned topreviewVersion. - After remount (
key={comparedVersionId}), the same request is an initial prop, socomponentDidUpdatenever sees a change; scroll then depends entirely onfileOptions/activeAnnotationIddeep link.
Same-version re-select is covered well; version-follow + scroll is not.
Fix (either):
- Only
setAnnotationScrollRequestwhenannotation.file_version.id === comparedVersionId, and also handle the request incomponentDidMount(or re-set the request aftercomparedVersionchanges so didUpdate fires), or - In the consumer, ignore requests whose
file_version.id≠ this pane’sgetVersionToPreview()id (and ensure remount still scrolls via deep link or a remount-safe request).
A test that simulates “comparing A → select annotation on B” (request + version change) would lock this in.
4. PR title fails semantic lint (blocking CI)
lint_pull_request failed: title "Route compared-pane annotation scroll and retry sidebar thread scroll" has no conventional-commit type. Please rename to something like fix(content-preview): route compared-pane annotation scroll and retry sidebar thread scroll (matches the commit / #4842 style).
Nits (non-blocking): While comparing, same-version path always emitScrollToAnnotation and skips the frame/deferScrollToOnload branch used off-compare — fine for PDF pages; worth a quick check for video frame annotations on the current pane during compare.
CI note: lint_pull_request already red on the title; lint_test_build was still in progress at review time.
greg-in-a-box
left a comment
greg-in-a-box
left a comment
There was a problem hiding this comment.
Follow-up to #4842 looks directionally right — compared-pane routing via onComparedAnnotationSelect + request-object identity, origin: 'annotation' suppression while comparing, and ActivityFeed waiting for the active thread all match the stated goals. Tests cover the main compare/version-origin paths well.
Please address the items below before merge (CI title lint is already red).
| if (didLoadFeedItems || hasActiveFeedEntryIdChanged) { | ||
| // Switching file versions replaces the items, so the active entry can be missing from | ||
| // the feed at the moment its id changes. Re-arm on any feed change to catch it later. | ||
| if (didLoadFeedItems || hasActiveFeedEntryIdChanged || prevFeedItems !== currFeedItems) { |
There was a problem hiding this comment.
Bug / regression: prevFeedItems !== currFeedItems unconditionally re-arms hasPendingActiveScroll.
After a successful scroll, any new feedItems array identity (parent re-render, pagination, polling, reply refresh) will set the flag again and call scrollIntoView — fighting the user’s own scroll position while an active entry stays selected.
That inequality is load-bearing today because the “missing item” branch clears pending when the active id changes before the version’s feed has arrived (old feed still loaded → hasActiveFeedItem() false → resetFeedScroll + clear). The next feed identity change is what re-arms and eventually scrolls.
Please tighten both sides together, e.g.:
- Only set
hasPendingActiveScrollondidLoadFeedItems/hasActiveFeedEntryIdChanged(not on every feed identity change). - While already pending, retry on feed updates.
- Do not treat “active id not in current
feedItems” as terminal until the feed has actually reloaded for that selection (e.g. only run the missing/error fallback ondidLoadFeedItems, or stay pending across the first post-id-change feed that still lacks the item).
Also worth a test that a feedItems reference change after a successful scroll does not call scrollIntoView again.
| if (isOtherVersion) { | ||
| onComparedAnnotationSelect(annotation, deferScrollToOnload); | ||
| } else { | ||
| this.emitScrollToAnnotation(id, target); |
There was a problem hiding this comment.
Nit / incomplete path (non-blocking if compare+video is out of scope): while isComparing and the annotation is on this pane’s version, this always emitScrollToAnnotation and returns — skipping deferScrollToOnload and the frame/video scrollToFrameAnnotation wait used on the non-compare path below.
Previously compare bailed out entirely, so this is still an improvement for page annotations, but deferred/video scrolls can still miss. Prefer sharing the same post-routing scroll helper as the non-compare branch (or call into that logic after the version routing decision).
This branch has not been deployed
2 participants
Summary
Test plan
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes