Zoom in monitor area under a mouse - #39
Conversation
|
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:
📝 WalkthroughWalkthroughThe program monitor now supports cursor-centered zooming, a Fit reset control, and panning with middle mouse or Alt-drag. Four reel timeline backup configurations are also added with media, layered clips, effects, audio, and markers. ChangesProgram Monitor View Controls
Reel Timeline Backup Configurations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant monitorScroll
participant state
participant preview
participant safeOverlay
User->>monitorScroll: Wheel over preview
monitorScroll->>state: Update viewZoom
state->>preview: Apply rendered size and scroll offsets
preview->>safeOverlay: Refresh overlay position
User->>monitorScroll: Middle mouse or Alt-drag
monitorScroll->>preview: Update native scroll offsets
preview->>safeOverlay: Refresh overlay position
User->>monitorScroll: Click Fit
monitorScroll->>state: Reset viewZoom to 1
state->>preview: Restore fit size
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
index.html (1)
65-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win"Reset to 100%" label/tooltip mismatches actual "fit stage" behavior.
state.viewZoom = 1is documented in app.js as"program-monitor display zoom (1 = fit stage)", i.e., it's a fit-to-stage baseline, not a literal native-pixel 100% zoom. Labeling the button "Reset to 100%" (and tooltip "Reset monitor zoom to fit (100%)") conflates these two concepts and may confuse users who expect true 1:1 pixel scale.✏️ Suggested wording
- <button class="btn tiny hidden" id="btnZoom100" title="Reset monitor zoom to fit (100%)">...Reset to 100%</button> + <button class="btn tiny hidden" id="btnZoom100" title="Reset monitor zoom to fit">...Reset zoom</button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.html` around lines 65 - 69, Update the Program Monitor reset button identified by btnZoom100 so its visible label and title describe restoring the fit-to-stage baseline rather than “100%” or native-pixel zoom; keep the existing reset behavior unchanged.app.js (2)
4430-4457: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo bounds on
state.viewPan.Neither the wheel-zoom pan adjustment nor the drag-pan
pointermovehandler clampsstate.viewPan, so a user can drag the zoomed canvas entirely out of the visible stage. Recovery only via the "Reset to 100%" button, which is a usable fallback but still a rough edge.🔧 Suggested clamp using existing bounding-rect data
function applyMonitorView() { const z = state.viewZoom, pan = state.viewPan; const zoomed = z > 1.001; if (!zoomed) { state.viewZoom = 1; state.viewPan = { x: 0, y: 0 }; els.preview.style.transform = ""; } else { + const stage = els.monitorStage.getBoundingClientRect(); + const maxX = (stage.width * (z - 1)) / 2; + const maxY = (stage.height * (z - 1)) / 2; + pan.x = clamp(pan.x, -maxX, maxX); + pan.y = clamp(pan.y, -maxY, maxY); els.preview.style.transform = `translate(${pan.x}px, ${pan.y}px) scale(${z})`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app.js` around lines 4430 - 4457, Clamp state.viewPan to valid bounds after both the wheel-zoom adjustment and drag-pan updates so the zoomed canvas cannot be moved entirely outside the visible stage. Reuse the existing monitor stage and content bounding-rect data where available, and apply the clamp before applyMonitorView() in the pointermove path and before storing the zoom-adjusted pan.
4398-4457: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
updateSafeOverlay()forces layout on every wheel tick and pan pointermove.
applyMonitorView()callsupdateSafeOverlay()(twogetBoundingClientRect()reads) right after mutatingels.preview.style.transform, and this runs on everywheelevent and everypointermovewhile panning (whenstate.guidesis on). That's a write-then-read cycle repeated at high frequency, forcing synchronous layout each time and risking visible jank during zoom/pan.⚡ Suggested throttling via requestAnimationFrame
+let viewRafPending = false; +function scheduleApplyMonitorView() { + if (viewRafPending) return; + viewRafPending = true; + requestAnimationFrame(() => { viewRafPending = false; applyMonitorView(); }); +}Then call
scheduleApplyMonitorView()from thewheelandpointermovehandlers instead ofapplyMonitorView()directly (transform mutation itself can stay immediate if desired; the goal is to coalesce theupdateSafeOverlayreflow to once per frame).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app.js` around lines 4398 - 4457, Throttle monitor view updates with requestAnimationFrame: add a scheduling helper around applyMonitorView that coalesces repeated calls into one frame, and use it from the wheel and pointermove handlers instead of calling applyMonitorView directly. Preserve immediate reset behavior for resetMonitorView and ensure updateSafeOverlay runs at most once per animation frame during zooming and panning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app.js`:
- Around line 4430-4457: Clamp state.viewPan to valid bounds after both the
wheel-zoom adjustment and drag-pan updates so the zoomed canvas cannot be moved
entirely outside the visible stage. Reuse the existing monitor stage and content
bounding-rect data where available, and apply the clamp before
applyMonitorView() in the pointermove path and before storing the zoom-adjusted
pan.
- Around line 4398-4457: Throttle monitor view updates with
requestAnimationFrame: add a scheduling helper around applyMonitorView that
coalesces repeated calls into one frame, and use it from the wheel and
pointermove handlers instead of calling applyMonitorView directly. Preserve
immediate reset behavior for resetMonitorView and ensure updateSafeOverlay runs
at most once per animation frame during zooming and panning.
In `@index.html`:
- Around line 65-69: Update the Program Monitor reset button identified by
btnZoom100 so its visible label and title describe restoring the fit-to-stage
baseline rather than “100%” or native-pixel zoom; keep the existing reset
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3637d4de-0f6f-4d48-ada1-f85ff1551fba
📒 Files selected for processing (3)
app.jsindex.htmlstyle.css
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.js`:
- Around line 4456-4474: The monitor update paths currently bypass the
coalescing helper. Replace direct applyMonitorView() calls in frequent monitor
zoom/pan update handlers and add scheduleMonitorView() after the scroll
handler’s updateSafeOverlay() call, while preserving resetMonitorView()’s
immediate applyMonitorView() behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d9f153c-4c59-480a-a4a0-c0f6c0d37c9c
📒 Files selected for processing (4)
README.mdapp.jsindex.htmlstyle.css
# Conflicts: # app.js
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 (2)
app.js (2)
1221-1221: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRemove duplicate module-scope declarations.
Each repeated
let/constcausesapp.jsto fail parsing before any monitor code can run.
app.js#L1221-L1221: retain onelet replaceFileInput = null, replaceFileTargetId = null;.app.js#L1880-L1880: retain onelet rulerWorker = null, rulerWorkerTried = false;.app.js#L2933-L2933: retain oneconst METER_SEG_W = 8, METER_SEG_H = 12, METER_SEG_GAP = 2;.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app.js` at line 1221, Remove the duplicate module-scope declarations in app.js: retain only one declaration of replaceFileInput and replaceFileTargetId at lines 1221-1221, one declaration of rulerWorker and rulerWorkerTried at lines 1880-1880, and one declaration of METER_SEG_W, METER_SEG_H, and METER_SEG_GAP at lines 2933-2933; eliminate any repeated let or const declarations while preserving the retained variables and initial values.
2878-2888: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winIsolate channels beyond left/right.
connectChannelIsolated()only isolates left and right, so clips withaudioChannel >= 2are routed through the full source mix instead of only their assigned channel. Split and route every non-negative channel index inconnectChannelIsolated()before the otherwise-connected final merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app.js` around lines 2878 - 2888, Update connectChannelIsolated() to isolate every non-negative channel index, not only channels 0 and 1. Create a splitter sized for the requested channel, route that channel into the graph, and connect the graph output to the matching merger input before returning the isolated output; retain the direct src-to-g path only for negative channel values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app.js`:
- Line 1221: Remove the duplicate module-scope declarations in app.js: retain
only one declaration of replaceFileInput and replaceFileTargetId at lines
1221-1221, one declaration of rulerWorker and rulerWorkerTried at lines
1880-1880, and one declaration of METER_SEG_W, METER_SEG_H, and METER_SEG_GAP at
lines 2933-2933; eliminate any repeated let or const declarations while
preserving the retained variables and initial values.
- Around line 2878-2888: Update connectChannelIsolated() to isolate every
non-negative channel index, not only channels 0 and 1. Create a splitter sized
for the requested channel, route that channel into the graph, and connect the
graph output to the matching merger input before returning the isolated output;
retain the direct src-to-g path only for negative channel values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f55f7db-6157-4a35-8eee-fad689392f1c
📒 Files selected for processing (8)
README.mdapp.jsindex.htmlproject-kesarix-reel2-backup-1783969307.jsonproject-porsche-backup-rev304.jsonproject-porsche-blackout-backup.jsonproject.backup-candle-reel.jsonstyle.css
🚧 Files skipped from review as they are similar to previous changes (3)
- index.html
- README.md
- style.css
# Conflicts: # app.js # style.css
2 participants
What does this PR do?
Sometimes a closer look is profitable. When hovering a mouse over a monitor, mousewheel action allows to zoom in the area under the cursor. There is also button to reset zoom to 100%.
Type of change
How was it verified?
node --check server.js && node --check app.js && node --check mcp-server.jspassesCLAUDE.md/README.mdif the schema, props, or API changedChecklist
Summary by CodeRabbit