feat: redesign desktop UI with unified panels and source-style layer editor - #1
Merged
Conversation
…editor Standardize frontend panels around one design system: single-field rows, reusable cards/callouts/stats, one primary CTA accent, and consistent status colors. Rework preview layer editor into a source-style list with visibility and lock controls, simplify rendering settings with an advanced toggle, and add missing backend endpoints needed by redesigned frontend flows. Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 202 files, which is 102 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (203)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request adds a frontend design system and redesigned desktop shell, refactors panels into shared card and control components, introduces toast and shortcut flows, adds media and lyrics backend endpoints, and updates runtime utilities, styling, documentation, and test infrastructure. ChangesFrontend redesign and interaction flows
Backend media and lyrics routes
Specifications, updater behavior, and runtime utilities
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/utils/performance.ts (1)
24-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle the trailing edge in
throttleto prevent dropped events.The current implementation only triggers on the leading edge. If this utility is used for continuous events like window resizing or scrolling, any intermediate events—including the final one where the user stops—will be completely ignored during the throttle period. This leaves the application state permanently out of sync with the final user input.
You can modify it to capture the latest arguments and execute them when the timeout resolves.
🐛 Proposed fix
export function throttle<T extends (...args: any[]) => any>( func: T, limit: number ): (...args: Parameters<T>) => void { let inThrottle: boolean; + let lastArgs: Parameters<T> | null = null; return function executedFunction(...args: Parameters<T>) { if (!inThrottle) { func(...args); inThrottle = true; - setTimeout(() => (inThrottle = false), limit); + setTimeout(() => { + inThrottle = false; + if (lastArgs) { + executedFunction(...lastArgs); + lastArgs = null; + } + }, limit); + } else { + lastArgs = args; } }; }🤖 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 `@frontend/src/utils/performance.ts` around lines 24 - 37, Update the throttle function to retain the latest arguments received during the active limit period and invoke func with those arguments when the timeout completes, while preserving the immediate leading-edge invocation and clearing any pending arguments afterward.
🧹 Nitpick comments (6)
frontend/src/utils/performance.ts (2)
147-154: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDeduplicate resource preloads.
Calling
preloadResourcemultiple times with the same URL will needlessly append duplicate<link rel="preload">elements to the document head. Consider checking if the link already exists before appending.♻️ Proposed fix
export function preloadResource(url: string, type: 'image' | 'video' | 'audio' | 'script' | 'style') { + if (document.querySelector(`link[rel="preload"][href="${url}"]`)) return; + const link = document.createElement('link'); link.rel = 'preload'; link.href = url;🤖 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 `@frontend/src/utils/performance.ts` around lines 147 - 154, Update preloadResource to check document.head for an existing preload link with the same URL before creating and appending a new element; return without appending when one exists, while preserving the current behavior for new resources.
156-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider native Web Worker instantiation for robust bundling.
Constructing a Worker by serializing a function via
toString()is fragile. It will fail ifworkerFunctionreferences any external variables (closures), imports, or if the bundler/transpiler (like Babel/SWC) injects external helper functions (e.g.,_asyncToGenerator).If your build tool supports it (e.g., Vite/Webpack), using native standard imports like
new Worker(new URL('./worker.ts', import.meta.url))is much safer and ensures all dependencies are properly resolved and bundled into the worker.🤖 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 `@frontend/src/utils/performance.ts` around lines 156 - 169, Update createWorker to use a native module-backed Worker entry point via the bundler-supported URL/import mechanism instead of serializing workerFunction with toString(). Move the heavy computation into a dedicated worker module so its imports, closures, and transpiler helpers are bundled correctly, while preserving the returned worker and cleanup behavior.frontend/src/components/ui/Toast.tsx (2)
124-134: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo guard against double-registering global error listeners.
setupGlobalErrorHandler()unconditionally addsunhandledrejection/errorlisteners with no idempotency check. If this is ever invoked more than once (HMR re-execution, re-mount, future refactor), listeners accumulate and every uncaught error/rejection produces duplicate toasts.♻️ Suggested fix
+let globalErrorHandlerInstalled = false; export function setupGlobalErrorHandler() { + if (globalErrorHandlerInstalled) return; + globalErrorHandlerInstalled = true; window.addEventListener('unhandledrejection', (event) => {🤖 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 `@frontend/src/components/ui/Toast.tsx` around lines 124 - 134, Make setupGlobalErrorHandler idempotent by adding a guard that records whether the global listeners have already been registered and returns on subsequent calls. Keep the existing unhandledrejection and error listener behavior unchanged after the first registration.
55-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winToast region lacks
aria-live/role for screen readers.The toast list is purely visual; screen-reader users get no announcement when a toast appears. Add
role="status" aria-live="polite"(orrole="alert"for errors) to the container.♻️ Suggested fix
<div className="fixed top-16 right-4 z-50 flex flex-col gap-2 pointer-events-none"> + <div role="status" aria-live="polite" className="sr-only" />🤖 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 `@frontend/src/components/ui/Toast.tsx` around lines 55 - 122, Update the toast messages container rendered by the enabled branch in the Toast component to include role="status" and aria-live="polite", ensuring newly added toasts are announced to screen readers while preserving the existing visual and interaction behavior.frontend/src/components/ui/PathInput.tsx (1)
164-183: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDebounced
!valuecheck now delays clearinginfoby 350ms.Moving the early-return inside the
setTimeoutcallback means programmatic clears ofvalue(not via the local "clear" button, which already callssetInfo(null)synchronously) keep showing staleinfo/status for up to 350ms, and still schedule an unnecessary timer for the empty-value case.♻️ Suggested fix
useEffect(() => { let cancelled = false; + if (!value) { + setInfo(null); + return; + } const t = setTimeout(async () => { - if (!value) { - setInfo(null); - return; - } - try {🤖 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 `@frontend/src/components/ui/PathInput.tsx` around lines 164 - 183, Update the useEffect handling value changes so an empty value clears info immediately and returns before scheduling the debounce timer. Keep the existing debounced path validation and cancellation behavior for non-empty values unchanged.frontend/src/components/panels/BrandingPanel.tsx (1)
90-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBumper preview response is discarded. The backend
/api/branding/bumper/previewreturns{ url, duration, hasVideo, hasAudio, resolution }, but the handler only callssetMessage('Preview bumper siap!')and never surfaces the returned media/metadata, so clicking Preview produces no visible preview. Consider storing the response and rendering it (e.g., a small video/metadata panel).🤖 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 `@frontend/src/components/panels/BrandingPanel.tsx` around lines 90 - 108, The bumper preview handler in the Preview button’s onClick currently discards the API response; capture the returned { url, duration, hasVideo, hasAudio, resolution } data and store it in component state, then render a small preview/metadata panel using that state so the returned media is visible after a successful request. Preserve the existing busy-state and error handling.
🤖 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 `@FASE1_AUDIT.md`:
- Line 3: Update the FASE1_AUDIT.md completion status so it does not claim the
audit is complete while AudioMixingPanel.tsx remains unaudited. Either audit
AudioMixingPanel.tsx and document verified findings, or revise the
heading/status to indicate the audit is incomplete and exclude it from
completion claims.
- Around line 144-145: Clarify the audit requirements so only multi-field form
field-group grids are replaced with FieldRow; preserve StatRow for statistics
and allow valid internal layout grids. Update the corresponding wording in both
referenced requirement sections without changing the separate StatRow or
SliderControl requirements.
In `@frontend/REDESIGN_PLAN.md`:
- Line 7: Update the fenced diagram block in REDESIGN_PLAN.md to include a
language identifier such as text immediately after the opening fence, satisfying
markdownlint MD040 while preserving the block’s content.
In `@frontend/src/components/panels/OverlayPanel.tsx`:
- Line 143: Update the Motion Blur Amount SliderControl in OverlayPanel so it
specifies a fractional step, allowing values between 0 and 1—including the
existing 0.5 default—instead of relying on the integer default step.
In `@frontend/src/components/panels/PreviewPane.tsx`:
- Around line 359-371: Update toggleLayerVisibility to derive the current state
from the visible layer data already computed in liveLayers, such as the matching
layer.enabled value, rather than defaulting getDeep(config, path, false) to
false. Use that resolved visible state to invert the value passed to
updateConfig, preserving correct first-click behavior for default-enabled
nowPlaying, spectrum, and logo layers.
In `@frontend/src/components/panels/QueuePanel.tsx`:
- Around line 124-213: Update queueAction and its callers so feedback is emitted
only once: remove queueAction’s generic success toast, and rethrow caught errors
after setting the message so startJob, cancelJob, duplicateJob, removeJob, and
moveJob can provide their specific success/error toasts. Preserve the existing
performanceAction and optimizePerformance feedback behavior.
In `@frontend/src/components/panels/SpectrumPanel.tsx`:
- Around line 100-107: Update the analysis message in SpectrumPanel to read the
sensitivity value from data.tuned?.sensitivity instead of
data.tuned?.Sensitivitas, matching the backend response and the existing StatRow
field while preserving the other tuned metrics.
In `@frontend/src/components/panels/TargetPanel.tsx`:
- Around line 47-59: Remove the hasVisual and hasAudio early-return guards from
createBatch, leaving scan.pairs.length === 0 as the batch validity check.
Preserve the existing scan-based payload and the button disabled condition so
valid folder batches with scanned pairs can be created without single-file
inputs.
In `@frontend/src/components/ui/design-system-components.tsx`:
- Around line 11-13: The createFieldId function currently produces duplicate IDs
for repeated labels. Replace label-derived ID generation with a per-instance
identifier, such as React.useId(), or support a caller-supplied ID, and update
the related field components so each input and its htmlFor label share that
unique ID.
In `@frontend/src/index.css`:
- Line 47: Update the font-family value in the --font-sans declaration and the
currentColor values around the referenced color rules to lowercase equivalents,
preserving the existing styling while satisfying Stylelint’s value-keyword-case
rule.
In `@frontend/src/lib/api.ts`:
- Around line 19-52: Remove both showToastFn error-toast calls from api(),
covering the non-OK response branch and the network-error catch path. Keep
ApiError creation and propagation unchanged so callers such as TargetPanel and
PreviewPane remain responsible for displaying context-specific error messages.
In `@frontend/src/styles.css`:
- Line 2: Update the stylesheet to satisfy Stylelint: change the Google Fonts
`@import` in the stylesheet to plain-string import notation, remove quotes from
the Inter/Roboto font-family names, and rename slideIn, slideInLeft, and scaleIn
keyframes to kebab-case names while updating every corresponding animation
reference.
- Around line 366-383: Increase the font sizes in the shared
input/select/textarea rule and the nearby .field span and .targetSummary small
rules to match the existing readable typography tokens. Replace the current 7px,
6px, and 5px values with the appropriate established scale values while
preserving the other styling.
In `@frontend/src/utils/performance.ts`:
- Around line 39-53: Update rafThrottle so each invocation stores its arguments
in shared closure state, replacing earlier values before the queued animation
frame runs. Have the requestAnimationFrame callback invoke func with the latest
stored arguments, then clear both the pending frame state and stored arguments.
---
Outside diff comments:
In `@frontend/src/utils/performance.ts`:
- Around line 24-37: Update the throttle function to retain the latest arguments
received during the active limit period and invoke func with those arguments
when the timeout completes, while preserving the immediate leading-edge
invocation and clearing any pending arguments afterward.
---
Nitpick comments:
In `@frontend/src/components/panels/BrandingPanel.tsx`:
- Around line 90-108: The bumper preview handler in the Preview button’s onClick
currently discards the API response; capture the returned { url, duration,
hasVideo, hasAudio, resolution } data and store it in component state, then
render a small preview/metadata panel using that state so the returned media is
visible after a successful request. Preserve the existing busy-state and error
handling.
In `@frontend/src/components/ui/PathInput.tsx`:
- Around line 164-183: Update the useEffect handling value changes so an empty
value clears info immediately and returns before scheduling the debounce timer.
Keep the existing debounced path validation and cancellation behavior for
non-empty values unchanged.
In `@frontend/src/components/ui/Toast.tsx`:
- Around line 124-134: Make setupGlobalErrorHandler idempotent by adding a guard
that records whether the global listeners have already been registered and
returns on subsequent calls. Keep the existing unhandledrejection and error
listener behavior unchanged after the first registration.
- Around line 55-122: Update the toast messages container rendered by the
enabled branch in the Toast component to include role="status" and
aria-live="polite", ensuring newly added toasts are announced to screen readers
while preserving the existing visual and interaction behavior.
In `@frontend/src/utils/performance.ts`:
- Around line 147-154: Update preloadResource to check document.head for an
existing preload link with the same URL before creating and appending a new
element; return without appending when one exists, while preserving the current
behavior for new resources.
- Around line 156-169: Update createWorker to use a native module-backed Worker
entry point via the bundler-supported URL/import mechanism instead of
serializing workerFunction with toString(). Move the heavy computation into a
dedicated worker module so its imports, closures, and transpiler helpers are
bundled correctly, while preserving the returned worker and cleanup behavior.
🪄 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 Plus
Run ID: fbab3a06-e763-465d-a17c-9cf38e7ee63b
📒 Files selected for processing (38)
FASE1_AUDIT.mdREDESIGN.txtbackend/ffmpeg-utils.mjsbackend/routes.mjselectron/updater.mjseslint.config.mjsfrontend/REDESIGN_PLAN.mdfrontend/src/App.tsxfrontend/src/components/layout/SettingsPane.tsxfrontend/src/components/layout/WorkspacePopup.tsxfrontend/src/components/panels/AudioMixingPanel.tsxfrontend/src/components/panels/BrandingPanel.tsxfrontend/src/components/panels/HelpPanel.tsxfrontend/src/components/panels/LoopingPanel.tsxfrontend/src/components/panels/LyricsPanel.tsxfrontend/src/components/panels/OverlayPanel.tsxfrontend/src/components/panels/PreviewPane.tsxfrontend/src/components/panels/QueuePanel.tsxfrontend/src/components/panels/SettingsPanel.tsxfrontend/src/components/panels/SpectrumPanel.tsxfrontend/src/components/panels/TargetPanel.tsxfrontend/src/components/panels/TemplatesPanel.tsxfrontend/src/components/ui/PathInput.tsxfrontend/src/components/ui/Toast.tsxfrontend/src/components/ui/button.tsxfrontend/src/components/ui/design-system-components.test.tsxfrontend/src/components/ui/design-system-components.tsxfrontend/src/components/ui/form-controls.tsxfrontend/src/components/ui/panel-primitives.tsxfrontend/src/design-system.tsfrontend/src/hooks/useKeyboardShortcuts.tsfrontend/src/index.cssfrontend/src/lib/api.tsfrontend/src/main.tsxfrontend/src/styles.cssfrontend/src/test/setup.tsfrontend/src/utils/performance.tsfrontend/src/workspace-fixes.css
💤 Files with no reviewable changes (3)
- frontend/src/components/layout/WorkspacePopup.tsx
- frontend/src/workspace-fixes.css
- frontend/src/main.tsx
|
|
||
| Transformasi dari layout "floating popup" ke layout **3-column fixed** yang mirip Filmora/CapCut: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced diagram block.
Use an identifier such as text after the opening fence so the document passes markdownlint MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@frontend/REDESIGN_PLAN.md` at line 7, Update the fenced diagram block in
REDESIGN_PLAN.md to include a language identifier such as text immediately after
the opening fence, satisfying markdownlint MD040 while preserving the block’s
content.
Source: Linters/SAST tools
| <Card title="Motion Effects"> | ||
| <Check label="Enable Motion Blur" checked={Boolean(getDeep(config, 'overlay.motionBlur.enabled', false))} onChange={v => updateConfig('overlay.motionBlur.enabled', v)} /> | ||
| <Slider label="Motion Blur Amount" value={Number(getDeep(config, 'overlay.motionBlur.amount', 0.5))} onChange={v => updateConfig('overlay.motionBlur.amount', v)} min={0} max={1} /> | ||
| <SliderControl label="Motion Blur Amount" value={Number(getDeep(config, 'overlay.motionBlur.amount', 0.5))} onChange={v => updateConfig('overlay.motionBlur.amount', v)} min={0} max={1} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Motion blur slider can only select 0 or 1. SliderControl defaults step = 1; with min={0} max={1} the range input snaps to whole numbers, so the 0.5 default and all fractional values are unreachable. Pass a fractional step.
🐛 Proposed fix
- <SliderControl label="Motion Blur Amount" value={Number(getDeep(config, 'overlay.motionBlur.amount', 0.5))} onChange={v => updateConfig('overlay.motionBlur.amount', v)} min={0} max={1} />
+ <SliderControl label="Motion Blur Amount" value={Number(getDeep(config, 'overlay.motionBlur.amount', 0.5))} onChange={v => updateConfig('overlay.motionBlur.amount', v)} min={0} max={1} step={0.05} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <SliderControl label="Motion Blur Amount" value={Number(getDeep(config, 'overlay.motionBlur.amount', 0.5))} onChange={v => updateConfig('overlay.motionBlur.amount', v)} min={0} max={1} /> | |
| <SliderControl label="Motion Blur Amount" value={Number(getDeep(config, 'overlay.motionBlur.amount', 0.5))} onChange={v => updateConfig('overlay.motionBlur.amount', v)} min={0} max={1} step={0.05} /> |
🤖 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 `@frontend/src/components/panels/OverlayPanel.tsx` at line 143, Update the
Motion Blur Amount SliderControl in OverlayPanel so it specifies a fractional
step, allowing values between 0 and 1—including the existing 0.5 default—instead
of relying on the integer default step.
Comment on lines
+359
to
+371
| function toggleLayerVisibility(id: LiveTarget) { | ||
| const pathMap: Record<LiveTarget, string> = { | ||
| nowPlaying: 'spectrum.nowPlaying', | ||
| spectrum: 'spectrum.enabled', | ||
| logo: 'branding.logoEnabled', | ||
| cta: 'branding.ctaEnabled', | ||
| watermark: 'branding.watermarkEnabled', | ||
| timestamp: 'overlay.timestamp', | ||
| lowerThird: 'overlay.lowerThirdEnabled', | ||
| }; | ||
| const path = pathMap[id]; | ||
| updateConfig(path, !Boolean(getDeep(config, path, false))); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Visibility toggle can no-op on the first click for default-enabled layers. toggleLayerVisibility computes the next value from getDeep(config, path, false), but nowPlaying/spectrum (and derived logo) render as enabled with a default of true when the config key is unset. In that state the toggle evaluates !false === true and re-enables the already-visible layer, so the first "hide" click does nothing and a second click is needed. Read the current visible state instead (e.g., from the layer.enabled already computed in liveLayers).
🐛 Proposed fix
- function toggleLayerVisibility(id: LiveTarget) {
+ function toggleLayerVisibility(id: LiveTarget) {
const pathMap: Record<LiveTarget, string> = {
nowPlaying: 'spectrum.nowPlaying',
spectrum: 'spectrum.enabled',
logo: 'branding.logoEnabled',
cta: 'branding.ctaEnabled',
watermark: 'branding.watermarkEnabled',
timestamp: 'overlay.timestamp',
lowerThird: 'overlay.lowerThirdEnabled',
};
const path = pathMap[id];
- updateConfig(path, !Boolean(getDeep(config, path, false)));
+ const current = liveLayers.find(l => l.id === id)?.enabled ?? false;
+ updateConfig(path, !current);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function toggleLayerVisibility(id: LiveTarget) { | |
| const pathMap: Record<LiveTarget, string> = { | |
| nowPlaying: 'spectrum.nowPlaying', | |
| spectrum: 'spectrum.enabled', | |
| logo: 'branding.logoEnabled', | |
| cta: 'branding.ctaEnabled', | |
| watermark: 'branding.watermarkEnabled', | |
| timestamp: 'overlay.timestamp', | |
| lowerThird: 'overlay.lowerThirdEnabled', | |
| }; | |
| const path = pathMap[id]; | |
| updateConfig(path, !Boolean(getDeep(config, path, false))); | |
| } | |
| function toggleLayerVisibility(id: LiveTarget) { | |
| const pathMap: Record<LiveTarget, string> = { | |
| nowPlaying: 'spectrum.nowPlaying', | |
| spectrum: 'spectrum.enabled', | |
| logo: 'branding.logoEnabled', | |
| cta: 'branding.ctaEnabled', | |
| watermark: 'branding.watermarkEnabled', | |
| timestamp: 'overlay.timestamp', | |
| lowerThird: 'overlay.lowerThirdEnabled', | |
| }; | |
| const path = pathMap[id]; | |
| const current = liveLayers.find(l => l.id === id)?.enabled ?? false; | |
| updateConfig(path, !current); | |
| } |
🤖 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 `@frontend/src/components/panels/PreviewPane.tsx` around lines 359 - 371,
Update toggleLayerVisibility to derive the current state from the visible layer
data already computed in liveLayers, such as the matching layer.enabled value,
rather than defaulting getDeep(config, path, false) to false. Use that resolved
visible state to invert the value passed to updateConfig, preserving correct
first-click behavior for default-enabled nowPlaying, spectrum, and logo layers.
| --color-ds-purple-glow: rgba(167, 139, 250, 0.15); | ||
|
|
||
| /* Typography - Premium font stack */ | ||
| --font-sans: "Inter", -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stylelint value-keyword-case errors may fail lint.
Stylelint flags BlinkMacSystemFont (Line 47) and currentColor (Lines 268, 272) as value-keyword-case errors. These are behavior-neutral but will fail a strict stylelint run. Lowercase them (blinkmacsystemfont, currentcolor) or adjust the rule.
Also applies to: 266-273
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 47-47: Expected "BlinkMacSystemFont" to be "blinkmacsystemfont" (value-keyword-case)
(value-keyword-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 `@frontend/src/index.css` at line 47, Update the font-family value in the
--font-sans declaration and the currentColor values around the referenced color
rules to lowercase equivalents, preserving the existing styling while satisfying
Stylelint’s value-keyword-case rule.
Source: Linters/SAST tools
Comment on lines
19
to
+52
| export async function api<T = any>(path: string, options?: RequestInit): Promise<T> { | ||
| const headers = new Headers(options?.headers) | ||
| if (!headers.has('content-type')) headers.set('content-type', 'application/json') | ||
|
|
||
| const response = await fetch(`${API_BASE_URL}${path}`, { ...options, headers }) | ||
| try { | ||
| const response = await fetch(`${API_BASE_URL}${path}`, { ...options, headers }) | ||
|
|
||
| if (!response.ok) { | ||
| const payload = await response.json().catch(() => ({})) as { error?: string } | ||
| throw new ApiError(payload.error || response.statusText, response.status) | ||
| } | ||
| if (!response.ok) { | ||
| const payload = await response.json().catch(() => ({})) as { error?: string } | ||
| const errorMsg = payload.error || response.statusText; | ||
| const error = new ApiError(errorMsg, response.status); | ||
|
|
||
| // Show error toast | ||
| if (showToastFn) { | ||
| showToastFn('error', `API Error: ${errorMsg}`); | ||
| } | ||
|
|
||
| throw error; | ||
| } | ||
|
|
||
| return response.json() as Promise<T> | ||
| return response.json() as Promise<T> | ||
| } catch (error) { | ||
| // Handle network errors | ||
| if (error instanceof ApiError) { | ||
| throw error; | ||
| } | ||
|
|
||
| const networkError = error as Error; | ||
| if (showToastFn) { | ||
| showToastFn('error', `Network Error: ${networkError.message || 'Failed to connect to server'}`); | ||
| } | ||
|
|
||
| throw new ApiError(networkError.message || 'Network error', 0); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
api() now double-toasts every failed request together with its callers.
api() calls showToastFn('error', ...) for both the non-OK-response path and the network-error path, then throws ApiError. Nearly every caller (TargetPanel.inspectTarget/createBatch, PreviewPane.renderPreview/snapshotPreview/sendPreviewToQueue, etc.) already wraps api(...) in try/catch and calls showToast('error', ...) again with a more specific message. Since both paths ultimately invoke the same showToast singleton (wired via setToastHandler(showToast) in App.tsx), every single API failure across the app will now surface two stacked toasts (a generic "API Error/Network Error: ..." plus the caller's specific message).
Pick one layer to own error toasting — either drop the toast calls here and let callers decide, or drop it from api() and centralize it, since callers already have (or usually have) more context-specific messaging.
🐛 Suggested fix (remove toasting here, keep callers in control)
if (!response.ok) {
const payload = await response.json().catch(() => ({})) as { error?: string }
const errorMsg = payload.error || response.statusText;
const error = new ApiError(errorMsg, response.status);
-
- // Show error toast
- if (showToastFn) {
- showToastFn('error', `API Error: ${errorMsg}`);
- }
-
throw error;
}🤖 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 `@frontend/src/lib/api.ts` around lines 19 - 52, Remove both showToastFn
error-toast calls from api(), covering the non-OK response branch and the
network-error catch path. Keep ApiError creation and propagation unchanged so
callers such as TargetPanel and PreviewPane remain responsible for displaying
context-specific error messages.
| @@ -1,4 +1,5 @@ | |||
| /* Modern Professional UI for PidioForge Desktop */ | |||
| @import url('https://fonts.googleapis.com/css2?family=Inter:wght@400;500;600;700;800&display=swap'); | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stylelint errors likely fail lint.
Static analysis flags: import-notation on the @import url(...) (Line 2), font-family-name-quotes for Inter/Roboto (Line 50), and keyframes-name-pattern for slideIn/slideInLeft/scaleIn (Lines 726/737/748). If stylelint gates CI these will fail; switch @import to the plain-string form, unquote generic/known font names, and use kebab-case keyframe names (updating references).
Also applies to: 50-50, 726-748
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 2-2: Expected "url('https://fonts.googleapis.com/css2?family=Inter:wght@400;500;600;700;800&display=swap')" to be "'https://fonts.googleapis.com/css2?family=Inter:wght@400;500;600;700;800&display=swap'" (import-notation)
(import-notation)
🤖 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 `@frontend/src/styles.css` at line 2, Update the stylesheet to satisfy
Stylelint: change the Google Fonts `@import` in the stylesheet to plain-string
import notation, remove quotes from the Inter/Roboto font-family names, and
rename slideIn, slideInLeft, and scaleIn keyframes to kebab-case names while
updating every corresponding animation reference.
Source: Linters/SAST tools
Comment on lines
+366
to
+383
| font-size: 6px; | ||
| color: var(--text-secondary); | ||
| text-align: right; | ||
| font-weight: 600; | ||
| letter-spacing: 0.3px; | ||
| } | ||
|
|
||
| input, | ||
| select, | ||
| textarea { | ||
| width: 100%; | ||
| min-height: 38px; | ||
| padding: 8px 12px; | ||
| min-height: 20px; | ||
| padding: 2px 4px; | ||
| background: var(--surface); | ||
| border: 1px solid var(--border-medium); | ||
| border: 2px solid var(--border-medium); | ||
| border-radius: var(--radius-sm); | ||
| color: var(--text-primary); | ||
| font-size: 13px; | ||
| transition: all var(--transition-fast); | ||
| font-size: 7px; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether legacy class selectors are still rendered by current panels
rg -n 'className="field"|class="field"|targetGroup|targetSummary' frontend/src || \
rg -n '\bfield\b|targetGroup|targetSummary' frontend/src/componentsRepository: asrocia/pidioforge-desktop
Length of output: 528
Raise the minimum font sizes here
input, select, and textarea at 7px, plus .field span at 6px and .targetSummary small at 5px, are below the design system’s readable text tokens. These sizes will be hard to read wherever the selectors apply; align them with the existing typography scale instead.
🤖 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 `@frontend/src/styles.css` around lines 366 - 383, Increase the font sizes in
the shared input/select/textarea rule and the nearby .field span and
.targetSummary small rules to match the existing readable typography tokens.
Replace the current 7px, 6px, and 5px values with the appropriate established
scale values while preserving the other styling.
Comment on lines
+39
to
53
| // Request animation frame wrapper for smooth animations | ||
| export function rafThrottle<T extends (...args: any[]) => any>( | ||
| func: T | ||
| ): (...args: Parameters<T>) => void { | ||
| let rafId: number | null = null; | ||
|
|
||
| return duration; | ||
| } | ||
|
|
||
| /** | ||
| * Performance observer for tracking long tasks | ||
| */ | ||
| export class PerformanceMonitor { | ||
| private observer: PerformanceObserver | null = null; | ||
| private longTasks: PerformanceEntry[] = []; | ||
| private readonly threshold: number; | ||
|
|
||
| constructor(threshold: number = 50) { | ||
| this.threshold = threshold; | ||
| } | ||
|
|
||
| start() { | ||
| if (!('PerformanceObserver' in window)) { | ||
| console.warn('PerformanceObserver not supported'); | ||
| return; | ||
| } | ||
|
|
||
| this.observer = new PerformanceObserver((list) => { | ||
| for (const entry of list.getEntries()) { | ||
| if (entry.duration > this.threshold) { | ||
| this.longTasks.push(entry); | ||
| console.warn(`[Performance] Long task detected: ${entry.name} (${entry.duration.toFixed(2)}ms)`); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| this.observer.observe({ entryTypes: ['measure', 'navigation', 'resource'] }); | ||
| } catch (e) { | ||
| console.warn('Failed to start performance observer:', e); | ||
| } | ||
| } | ||
|
|
||
| stop() { | ||
| if (this.observer) { | ||
| this.observer.disconnect(); | ||
| this.observer = null; | ||
| } | ||
| } | ||
|
|
||
| getLongTasks() { | ||
| return [...this.longTasks]; | ||
| } | ||
|
|
||
| clearLongTasks() { | ||
| this.longTasks = []; | ||
| } | ||
|
|
||
| getStats() { | ||
| const totalDuration = this.longTasks.reduce((sum, task) => sum + task.duration, 0); | ||
| const avgDuration = this.longTasks.length > 0 ? totalDuration / this.longTasks.length : 0; | ||
| return function executedFunction(...args: Parameters<T>) { | ||
| if (rafId) return; | ||
|
|
||
| return { | ||
| count: this.longTasks.length, | ||
| totalDuration, | ||
| avgDuration, | ||
| maxDuration: Math.max(...this.longTasks.map(t => t.duration), 0) | ||
| }; | ||
| } | ||
| rafId = requestAnimationFrame(() => { | ||
| func(...args); | ||
| rafId = null; | ||
| }); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Always use the latest arguments when the animation frame fires.
The current rafThrottle captures the args from the first call and queues it. Any subsequent calls before the next animation frame are dropped, meaning when the frame finally renders, the callback will execute with stale data rather than the most recent input (e.g., the final mouse coordinates).
Update the closure to track and use the most recent arguments.
🐛 Proposed fix
export function rafThrottle<T extends (...args: any[]) => any>(
func: T
): (...args: Parameters<T>) => void {
let rafId: number | null = null;
+ let lastArgs: Parameters<T> | null = null;
return function executedFunction(...args: Parameters<T>) {
+ lastArgs = args;
+
if (rafId) return;
rafId = requestAnimationFrame(() => {
- func(...args);
+ const argsToUse = lastArgs;
rafId = null;
+ lastArgs = null;
+ if (argsToUse) func(...argsToUse);
});
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Request animation frame wrapper for smooth animations | |
| export function rafThrottle<T extends (...args: any[]) => any>( | |
| func: T | |
| ): (...args: Parameters<T>) => void { | |
| let rafId: number | null = null; | |
| return duration; | |
| } | |
| /** | |
| * Performance observer for tracking long tasks | |
| */ | |
| export class PerformanceMonitor { | |
| private observer: PerformanceObserver | null = null; | |
| private longTasks: PerformanceEntry[] = []; | |
| private readonly threshold: number; | |
| constructor(threshold: number = 50) { | |
| this.threshold = threshold; | |
| } | |
| start() { | |
| if (!('PerformanceObserver' in window)) { | |
| console.warn('PerformanceObserver not supported'); | |
| return; | |
| } | |
| this.observer = new PerformanceObserver((list) => { | |
| for (const entry of list.getEntries()) { | |
| if (entry.duration > this.threshold) { | |
| this.longTasks.push(entry); | |
| console.warn(`[Performance] Long task detected: ${entry.name} (${entry.duration.toFixed(2)}ms)`); | |
| } | |
| } | |
| }); | |
| try { | |
| this.observer.observe({ entryTypes: ['measure', 'navigation', 'resource'] }); | |
| } catch (e) { | |
| console.warn('Failed to start performance observer:', e); | |
| } | |
| } | |
| stop() { | |
| if (this.observer) { | |
| this.observer.disconnect(); | |
| this.observer = null; | |
| } | |
| } | |
| getLongTasks() { | |
| return [...this.longTasks]; | |
| } | |
| clearLongTasks() { | |
| this.longTasks = []; | |
| } | |
| getStats() { | |
| const totalDuration = this.longTasks.reduce((sum, task) => sum + task.duration, 0); | |
| const avgDuration = this.longTasks.length > 0 ? totalDuration / this.longTasks.length : 0; | |
| return function executedFunction(...args: Parameters<T>) { | |
| if (rafId) return; | |
| return { | |
| count: this.longTasks.length, | |
| totalDuration, | |
| avgDuration, | |
| maxDuration: Math.max(...this.longTasks.map(t => t.duration), 0) | |
| }; | |
| } | |
| rafId = requestAnimationFrame(() => { | |
| func(...args); | |
| rafId = null; | |
| }); | |
| }; | |
| } | |
| // Request animation frame wrapper for smooth animations | |
| export function rafThrottle<T extends (...args: any[]) => any>( | |
| func: T | |
| ): (...args: Parameters<T>) => void { | |
| let rafId: number | null = null; | |
| let lastArgs: Parameters<T> | null = null; | |
| return function executedFunction(...args: Parameters<T>) { | |
| lastArgs = args; | |
| if (rafId) return; | |
| rafId = requestAnimationFrame(() => { | |
| const argsToUse = lastArgs; | |
| rafId = null; | |
| lastArgs = null; | |
| if (argsToUse) func(...argsToUse); | |
| }); | |
| }; | |
| } |
🤖 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 `@frontend/src/utils/performance.ts` around lines 39 - 53, Update rafThrottle
so each invocation stores its arguments in shared closure state, replacing
earlier values before the queued animation frame runs. Have the
requestAnimationFrame callback invoke func with the latest stored arguments,
then clear both the pending frame state and stored arguments.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add DialogProvider with showConfirm/showPrompt (replaces 9 native prompt/confirm/alert calls) - Unify Button: design-system-components and panel-primitives now delegate to shared CVA button - Add aria-live/role=status to 7 message regions across panels - Add aria-label to stem drag handles and volume sliders - Add role=status to SettingsPanel loading state Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Extract usePreviewActions (render, snapshot, send-to-queue) - Extract useQueueActions (start, cancel, reset, clearLogs) - Extract useLiveDrag (drag/resize/nudge/layer management) - Add PreviewPane.ui.test.tsx regression guard - 810 LOC → orchestrator + 4 hook files Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd tooling Includes: - Backend engine updates (config, render, preview, media, target, http-utils) - Electron IPC and preload updates - New UI components (GalleryInput, LayerOrderInput, LiveOverlayCanvas, Preview subcomponents) - Updated design system, types, hooks (useLyricPreview, useSafeElectronBridge, usePreviewStream) - Smoke test tools (gallery, layers, packaged) - Utility updates (stem-utils, format-presets, media, performance) - Test coverage for new components - Fix set-state-in-effect lint errors in UpdateBanner and useLyricPreview Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add trailing edge to throttle() to prevent dropped final events - Make setupGlobalErrorHandler() idempotent with guard flag - Add role=status and aria-live=polite to toast container - Deduplicate preloadResource() to prevent duplicate link elements - Update throttle test to verify trailing edge behavior Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Fix all 52 ESLint errors/warnings across backend, tools, and electron - Add Node.js globals (fetch, URL, structuredClone, etc.) to eslint config - Fix unused vars, empty catch blocks, useless escapes, irregular whitespace - Add tools/rebuild-sqlite-dev.mjs for automatic vendor SQLite rebuild - Wire rebuild into dev:api and tools/dev.mjs for seamless dev experience - Add smoke:project (CRUD, export/import, templates, presets) - Add smoke:pipeline (config → validate → render → history → clear) - Add smoke:presets (all preset lists, save/apply/delete, validate) - All 3 new E2E smokes pass Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Remove double overflow-y-auto from SettingsPane container - Strip redundant scroll wrapper from HelpPanel to match other panels - Ensure only one master scrollbar exists per column Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Move audit/testing/performance/preview/release docs into docs/ - Add docs/README.md documentation index - Update root README with docs link and latest validation numbers - Finalize single-scroll layout cleanup for HelpPanel/TemplatesPanel Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
asrocia
force-pushed
the
feat/redesign-ui-final
branch
from
July 23, 2026 15:45
c954700 to
9744e07
Compare
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\n- redesign UI desktop agar panel lebih konsisten dan modular\n- ubah Layer Editor jadi source-style list dengan visibility + lock controls\n- sederhanakan Rendering Settings dengan advanced toggle\n- samakan CTA utama, status colors, dan reusable cards/callouts/stats\n- tambah coverage endpoint backend agar frontend redesign tidak 404\n\n## Verification\n- npm run typecheck lulus\n- npm run build:web lulus\n- Vitest komponen design system 10/10\n\n🤖 Generated with Claude Code