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:
📝 WalkthroughWalkthroughReplaced palette-only color controls with a new dual-mode ColorPicker (wheel + palette), added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e563166a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| </span> | ||
| </div> | ||
| <Colorful | ||
| color={annotation.style.backgroundColor} |
There was a problem hiding this comment.
For new annotations, backgroundColor defaults to "transparent", and with colorMode defaulting to "wheel" this value is passed directly into Colorful. @uiw/react-color-colorful only treats valid hex strings as color input, so "transparent" is not parsed into HSVA and the wheel initializes with invalid state, which can prevent users from reliably picking a background color until they switch modes. Please map transparent to a valid hex fallback (as the palette path already does) before passing it to Colorful.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Colorful handles transparent value.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/AnnotationSettingsPanel.tsx`:
- Line 71: The shared colorMode state in AnnotationSettingsPanel couples
independent popovers (text color and background color) so toggling one affects
the other; replace the single useState<"wheel" | "palette">("wheel") (colorMode
/ setColorMode) with two independent states (e.g.,
textColorMode/setTextColorMode and bgColorMode/setBgColorMode) and update all
references and handlers (including the popover controls and any calls that
read/write colorMode) so each popover reads and writes its own mode; apply the
same split for the other duplicated usages mentioned (lines around 398-410,
437-447, 488-505, 533-543) ensuring unique state names for each control to avoid
cross-updates.
- Around line 517-525: The Colorful picker can't accept the literal
"transparent", so change the code that sets or passes
annotation.style.backgroundColor to normalize "transparent" to a supported
transparent hex (use "#0000" or "#00000000"); specifically, update the
clear-path where backgroundColor is set (via onStyleChange) so it uses "#0000"
instead of "transparent", and update the value passed into the Colorful
component (where color={annotation.style.backgroundColor}) to coerce any
"transparent" string to "#0000" before rendering the picker.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Line 1017: The background color panel in SettingsPanel.tsx is using
translation keys for annotations (t("annotation.colorWheel") and
t("annotation.colorPalette")), which should be background-specific; update the
translation keys to use a background namespace (e.g., t("background.colorWheel")
and t("background.colorPalette")) in the SettingsPanel component (the background
color panel render block), and add corresponding entries to the background
translation resource files so the new keys resolve correctly.
- Around line 1056-1063: The current Input onChange updates selectedColor and
immediately calls onWallpaperChange on every keystroke, causing transient
invalid values (e.g. "#" or "#12") to be committed; change the handler so
setSelectedColor(e.target.value) always runs but only invoke onWallpaperChange
when the value is a valid hex color (use a hex validation regex like
/^#([0-9A-Fa-f]{3}|[0-9A-Fa-f]{6})$/) and also add commit points: call
onWallpaperChange if the input is valid on onBlur and when Enter is pressed
(handle onKeyDown checking for Enter). Keep the Input component's props
(value={selectedColor}) and ensure validation logic references selectedColor /
e.target.value and uses setSelectedColor + conditional onWallpaperChange.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff31ed67-47b9-494f-88a1-72103f99b7e8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.jsonsrc/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/zh-CN/settings.json
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 325-333: Extract the duplicated getTextColor logic from
SettingsPanel.tsx and AnnotationSettingsPanel.tsx into a shared utility (e.g.,
export getContrastingTextColor in src/utils/colorUtils.ts) and update both files
to import and use that function; ensure the utility handles "transparent",
expands 3-char shorthand hex (e.g., "#abc" -> "#aabbcc"), validates parseInt
results (fallback to "#ffffff" on NaN), and uses the same luminance calculation
and threshold as the original getTextColor.
- Around line 1045-1054: The Colorful picker currently calls onWallpaperChange
on every onChange which causes pushState in VideoEditor.tsx to create undo
checkpoints continuously; change the handler so setSelectedColor(color.hex)
still runs immediately but onWallpaperChange is invoked via a debounced wrapper
(e.g., create a debounced version of onWallpaperChange with useRef/useCallback
or lodash.debounce) so pushState only runs after the user stops dragging; ensure
the debounced function is created once, canceled on unmount, and referenced
instead of calling onWallpaperChange directly in the Colorful onChange handler.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dacdd2dd-d9a5-40bf-ad01-2dc2e5be7850
📒 Files selected for processing (4)
src/components/video-editor/SettingsPanel.tsxsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/zh-CN/settings.json
imAaryash
left a comment
imAaryash
left a comment
There was a problem hiding this comment.
Nice improvement overall, i really liked it, but in SettingsPanel.tsx, user can type any text into the new input and it directly calls, so no validation for hex format, so invalid strings may break something
…ation for the input
|
Thanks for the review @imAaryash, I've factored the color picker with the 2 selection modes into a ColorPicker component and added some validation for the Input. Let me know WYT ! |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/components/video-editor/SettingsPanel.tsx (1)
998-1001:⚠️ Potential issue | 🟠 Majorlowkey risky: committing wallpaper on every picker tick can flood history.
Line 998–1001 calls
onWallpaperChangefor everyonUpdateColoremission. During wheel drag, that can spam undo checkpoints and trigger heavy preview rerenders. KeepsetSelectedColorimmediate, debounce commit calls.🛠️ Proposed fix
+ const wallpaperCommitTimerRef = useRef<number | null>(null); + const commitWallpaperColor = useCallback( + (color: string) => { + if (wallpaperCommitTimerRef.current !== null) { + window.clearTimeout(wallpaperCommitTimerRef.current); + } + wallpaperCommitTimerRef.current = window.setTimeout(() => { + onWallpaperChange(color); + }, 120); + }, + [onWallpaperChange], + ); + + useEffect(() => { + return () => { + if (wallpaperCommitTimerRef.current !== null) { + window.clearTimeout(wallpaperCommitTimerRef.current); + } + }; + }, []); ... onUpdateColor={(color) => { setSelectedColor(color); - onWallpaperChange(color); + commitWallpaperColor(color); }}#!/bin/bash set -euo pipefail # Verify whether wallpaper updates are persisted as history checkpoints. rg -n -C3 '\bonWallpaperChange\b' src/components/video-editor/VideoEditor.tsx rg -n -C3 'pushState\(\{\s*wallpaper' src/components/video-editor/VideoEditor.tsx # Confirm this callback path in SettingsPanel. rg -n -C3 'onUpdateColor=\{\(color\)\s*=>\s*\{' src/components/video-editor/SettingsPanel.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` around lines 998 - 1001, The current onUpdateColor handler in SettingsPanel calls setSelectedColor and immediately invokes onWallpaperChange for every color tick, which can spam history and heavy rerenders; change this so setSelectedColor(color) remains immediate but debounce or throttle the call to onWallpaperChange—e.g., create a debounced commit function (using a shared debounce utility or a local setTimeout stored in a ref) and call that from onUpdateColor, and ensure any final commit runs on color-picker close/confirm; update the onUpdateColor handler, the debounced commit function, and any cleanup (clearTimeout) references to onWallpaperChange, setSelectedColor, and SettingsPanel to implement this.src/components/ui/color-picker.tsx (2)
96-99:⚠️ Potential issue | 🟠 MajorNormalize cleared color before passing it to
Colorful.Line 97 passes
selectedColordirectly, but Line 133 can set it to"transparent". That value can make the wheel state kinda cursed in controlled mode. Keep"transparent"in app state, but coerce picker input to a valid color string.🛠️ Proposed fix
+ const colorfulColor = selectedColor === "transparent" ? "#000000" : selectedColor; ... <Colorful - color={selectedColor} + color={colorfulColor} onChange={(color) => { onUpdateColor(color.hex); }}In `@uiw/react-color-colorful` v2.9.2, what controlled `color` string formats are officially supported, and is literal "transparent" supported?Also applies to: 127-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/color-picker.tsx` around lines 96 - 99, The Colorful picker is receiving the app state's literal "transparent" which breaks controlled behavior; when rendering Colorful use a normalized value (e.g., const normalizedColor = selectedColor === "transparent" ? "#000000" : selectedColor) and pass normalizedColor into the Colorful color prop, while preserving "transparent" in your app state when the user clears color (onUpdateColor should still set "transparent" as needed); update both the Colorful usage around the Colorful component and the clear-handling logic (the selectedColor state setter used in the clear branch around the onUpdateColor/click handlers at lines ~127-134) to follow this normalization approach.
28-36:⚠️ Potential issue | 🟡 Minor
#RGBis accepted but contrast parsing assumes#RRGGBB.Line 53 accepts shorthand hex, but Lines 30-32 parse as 6-digit only. That can yield
NaNluminance and wrong preview text color for valid shorthand input.♻️ Proposed fix
const getTextColor = (color: string) => { if (color === "transparent") return "#ffffff"; - const r = parseInt(color.slice(1, 3), 16); - const g = parseInt(color.slice(3, 5), 16); - const b = parseInt(color.slice(5, 7), 16); + const hex = + /^#[0-9A-Fa-f]{3}$/.test(color) + ? `#${color[1]}${color[1]}${color[2]}${color[2]}${color[3]}${color[3]}` + : color; + const r = parseInt(hex.slice(1, 3), 16); + const g = parseInt(hex.slice(3, 5), 16); + const b = parseInt(hex.slice(5, 7), 16); + if (Number.isNaN(r) || Number.isNaN(g) || Number.isNaN(b)) return "#ffffff"; const luminance = 0.299 * r + 0.587 * g + 0.114 * b; if (luminance > 186) return "#000000"; return "#ffffff"; };Also applies to: 52-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/color-picker.tsx` around lines 28 - 36, getTextColor currently parses only 6-digit hex strings and returns NaN for valid 3-digit shorthand; update getTextColor to first normalize the input: trim and lowercase the string, keep the existing "transparent" check, then if the hex is in 3-digit form (`#RGB`) expand it to 6-digit (`#RRGGBB`) by duplicating each nibble, validate the final format is `#RRGGBB` before parsing r/g/b, compute luminance as before, and fall back to a safe color (e.g., "#ffffff") on invalid input so shorthand and full hex both produce correct contrast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/ui/color-picker.tsx`:
- Around line 96-99: The Colorful picker is receiving the app state's literal
"transparent" which breaks controlled behavior; when rendering Colorful use a
normalized value (e.g., const normalizedColor = selectedColor === "transparent"
? "#000000" : selectedColor) and pass normalizedColor into the Colorful color
prop, while preserving "transparent" in your app state when the user clears
color (onUpdateColor should still set "transparent" as needed); update both the
Colorful usage around the Colorful component and the clear-handling logic (the
selectedColor state setter used in the clear branch around the
onUpdateColor/click handlers at lines ~127-134) to follow this normalization
approach.
- Around line 28-36: getTextColor currently parses only 6-digit hex strings and
returns NaN for valid 3-digit shorthand; update getTextColor to first normalize
the input: trim and lowercase the string, keep the existing "transparent" check,
then if the hex is in 3-digit form (`#RGB`) expand it to 6-digit (`#RRGGBB`) by
duplicating each nibble, validate the final format is `#RRGGBB` before parsing
r/g/b, compute luminance as before, and fall back to a safe color (e.g.,
"#ffffff") on invalid input so shorthand and full hex both produce correct
contrast.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 998-1001: The current onUpdateColor handler in SettingsPanel calls
setSelectedColor and immediately invokes onWallpaperChange for every color tick,
which can spam history and heavy rerenders; change this so
setSelectedColor(color) remains immediate but debounce or throttle the call to
onWallpaperChange—e.g., create a debounced commit function (using a shared
debounce utility or a local setTimeout stored in a ref) and call that from
onUpdateColor, and ensure any final commit runs on color-picker close/confirm;
update the onUpdateColor handler, the debounced commit function, and any cleanup
(clearTimeout) references to onWallpaperChange, setSelectedColor, and
SettingsPanel to implement this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98509440-852b-4ab2-b379-fc4517802702
📒 Files selected for processing (3)
src/components/ui/color-picker.tsxsrc/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/SettingsPanel.tsx
imAaryash
left a comment
imAaryash
left a comment
There was a problem hiding this comment.
looks clean now, just one small edge case I noticed that if "transparent" gets passed into Colorful in wheel mode (color={selectedColor}) it might act weird since it expects a proper color value.
maybe just map it to a fallback hex before passing it in, pretty minor though, I’d have patched it but not a maintainer here :)
|
Indeed, nice catch @imAaryash , thanks ! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/ui/color-picker.tsx (1)
17-19: Tighten prop typing so clear button text can’t silently disappear.Right now
clearBackgroundis optional even whenclearBackgroundOptionis true. That can render an unlabeled button later if a caller forgets the key. nit: cleaner to enforce this at type level.typed contract refactor
-export default function ColorPicker({ +type BaseProps = { + selectedColor: string; + colorPalette: string[]; + onUpdateColor: (color: string) => void; +}; + +type ColorPickerProps = + | (BaseProps & { + clearBackgroundOption?: false; + translations: Record<"colorWheel" | "colorPalette", string>; + }) + | (BaseProps & { + clearBackgroundOption: true; + translations: Record<"colorWheel" | "colorPalette" | "clearBackground", string>; + }); + +export default function ColorPicker({ selectedColor, colorPalette, translations, clearBackgroundOption = false, onUpdateColor, -}: { - selectedColor: string; - colorPalette: string[]; - translations: Record<"colorWheel" | "colorPalette", string> & - Partial<Record<"clearBackground", string>>; - clearBackgroundOption?: boolean; - onUpdateColor: (color: string) => void; -}) { +}: ColorPickerProps) {Also applies to: 140-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/color-picker.tsx` around lines 17 - 19, The translations prop and clearBackgroundOption should be modeled as a discriminated union so the "clearBackground" string is required whenever clearBackgroundOption is true; update the component props (the translations and clearBackgroundOption declarations in color-picker.tsx) to a union like: { clearBackgroundOption?: false; translations: Record<"colorWheel" | "colorPalette", string> } | { clearBackgroundOption: true; translations: Record<"colorWheel" | "colorPalette" | "clearBackground", string> } and apply the same change to the other props block referenced around the second occurrence (the block that currently uses the same translations/clearBackgroundOption types) so callers cannot omit the clear button label when the option is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/color-picker.tsx`:
- Around line 35-43: The getTextColor function mis-parses shorthand hex like
"#RGB" causing NaN luminance; normalize the input hex before parsing by
expanding 3-character shorthand to 6-character form (e.g., "#abc" -> "#aabbcc"),
validate the string starts with '#' and is the right length, and fall back to
safe defaults (e.g., return "#ffffff") for invalid inputs; update the logic in
getTextColor (and the same normalization used around the preview/contrast code
referenced) so r/g/b are always parsed from a 6-char hex string and luminance
calculation is reliable.
- Around line 66-70: The toTransparent helper currently calls
hexToHsva(unconditionally) which breaks when selectedColor can be the sentinel
string "transparent"; guard inside toTransparent so it returns a valid
transparent HSVA when input === "transparent" (or when hexToHsva would fail)
instead of calling hexToHsva on that sentinel; update any callers (e.g., where
selectedColor is set/cleared by the clear button) to pass through the sentinel
unchanged or to call toTransparent only for real hex strings, referencing the
toTransparent and hexToHsva functions and the selectedColor usage to locate the
change.
---
Nitpick comments:
In `@src/components/ui/color-picker.tsx`:
- Around line 17-19: The translations prop and clearBackgroundOption should be
modeled as a discriminated union so the "clearBackground" string is required
whenever clearBackgroundOption is true; update the component props (the
translations and clearBackgroundOption declarations in color-picker.tsx) to a
union like: { clearBackgroundOption?: false; translations: Record<"colorWheel" |
"colorPalette", string> } | { clearBackgroundOption: true; translations:
Record<"colorWheel" | "colorPalette" | "clearBackground", string> } and apply
the same change to the other props block referenced around the second occurrence
(the block that currently uses the same translations/clearBackgroundOption
types) so callers cannot omit the clear button label when the option is enabled.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21893205-b7bd-403c-8032-0ca049ab0554
📒 Files selected for processing (1)
src/components/ui/color-picker.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/ui/color-picker.tsx (1)
39-46:⚠️ Potential issue | 🟡 Minor
getTextColorstill mis-parses#RGB(same unresolved nit, lowkey risky for contrast preview).Line 41–43 assume
#RRGGBB, but input validation (Line 64) allows#RGB. That makes luminance unreliable for shorthand values.nit: cleaner + safe normalization before luminance
const getTextColor = (color: string) => { if (color === "transparent") return "#ffffff"; - const r = parseInt(color.slice(1, 3), 16); - const g = parseInt(color.slice(3, 5), 16); - const b = parseInt(color.slice(5, 7), 16); + const normalized = + /^#[0-9A-Fa-f]{3}$/.test(color) + ? `#${color[1]}${color[1]}${color[2]}${color[2]}${color[3]}${color[3]}` + : color; + if (!/^#[0-9A-Fa-f]{6}$/.test(normalized)) return "#ffffff"; + const r = parseInt(normalized.slice(1, 3), 16); + const g = parseInt(normalized.slice(3, 5), 16); + const b = parseInt(normalized.slice(5, 7), 16); const luminance = 0.299 * r + 0.587 * g + 0.114 * b; if (luminance > 186) return "#000000"; return "#ffffff"; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/color-picker.tsx` around lines 39 - 46, getTextColor currently assumes a 7-char hex ("#RRGGBB") which mis-parses shorthand ("#RGB") allowed elsewhere; normalize the input inside getTextColor by handling "transparent", expanding 4-char "#RGB" into full 7-char "#RRGGBB" (duplicate each hex nibble), validate the resulting string matches /^#([0-9a-fA-F]{6})$/ before parsing, then compute r/g/b and luminance as before and return "#000000" or "#ffffff"; update getTextColor to treat invalid formats safely (fallback to a default color) so contrast preview is reliable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/color-picker.tsx`:
- Around line 132-135: The palette-mode Block is passed selectedColor which may
be the literal string "transparent" and breaks `@uiw/react-color-block`; update
the Block invocation used when colorMode === "palette" to pass selectedColor !==
"transparent" ? selectedColor : transparentColorHSVA (the same check used in
wheel mode) so Block always receives a valid hex/HsvaColor value; locate the
Block component usage and replace the direct selectedColor prop with the ternary
that falls back to transparentColorHSVA.
---
Duplicate comments:
In `@src/components/ui/color-picker.tsx`:
- Around line 39-46: getTextColor currently assumes a 7-char hex ("#RRGGBB")
which mis-parses shorthand ("#RGB") allowed elsewhere; normalize the input
inside getTextColor by handling "transparent", expanding 4-char "#RGB" into full
7-char "#RRGGBB" (duplicate each hex nibble), validate the resulting string
matches /^#([0-9a-fA-F]{6})$/ before parsing, then compute r/g/b and luminance
as before and return "#000000" or "#ffffff"; update getTextColor to treat
invalid formats safely (fallback to a default color) so contrast preview is
reliable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5386c3b8-682f-4031-8c66-f8a9b37928b4
📒 Files selected for processing (1)
src/components/ui/color-picker.tsx
|
you're welcome! usually they review PRs, but not sure if they've looked at this one yet. You could try tagging someone, might speed things |
2 participants
Pull Request Template
Description
Adds a color wheel option to the background color picker and the annotations color picker.
Motivation
More practical to use in my opinion.
Type of Change
Screenshots / Video
Video (if applicable):
demo-colorwheel.1.mp4
Testing
Checklist
Thank you for contributing!
Summary by CodeRabbit
New Features
Localization