chore(release): cherry-pick remaining RC-window bugfixes for 1.7.0 rc.3 - #128
Merged
Merged
Conversation
… cap Two bugs found during the 1.7.0 RC end-to-end pass: 1. New text annotations were created with the literal string "Enter text..." baked into their actual content, instead of starting empty with a real placeholder. The properties panel's textarea already had a proper `placeholder` attribute wired up, but since the field's *value* was never actually empty, the placeholder never showed and clicking in to type appended after the baked-in text instead of replacing it. Fixed by defaulting new text annotations' content to "" (both on creation and when switching an existing annotation's type to "text"), matching the pattern already used for "image"/"figure"/"blur" types. 2. After Save Project -> Load Project, the editor's scrubber/seek range was capped at the last timeline element's end time instead of the true recording duration. Root cause: `applyLoadedProject` sets `duration` to an inferred placeholder value (max of all region endMs) as a provisional guess before the real video metadata loads, expecting `VideoPlayback`'s `syncResolvedDuration` to correct it once the video element reports its real duration. But `syncResolvedDuration` skips calling `onDurationChange` whenever the resolved value matches `lastResolvedDurationRef` -- a memoization guard against redundant updates that has no way to know `applyLoadedProject` just set `duration` to something else. If the real duration happened to match what the ref already remembered from before the reload (the common case, since it's the same video file), the correction never fired and the inferred placeholder stuck. Fixed by exposing a `resetDurationResolution()` method on `VideoPlaybackRef` that clears the memoization guard, called from `applyLoadedProject` right when it sets the placeholder duration -- forcing the next metadata load to always re-sync regardless of what the guard last saw. Verified via computer-use against a real dev build: - New annotation's text field now shows a genuine empty value (real greyed placeholder, timeline label reads "Empty text"); typing replaces cleanly with no leftover baked-in text. - Loading a saved project with a trim region correctly shows the full original duration (1:10) and scrubs all the way to the end, instead of being capped at the last element's end time (previously 0:47). Full suite: 53 files / 423 tests pass. tsc --noEmit and biome check clean.
Two findings on the original fix: 1. Added regression tests. Extracted the "create empty text annotation" and "resolve content when converting to text" logic out of VideoEditor's inline handlers into createTextAnnotationRegion()/ resolveTextAnnotationContent() in types.ts, so both paths are directly unit-testable without rendering the whole (3000+ line) VideoEditor component. 2. resetDurationResolution() only cleared the memoization guard and relied on a future `loadedmetadata` event to re-sync -- which never fires when reloading a project referencing the same video file, since the <video> element's src string doesn't change. Hardened it to also immediately attempt resolution if the video is already loaded. This alone wasn't enough, though: it was being called *before* the placeholder `setDuration(inferredDurationMs...)` in applyLoadedProject, so its correction was immediately clobbered by that same-tick placeholder assignment winning the state-update race. Moved the call to run after all the state setters, so it's the one that wins. Verified via computer-use: loaded the same saved project three times in a row from a fresh app launch (the exact scenario the ordering bug hid in -- same video file, so `loadedmetadata` never refires). All three loads now correctly show 1:10, not just the first one. Full suite: 54 files / 426 tests pass (3 new). tsc --noEmit and biome check clean.
Fixes #100: dragging the HUD by its grip handle would drift away from the cursor rather than tracking it 1:1. Root cause: two independent IPC channels can both reposition the HUD BrowserWindow. Pointer drags send incremental deltas via hud-overlay-move-by; separately, a ResizeObserver watching the HUD's content calls hud-overlay-set-size whenever the observed content size changes, which recomputes the window's bounds from a bottom-centre anchor using the window's *current* bounds at the time of the call. If a resize observation fires mid-drag (even a spurious one from transient reflow), its anchor recompute races the drag's own incremental repositioning, and the two compound into a net offset — read by users as the HUD "drifting away". Fix: freeze content-size measurement while a drag is in progress (isDraggingHudRef), so hud-overlay-set-size cannot fire mid-drag. The content size is re-measured once via measureHudSize() right after the drag ends, so a real size change (e.g. from a toggle that was clicked mid-drag) still gets picked up promptly. Verified via computer-use against a real dev build: multiple multi-step drags (7-9 incremental pointer moves each, matching how a real drag event stream looks) all track the cursor to within a few px, both immediately on release and after a 2s settle -- no drift, no post-drop jump.
…ssion Per CodeRabbit review on #125: covers the fix's actual behavior end-to-end through the real drag handlers rather than testing isDraggingHudRef directly. Adds data-testid="hud-drag-handle" (the drag grip had no selector) and a test that: fires a real pointerdown on the handle, triggers a ResizeObserver callback mid-drag and asserts setHudOverlaySize is NOT called (the race this PR fixes), then fires pointermove/pointerup and asserts a measurement DOES happen once released -- so a real size change made mid-drag still gets picked up promptly. Full suite: 53 files / 424 tests pass. tsc --noEmit and biome check clean.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second round of cherry-picks onto
release/v1.7.0, per the release-branch-freeze contract in.harness/docs/git-workflow.md§ Release branches. These landed onmainafter the first cherry-pick (#124) and are still bugfixes discovered during RC testing, not features.docs/testing/rc-e2e-checklist.md): annotation placeholder text baked into content, and post-reload duration capped at the last timeline element's end time (+ the CodeRabbit-requested fixes/tests for both)Docs-only commits from the same window (the checklist doc itself) are intentionally not included — they don't affect the shipped build.
Verification
On the resulting tree:
tsc --noEmit— cleanbiome check— cleanwgc-capture.exerebuilds cleanly (CMake/Ninja, VS 18 Insiders)Once merged,
release/v1.7.0has every bugfix found during this RC window. Ready for anrc.3cut for final testing before promotion to stable.