Conversation
brionmario
requested review from
DonOmalVindula and
jeradrutnam
as code owners
📝 WalkthroughWalkthroughIntroduces a comprehensive design system featuring reusable UI components, custom React hooks, and complete theme/layout builder modules with API integration. Adds notification infrastructure, CRUD operations for layouts and themes, with associated editor pages, form validation schemas, and TypeScript models. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
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. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Implements the initial “Design UI” experience by adding full CRUD flows for Themes and Layouts, plus a unified Design Studio with live preview and supporting design-system utilities.
Changes:
- Added Themes & Layouts list pages, builders, previews, and API hooks (React Query) for create/update/delete/duplicate/bulk delete.
- Introduced Design Studio page with left/right drawers for theme + layout controls and responsive device preview.
- Added shared design-system components/hooks for search, notifications, confirmations, unsaved-changes detection, keyboard shortcuts, multi-select, and color picking.
Reviewed changes
Copilot reviewed 61 out of 61 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/apps/thunder-develop/src/features/themes/schemas/themeSchema.ts | Adds Zod schema + defaults for theme builder form validation. |
| frontend/apps/thunder-develop/src/features/themes/pages/ThemesListPage.tsx | New themes listing page with search + navigation to builder. |
| frontend/apps/thunder-develop/src/features/themes/pages/ThemeBuilderPage.tsx | New theme builder page with save flow, preview, and unsaved-changes UX. |
| frontend/apps/thunder-develop/src/features/themes/models/theme.ts | Defines Theme/ThemeConfig TypeScript models. |
| frontend/apps/thunder-develop/src/features/themes/models/responses.ts | Defines themes listing API response model. |
| frontend/apps/thunder-develop/src/features/themes/models/requests.ts | Defines create/update request payload types for themes. |
| frontend/apps/thunder-develop/src/features/themes/constants/theme-query-keys.ts | Adds React Query cache keys for themes. |
| frontend/apps/thunder-develop/src/features/themes/components/ThemesList.tsx | Theme card grid with search, multi-select, context menu, bulk delete. |
| frontend/apps/thunder-develop/src/features/themes/components/ThemePreviewPlaceholder.tsx | Thumbnail preview placeholder for theme cards. |
| frontend/apps/thunder-develop/src/features/themes/components/ThemePreview.tsx | Full preview rendering with MUI ThemeProvider + optional layout influence. |
| frontend/apps/thunder-develop/src/features/themes/components/ThemeDeleteDialog.tsx | Confirm dialog for deleting a single theme. |
| frontend/apps/thunder-develop/src/features/themes/components/ThemeBuilderForm.tsx | RHF + Zod theme editing form (light/dark scheme editing). |
| frontend/apps/thunder-develop/src/features/themes/api/useUpdateTheme.ts | React Query mutation for updating a theme. |
| frontend/apps/thunder-develop/src/features/themes/api/useGetThemes.ts | React Query query for listing themes (pagination params). |
| frontend/apps/thunder-develop/src/features/themes/api/useGetTheme.ts | React Query query for fetching a theme by id. |
| frontend/apps/thunder-develop/src/features/themes/api/useDuplicateTheme.ts | Duplicates a theme by GET then POST copy. |
| frontend/apps/thunder-develop/src/features/themes/api/useDeleteTheme.ts | React Query mutation for deleting a theme. |
| frontend/apps/thunder-develop/src/features/themes/api/useCreateTheme.ts | React Query mutation for creating a theme. |
| frontend/apps/thunder-develop/src/features/themes/api/useBulkDeleteThemes.ts | Bulk delete themes mutation (parallel DELETEs). |
| frontend/apps/thunder-develop/src/features/layouts/schemas/layoutSchema.ts | Adds Zod schema + defaults for layout builder form validation. |
| frontend/apps/thunder-develop/src/features/layouts/pages/LayoutsListPage.tsx | New layouts listing page with search + navigation to builder. |
| frontend/apps/thunder-develop/src/features/layouts/pages/LayoutBuilderPage.tsx | New layout builder page with save flow, preview, and unsaved-changes UX. |
| frontend/apps/thunder-develop/src/features/layouts/models/responses.ts | Defines layouts listing API response model. |
| frontend/apps/thunder-develop/src/features/layouts/models/requests.ts | Defines create/update request payload types for layouts. |
| frontend/apps/thunder-develop/src/features/layouts/models/layout.ts | Defines Layout/LayoutConfig TypeScript models. |
| frontend/apps/thunder-develop/src/features/layouts/constants/layout-query-keys.ts | Adds React Query cache keys for layouts. |
| frontend/apps/thunder-develop/src/features/layouts/components/LayoutsList.tsx | Layout card grid with search, multi-select, context menu, bulk delete. |
| frontend/apps/thunder-develop/src/features/layouts/components/LayoutPreviewPlaceholder.tsx | Thumbnail preview placeholder for layout cards. |
| frontend/apps/thunder-develop/src/features/layouts/components/LayoutPreview.tsx | Layout preview component used by layout builder/design studio. |
| frontend/apps/thunder-develop/src/features/layouts/components/LayoutDeleteDialog.tsx | Confirm dialog for deleting a single layout. |
| frontend/apps/thunder-develop/src/features/layouts/components/LayoutBuilderForm.tsx | RHF + Zod layout editing form. |
| frontend/apps/thunder-develop/src/features/layouts/api/useUpdateLayout.ts | React Query mutation for updating a layout. |
| frontend/apps/thunder-develop/src/features/layouts/api/useGetLayouts.ts | React Query query for listing layouts (pagination params). |
| frontend/apps/thunder-develop/src/features/layouts/api/useGetLayout.ts | React Query query for fetching a layout by id. |
| frontend/apps/thunder-develop/src/features/layouts/api/useDuplicateLayout.ts | Duplicates a layout by GET then POST copy. |
| frontend/apps/thunder-develop/src/features/layouts/api/useDeleteLayout.ts | React Query mutation for deleting a layout. |
| frontend/apps/thunder-develop/src/features/layouts/api/useCreateLayout.ts | React Query mutation for creating a layout. |
| frontend/apps/thunder-develop/src/features/layouts/api/useBulkDeleteLayouts.ts | Bulk delete layouts mutation (parallel DELETEs). |
| frontend/apps/thunder-develop/src/features/design/pages/DesignStudioPage.tsx | Unified studio UI combining theme editing + layout selection + device preview + save dialog. |
| frontend/apps/thunder-develop/src/features/design/models/design.ts | Adds device viewport constants and (future) combined design config types. |
| frontend/apps/thunder-develop/src/features/design/components/ThemeSaveDialog.tsx | Save dialog for naming/overriding themes from design studio. |
| frontend/apps/thunder-develop/src/features/design/components/ThemeControls.tsx | Theme editor controls for design studio (color pickers, typography, etc.). |
| frontend/apps/thunder-develop/src/features/design/components/Ruler.tsx | Ruler component for layout builder canvas. |
| frontend/apps/thunder-develop/src/features/design/components/LayoutControls.tsx | Layout selector panel for design studio. |
| frontend/apps/thunder-develop/src/features/design/components/LayoutBuilder.tsx | In-studio layout builder flow with zoom/rulers/grid + save. |
| frontend/apps/thunder-develop/src/features/design/components/DeviceSelector.tsx | Device viewport toggle for responsive preview. |
| frontend/apps/thunder-develop/src/features/design-system/providers/NotificationProvider.tsx | Adds snackbar/toast notification provider. |
| frontend/apps/thunder-develop/src/features/design-system/hooks/useUnsavedChanges.ts | Adds browser unload protection + dialog state for unsaved changes. |
| frontend/apps/thunder-develop/src/features/design-system/hooks/useNotification.ts | Hook for accessing notifications from provider. |
| frontend/apps/thunder-develop/src/features/design-system/hooks/useMultiSelect.ts | Generic multi-select state hook used by list pages. |
| frontend/apps/thunder-develop/src/features/design-system/hooks/useKeyboardShortcuts.ts | Global keyboard shortcut registration hook. |
| frontend/apps/thunder-develop/src/features/design-system/hooks/useDebounce.ts | Generic debounce hook used for search/preview updates. |
| frontend/apps/thunder-develop/src/features/design-system/components/UnsavedChangesDialog.tsx | Modal dialog prompting to save/discard/cancel navigation. |
| frontend/apps/thunder-develop/src/features/design-system/components/SimpleColorPicker.tsx | Standalone color picker input used by studio controls. |
| frontend/apps/thunder-develop/src/features/design-system/components/SearchField.tsx | Reusable search input with clear button. |
| frontend/apps/thunder-develop/src/features/design-system/components/EmptyState.tsx | Reusable empty state component with optional action. |
| frontend/apps/thunder-develop/src/features/design-system/components/ConfirmDialog.tsx | Reusable confirm dialog with async loading state. |
| frontend/apps/thunder-develop/src/features/design-system/components/ColorPickerField.tsx | RHF-integrated color picker with contrast assist. |
| frontend/apps/thunder-develop/src/features/design-system/components/CardSkeleton.tsx | Card-grid loading skeleton. |
| frontend/apps/thunder-develop/src/features/design-system/components/BulkActionBar.tsx | Sticky bulk action bar for multi-select lists. |
Comments suppressed due to low confidence (6)
frontend/apps/thunder-develop/src/features/themes/pages/ThemeBuilderPage.tsx:1
isSavingis referenced before it’s declared (it’s defined later at line 169), which will throw at render time due to the temporal dead zone forconst. ComputeisSavingbefore callinguseKeyboardShortcuts(or inline it asisCreating || isUpdating) so the callback can reference it safely.
frontend/apps/thunder-develop/src/features/layouts/pages/LayoutBuilderPage.tsx:1isSavingis referenced before it’s declared (declared later at line 169), which will throw at render time. Move theconst isSaving = ...computation aboveuseKeyboardShortcuts(or inline it) to avoid the TDZ error.
frontend/apps/thunder-develop/src/features/layouts/components/LayoutPreview.tsx:1signinScreen.backgroundis modeled as an object ({ type, value }), but the code treats it as a string and assigns it directly tobgcolorlater. Also,container.borderRadiusis modeled as a number, but the code falls back to'8px'(string). This will lead to invalid styles at runtime. Parsebackgroundbased ontype(solid/gradient/image) and normalizeborderRadiusconsistently (e.g., number →${n}px).
frontend/apps/thunder-develop/src/features/themes/components/ThemePreviewPlaceholder.tsx:1- The generated SVG gradient
ids only depend oncolorScheme, so multiple cards on the same page will reuse the same IDs. Duplicate SVG IDs can cause gradients to resolve unpredictably across elements. Include a unique suffix (e.g., theme id) in the gradient IDs and theirurl(#...)references.
frontend/apps/thunder-develop/src/features/themes/components/ThemePreview.tsx:1 widthandheightare accepted as props but never used, which makes the API confusing for callers (e.g.,DesignStudioPagesupplies them). Either apply them to the preview container styles (e.g., fixed sizing) or remove them from the props.
frontend/apps/thunder-develop/src/features/themes/pages/ThemeBuilderPage.tsx:1formRefis declared but not used anywhere in this file. If it’s not needed, remove it; if it’s intended for imperative submit, wire it toThemeBuilderFormand initialize it asuseRef<... | null>(null)to avoid an implicitundefinedref value.
| const handleSaveTheme = async (themeName: string, override: boolean) => { | ||
| try { | ||
| if (override && selectedBaseThemeId) { | ||
| // Override existing theme | ||
| await updateTheme.mutateAsync({ | ||
| id: selectedBaseThemeId, | ||
| data: { | ||
| displayName: themeName, | ||
| theme: themeConfig, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
useUpdateTheme expects { themeId, themeData } (per UpdateThemeParams), but this code passes { id, data }. This will fail type-checking and/or break at runtime. Update the call to pass the correct keys and payload shape (e.g., themeId: selectedBaseThemeId, themeData: { displayName, theme }).
| const [themeName, setThemeName] = useState(currentName); | ||
| const [saveMode, setSaveMode] = useState<'new' | 'override'>(baseThemeName ? 'new' : 'new'); |
There was a problem hiding this comment.
themeName is initialized from currentName only once; if the dialog is reopened with a different currentName, the input will keep the stale previous value unless manually changed. Consider resetting themeName/saveMode inside an effect when open becomes true (and simplify the redundant ternary for saveMode).
| */ | ||
| export default function useUnsavedChanges(isDirty: boolean): UseUnsavedChangesReturn { | ||
| const [showDialog, setShowDialog] = useState(false); | ||
| const [pendingNavigation, setPendingNavigation] = useState<(() => void) | null>(null); |
There was a problem hiding this comment.
pendingNavigation is never set anywhere in this hook, so confirmNavigation’s conditional execution path is effectively dead code. Either remove pendingNavigation entirely or expose an API that allows callers to register a pending navigation action so the hook’s behavior matches the comment/docs.
| const confirmNavigation = useCallback(() => { | ||
| setShowDialog(false); | ||
| if (pendingNavigation) { | ||
| pendingNavigation(); | ||
| setPendingNavigation(null); | ||
| } | ||
| }, [pendingNavigation]); |
There was a problem hiding this comment.
pendingNavigation is never set anywhere in this hook, so confirmNavigation’s conditional execution path is effectively dead code. Either remove pendingNavigation entirely or expose an API that allows callers to register a pending navigation action so the hook’s behavior matches the comment/docs.
| const handleColorChange = (path: string, value: string) => { | ||
| const keys = path.split('.'); | ||
| const newConfig = JSON.parse(JSON.stringify(themeConfig)); // Deep clone | ||
| let current: any = newConfig; |
There was a problem hiding this comment.
Deep-cloning the entire theme config via JSON.parse(JSON.stringify(...)) on every color change can become expensive and will also drop any non-JSON-safe values if they’re introduced later. Prefer structuredClone(themeConfig) (where available) or a targeted immutable update (shallow clone only along the update path), or consider using an immutable helper like Immer.
| <IconButton | ||
| aria-label="Clear search" | ||
| onClick={handleClear} |
There was a problem hiding this comment.
The aria-label is hard-coded in English. Since the rest of the UI uses i18n, this should be localized (e.g., using useTranslation) so screen reader users get the correct language.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In
`@frontend/apps/thunder-develop/src/features/design-system/components/BulkActionBar.tsx`:
- Line 20: The import in BulkActionBar.tsx currently pulls Button, Paper, Stack,
Typography from '@mui/material' causing inconsistent UI library usage; update
the import to use the same library used elsewhere ('@wso2/oxygen-ui') by
replacing the import statement that references Button, Paper, Stack, Typography
so the component uses `@wso2/oxygen-ui` exports and aligns theming and bundle
usage with ThemesList.tsx and LayoutBuilderPage.tsx.
In
`@frontend/apps/thunder-develop/src/features/design-system/components/UnsavedChangesDialog.tsx`:
- Around line 88-107: The Dialog currently calls onCancel via onClose which
allows ESC/backdrop to cancel during an in-flight save; modify the Dialog's
onClose handler to accept (event, reason) and, when isSaving is true, ignore
reasons 'backdropClick' and 'escapeKeyDown' (thus preventing cancel while
saving) and also set disableEscapeKeyDown={isSaving} on the Dialog; keep calling
onCancel/onDiscard when isSaving is false or when reason is not a user-initiated
close. Reference: the Dialog component's onClose prop, the onCancel callback,
the isSaving flag, and handleSave.
In
`@frontend/apps/thunder-develop/src/features/design-system/hooks/useUnsavedChanges.ts`:
- Around line 28-35: The pendingNavigation callback is never set so
confirmNavigation cannot proceed; update showUnsavedDialog to accept and store a
callback into pendingNavigation (or alternatively remove pendingNavigation usage
and the related methods), then make confirmNavigation invoke the stored
callback, clear pendingNavigation, and hide the dialog; update the
types/signature of showUnsavedDialog (and related docs/comments) and ensure
hideUnsavedDialog/cancelNavigation also clear pendingNavigation to avoid stale
references (affecting useUnsavedChanges, showUnsavedDialog, confirmNavigation,
hideUnsavedDialog, cancelNavigation).
In
`@frontend/apps/thunder-develop/src/features/design/components/DeviceSelector.tsx`:
- Around line 48-55: The icon-only device buttons rendered in the devices.map
block (using Tooltip and Button, with props selectedDevice and onDeviceChange
and looking up sizes in DEVICE_VIEWPORTS) lack accessible names; add an
accessible label to each button by providing an aria-label (or aria-labelledby
to a visually-hidden element) that uses the device label and size (e.g.,
`${label} ${DEVICE_VIEWPORTS[type].width}x${DEVICE_VIEWPORTS[type].height}`) so
screen readers can announce the control while keeping the visual UI the same.
In
`@frontend/apps/thunder-develop/src/features/design/components/LayoutBuilder.tsx`:
- Around line 78-82: The code in LayoutBuilder applies zoom twice by computing
scaledWidth/scaledHeight from canvasWidth/canvasHeight using zoom and also
applying a CSS transform scale; pick one strategy and remove the other. For a
minimal fix: keep canvasWidth and canvasHeight as the source sizes and remove
use of scaledWidth/scaledHeight (and any calculations using them) so the CSS
transform(scale(zoom/100)) on the canvas container handles visual zoom;
alternatively, if you prefer sizing-based zoom, remove the transform and use
scaledWidth/scaledHeight everywhere instead. Update all usages of
scaledWidth/scaledHeight (including the other occurrence around the block
referenced at 231-238) and ensure zoom (the zoom prop/variable) is only applied
once.
In `@frontend/apps/thunder-develop/src/features/design/components/Ruler.tsx`:
- Around line 37-46: The Ruler component can crash when zoom ≤ 0 because
adjustedStep becomes non-positive and count/Array.from fails; clamp zoom to a
safe minimum before computing adjustedStep (e.g., const safeZoom =
Math.max(zoom, 1)) or guard adjustedStep with Math.max(adjustedStep, 1) so
adjustedStep > 0, then compute count = Math.max(1, Math.ceil(size /
adjustedStep)) and build markers from that count; update uses of
zoom/adjustedStep in the Ruler function (symbols: Ruler, zoom, adjustedStep,
count, markers) accordingly.
In
`@frontend/apps/thunder-develop/src/features/design/pages/DesignStudioPage.tsx.backup`:
- Around line 1-666: The file is a temporary backup (filename ends with .backup)
containing the exported React component DesignStudioPage and should not be
committed; either delete the backup file, or if you intend to keep an alternate
implementation rename it (e.g., DesignStudioPageAlt.tsx) and add a short comment
documenting its purpose near the exported DesignStudioPage alternative, and add
"*.backup" to .gitignore to prevent future accidental commits.
In
`@frontend/apps/thunder-develop/src/features/layouts/api/useBulkDeleteLayouts.ts`:
- Around line 34-51: The mutation in useBulkDeleteLayouts currently invalidates
queries in onSuccess, which won't run if Promise.all rejects; change the
mutation option to use onSettled instead of onSuccess and call
queryClient.invalidateQueries({queryKey: [LayoutQueryKeys.LAYOUTS]}). Ensure the
onSettled handler has the same invalidate call and swallows errors like the
current catch(), so the cache is refreshed regardless of whether some deletes
failed; update references in useBulkDeleteLayouts and the mutation config
accordingly.
In
`@frontend/apps/thunder-develop/src/features/layouts/components/LayoutPreview.tsx`:
- Around line 36-55: The layout preview is using background as if it were a
simple color and uses || which hides valid zero-values and returns a string
borderRadius; update LayoutPreview (variables signinScreen, template, container,
background) to treat background as a ScreenBackground object: compute a
background style/value from background.type and background.value (e.g. use
bgcolor when type === 'color', backgroundImage/gradient when type === 'gradient'
or 'image') and apply that to the Box sx; replace all || defaults with nullish
coalescing (??) for container.maxWidth, container.padding, container.elevation,
etc., and ensure container.borderRadius is normalized to a number (use
Number(...)? ?? defaultNumber) instead of a string default so the model types
are respected.
In
`@frontend/apps/thunder-develop/src/features/layouts/pages/LayoutBuilderPage.tsx`:
- Around line 73-86: The keyboard shortcut callback uses isSaving before it's
declared, causing a TDZ ReferenceError; move the isSaving state/variable
declaration so it appears before the useKeyboardShortcuts call (or compute a
stable derived value used by the shortcut) — locate the isSaving identifier in
this component and hoist its declaration above the useKeyboardShortcuts
invocation (ensuring handleSave, formData and isDirty remain available to the
callback).
- Around line 219-225: The LayoutPreview prop name is wrong: change the prop
passed to the LayoutPreview component from layoutConfig to layout so it matches
the LayoutPreview props interface; update the JSX where LayoutPreview is used
(LayoutPreview component invocation) to pass layout={debouncedFormData.layout}
instead of layoutConfig={...} so the LayoutPreview component receives the
expected layout prop.
In
`@frontend/apps/thunder-develop/src/features/themes/api/useBulkDeleteThemes.ts`:
- Around line 34-51: The bulk-delete mutation in useBulkDeleteThemes uses
Promise.all inside mutationFn and only invalidates the cache in onSuccess, so if
any DELETE fails the Promise rejects and onSuccess never runs leaving stale
cache; update mutation to either use Promise.allSettled for the
themeIds.map(http.request...) call or keep Promise.all but ensure failures don't
short-circuit, and move the queryClient.invalidateQueries call from onSuccess to
onSettled (referencing useBulkDeleteThemes, mutationFn, themeIds, and the
http.request DELETE calls) so the Themes cache (ThemeQueryKeys.THEMES) is
invalidated regardless of individual delete outcomes.
In
`@frontend/apps/thunder-develop/src/features/themes/components/ThemeBuilderForm.tsx`:
- Around line 72-89: The form's defaultValues passed to useForm in
ThemeBuilderForm are only applied on mount, so when initialValues changes (e.g.,
after an API load) the form doesn't update; add a useEffect that watches
initialValues and calls the useForm reset(initialValues) to sync the form state
(reseting errors/dirty as appropriate) so watch(), handleSubmit, and
onValuesChange reflect the new data. Ensure you reference the existing
control/handleSubmit/watch/reset from useForm and call reset(initialValues)
inside the effect that depends on initialValues.
In
`@frontend/apps/thunder-develop/src/features/themes/components/ThemePreview.tsx`:
- Around line 42-82: The component ThemePreview currently returns early when
themeConfig is falsy which causes the later call to useMemo to run conditionally
and violate React Hooks rules; change the prop signature to accept theme?:
ThemeConfig | null (or remove the internal null check and require the parent to
handle loading), and move all hooks (e.g., the useTranslation call and the
useMemo usage referenced by the existing useMemo call) to the top of
ThemePreview so they run unconditionally before any early returns; then perform
the themeConfig null check only after hooks and render the loading Box when
themeConfig is null.
In
`@frontend/apps/thunder-develop/src/features/themes/components/ThemePreviewPlaceholder.tsx`:
- Around line 65-88: The SVG gradient IDs in ThemePreviewPlaceholder (e.g.,
primaryGrad-${colorScheme} and secondaryGrad-${colorScheme}) can collide across
instances; import and call React's useId inside the ThemePreviewPlaceholder
component to generate a unique baseId, concatenate it with your colorScheme
(e.g., `${baseId}-primary-${colorScheme}` /
`${baseId}-secondary-${colorScheme}`) and replace the id attributes on
<linearGradient> and the corresponding fill="url(#...)" usages so each instance
has unique gradient IDs; ensure you add the useId import and use the generated
ids consistently where ids and url(...) are referenced.
In
`@frontend/apps/thunder-develop/src/features/themes/constants/theme-query-keys.ts`:
- Around line 1-27: Rename the file containing the ThemeQueryKeys constant from
theme-query-keys.ts to a camelCase name such as themeQueryKeys.ts, update all
imports that reference the old filename to the new filename, and ensure the
exported symbol ThemeQueryKeys remains unchanged (export default ThemeQueryKeys)
so consumers continue to work; run a quick search for "theme-query-keys" across
the repo and replace with "themeQueryKeys" in import paths to complete the
change.
In
`@frontend/apps/thunder-develop/src/features/themes/pages/ThemeBuilderPage.tsx`:
- Around line 73-86: The keyboard shortcut callback used in useKeyboardShortcuts
references isSaving before it's declared which produces a TDZ ReferenceError;
move the isSaving declaration (and any related state or variables used by the
callback such as formData and handleSave) above the useKeyboardShortcuts call so
the callback closes over already-initialized values, ensuring
useKeyboardShortcuts(...) runs after isSaving is defined.
🟡 Minor comments (15)
frontend/apps/thunder-develop/src/features/design-system/components/ConfirmDialog.tsx-106-112 (1)
106-112:⚠️ Potential issue | 🟡 MinorDialog can be dismissed during async operation via backdrop click or Escape key.
While the buttons are correctly disabled during loading, users can still close the dialog by clicking the backdrop or pressing Escape. This could lead to confusing UX where the async operation continues in the background after the dialog closes.
Consider disabling close during loading:
Proposed fix to prevent close during loading
<Dialog open={open} - onClose={onClose} + onClose={isLoading ? undefined : onClose} aria-labelledby="confirm-dialog-title" aria-describedby="confirm-dialog-description" maxWidth="sm" fullWidth + disableEscapeKeyDown={isLoading} >frontend/apps/thunder-develop/src/features/themes/api/useDeleteTheme.ts-34-46 (1)
34-46:⚠️ Potential issue | 🟡 MinorAlso invalidate per-theme detail cache after deletion.
A detail query exists using[ThemeQueryKeys.THEME, themeId](seeuseGetTheme.ts). When deleting a theme, both the list and detail caches should be cleared to prevent stale data. This pattern is already established inuseUpdateTheme.tsand should be consistent here.🔧 Suggested patch
- onSuccess: () => { - queryClient.invalidateQueries({queryKey: [ThemeQueryKeys.THEMES]}).catch(() => {}); - }, + onSuccess: (_data, themeId) => { + queryClient.invalidateQueries({queryKey: [ThemeQueryKeys.THEMES]}).catch(() => {}); + queryClient.invalidateQueries({queryKey: [ThemeQueryKeys.THEME, themeId]}).catch(() => {}); + },frontend/apps/thunder-develop/src/features/themes/components/ThemeDeleteDialog.tsx-42-50 (1)
42-50:⚠️ Potential issue | 🟡 MinorPass the Error object to logger.error for proper stack capture.
The current call
logger.error('Failed to delete theme', {error, themeId})passes a context object as the second parameter. The logger'serror()method signature iserror(message: string, errorOrContext?: Error | LogContext, context?: LogContext)— when a non-Error object is passed as the second parameter, it's treated as context and the Error is not captured separately.🔧 Suggested patch
- onError: (error) => { - logger.error('Failed to delete theme', {error, themeId}); - }, + onError: (error) => { + logger.error('Failed to delete theme', error, {themeId}); + },frontend/apps/thunder-develop/src/features/design/components/ThemeSaveDialog.tsx-65-66 (1)
65-66:⚠️ Potential issue | 🟡 MinorRedundant ternary and stale state initialization.
Two issues here:
Line 66: The ternary
baseThemeName ? 'new' : 'new'always evaluates to'new'. Was'override'intended for one branch?Line 65:
themeNameis initialized fromcurrentNameonly on mount. IfcurrentNameprop changes (e.g., dialog reopens with different theme), the state won't update.🐛 Proposed fix
- const [themeName, setThemeName] = useState(currentName); - const [saveMode, setSaveMode] = useState<'new' | 'override'>(baseThemeName ? 'new' : 'new'); + const [themeName, setThemeName] = useState(currentName); + const [saveMode, setSaveMode] = useState<'new' | 'override'>('new'); + + // Reset state when dialog opens with new values + useEffect(() => { + if (open) { + setThemeName(currentName); + setSaveMode('new'); + } + }, [open, currentName]);Don't forget to add
useEffectto the import at Line 19.frontend/apps/thunder-develop/src/features/design-system/components/ColorPickerField.tsx-118-120 (1)
118-120:⚠️ Potential issue | 🟡 MinorGuard against undefined field values before string ops.
If the field value is undefined,
startsWithwill throw. Normalizing to an empty string avoids runtime crashes during first render.🛠️ Proposed fix
- const {value, onChange} = field; - const isValidHex = /^#[0-9A-F]{6}$/i.test(value); + const {value, onChange} = field; + const safeValue = typeof value === 'string' ? value : ''; + const isValidHex = /^#[0-9A-F]{6}$/i.test(safeValue); @@ - <TextField - {...field} + <TextField + {...field} + value={safeValue} size="small" fullWidth placeholder="#000000" error={!!error} InputProps={{ - startAdornment: !value.startsWith('#') ? ( + startAdornment: !safeValue.startsWith('#') ? ( <InputAdornment position="start"> <Typography variant="body2" color="text.secondary"> #Also applies to: 163-176
frontend/apps/thunder-develop/src/features/design-system/components/SimpleColorPicker.tsx-74-93 (1)
74-93:⚠️ Potential issue | 🟡 MinorAdd keyboard support to the color swatch.
The swatch is only clickable and lacks keyboard semantics. Users navigating with keyboard cannot open the picker. Add
role="button",tabIndex={0}, and anonKeyDownhandler to meet WCAG 2.1 accessibility requirements.Proposed fix
<Box ref={anchorRef} onClick={() => setPickerOpen(!pickerOpen)} + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + setPickerOpen(!pickerOpen); + } + }} sx={{frontend/apps/thunder-develop/src/features/design-system/components/ColorPickerField.tsx-140-159 (1)
140-159:⚠️ Potential issue | 🟡 MinorMake the color swatch keyboard-accessible.
The swatch is clickable but not focusable. Add keyboard semantics so non-mouse users can open the picker.
🛠️ Proposed fix
<Box ref={anchorRef} onClick={() => setPickerOpen(!pickerOpen)} + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + setPickerOpen(!pickerOpen); + } + }} sx={{frontend/apps/thunder-develop/src/features/layouts/constants/layout-query-keys.ts-22-27 (1)
22-27:⚠️ Potential issue | 🟡 MinorFile name violates naming convention.
The file uses kebab-case (
layout-query-keys.ts) but the coding guideline requires camelCase for pure TypeScript files (.tsextension). Consider renaming tolayoutQueryKeys.ts.As per coding guidelines: "Pure TypeScript source file names (with .ts extension) must follow camelCase"
frontend/apps/thunder-develop/src/features/themes/pages/ThemesListPage.tsx-54-63 (1)
54-63:⚠️ Potential issue | 🟡 MinorRemove unnecessary async IIFE wrapper around
navigate.In
BrowserRouter(used in this app),useNavigatereturns a synchronous function that returnsvoid. The async IIFE with.catch()will never execute—there's no promise to catch. Simply callnavigate()directly. If you need to handle navigation errors, use route error boundaries (errorElement) rather than try/catch patterns.frontend/apps/thunder-develop/src/features/layouts/components/LayoutDeleteDialog.tsx-48-50 (1)
48-50:⚠️ Potential issue | 🟡 MinorPass the Error object as the second argument to
logger.error().The
logger.error()method usesinstanceof Errorto detect if the second argument is an Error object. Currently,{error, layoutId}is a plain object, so the actual error is lost in the context and the error parameter remains undefined. Pass the Error directly and keep the context separate.🧾 Suggested change
onError: (error) => { - logger.error('Failed to delete layout', {error, layoutId}); + logger.error('Failed to delete layout', error, {layoutId}); },frontend/apps/thunder-develop/src/features/layouts/components/LayoutsList.tsx-254-272 (1)
254-272:⚠️ Potential issue | 🟡 MinorIncorrect event type cast and redundant handler.
The
onChangehandler castsChangeEventtoMouseEvent, which is incorrect. Additionally, there's a redundantonClickhandler on the sameCheckboxthat only callsstopPropagation().🐛 Proposed fix
<Checkbox checked={isSelected(layout.id)} - onChange={(e) => handleCheckboxClick(e as unknown as MouseEvent, layout.id)} + onClick={(e) => handleCheckboxClick(e, layout.id)} className="card-checkbox" sx={{ position: 'absolute', top: 8, left: 8, zIndex: 1, bgcolor: 'background.paper', borderRadius: 1, opacity: selectedCount > 0 || isSelected(layout.id) ? 1 : 0, transition: 'opacity 0.2s', '&:hover': { bgcolor: 'background.paper', }, }} - onClick={(e) => e.stopPropagation()} />This uses
onClickwhich provides aMouseEventdirectly, eliminating the type cast and removing the redundant handler.frontend/apps/thunder-develop/src/features/design-system/hooks/useMultiSelect.ts-39-42 (1)
39-42:⚠️ Potential issue | 🟡 MinorDocumentation mentions unimplemented feature.
The JSDoc states "with support for shift-click range selection" but this functionality is not implemented in the hook. Either remove this claim from the documentation or implement the feature.
📝 Suggested documentation fix
/** - * Hook to manage multi-select state for lists with support for shift-click range selection. + * Hook to manage multi-select state for lists. * * `@returns` Multi-select state and handlersfrontend/apps/thunder-develop/src/features/design/components/ThemeControls.tsx-194-202 (1)
194-202:⚠️ Potential issue | 🟡 MinorVariable shadowing:
tshadows the translation function.The callback parameter
tinthemes.find((t) => ...)shadows thettranslation function fromuseTranslation(). This is confusing and could lead to bugs.🔧 Use a different variable name
{selectedBaseThemeId && ( <Chip icon={<Palette size={16} />} - label={themes.find((t) => t.id === selectedBaseThemeId)?.displayName || 'Unknown'} + label={themes.find((theme) => theme.id === selectedBaseThemeId)?.displayName || 'Unknown'} size="small" color="primary" variant="outlined" /> )}frontend/apps/thunder-develop/src/features/design/components/ThemeControls.tsx-251-254 (1)
251-254:⚠️ Potential issue | 🟡 MinorType narrowing needed for Select
onChangehandler.
e.target.valueis typed asstring, buthandleDefaultColorSchemeChangeexpects'light' | 'dark'. This may cause a TypeScript error depending on strict settings.🔧 Add type assertion
<Select value={themeConfig?.defaultColorScheme || 'light'} - onChange={(e) => handleDefaultColorSchemeChange(e.target.value)} + onChange={(e) => handleDefaultColorSchemeChange(e.target.value as 'light' | 'dark')} label={t('design:theme.defaultColorScheme', {defaultValue: 'Default Color Scheme'})} >frontend/apps/thunder-develop/src/features/themes/components/ThemePreview.tsx-49-52 (1)
49-52:⚠️ Potential issue | 🟡 MinorUnused
widthandheightprops.The
widthandheightprops are defined but never used in the component. The parent (DesignStudioPage) passes viewport dimensions that are currently ignored.🔧 Apply the props or remove them
If the props should control the preview size:
<Box sx={{ bgcolor: 'background.default', // ... - height: '100%', + width: width ?? '100%', + height: height ?? '100%', minHeight: 500, // ... }} >Otherwise, remove the unused props from the interface.
🧹 Nitpick comments (24)
frontend/apps/thunder-develop/src/features/design-system/components/ConfirmDialog.tsx (1)
94-101: Error handling silently swallows exceptions.If
onConfirmthrows, the error is caught by thefinallyblock which resets the loading state, but the error itself is not propagated or surfaced to the user. The caller's error handling inonConfirmmust be self-contained.This may be intentional (caller handles their own errors), but consider whether re-throwing or adding an
onErrorcallback would be beneficial for consistent error handling across usages.frontend/apps/thunder-develop/src/features/design-system/providers/NotificationProvider.tsx (2)
19-22: Circular import between provider and hook.The provider imports
UseNotificationReturnfrom the hook, while the hook importsNotificationContextfrom the provider. This circular dependency works in most bundlers but can cause issues with tree-shaking and code splitting.Consider defining
UseNotificationReturnin a separate types file (e.g.,types/notification.ts) that both files can import from.
133-138: MemoizecontextValueto prevent consumer re-renders.While the individual functions are memoized with
useCallback, thecontextValueobject is recreated on every render, causing all context consumers to re-render unnecessarily.♻️ Proposed fix using useMemo
+import {createContext, useCallback, useMemo, useState} from 'react'; -import {createContext, useCallback, useState} from 'react'; ... - const contextValue: UseNotificationReturn = { - showSuccess, - showError, - showInfo, - showWarning, - }; + const contextValue: UseNotificationReturn = useMemo( + () => ({ + showSuccess, + showError, + showInfo, + showWarning, + }), + [showSuccess, showError, showInfo, showWarning] + );frontend/apps/thunder-develop/src/features/layouts/components/LayoutBuilderForm.tsx (2)
80-86: Consider throttling/debouncingonValuesChangecalls.
watch()returns a new object reference on every render, causing this effect to fire on every keystroke. For complex preview updates, this could cause performance issues.♻️ Optional: Debounce the callback
+import {useEffect, useMemo, type JSX} from 'react'; +import useDebounce from '@/features/design-system/hooks/useDebounce'; ... // Watch all values for preview const formValues = watch(); + const debouncedFormValues = useDebounce(formValues, 150); // Notify parent of form value changes useEffect(() => { - onValuesChange?.(formValues); - }, [formValues, onValuesChange]); + onValuesChange?.(debouncedFormValues); + }, [debouncedFormValues, onValuesChange]);
54-55:isSubmittingprop is unused.The prop is defined and destructured but never used. Consider either removing it or using it to disable form inputs during submission.
♻️ Option 1: Remove if not needed
interface LayoutBuilderFormProps { /** Initial form values */ initialValues: LayoutFormData; /** Callback when form is submitted */ onSubmit: (data: LayoutFormData) => void; /** Callback when form values change (for preview) */ onValuesChange?: (data: LayoutFormData) => void; /** Callback when isDirty state changes */ onDirtyChange?: (isDirty: boolean) => void; - /** Whether form is currently submitting */ - isSubmitting?: boolean; }♻️ Option 2: Use to disable inputs
<TextField {...field} fullWidth label={t('layouts:builder.displayName', {defaultValue: 'Display Name'})} error={!!errors.displayName} helperText={errors.displayName?.message} required + disabled={isSubmitting} />Also applies to: 65-65
frontend/apps/thunder-develop/src/features/layouts/components/LayoutPreviewPlaceholder.tsx (2)
75-78: Fragile gradient color extraction.The gradient parsing splits on comma and attempts to extract the first color, but CSS gradients like
linear-gradient(135deg,#667eea0%,#764ba2100%)would yield135degas the first part after splitting, not a color.Consider a more robust approach or simply use the fallback color for gradient backgrounds in this placeholder preview.
♻️ Suggested fix
} else if (screen?.background?.type === 'gradient') { - // Use first color from gradient if possible - backgroundFill = screen.background.value.split(',')[0]?.replace('linear-gradient(', '').trim() || '#667eea'; + // For placeholder preview, use fallback color for gradients + // as parsing CSS gradient strings reliably is complex + backgroundFill = '#667eea'; }
119-119: Remove unusedindexparameter.The
indexparameter in the map callback is declared but never used.♻️ Suggested fix
- {layouts.map((layout, index) => ( + {layouts.map((layout) => (frontend/apps/thunder-develop/src/features/layouts/api/useDuplicateLayout.ts (2)
49-53: Potential naming collision with repeated duplications.When duplicating a layout that was already duplicated, the name becomes "Layout (Copy) (Copy)". Consider stripping existing " (Copy)" suffixes before appending, or using a counter pattern like "Layout (Copy 2)".
65-67: Silent error swallowing on cache invalidation.The
.catch(() => {})silently ignores any errors frominvalidateQueries. While cache invalidation failures are typically non-critical, consider logging them for debugging purposes.frontend/apps/thunder-develop/src/features/design/components/LayoutControls.tsx (2)
31-31: Unused import.
Divideris imported but never used in the component.♻️ Suggested fix
CircularProgress, Stack, - Divider, Chip,
140-144: Visual layout shift on selection state change.The
ListItemIconis conditionally rendered only when selected, and theListItemTextpadding adjusts accordingly (Line 161). This causes a horizontal shift when items are selected/deselected.Consider always rendering the icon container with visibility toggled, or using a consistent padding approach.
♻️ Suggested fix
+ <ListItemIcon sx={{minWidth: 32, visibility: selectedLayoutId === layout.id ? 'visible' : 'hidden'}}> + <Check size={16} color="primary" /> + </ListItemIcon> - {selectedLayoutId === layout.id && ( - <ListItemIcon sx={{minWidth: 32}}> - <Check size={16} color="primary" /> - </ListItemIcon> - )} <ListItemText ... - sx={{pl: selectedLayoutId === layout.id ? 0 : 4}} + sx={{pl: 0}} />Also applies to: 161-161
frontend/apps/thunder-develop/src/features/themes/models/requests.ts (1)
24-35: Consider consolidating identical interfaces.
CreateThemeRequestandUpdateThemeRequestare structurally identical. While separate types can aid future extensibility, you could reduce duplication now with a shared base type.♻️ Optional: Use a shared base type
+/** + * Base request payload for theme operations + */ +interface ThemeRequestBase { + displayName: string; + theme: ThemeConfig; +} + /** * Request payload for creating a theme */ -export interface CreateThemeRequest { - displayName: string; - theme: ThemeConfig; -} +export type CreateThemeRequest = ThemeRequestBase; /** * Request payload for updating a theme */ -export interface UpdateThemeRequest { - displayName: string; - theme: ThemeConfig; -} +export type UpdateThemeRequest = ThemeRequestBase;frontend/apps/thunder-develop/src/features/layouts/pages/LayoutsListPage.tsx (1)
56-62: Simplify navigation handler.The async IIFE wrapper around
navigate()adds complexity. React Router'snavigate()is synchronous in typical usage and doesn't require error handling for simple path navigation.♻️ Simplified navigation
onClick={() => { - (async () => { - await navigate('/layouts/builder'); - })().catch((error: unknown) => { - logger.error('Navigation to layout builder failed', {error}); - }); + navigate('/layouts/builder'); }}frontend/apps/thunder-develop/src/features/layouts/components/LayoutsList.tsx (1)
79-79: Potential stale selection after data refresh.The
useMultiSelecthook maintains selected IDs independently of the fetched layouts. If a layout is deleted externally or the data is refreshed,selectedIdsmay reference non-existent layouts. Consider clearing or synchronizing selection when data changes.♻️ Sync selection with available data
Add a
useEffectto filter out stale selections:import {useEffect} from 'react'; // After useMultiSelect hook useEffect(() => { if (data?.layouts) { const validIds = new Set(data.layouts.map(l => l.id)); const staleIds = Array.from(selectedIds).filter(id => !validIds.has(id)); if (staleIds.length > 0) { // Clear stale selections staleIds.forEach(id => toggleSelection(id)); } } }, [data?.layouts]);Alternatively, expose a
removeIdsmethod fromuseMultiSelectfor cleaner batch removal.frontend/apps/thunder-develop/src/features/themes/api/useGetThemes.ts (1)
53-57: Consider addressing the type assertion instead of working around it.The
as unknown as Parameters<typeof http.request>[0]cast suggests a type mismatch with the HTTP client. This pattern bypasses type safety and could hide actual API contract issues.Consider either:
- Updating the request object to match the expected type
- Creating a typed wrapper for the HTTP client
- Fixing the upstream type definition if incorrect
frontend/apps/thunder-develop/src/features/themes/components/ThemesList.tsx (1)
254-272: Simplify Checkbox event handling.The Checkbox has both
onChange(line 256) with a type cast andonClick(line 271) forstopPropagation. This is confusing and the type cast suggests a mismatch.Consider using only
onClickfor both actions:♻️ Proposed simplification
<Checkbox checked={isSelected(theme.id)} - onChange={(e) => handleCheckboxClick(e as unknown as MouseEvent, theme.id)} className="card-checkbox" sx={{ position: 'absolute', top: 8, left: 8, zIndex: 1, bgcolor: 'background.paper', borderRadius: 1, opacity: selectedCount > 0 || isSelected(theme.id) ? 1 : 0, transition: 'opacity 0.2s', '&:hover': { bgcolor: 'background.paper', }, }} - onClick={(e) => e.stopPropagation()} + onClick={(e) => handleCheckboxClick(e, theme.id)} />frontend/apps/thunder-develop/src/features/layouts/pages/LayoutBuilderPage.tsx (1)
55-55: Remove unusedformRef.
formRefis declared but never used in the component.🧹 Remove unused variable
- const formRef = useRef<{submit: () => void}>();frontend/apps/thunder-develop/src/features/design-system/components/BulkActionBar.tsx (1)
110-134: Missing internationalization for UI text.The strings "selected" (line 111) and "Clear" (line 133) are hardcoded. For consistency with other components in the PR that use
useTranslation, these should be internationalized.🌐 Proposed i18n fix
+import {useTranslation} from 'react-i18next'; + export default function BulkActionBar({selectedCount, onClearSelection, actions}: BulkActionBarProps): JSX.Element { + const {t} = useTranslation(); + return ( // ... <Typography variant="body1" fontWeight={600} color="text.primary"> - {selectedCount} selected + {t('common:bulkActions.selectedCount', {count: selectedCount, defaultValue: '{{count}} selected'})} </Typography> // ... - <Button variant="outlined" onClick={onClearSelection} size="small"> - Clear - </Button> + <Button variant="outlined" onClick={onClearSelection} size="small"> + {t('common:actions.clear', {defaultValue: 'Clear'})} + </Button>frontend/apps/thunder-develop/src/features/themes/pages/ThemeBuilderPage.tsx (2)
55-55: Remove unusedformRef.
formRefis declared but never used in the component.🧹 Remove unused variable
- const formRef = useRef<{submit: () => void}>();
41-249: Consider extracting shared builder page logic.
ThemeBuilderPageandLayoutBuilderPageshare nearly identical structure: loading state, keyboard shortcuts, save handlers, back navigation, and unsaved changes dialog. Consider extracting a shared pattern or custom hook (e.g.,useBuilderPage) to reduce duplication and ensure consistent behavior.frontend/apps/thunder-develop/src/features/themes/components/ThemePreview.tsx (1)
112-114:parseInton CSS value string is fragile.
themeConfig.shape.borderRadiusis a string like"8px"(as seen in ThemeControls). WhileparseInt("8px")happens to return8in JavaScript, this behavior is implicit and could break if the format changes (e.g.,"0.5rem").♻️ Consider explicit parsing
shape: { - borderRadius: parseInt(themeConfig.shape.borderRadius) || 8, + borderRadius: parseFloat(themeConfig.shape.borderRadius.replace(/[^0-9.]/g, '')) || 8, },Alternatively, consider storing
borderRadiusas a number inShapeConfigand appending units only when displaying.frontend/apps/thunder-develop/src/features/design/pages/DesignStudioPage.tsx (1)
89-91: Unused computed valuethemeConfigFromQuery.
themeConfigFromQueryis computed on line 90 but never used. The component usesthemeConfigstate instead, which is synced viauseEffect. This appears to be dead code.♻️ Remove unused computation
- // Computed values from queries - const themeConfigFromQuery = selectedBaseThemeId ? themeData?.theme : defaultThemeFormData.theme; const layoutConfigFromQuery = selectedLayoutId ? layoutData?.layout : null;frontend/apps/thunder-develop/src/features/themes/models/theme.ts (1)
22-26: Consider addinglightproperty toBrandColorConfig.MUI's palette color objects typically include
main,light,dark, andcontrastText. Thelightvariant is missing here, which limits theme flexibility.♻️ Add light variant for completeness
export interface BrandColorConfig { main: string; + light?: string; dark: string; contrastText: string; }frontend/apps/thunder-develop/src/features/design/components/ThemeControls.tsx (1)
84-98: Deep clone withJSON.parse(JSON.stringify())loses type safety.The manual path traversal with
anytyping bypasses TypeScript's type checking. While functional, this pattern is error-prone - a typo in the path string won't be caught at compile time.♻️ Consider a type-safe immutable update helper
For better maintainability, consider using a library like
immeror creating typed update functions:// With immer (if already a dependency) import { produce } from 'immer'; const handleColorChange = (path: string, value: string) => { const newConfig = produce(themeConfig, draft => { // Path-based update with draft }); onThemeConfigChange(newConfig); }; // Or typed helper functions const updatePrimaryColor = (scheme: 'light' | 'dark', key: keyof BrandColorConfig, value: string) => { onThemeConfigChange({ ...themeConfig, colorSchemes: { ...themeConfig.colorSchemes, [scheme]: { ...themeConfig.colorSchemes[scheme], colors: { ...themeConfig.colorSchemes[scheme].colors, primary: { ...themeConfig.colorSchemes[scheme].colors.primary, [key]: value, }, }, }, }, }); };
| */ | ||
|
|
||
| import type {JSX, ReactNode} from 'react'; | ||
| import {Button, Paper, Stack, Typography} from '@mui/material'; |
There was a problem hiding this comment.
Inconsistent UI library import.
This file imports from @mui/material while other files in the PR (e.g., ThemesList.tsx, LayoutBuilderPage.tsx) import from @wso2/oxygen-ui. This inconsistency could cause:
- Bundle bloat (two copies of MUI)
- Styling/theming inconsistencies
Use @wso2/oxygen-ui consistently across the codebase.
🐛 Proposed fix
-import {Button, Paper, Stack, Typography} from '@mui/material';
+import {Button, Paper, Stack, Typography} from '@wso2/oxygen-ui';📝 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.
| import {Button, Paper, Stack, Typography} from '@mui/material'; | |
| import {Button, Paper, Stack, Typography} from '@wso2/oxygen-ui'; |
🤖 Prompt for AI Agents
In
`@frontend/apps/thunder-develop/src/features/design-system/components/BulkActionBar.tsx`
at line 20, The import in BulkActionBar.tsx currently pulls Button, Paper,
Stack, Typography from '@mui/material' causing inconsistent UI library usage;
update the import to use the same library used elsewhere ('@wso2/oxygen-ui') by
replacing the import statement that references Button, Paper, Stack, Typography
so the component uses `@wso2/oxygen-ui` exports and aligns theming and bundle
usage with ThemesList.tsx and LayoutBuilderPage.tsx.
| <Dialog | ||
| open={open} | ||
| onClose={onCancel} | ||
| aria-labelledby="unsaved-changes-dialog-title" | ||
| aria-describedby="unsaved-changes-dialog-description" | ||
| maxWidth="sm" | ||
| fullWidth | ||
| > | ||
| <DialogTitle id="unsaved-changes-dialog-title">Unsaved Changes</DialogTitle> | ||
| <DialogContent> | ||
| <DialogContentText id="unsaved-changes-dialog-description">{message}</DialogContentText> | ||
| </DialogContent> | ||
| <DialogActions> | ||
| <Button onClick={onCancel} disabled={isSaving}> | ||
| Cancel | ||
| </Button> | ||
| <Button onClick={onDiscard} color="error" disabled={isSaving}> | ||
| Discard Changes | ||
| </Button> | ||
| <Button onClick={handleSave} variant="contained" disabled={isSaving} autoFocus> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "UnsavedChangesDialog.tsx" | head -5Repository: asgardeo/thunder
Length of output: 154
🏁 Script executed:
cat -n frontend/apps/thunder-develop/src/features/design-system/components/UnsavedChangesDialog.tsxRepository: asgardeo/thunder
Length of output: 4124
Prevent backdrop/ESC close while saving.
When a user presses ESC or clicks the backdrop while a save is in-flight, onClose={onCancel} is triggered directly, invoking onCancel() before the save completes. This creates a race condition where navigation/cancellation proceeds while onSave() is still pending. Guard the close handler and disable ESC during save.
🛠️ Proposed fix
const handleSave = async (): Promise<void> => {
setIsSaving(true);
try {
await onSave();
} finally {
setIsSaving(false);
}
};
+
+ const handleClose = (_: object, reason?: 'backdropClick' | 'escapeKeyDown'): void => {
+ if (isSaving) {
+ return;
+ }
+ onCancel();
+ };
@@
<Dialog
open={open}
- onClose={onCancel}
+ onClose={handleClose}
+ disableEscapeKeyDown={isSaving}
aria-labelledby="unsaved-changes-dialog-title"🤖 Prompt for AI Agents
In
`@frontend/apps/thunder-develop/src/features/design-system/components/UnsavedChangesDialog.tsx`
around lines 88 - 107, The Dialog currently calls onCancel via onClose which
allows ESC/backdrop to cancel during an in-flight save; modify the Dialog's
onClose handler to accept (event, reason) and, when isSaving is true, ignore
reasons 'backdropClick' and 'escapeKeyDown' (thus preventing cancel while
saving) and also set disableEscapeKeyDown={isSaving} on the Dialog; keep calling
onCancel/onDiscard when isSaving is false or when reason is not a user-initiated
close. Reference: the Dialog component's onClose prop, the onCancel callback,
the isSaving flag, and handleSave.
| showUnsavedDialog: () => void; | ||
| /** Hide the unsaved changes dialog */ | ||
| hideUnsavedDialog: () => void; | ||
| /** Confirm navigation (proceed with leaving the page) */ | ||
| confirmNavigation: () => void; | ||
| /** Cancel navigation (stay on the page) */ | ||
| cancelNavigation: () => void; | ||
| /** Callback to check before navigation */ |
There was a problem hiding this comment.
pendingNavigation is never set; confirmNavigation can’t proceed.
showUnsavedDialog doesn’t capture a callback, so confirmNavigation only hides the dialog. Either accept a callback or remove pendingNavigation and update docs.
🛠️ Proposed fix
export interface UseUnsavedChangesReturn {
/** Whether unsaved changes dialog should be shown */
showDialog: boolean;
/** Show the unsaved changes dialog */
- showUnsavedDialog: () => void;
+ showUnsavedDialog: (navigate?: () => void) => void;
@@
- const showUnsavedDialog = useCallback(() => {
- setShowDialog(true);
- }, []);
+ const showUnsavedDialog = useCallback((navigate?: () => void) => {
+ if (navigate) {
+ setPendingNavigation(() => navigate);
+ }
+ setShowDialog(true);
+ }, []);Also applies to: 84-100
🤖 Prompt for AI Agents
In
`@frontend/apps/thunder-develop/src/features/design-system/hooks/useUnsavedChanges.ts`
around lines 28 - 35, The pendingNavigation callback is never set so
confirmNavigation cannot proceed; update showUnsavedDialog to accept and store a
callback into pendingNavigation (or alternatively remove pendingNavigation usage
and the related methods), then make confirmNavigation invoke the stored
callback, clear pendingNavigation, and hide the dialog; update the
types/signature of showUnsavedDialog (and related docs/comments) and ensure
hideUnsavedDialog/cancelNavigation also clear pendingNavigation to avoid stale
references (affecting useUnsavedChanges, showUnsavedDialog, confirmNavigation,
hideUnsavedDialog, cancelNavigation).
| {devices.map(({type, icon, label}) => ( | ||
| <Tooltip key={type} title={`${label} (${DEVICE_VIEWPORTS[type].width}x${DEVICE_VIEWPORTS[type].height})`}> | ||
| <Button | ||
| variant={selectedDevice === type ? 'contained' : 'outlined'} | ||
| onClick={() => onDeviceChange(type)} | ||
| sx={{minWidth: 48}} | ||
| > | ||
| {icon} |
There was a problem hiding this comment.
Add accessible labels to icon-only buttons.
Icon-only controls need an accessible name; tooltips don’t reliably provide one to screen readers.
✅ Suggested fix
<Button
variant={selectedDevice === type ? 'contained' : 'outlined'}
onClick={() => onDeviceChange(type)}
sx={{minWidth: 48}}
+ aria-label={label}
>
{icon}
</Button>📝 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.
| {devices.map(({type, icon, label}) => ( | |
| <Tooltip key={type} title={`${label} (${DEVICE_VIEWPORTS[type].width}x${DEVICE_VIEWPORTS[type].height})`}> | |
| <Button | |
| variant={selectedDevice === type ? 'contained' : 'outlined'} | |
| onClick={() => onDeviceChange(type)} | |
| sx={{minWidth: 48}} | |
| > | |
| {icon} | |
| {devices.map(({type, icon, label}) => ( | |
| <Tooltip key={type} title={`${label} (${DEVICE_VIEWPORTS[type].width}x${DEVICE_VIEWPORTS[type].height})`}> | |
| <Button | |
| variant={selectedDevice === type ? 'contained' : 'outlined'} | |
| onClick={() => onDeviceChange(type)} | |
| sx={{minWidth: 48}} | |
| aria-label={label} | |
| > | |
| {icon} |
🤖 Prompt for AI Agents
In
`@frontend/apps/thunder-develop/src/features/design/components/DeviceSelector.tsx`
around lines 48 - 55, The icon-only device buttons rendered in the devices.map
block (using Tooltip and Button, with props selectedDevice and onDeviceChange
and looking up sizes in DEVICE_VIEWPORTS) lack accessible names; add an
accessible label to each button by providing an aria-label (or aria-labelledby
to a visually-hidden element) that uses the device label and size (e.g.,
`${label} ${DEVICE_VIEWPORTS[type].width}x${DEVICE_VIEWPORTS[type].height}`) so
screen readers can announce the control while keeping the visual UI the same.
| const canvasWidth = 1440; | ||
| const canvasHeight = 900; | ||
| const scaledWidth = (canvasWidth * zoom) / 100; | ||
| const scaledHeight = (canvasHeight * zoom) / 100; | ||
|
|
There was a problem hiding this comment.
Zoom is applied twice (scaled dimensions + transform).
Using scaled dimensions and CSS scale() together squares the zoom factor (e.g., 50% renders at 25%). Use either scaled sizes or transform, not both.
🛠️ Proposed fix
<Paper
elevation={4}
sx={{
- width: scaledWidth,
- height: scaledHeight,
+ width: canvasWidth,
+ height: canvasHeight,
transform: `scale(${zoom / 100})`,
transformOrigin: 'center center',
overflow: 'hidden',
}}Also applies to: 231-238
🤖 Prompt for AI Agents
In
`@frontend/apps/thunder-develop/src/features/design/components/LayoutBuilder.tsx`
around lines 78 - 82, The code in LayoutBuilder applies zoom twice by computing
scaledWidth/scaledHeight from canvasWidth/canvasHeight using zoom and also
applying a CSS transform scale; pick one strategy and remove the other. For a
minimal fix: keep canvasWidth and canvasHeight as the source sizes and remove
use of scaledWidth/scaledHeight (and any calculations using them) so the CSS
transform(scale(zoom/100)) on the canvas container handles visual zoom;
alternatively, if you prefer sizing-based zoom, remove the transform and use
scaledWidth/scaledHeight everywhere instead. Update all usages of
scaledWidth/scaledHeight (including the other occurrence around the block
referenced at 231-238) and ensure zoom (the zoom prop/variable) is only applied
once.
| const { | ||
| control, | ||
| handleSubmit, | ||
| watch, | ||
| formState: {errors, isDirty}, | ||
| } = useForm<ThemeFormData>({ | ||
| resolver: zodResolver(themeFormSchema), | ||
| defaultValues: initialValues, | ||
| mode: 'onChange', | ||
| }); | ||
|
|
||
| // Watch all values for preview | ||
| const formValues = watch(); | ||
|
|
||
| // Notify parent of form value changes | ||
| useEffect(() => { | ||
| onValuesChange?.(formValues); | ||
| }, [formValues, onValuesChange]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find frontend -name "ThemeBuilderForm.tsx" -type fRepository: asgardeo/thunder
Length of output: 141
🏁 Script executed:
# Check the file exists and read the relevant lines
if [ -f "frontend/apps/thunder-develop/src/features/themes/components/ThemeBuilderForm.tsx" ]; then
wc -l "frontend/apps/thunder-develop/src/features/themes/components/ThemeBuilderForm.tsx"
fiRepository: asgardeo/thunder
Length of output: 145
🏁 Script executed:
# Read the file to see the structure and context
cat -n "frontend/apps/thunder-develop/src/features/themes/components/ThemeBuilderForm.tsx" | head -100Repository: asgardeo/thunder
Length of output: 3678
🏁 Script executed:
# Search for "reset" in the file to see if it's already being used
cat -n "frontend/apps/thunder-develop/src/features/themes/components/ThemeBuilderForm.tsx" | grep -i "reset"Repository: asgardeo/thunder
Length of output: 42
🏁 Script executed:
# Check the entire file to see if there's a reset useEffect already
cat -n "frontend/apps/thunder-develop/src/features/themes/components/ThemeBuilderForm.tsx" | sed -n '72,200p'Repository: asgardeo/thunder
Length of output: 5906
🏁 Script executed:
# Search for where ThemeBuilderForm is used/imported
rg "ThemeBuilderForm" --type tsx --type ts -B 2 -A 2Repository: asgardeo/thunder
Length of output: 87
🏁 Script executed:
# Check the entire component to see context about how it's used
cat -n "frontend/apps/thunder-develop/src/features/themes/components/ThemeBuilderForm.tsx" | tail -100Repository: asgardeo/thunder
Length of output: 4547
🏁 Script executed:
# Search for ThemeBuilderForm usage
rg "ThemeBuilderForm" -g "*.tsx" -g "*.ts" -B 2 -A 2Repository: asgardeo/thunder
Length of output: 3193
🏁 Script executed:
# Also search in JSX/TSX files specifically
find . -type f \( -name "*.tsx" -o -name "*.ts" \) -exec grep -l "ThemeBuilderForm" {} \;Repository: asgardeo/thunder
Length of output: 222
🏁 Script executed:
# Read ThemeBuilderPage.tsx to understand how formData changes
cat -n "frontend/apps/thunder-develop/src/features/themes/pages/ThemeBuilderPage.tsx" | head -150Repository: asgardeo/thunder
Length of output: 6541
🏁 Script executed:
# Read more of ThemeBuilderPage to see the complete usage
cat -n "frontend/apps/thunder-develop/src/features/themes/pages/ThemeBuilderPage.tsx" | sed -n '150,250p'Repository: asgardeo/thunder
Length of output: 4340
🏁 Script executed:
# Verify the file size and check imports to understand dependencies
cat -n "frontend/apps/thunder-develop/src/features/themes/pages/ThemeBuilderPage.tsx" | wc -lRepository: asgardeo/thunder
Length of output: 63
Sync the form when initialValues changes.
useForm applies defaultValues only on mount. When editing an existing theme, the API response updates the parent's formData state, but the form won't reflect these new values without calling reset().
Suggested fix
const {
control,
handleSubmit,
watch,
+ reset,
formState: {errors, isDirty},
} = useForm<ThemeFormData>({
resolver: zodResolver(themeFormSchema),
defaultValues: initialValues,
mode: 'onChange',
});
// Watch all values for preview
const formValues = watch();
+ useEffect(() => {
+ reset(initialValues);
+ }, [initialValues, reset]);
+
// Notify parent of form value changes
useEffect(() => {
onValuesChange?.(formValues);
}, [formValues, onValuesChange]);🤖 Prompt for AI Agents
In
`@frontend/apps/thunder-develop/src/features/themes/components/ThemeBuilderForm.tsx`
around lines 72 - 89, The form's defaultValues passed to useForm in
ThemeBuilderForm are only applied on mount, so when initialValues changes (e.g.,
after an API load) the form doesn't update; add a useEffect that watches
initialValues and calls the useForm reset(initialValues) to sync the form state
(reseting errors/dirty as appropriate) so watch(), handleSubmit, and
onValuesChange reflect the new data. Ensure you reference the existing
control/handleSubmit/watch/reset from useForm and call reset(initialValues)
inside the effect that depends on initialValues.
| interface ThemePreviewProps { | ||
| /** Theme configuration to preview */ | ||
| theme: ThemeConfig; | ||
| /** Optional layout configuration to apply */ | ||
| layout?: LayoutConfig | null; | ||
| /** Which color scheme to preview (defaults to defaultColorScheme) */ | ||
| activeColorScheme?: 'light' | 'dark'; | ||
| /** Preview width */ | ||
| width?: number; | ||
| /** Preview height */ | ||
| height?: number; | ||
| } | ||
|
|
||
| /** | ||
| * Enhanced preview component that shows an actual login page with the theme applied. | ||
| */ | ||
| export default function ThemePreview({ | ||
| theme: themeConfig, | ||
| layout, | ||
| activeColorScheme, | ||
| width, | ||
| height, | ||
| }: ThemePreviewProps): JSX.Element { | ||
| const {t} = useTranslation(); | ||
|
|
||
| // Handle null themeConfig | ||
| if (!themeConfig) { | ||
| return ( | ||
| <Box | ||
| sx={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| height: '100%', | ||
| bgcolor: 'background.default', | ||
| }} | ||
| > | ||
| <CircularProgress /> | ||
| </Box> | ||
| ); | ||
| } |
There was a problem hiding this comment.
React Hooks rule violation: useMemo called after conditional return.
The useMemo hook at line 89 is called after the early return at line 68. This violates React's rules of hooks - hooks must be called unconditionally at the top level. React may fail to track hook state correctly across renders.
Additionally, the theme prop is typed as required (ThemeConfig), yet the component performs a null check and returns a loading indicator. Either:
- Make the prop optional (
theme?: ThemeConfig | null), or - Handle the loading state at the parent level
🐛 Proposed fix: Move null check after hooks
export default function ThemePreview({
theme: themeConfig,
layout,
activeColorScheme,
width,
height,
}: ThemePreviewProps): JSX.Element {
const {t} = useTranslation();
- // Handle null themeConfig
- if (!themeConfig) {
- return (
- <Box
- sx={{
- display: 'flex',
- alignItems: 'center',
- justifyContent: 'center',
- height: '100%',
- bgcolor: 'background.default',
- }}
- >
- <CircularProgress />
- </Box>
- );
- }
-
// Determine which color scheme to show
- const displayScheme = activeColorScheme || themeConfig.defaultColorScheme;
- const colors = themeConfig.colorSchemes[displayScheme].colors;
+ const displayScheme = activeColorScheme || themeConfig?.defaultColorScheme || 'light';
+ const colors = themeConfig?.colorSchemes?.[displayScheme]?.colors;
// Create a live MUI theme from the configuration
const previewTheme = useMemo(() => {
+ if (!themeConfig || !colors) return null;
return createTheme({
// ... theme config
});
}, [themeConfig, displayScheme, colors]);
+ // Handle null themeConfig after hooks
+ if (!themeConfig || !previewTheme) {
+ return (
+ <Box sx={{ /* loading styles */ }}>
+ <CircularProgress />
+ </Box>
+ );
+ }
+
return (
// ... rest of component
);
}🤖 Prompt for AI Agents
In
`@frontend/apps/thunder-develop/src/features/themes/components/ThemePreview.tsx`
around lines 42 - 82, The component ThemePreview currently returns early when
themeConfig is falsy which causes the later call to useMemo to run conditionally
and violate React Hooks rules; change the prop signature to accept theme?:
ThemeConfig | null (or remove the internal null check and require the parent to
handle loading), and move all hooks (e.g., the useTranslation call and the
useMemo usage referenced by the existing useMemo call) to the top of
ThemePreview so they run unconditionally before any early returns; then perform
the themeConfig null check only after hooks and render the loading Box when
themeConfig is null.
| <svg width="100%" height="100%" viewBox="0 0 300 140" xmlns="http://www.w3.org/2000/svg"> | ||
| <defs> | ||
| {/* Gradient for primary color */} | ||
| <linearGradient id={`primaryGrad-${colorScheme}`} x1="0%" y1="0%" x2="100%" y2="0%"> | ||
| <stop offset="0%" style={{stopColor: colors.primary.main, stopOpacity: 1}} /> | ||
| <stop offset="100%" style={{stopColor: colors.primary.dark, stopOpacity: 1}} /> | ||
| </linearGradient> | ||
| {/* Gradient for secondary color */} | ||
| <linearGradient id={`secondaryGrad-${colorScheme}`} x1="0%" y1="0%" x2="100%" y2="0%"> | ||
| <stop offset="0%" style={{stopColor: colors.secondary.main, stopOpacity: 1}} /> | ||
| <stop offset="100%" style={{stopColor: colors.secondary.dark, stopOpacity: 1}} /> | ||
| </linearGradient> | ||
| </defs> | ||
| {/* Background */} | ||
| <rect width="300" height="140" fill={colors.background.default} /> | ||
| {/* Top bar with color indicators */}{' '} | ||
| <rect x="0" y="0" width="300" height="24" fill={colors.background.paper} opacity="0.6" /> | ||
| {/* Browser chrome dots */} | ||
| <circle cx="12" cy="12" r="3" fill={colors.text.secondary} opacity="0.4" /> | ||
| <circle cx="24" cy="12" r="3" fill={colors.text.secondary} opacity="0.4" /> | ||
| <circle cx="36" cy="12" r="3" fill={colors.text.secondary} opacity="0.4" /> | ||
| {/* Color swatches (top right) */} | ||
| <circle cx="272" cy="12" r="5" fill={`url(#primaryGrad-${colorScheme})`} opacity="0.95" /> | ||
| <circle cx="286" cy="12" r="5" fill={`url(#secondaryGrad-${colorScheme})`} opacity="0.95" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/apps/thunder-develop/src/features/themes/components/ThemePreviewPlaceholder.tsxRepository: asgardeo/thunder
Length of output: 5638
🏁 Script executed:
find . -name "package.json" -type f | head -5 | xargs grep -l "react" | head -3Repository: asgardeo/thunder
Length of output: 166
🏁 Script executed:
cat frontend/apps/thunder-develop/package.json | grep -A 2 '"react"'Repository: asgardeo/thunder
Length of output: 148
🏁 Script executed:
cat package.json 2>/dev/null | grep -A 2 '"react"' || echo "No root package.json"Repository: asgardeo/thunder
Length of output: 80
🏁 Script executed:
find . -name "pnpm-workspace.yaml" -o -name ".pnpmfile.cjs" | head -5Repository: asgardeo/thunder
Length of output: 90
🏁 Script executed:
find . -maxdepth 2 -name "*.yaml" -o -name "*.json" | xargs grep -l "catalog\|react.*version" 2>/dev/null | head -5Repository: asgardeo/thunder
Length of output: 160
🏁 Script executed:
cat frontend/package.json | grep -A 10 "react"Repository: asgardeo/thunder
Length of output: 42
🏁 Script executed:
cat frontend/pnpm-lock.yaml | grep -A 5 "react@" | head -20Repository: asgardeo/thunder
Length of output: 1462
Prevent SVG gradient ID collisions across instances.
When multiple ThemePreviewPlaceholder components render with the same colorScheme, the SVG gradient IDs (primaryGrad-dark, secondaryGrad-dark, etc.) will be duplicated, violating SVG spec and causing rendering issues. Use useId() to generate unique identifiers per component instance. React 19.2.3 supports this hook.
🔧 Suggested patch
-import type {JSX} from 'react';
+import type {JSX} from 'react';
+import {useId} from 'react';
@@
export default function ThemePreviewPlaceholder({
theme,
colorScheme = 'dark',
}: ThemePreviewPlaceholderProps): JSX.Element {
+ const gradientId = useId();
@@
- <linearGradient id={`primaryGrad-${colorScheme}`} x1="0%" y1="0%" x2="100%" y2="0%">
+ <linearGradient id={`primaryGrad-${gradientId}-${colorScheme}`} x1="0%" y1="0%" x2="100%" y2="0%">
@@
- <linearGradient id={`secondaryGrad-${colorScheme}`} x1="0%" y1="0%" x2="100%" y2="0%">
+ <linearGradient id={`secondaryGrad-${gradientId}-${colorScheme}`} x1="0%" y1="0%" x2="100%" y2="0%">
@@
- <circle cx="272" cy="12" r="5" fill={`url(`#primaryGrad-`${colorScheme})`} opacity="0.95" />
- <circle cx="286" cy="12" r="5" fill={`url(`#secondaryGrad-`${colorScheme})`} opacity="0.95" />
+ <circle cx="272" cy="12" r="5" fill={`url(`#primaryGrad-`${gradientId}-${colorScheme})`} opacity="0.95" />
+ <circle cx="286" cy="12" r="5" fill={`url(`#secondaryGrad-`${gradientId}-${colorScheme})`} opacity="0.95" />
@@
- fill={`url(`#primaryGrad-`${colorScheme})`}
+ fill={`url(`#primaryGrad-`${gradientId}-${colorScheme})`}
@@
- fill={`url(`#secondaryGrad-`${colorScheme})`}
+ fill={`url(`#secondaryGrad-`${gradientId}-${colorScheme})`}🤖 Prompt for AI Agents
In
`@frontend/apps/thunder-develop/src/features/themes/components/ThemePreviewPlaceholder.tsx`
around lines 65 - 88, The SVG gradient IDs in ThemePreviewPlaceholder (e.g.,
primaryGrad-${colorScheme} and secondaryGrad-${colorScheme}) can collide across
instances; import and call React's useId inside the ThemePreviewPlaceholder
component to generate a unique baseId, concatenate it with your colorScheme
(e.g., `${baseId}-primary-${colorScheme}` /
`${baseId}-secondary-${colorScheme}`) and replace the id attributes on
<linearGradient> and the corresponding fill="url(#...)" usages so each instance
has unique gradient IDs; ensure you add the useId import and use the generated
ids consistently where ids and url(...) are referenced.
| /** | ||
| * Copyright (c) 2025, WSO2 LLC. (https://www.wso2.com). | ||
| * | ||
| * WSO2 LLC. licenses this file to you under the Apache License, | ||
| * Version 2.0 (the "License"); you may not use this file except | ||
| * in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| /** | ||
| * React Query cache keys for theme operations | ||
| */ | ||
| const ThemeQueryKeys = { | ||
| THEMES: 'themes', | ||
| THEME: 'theme', | ||
| }; | ||
|
|
||
| export default ThemeQueryKeys; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename file to camelCase to meet repo rules.
theme-query-keys.ts violates the .ts filename convention; please rename to something like themeQueryKeys.ts and update imports accordingly.
As per coding guidelines, **/*.ts: Pure TypeScript source file names (with .ts extension) must follow camelCase (for example, useCreateApplication.ts).
🤖 Prompt for AI Agents
In
`@frontend/apps/thunder-develop/src/features/themes/constants/theme-query-keys.ts`
around lines 1 - 27, Rename the file containing the ThemeQueryKeys constant from
theme-query-keys.ts to a camelCase name such as themeQueryKeys.ts, update all
imports that reference the old filename to the new filename, and ensure the
exported symbol ThemeQueryKeys remains unchanged (export default ThemeQueryKeys)
so consumers continue to work; run a quick search for "theme-query-keys" across
the repo and replace with "themeQueryKeys" in import paths to complete the
change.
| // Keyboard shortcuts | ||
| useKeyboardShortcuts([ | ||
| { | ||
| key: 's', | ||
| meta: true, | ||
| callback: (e) => { | ||
| e.preventDefault(); | ||
| if (isDirty && !isSaving) { | ||
| handleSave(formData); | ||
| } | ||
| }, | ||
| description: 'Save theme', | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
Critical: isSaving referenced before declaration causes TDZ error.
Same issue as LayoutBuilderPage.tsx: the keyboard shortcut callback references isSaving (line 80), but it's declared on line 169. This will throw a ReferenceError at runtime.
🐛 Proposed fix - move isSaving declaration before useKeyboardShortcuts
+ const isSaving = isCreating || isUpdating;
+
// Keyboard shortcuts
useKeyboardShortcuts([
{
key: 's',
meta: true,
callback: (e) => {
e.preventDefault();
if (isDirty && !isSaving) {
handleSave(formData);
}
},
description: 'Save theme',
},
]);
// ... later in the file, remove:
- const isSaving = isCreating || isUpdating;🤖 Prompt for AI Agents
In `@frontend/apps/thunder-develop/src/features/themes/pages/ThemeBuilderPage.tsx`
around lines 73 - 86, The keyboard shortcut callback used in
useKeyboardShortcuts references isSaving before it's declared which produces a
TDZ ReferenceError; move the isSaving declaration (and any related state or
variables used by the callback such as formData and handleSave) above the
useKeyboardShortcuts call so the callback closes over already-initialized
values, ensuring useKeyboardShortcuts(...) runs after isSaving is defined.
1 participant
Purpose
Approach
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit