copy paste region attributes with shortcut#33
Merged
Conversation
EtienneLescot
requested changes
Jun 25, 2026
Collaborator
There was a problem hiding this comment.
Ey @446f6e6e79 thanks for the PR!
Summary
Clean, well-tested feature. One real user-facing functional gap (blur paste), some consistency asks, and ~55 lines of shrink. Reviewing on request changes mainly for F2.
Functional issues
F1. Toast copy inconsistency. TimelineEditor.tsx:1131,1160,1189 already uses errors.cannotPlaceZoom/Trim/Speed (with description) for the same "no room here" scenario this PR invents regionClipboard.cannotPlace for. Reuse the existing keys (or factor one shared toast). Less translation churn, consistent UX.
F2. Paste silently ignores blur selection. handleCopySelected reads annotationRegions.find(r => r.id === selectedAnnotationId), so you CAN copy a blur's attributes. But handlePaste only checks selectedAnnotationId, never selectedBlurId. If a blur is selected and you Ctrl+V, the apply-to-selected branch is false and it falls through to "create new region at playhead" — a NEW annotation appears next to your still-selected blur. No feedback that selection was ignored. Either (a) also check selectedBlurId as a paste target for blurs, (b) toast "select an annotation or blur of the right kind", or (c) document.
F3. Duplicated "find free gap at playhead" logic — 5th copy. TimelineEditor.tsx:1112-1199 already implements "sort → next-region → gapToNext → overlap" 3 times (zoom/trim/speed). This PR adds a 4th and 5th copy in VideoEditor.tsx:1741 and :1769. Extract once: findFreeGapAt(regions, startPos, totalMs): { ok, gapMs } and use in all 5 sites.
F4. customScale clobbering. regionClipboard.ts:89 does customScale: attrs.customScale — if source had no customScale (preset-only), target's previous customScale is silently wiped. Probably intentional, but undocumented. Add a test or rename applyZoomAttributes → replaceZoomAttributes to flag destructive merge.
F5. Annotation paste may attach figureData to a non-figure target. applyAnnotationAttributes (regionClipboard.ts:111) does figureData: attrs.figureData ? {...} : region.figureData. Copy a figure, paste onto a text annotation → text now has figureData set. Valid type-wise, nonsensical in the UI. Guard on region.type === "figure" or document.
F6. Blur blurData not captured. AnnotationAttributes lacks blurData. Copy/paste a blur only transfers style. Acceptable for v1; follow-up note.
Test coverage gaps
- T1. Keydown handler dispatch (
editingTextskip, shortcut match,preventDefault). - T2. Paste when
currentTime > totalMs— clamps tototalMs, no test. - T3. Paste when
duration === 0— early return, no test. - T4. Paste preset-only zoom over customScale zoom — confirms F4.
- T5. Paste figure onto text — confirms F5.
UX
- Sonner doesn't dedupe — Ctrl+C 5× produces 5 toasts. Use
toast.success(msg, { id: "regionClipboard.copied" })to replace. - i18n: all 13 locales updated;
i18n:checkwill pass. Good.
Over-engineering / shrink
| Where | What | Net |
|---|---|---|
VideoEditor.tsx:1656 |
3 duplicated copy branches → one dispatch table | −10 |
VideoEditor.tsx:1741 & :1769 |
zoom/speed gap+overlap duplicated → helper (also de-dupes 3 sites in TimelineEditor.tsx) |
−8 here, more globally |
VideoEditor.tsx:1756-1757 |
dead depth/focus in zoom paste base (overwritten by applyZoomAttributes) |
−2 |
VideoEditor.tsx:1784 |
dead speed: DEFAULT_PLAYBACK_SPEED (overwritten) |
−1 |
VideoEditor.tsx:1703 |
pastedToast closure around 5 constant calls → inline |
−2 |
VideoEditor.tsx:314 |
regionClipboardRef → module-level let in regionClipboard.ts |
−3 |
regionClipboard.ts:22,32,39 |
kind discriminator on every attrs interface → drop, rely on CopiedRegion union |
−3 |
regionClipboard.ts:85 |
9-line applyZoomAttributes → ...omit(attrs,"kind") spread |
−5 |
VideoEditor.tsx:1847 |
isContentEditable arm inconsistent with TimelineEditor.tsx:1241 → lift helper or drop |
−3 |
ROADMAP.md |
Unrelated churn in this PR — rebase out | −17 |
Net ~55 lines removable.
Bottom line
Headline asks:
- Extract
findFreeGapAt— pays off across 5 sites (this PR + 3 existing). - Decide blur-paste semantics — close F2.
- Reuse
errors.cannotPlace*keys — consistency. - Rebase ROADMAP.md out — unrelated.
- Trim ~55 lines via the shrinks above.
Author
|
Got it, will work on it ASAP! |
Collaborator
|
@446f6e6e79 just to let you know, my review is an automated review made with https://github.com/DietrichGebert/ponytail This is why it can feel a bit nitpicky. In the future, those reviews will be better marked as automated, with better P0 → nitpicky ranking. |
Add copy/paste of region attributes (zoom/speed/annotation) with region placement helpers, and lift isTextEditingTarget into src/lib/shortcuts.ts so VideoEditor and TimelineEditor share one keyboard guard. Includes i18n strings for the new actions.
|
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 (15)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds region clipboard support for zoom, speed, annotation, and blur regions, plus shared placement checks, shortcut bindings, and localized editor and shortcut strings. ChangesTimeline region clipboard
Sequence Diagram(s)sequenceDiagram
participant User
participant VideoEditor as VideoEditor.tsx
participant Clipboard as regionClipboard.ts
participant Placement as regionPlacement.ts
User->>VideoEditor: Ctrl+C or Ctrl+V
VideoEditor->>VideoEditor: isTextEditingTarget(e.target)
alt copy selected region
VideoEditor->>Clipboard: extractZoomAttributes / extractSpeedAttributes / extractAnnotationAttributes
VideoEditor->>Clipboard: setCopiedRegion(...)
else paste region attributes
VideoEditor->>Clipboard: getCopiedRegion()
alt compatible selected region exists
VideoEditor->>Clipboard: replaceAnnotationAttributes(...)
else create new region
VideoEditor->>Placement: findFreeGapAt(...)
VideoEditor->>Clipboard: buildZoomRegion / buildSpeedRegion / buildPastedAnnotation(...)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
src/components/video-editor/VideoEditor.tsx (1)
1657-1693: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMinor: the unconditional
returninside the loop swallows the "nothing to copy" path on a truthy-but-unmatched id.If an
idis truthy but no matching region is found (stale selection), the loop returns without copying and without thenothingToCopytoast. Movingreturninside theif (region)block (andcontinue-ing otherwise) keeps the fallback toast reachable. Low impact, but tightens the edge case.🤖 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 `@src/components/video-editor/VideoEditor.tsx` around lines 1657 - 1693, The handleCopySelected callback returns from the first truthy copy target even when no matching region is found, which prevents the fallback “nothing to copy” toast on stale selections. Update the loop in handleCopySelected so it only returns after a successful match and copy for the selected region, and otherwise continues checking the remaining targets; keep the toast.info(t("regionClipboard.nothingToCopy")) path reachable when no region matches.src/components/video-editor/regionPlacement.test.ts (1)
32-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a case for placement exactly at a region's start.
The suite covers adjacency at a region's end but not landing on a region's
startMs(which the implementation rejects). Adding it documents the boundary and guards against regressions.🤖 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 `@src/components/video-editor/regionPlacement.test.ts` around lines 32 - 37, The placement tests in findFreeGapAt currently cover adjacency at a region’s end but do not verify the boundary where placement lands exactly on an existing region’s start. Add a new test in regionPlacement.test.ts alongside the existing adjacent-placement case to assert the behavior when the requested placement time equals a region’s startMs, and document that this boundary is rejected so future changes to findFreeGapAt preserve it.
🤖 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/regionClipboard.ts`:
- Around line 32-44: `CopiedAnnotation` and the annotation copy/paste flow in
`regionClipboard.ts` are dropping `blurData`, so blur regions lose their
settings when copied or pasted. Add `blurData` to the copied annotation shape
and make `extractAnnotationAttributes` capture it, then ensure
`buildPastedAnnotation` clones it for new blur regions. If overwrite-paste is
meant to carry blur settings onto an existing region, also update
`replaceAnnotationAttributes` to merge `blurData` alongside the other annotation
fields.
In `@src/components/video-editor/regionPlacement.ts`:
- Around line 8-12: The doc comment in regionPlacement.ts is inconsistent with
the overlap logic used by the placement check. Update the wording near the
placement rules to match the actual behavior in the region overlap/adjacency
logic: landing on an existing region’s start is rejected, while adjacency is
only allowed at a region’s end. Keep the comment aligned with the implementation
in the placement helper so future readers are not misled.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1870-1881: The global copy/paste handling in VideoEditor’s
keyboard shortcut path is too aggressive and blocks native browser behavior when
no region is selected. Update the shortcut branch around matchesShortcut,
handleCopySelected, and handlePaste so e.preventDefault() and the custom
handlers only run when a region selection/clipboard target actually exists;
otherwise let Cmd/Ctrl+C and Cmd/Ctrl+V fall through to the browser. Keep this
logic outside editingText, and use the existing selection state helpers in
VideoEditor to decide when to intercept.
In `@src/i18n/locales/en/editor.json`:
- Around line 81-90: Add the missing blur label to the regionClipboard.kinds map
in the editor locale so the copy/paste toast can resolve blur regions correctly.
Update the existing regionClipboard translation block to include a blur entry
alongside zoom, speed, and annotation, keeping the naming consistent with the
keys used by the region clipboard UI. This will ensure the toast message in the
regionClipboard flow does not fall back to a missing-translation key for blur
regions.
In `@src/i18n/locales/es/editor.json`:
- Around line 86-89: The `annotation` label in the Spanish editor locale is too
specific and is used by `VideoEditor.tsx` for non-text annotation regions like
blur, so update the `kinds.annotation` translation to a broader neutral term
such as `Anotación`. Keep the change localized to the `kinds` entries so
clipboard/toast messages remain accurate for all annotation-region types.
In `@src/i18n/locales/fr/editor.json`:
- Around line 86-89: The French label for the annotation kind is too specific
and should be broadened because the same `annotation` key is used for blur
regions at runtime. Update the `kinds` mapping in `editor.json` so `annotation`
uses a neutral label like `Annotation` instead of `Texte`, keeping the
translation aligned with how `annotation` is used throughout the editor.
In `@src/i18n/locales/it/editor.json`:
- Around line 70-73: The `kinds.annotation` label in the Italian editor locale
is too specific as `Testo` and is also reused by blur-region copy/paste toasts,
so update the translation in the `kinds` object to a broader label like
`Annotazione`. Keep the change localized to the `annotation` key in
`editor.json` so all runtime uses of that symbol show the more accurate wording.
In `@src/i18n/locales/ja-JP/editor.json`:
- Around line 86-89: The translation for regionClipboard.kinds.annotation in the
ja-JP editor locale is too specific and causes incorrect copy/paste messaging
for non-text annotations. Update the annotation label in the localized editor
JSON to a generic term that matches how VideoEditor.tsx uses it for blur and
other annotation types, so the same key works correctly in the runtime toast
messages.
In `@src/i18n/locales/ko-KR/editor.json`:
- Around line 86-89: The label for regionClipboard.kinds.annotation in the ko-KR
editor locale is too specific and is reused by VideoEditor when showing
copy/paste toasts for blur and other non-text annotations. Update the annotation
translation in the editor locale JSON to a generic term that matches the shared
runtime usage, and keep the key aligned with the
regionClipboard.kinds.annotation lookup used in VideoEditor.tsx.
In `@src/i18n/locales/pt-BR/editor.json`:
- Around line 69-72: The `annotation` label in the `kinds` localization map is
too specific and causes clipboard toasts for blur regions to read like text
actions. Update the `annotation` entry in the `editor.json` pt-BR locale to a
broader term such as `Anotação`, keeping it consistent with how `VideoEditor`
uses the `annotation` clipboard kind for both text and blur region copy/paste
feedback.
In `@src/i18n/locales/ru/editor.json`:
- Around line 86-89: The `annotation` label in `editor.json` is too specific and
causes blur-region clipboard toasts to look like plain text. Update the
`kinds.annotation` translation in the Russian locale to a broader term such as
`Аннотация` so the `annotation` clipboard kind used by `VideoEditor` correctly
covers both text and blur regions.
In `@src/i18n/locales/tr/editor.json`:
- Around line 86-89: The Turkish `annotation` locale label is too specific for a
kind that is also used by blur regions in the clipboard toast flow. Update the
`kinds.annotation` entry in the locale JSON from the text-only label to a
broader term such as `Açıklama` so the `VideoEditor` clipboard/toast paths that
reuse `annotation` for blur regions show a correct message.
In `@src/i18n/locales/zh-CN/editor.json`:
- Around line 86-89: The zh-CN locale label for the clipboard kind used by both
text annotations and blur regions is too specific, so the toast can misdescribe
blur copy/paste as “文本”. Update the `kinds.annotation` entry in the editor
locale to a broader neutral term like `标注`, keeping it aligned with the shared
kind used by the related editor copy/paste UI.
---
Nitpick comments:
In `@src/components/video-editor/regionPlacement.test.ts`:
- Around line 32-37: The placement tests in findFreeGapAt currently cover
adjacency at a region’s end but do not verify the boundary where placement lands
exactly on an existing region’s start. Add a new test in regionPlacement.test.ts
alongside the existing adjacent-placement case to assert the behavior when the
requested placement time equals a region’s startMs, and document that this
boundary is rejected so future changes to findFreeGapAt preserve it.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1657-1693: The handleCopySelected callback returns from the first
truthy copy target even when no matching region is found, which prevents the
fallback “nothing to copy” toast on stale selections. Update the loop in
handleCopySelected so it only returns after a successful match and copy for the
selected region, and otherwise continues checking the remaining targets; keep
the toast.info(t("regionClipboard.nothingToCopy")) path reachable when no region
matches.
🪄 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: 24a4b329-2860-484f-a1c6-c3c2dac8e036
📒 Files selected for processing (33)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/regionClipboard.test.tssrc/components/video-editor/regionClipboard.tssrc/components/video-editor/regionPlacement.test.tssrc/components/video-editor/regionPlacement.tssrc/components/video-editor/timeline/TimelineEditor.tsxsrc/i18n/locales/ar/editor.jsonsrc/i18n/locales/ar/shortcuts.jsonsrc/i18n/locales/en/editor.jsonsrc/i18n/locales/en/shortcuts.jsonsrc/i18n/locales/es/editor.jsonsrc/i18n/locales/es/shortcuts.jsonsrc/i18n/locales/fr/editor.jsonsrc/i18n/locales/fr/shortcuts.jsonsrc/i18n/locales/it/editor.jsonsrc/i18n/locales/it/shortcuts.jsonsrc/i18n/locales/ja-JP/editor.jsonsrc/i18n/locales/ja-JP/shortcuts.jsonsrc/i18n/locales/ko-KR/editor.jsonsrc/i18n/locales/ko-KR/shortcuts.jsonsrc/i18n/locales/pt-BR/editor.jsonsrc/i18n/locales/pt-BR/shortcuts.jsonsrc/i18n/locales/ru/editor.jsonsrc/i18n/locales/ru/shortcuts.jsonsrc/i18n/locales/tr/editor.jsonsrc/i18n/locales/tr/shortcuts.jsonsrc/i18n/locales/vi/editor.jsonsrc/i18n/locales/vi/shortcuts.jsonsrc/i18n/locales/zh-CN/editor.jsonsrc/i18n/locales/zh-CN/shortcuts.jsonsrc/i18n/locales/zh-TW/editor.jsonsrc/i18n/locales/zh-TW/shortcuts.jsonsrc/lib/shortcuts.ts
- Fix handleCopySelected loop so stale ids fall through to the 'nothing to copy' toast instead of silently returning. - Use regionClipboard.kinds.blur label when copying a blur region. - Add 'blur' to regionClipboard.kinds in all 13 locales; broaden the 'annotation' label to neutral terms in es/fr/it/ja-JP/ko-KR/ pt-BR/ru/tr/zh-CN so the toast no longer reads 'Text' for blurs. - Cover the placement-at-region-start boundary in regionPlacement tests.
EtienneLescot
approved these changes
Jun 28, 2026
1 task
Collaborator
|
@446f6e6e79 nice feature, thanks! |
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
2 participants
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
Adds copy/paste of timeline region attributes to the video editor. Select a zoom, speed, or text/annotation region and press ⌘C/⌘V (Ctrl on Windows/Linux):
The copy lives in a session clipboard (not persisted, not part of undo history), while each paste goes through the editor's history so it's fully undoable. Trims are excluded since they carry no attributes. Helpers live in a new regionClipboard.ts module with unit tests, and the actions are remappable via the copySelected / paste shortcut bindings.
Related issue
Closes #24
Type of change
Release impact
Desktop impact
Screenshots / video
Testing
This was tested manually and seem to work as intended. Unit tests are also working!
Summary by CodeRabbit