add: drawIcons for extensions, SettingsPanel for extension fixed.#459
Open
ExtraBinoss wants to merge 16 commits intowebadderallorg:mainfrom
Open
add: drawIcons for extensions, SettingsPanel for extension fixed.#459ExtraBinoss wants to merge 16 commits intowebadderallorg:mainfrom
ExtraBinoss wants to merge 16 commits intowebadderallorg:mainfrom
Conversation
…phor/react fix: SliderControl is now the default for Settings Panel of the extensions fix: resetting of values on shadows/corner radius
Contributor
|
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:
📝 WalkthroughWalkthroughAdds drawIcon extension API and Path2D resolver/caching, introduces a debounced in-memory full settings store, and updates SettingsPanel and VideoPlayback to respond to extension-host changes; docs include a Drawing Icons example. ChangesExtension Icon API and Settings System
Sequence Diagram(s)sequenceDiagram
participant Extension
participant extensionHost
participant iconPathCache
participant Canvas
Extension->>extensionHost: drawIcon(ctx,name,x,y,size,color,weight)
extensionHost->>iconPathCache: lookup resolveIconPath(name,weight)
alt cached
iconPathCache-->>extensionHost: Path2D
else not cached
extensionHost->>iconPathCache: generate & cache Path2D
end
extensionHost->>Canvas: translate/scale + fill Path2D
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the extensions API and UI to support drawing Phosphor icons onto canvases and to improve extension settings sliders, while attempting to make extension-driven parameter changes reflect immediately in the preview.
Changes:
- Added
api.drawIcon(...)to the extension API and documented it inEXTENSIONS.md. - Switched extension “slider” fields in the Settings panel to use the shared
SliderControl. - Added preview invalidation wiring so extension host changes can trigger frame redraws, and optimized extension settings persistence with an in-memory cache + debounced localStorage writes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/extensions/types.ts | Extends the public extension API typing to include drawIcon. |
| src/lib/extensions/extensionHost.ts | Implements drawIcon, adds settings-store caching + debounced persistence, and expands host change notifications. |
| src/components/video-editor/VideoPlayback.tsx | Subscribes to extensionHost changes and forces frame sprite regeneration to reflect updates. |
| src/components/video-editor/SettingsPanel.tsx | Replaces <input type="range"> with SliderControl for extension slider fields; adjusts wallpaper/custom image effects and reset behavior. |
| EXTENSIONS.md | Documents drawIcon usage for extension authors. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/extensions/extensionHost.ts (1)
1006-1024: 🏗️ Heavy liftAvoid coupling
drawIcon()to React component internals.This extraction path walks
Icon.render,element.props.weights, and nestedchildren, but Phosphor documents rendering icon components and defining custom icons viaIconBase/weights—not reading those internals back out. That makes the helper fragile across React or Phosphor upgrades. Prefer a stable source such as@phosphor-icons/coreor a generated path map for canvas rendering. (github.com)🤖 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/lib/extensions/extensionHost.ts` around lines 1006 - 1024, The drawIcon() helper currently reaches into React/Phosphor internals (Icon.render, element.props.weights, children) to extract SVG path data which is fragile; instead provide a stable path source: add or import a dedicated path map (e.g., a generated record mapping icon name+weight to SVG path strings) and use that map in drawIcon() to create Path2D and populate host.iconPathCache (using the existing cacheKey/Path2D logic); update any callers that rely on Icon.render to use the map, and remove the internal-inspection code block (Icon.render, element, weights, children, pathElement) so drawIcon() only reads from the stable path map or a `@phosphor-icons/core` export.
🤖 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/SettingsPanel.tsx`:
- Around line 1461-1467: initialEditorPreferences.wallpaper is used
unconditionally as the reset target in onWallpaperChange which can keep a stale
path (e.g., removed extension or deleted custom asset); before preferring
initialEditorPreferences.wallpaper, validate it exists in the current sets by
checking that the value is included in builtInWallpaperPaths,
extensionWallpaperPaths, or matches any BUILT_IN_WALLPAPERS[].publicPath (or the
current custom assets collection), and only pass it to onWallpaperChange if
found; otherwise fall back to builtInWallpaperPaths[0] →
extensionWallpaperPaths[0] → BUILT_IN_WALLPAPERS[0]?.publicPath → "".
In `@src/lib/extensions/extensionHost.ts`:
- Line 475: The call to notifyListeners() in setVideoLayout() and
setShadowConfig() is wrong because onChange()/notifyListeners() is meant only
for extension-registry changes (used by SettingsPanel) and is causing unrelated
refreshes and feedback loops (VideoPlayback/frameUpdateCounter). Remove the
notifyListeners() calls from setVideoLayout() and setShadowConfig(), and instead
add a dedicated layout/project-state signal (e.g., new methods
registerLayoutListener/onLayoutChange and notifyLayoutListeners or
emitProjectStateChange) that layoutVideoContent() and VideoPlayback can
subscribe to; ensure SettingsPanel continues to use onChange()/notifyListeners()
for registry updates only.
- Around line 995-996: The drawIcon() lookup currently does a direct
PhosphorIcons[name] lookup and silently fails; update drawIcon() to reuse the
normalization strategy from ExtensionIcon.tsx/resolvePhosphorIcon(): attempt the
provided name first, then try name + "Icon", and also consult ICON_NAME_ALIASES
if present, so names like "CaretLeft" and "ArrowClockwise" resolve to their
exported symbols; remove the duplicated lookup logic (the pair at the current
lookup and the later block around lines 1000-1004) and consolidate into a single
normalized resolution path that returns the resolved Icon or null if none found.
---
Nitpick comments:
In `@src/lib/extensions/extensionHost.ts`:
- Around line 1006-1024: The drawIcon() helper currently reaches into
React/Phosphor internals (Icon.render, element.props.weights, children) to
extract SVG path data which is fragile; instead provide a stable path source:
add or import a dedicated path map (e.g., a generated record mapping icon
name+weight to SVG path strings) and use that map in drawIcon() to create Path2D
and populate host.iconPathCache (using the existing cacheKey/Path2D logic);
update any callers that rely on Icon.render to use the map, and remove the
internal-inspection code block (Icon.render, element, weights, children,
pathElement) so drawIcon() only reads from the stable path map or a
`@phosphor-icons/core` export.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e58abad-0e09-42b4-bc25-933086b39063
📒 Files selected for processing (5)
EXTENSIONS.mdsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/lib/extensions/extensionHost.tssrc/lib/extensions/types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/SettingsPanel.tsx`:
- Around line 1555-1559: When resetting preferences in SettingsPanel, validate
initialEditorPreferences.frame against availableFrames before calling
onFrameChange: if availableFrames contains that frame id call
onFrameChange?.(initialEditorPreferences.frame) as before, otherwise call
onFrameChange?.(undefined) (or the component's “empty” frame value) to clear the
selection; reference the initialEditorPreferences.frame, availableFrames array,
and onFrameChange handler when making this change.
- Around line 267-280: The SliderControl currently forces one decimal place via
formatValue={(v) => v.toFixed(1)}, which misrepresents sliders with different
step precision; compute the number of decimal places from field.step (falling
back to 0 or 1 as appropriate) by counting decimals in the step (e.g., convert
step to string or repeatedly multiply by 10 until integer) and use that
precision in formatValue (e.g., v.toFixed(precision)); ensure the same precision
logic is used for displaying defaultValue/current value and leave parseInput as
parseFloat so typed input round-trips correctly; update the SliderControl usage
in SettingsPanel (the component around SliderControl, field.step,
field.defaultValue, and the onChange/formatValue props) to use this derived
precision.
In `@src/lib/extensions/extensionHost.ts`:
- Around line 612-619: Debounced writes can be lost on quick exit; ensure any
pending persist is flushed during teardown by checking this.persistTimeout in
the extension host's shutdown path (e.g., deactivate/dispose/unload handler) and
if set, clearTimeout(this.persistTimeout), call
this.writePersistedSettingsStore(store) immediately with the current store
state, and set this.persistTimeout = null so the pending change is persisted
before exit; update any relevant teardown methods where lifecycle ends to
perform this flush using the existing persistTimeout and
writePersistedSettingsStore symbols.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b85460ac-70c2-449e-8743-05082b2dbd15
📒 Files selected for processing (4)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/lib/extensions/extensionHost.tssrc/lib/extensions/iconDraw.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/video-editor/VideoPlayback.tsx
Contributor
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 `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 265-278: The SliderControl fallback values were changed to min=0,
max=1, step=0.01 which breaks extensions expecting native range defaults; in
SettingsPanel (the code around the SliderControl render where field.step,
field.min, field.max are read) restore the original native HTML input defaults
when the plugin/field doesn't provide them by using min = field.min ?? 0, max =
field.max ?? 100, and step = field.step ?? 1 (or otherwise coerce/compute
precision from step when provided) so omitted values get 0–100 integer semantics
instead of 0–1 fractional semantics.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d617057-bb85-4c9c-86db-6f26b20e5f6c
📒 Files selected for processing (2)
src/components/video-editor/SettingsPanel.tsxsrc/lib/extensions/extensionHost.ts
Collaborator
|
Extension icons for their own settings sections will not render correctly in the sidebar "In VideoEditor.tsx (line 1666), the derived extensionSectionButtons state keeps only id, label, and icon, so by the time the button renders at VideoEditor.tsx (line 5985), ExtensionIcon is called without an extensionPath. But ExtensionIcon.tsx (line 72) requires extensionPath to resolve relative image paths; otherwise it returns the raw relative string at ExtensionIcon.tsx (line 90), which won’t resolve to the extension directory. Any extension using icon: "icons/panel.svg" will therefore break in this new sidebar path." |
…com:ExtraBinoss/Recordly-forked into feat/settings-panel-auto-props-extension-ui
Collaborator
Author
This is valid. and is fixed in |
Collaborator
Author
|
Waiitng for copilot review again. but most of the issues are fixed |
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
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
This PR introduces two main changes :
EXTENSION.mdType of Change
Related Issue(s)
I needed icons in my extension, so I added it. It's very useful if your extension needs to use Phospor react icons.
I also found the "range" HTML element very lacking. The [SliderControl] is much more nicer. Extension UI feels less lacking compared to the real UI.
Screenshots / Video
Code is from my extension for creating frames. We can create Terminal/Browser or Minimal framing.
It will be available after this PR is merged, since I need the icons to make it work. and we need to see if it should be a builtin. .
Testing Guide
Add an extension with a slider parameter, should look like the screenshots
Checklist
Thank you for contributing!
Summary by CodeRabbit
New Features
Improvements
Documentation