Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved test, observer lifecycle, synchronization, and packaged-build issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prevents unwanted preview scrolling during refreshes while preserving appropriate editor-to-preview synchronization.
Changes:
- Preserves preview offsets and guards against stale renders.
- Limits synchronization during overlay mode.
- Adds regression coverage for refresh and scroll behavior.
File summaries
| File | Summary | Findings |
|---|---|---|
tests/view.hiddenSyntaxMode.test.ts |
Tests refresh positioning and stale renders. | Critical (3 votes): Module resets create fresh mocks, so the configured top-level mocks are not used and tests may fail. |
tests/scroll.test.ts |
Tests scroll synchronization rules. | No findings. |
src/view.ts |
Adds offset preservation and render-version checks. | Moderate (1 vote): Regenerate and commit the missing dist bundles. |
src/scroll.ts |
Filters unwanted overlay synchronization. | Moderate (1 vote): Restarting observation can leave stale listeners active. Moderate (1 vote): Selection comparison can mistake document changes for user navigation. |
Review details
Suppressed comments (3)
src/scroll.ts:12
- Clearing the pending timeout does not stop the listener installed by the previous
startObservingcall. After an observation restart, an event from the old source can still schedule its stale callback in thescrollfallback, andscrollendbrowsers retain both listeners; this can apply synchronization from a superseded observer. Store/remove the previous listener (or add an observer-generation guard) when restarting observation.
if (states.scrollUpdater !== undefined) {
clearTimeout(states.scrollUpdater);
}
src/scroll.ts:21
- Comparing selections alone does not distinguish user navigation from a document update: CodeMirror maps the current selection through inserted/deleted text, so an external edit before the caret can make
selectionChangedtrue. The next editor scroll event will then callsyncScrollProgresseven while the preview has theoverlayclass, reintroducing the automatic jump this change is intended to prevent. Track an explicit user-selection/navigation signal or suppress this path for doc-change transactions.
const selection = MarkEdit.editorView.state.selection;
const selectionChanged = !selection.eq(lastSelection);
if (!selectionChanged && Math.abs(sourcePane.scrollTop - states.lastSourceScrollTop) < 0.5) {
src/view.ts:201
- The committed
dist/markedit-preview.jsanddist/lite/markedit-preview.jsbundles do not contain the newrenderVersion, selection-change, or overlay-guard logic, so the packaged extension will continue using the old synchronization behavior. Please regenerate and commit both build outputs; the repository documentsyarn buildas the deployment build inREADME.md:9-13.
export async function renderHtmlPreview(syncScroll = currentViewMode() !== ViewMode.preview || !previewPane.hasChildNodes()) {
const renderVersion = ++states.renderVersion;
- Files reviewed: 4/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate source issues and missing generated bundles remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/view.ts:200
- The changed runtime sources are not accompanied by regenerated
dist/markedit-preview.jsanddist/lite/markedit-preview.js. These committed bundles are the artifacts built and deployed by this repository (README.md:9-13), so consumers loading them will retain the old synchronization behavior even if the source tests pass. Run the build and include both generated outputs.
export async function renderHtmlPreview(syncScroll = currentViewMode() !== ViewMode.preview || !previewPane.hasChildNodes()) {
- Files reviewed: 4/6 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Regenerate the shipped bundles and correct initial preview alignment for newly created editors.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/view.ts:202
- When a new editor is created,
main.ts:135callsrenderHtmlPreview()while the mode can still bepreviewand this pane still contains the previous document. The new default then evaluates tofalse, so the first render of the new document restores the old preview offset instead of aligning to the new editor; use an explicit sync for the editor-ready render or otherwise reset this heuristic for a new editor.
export async function renderHtmlPreview(syncScroll = currentViewMode() !== ViewMode.preview || !previewPane.hasChildNodes()) {
- Files reviewed: 4/6 changed files
- Comments generated: 1
- Review effort level: Lite
See MarkEdit-app/MarkEdit#1747.