Preserve scroll position when toggling markdown Rendered/Raw views - #13967
Preserve scroll position when toggling markdown Rendered/Raw views#13967slash9494 wants to merge 4 commits into
Conversation
14d87e2 to
2ac5964
Compare
Toggling a markdown file between Rendered and Raw replaces the entire pane (FileNotebookView <-> CodeView), so the new pane always started at the top of the document. Capture the outgoing pane's vertical scroll as a fraction of its scrollable range, carry it on the pane-replacement events, and re-apply it after the new pane's content is laid out. A fraction is used instead of a char-offset snapshot because the two modes render different documents. Application is deferred through the render model's element-update cycle (version- and viewport-gated), since a freshly created pane has no laid-out content yet and applying immediately would clamp the scroll to the top. Fixes the scroll-preservation half of warpdotdev#7897; search/replace is out of scope.
2ac5964 to
2db7358
Compare
|
Every PR must be linked to a same-repo issue before Oz can review it. This PR is linked to #14062, but no linked issue is marked See the contribution guidelines for the full readiness model. Powered by Oz |
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
Next step: open or find a same-repo issue describing this change, then link it to this PR by adding Closes #123 to the PR description (or using the "Development" sidebar on GitHub). A maintainer will mark the issue ready-to-implement when it is ready. Once it is marked, comment /oz-review to re-trigger review.
See the contribution guidelines for the full readiness model.
Powered by Oz
|
/oz-review |
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
This PR is linked to #14062, but no linked issue is marked ready-to-implement yet. Only repository maintainers apply that label, so please wait for a maintainer to mark the issue. Once it is marked, push a new commit or comment /oz-review to re-trigger review.
See the contribution guidelines for the full readiness model.
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR preserves markdown viewer scroll position when switching between Rendered and Raw modes by carrying a scroll fraction through pane replacement and applying it after the incoming editor has laid out. The implementation updates both code-editor and rendered-notebook paths, includes focused viewport math coverage, and the PR description includes before/after screenshots for the user-facing behavior.
Concerns
No blocking correctness, security, or spec-alignment concerns found. No approved or repository spec context was available for this PR.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Hi @bnavetta — gentle nudge on this one. Oz approved it (0 concerns) and requested your review on 07-22, and the linked issue #14062 is marked |
|
@oss-maintainers — flagging this one per the CONTRIBUTING escalation note. Oz approved this PR on 07-22 with 0 concerns and requested human review from I've just merged the latest Could someone on the team take a look, or route it to another reviewer if |
bnavetta
left a comment
There was a problem hiding this comment.
Sorry for the delay! One small question, but the code otherwise looks good
| } | ||
|
|
||
| /// The current vertical scroll fraction of the active tab's editor, in `0..=1`. | ||
| #[cfg(feature = "local_fs")] |
There was a problem hiding this comment.
Does this need to be gated on local_fs? The functionality itself isn’t FS-specific; if this is dead code without a filesystem then we can add a conditional expect(dead_code) attribute
There was a problem hiding this comment.
Good catch — you're right that it isn't FS-specific.
Digging in, code/view.rs turned out to be a special case: code/mod.rs path-swaps it to wasm.rs for wasm, so local_fs is always enabled wherever this file compiles and the gate never actually applied. I dropped it rather than swapping the attribute.
notebooks/file/mod.rs does compile for wasm, so its two helpers use the conditional expect(dead_code) you suggested, matching terminal/recorder.rs.
While there I also narrowed set_pending_scroll_fraction to pub(crate) since pane_group is the only caller — that turned out to surface a real dead_code error on wasm that the pub had been masking, now covered by the same attribute.
Verified with cargo clippy -p warp -- -D warnings on both the native target and wasm32-unknown-unknown, plus the viewport tests. Latest master is merged in as well.
…-toggle-scroll # Conflicts: # app/src/notebooks/file/mod.rs
…hem out These helpers only read editor scroll state, so `local_fs` was standing in for "this build has no caller" rather than for an actual filesystem dependency. `code/view.rs` is path-swapped to `wasm.rs` for wasm, so `local_fs` is always enabled wherever it compiles and the gate never applied — drop it. The notebook view does compile for wasm, so mark its helpers with a conditional `expect(dead_code)`, matching `terminal/recorder.rs`. Also narrow `set_pending_scroll_fraction` to `pub(crate)`; it is only called from `pane_group`. Verified with `cargo clippy -p warp -- -D warnings` on both the native target and `wasm32-unknown-unknown`, plus the viewport tests.
22d5b72 to
318299f
Compare
Description
Toggling a markdown file between Rendered and Raw view modes always reset the view to the top of the document, because the toggle replaces the entire pane (
FileNotebookView⇄CodeView) and the new pane starts with a fresh scroll state.This PR preserves the reading position across the toggle:
scroll_top / max_scroll, clamped0..=1).PaneEvent::ReplaceWithCodePane/ReplaceWithFilePane(asOption<OrderedFloat<f32>>soPaneEventkeepsEq).ScrollPosition::Fractionvariant on the existing pending-scroll (ScrollTrigger) path.LayoutAction::ScrollToFraction, stored as a pending fraction inRenderStateand applied inapply_element_update, gated on the viewport having been laid out (viewport.height() > 0) and the reset content's buffer version. Applying immediately would read a stale (~0) content height on a freshly created pane and clamp to the top.A fraction is used instead of a char-offset snapshot (
ScrollPositionSnapshot) because the two modes render different documents, so a char offset cannot map between them. The landing position is therefore proportional rather than line-exact; documents whose blocks resize late (e.g. large tables) may land slightly off, which seems like an acceptable trade-off for the common case.All pre-existing callers of the pane constructors pass
Noneand keep the current scroll-to-top behavior.Linked Issue
Closes #14062
#14062 is the focused, single-scope issue for scroll preservation (following the repo's
Markdown viewer: …issue convention). It was split out of #7897, which bundles two separate asks — scroll preservation (this PR) and find/replace in the Markdown viewer (still tracked on #7897).ready-to-specorready-to-implement. — Note: Preserve visual context when switching Markdown view modes + search/replace support #7897 currently has neither label; happy to go through the spec process first if the team prefers.Testing
Added a unit test for the fraction ⇄ scroll-top math (
crates/editor/src/render/model/viewport_tests.rs::test_scroll_fraction): midpoint mapping, out-of-range clamping at both ends, and content-fits-in-viewport (no scrollable range → 0).cargo nextest-compatible: fullwarp_editorsuite passes locally;cargo check -p warpclean;./script/format --checkclean.No UI integration test: the integration framework cannot simulate clicking the Rendered/Raw toggle (noted in
crates/integration/src/test/notebooks.rs).I have manually tested my changes locally with
./script/runManually verified end-to-end in a local build, both directions:
scroll_topmatchesfraction × (content_height − viewport_height)exactly in both directions, and that panes opened without a toggle (file tree, session restore) still start at the top.Screenshots / Videos
Before (master build): Raw scrolled to the bottom of the document (§37–40 + table) → toggling to Rendered resets to the top:
After (this branch): the same action lands at the equivalent position (§40 + table still in view):
Agent Mode
CHANGELOG-IMPROVEMENT: The markdown viewer now preserves your scroll position when switching between Rendered and Raw view modes.