Conversation
…mprove layout - Added applyFilters functionality to the Settings context for extensibility. - Refactored FieldRenderer to support new CustomizeRadioField type. - Improved layout structure in Settings component for better mobile responsiveness. - Updated validation logic in settings formatter to support nested subpages and pipe-delimited rules. - Enhanced SettingsSidebar to dynamically render subpages and improve navigation experience.
- Replaced `title` with `label` in various settings components for consistency. - Updated FieldWrapper, SettingsContent, and other components to utilize `label` as the primary display text. - Enhanced settings formatter to support new label structure and maintain backward compatibility with existing `title` fields. - Improved overall readability and maintainability of the settings codebase.
…tionality - Removed the `isDirty` property and replaced it with `isPageDirty` to track modifications per page. - Introduced `getPageValues` and `onSave` methods for better handling of page-specific data. - Updated `SettingsContent` to include a save button that triggers the new save functionality. - Refactored `SettingsProvider` to manage dirty state and values more effectively across subpages. - Enhanced Storybook examples to demonstrate the new event logging for changes and saves.
- Introduced `renderSaveButton` prop to allow custom rendering of the save button in the Settings component. - Updated `onChange` and `onSave` handlers to use `scopeId` for better clarity and consistency. - Enhanced `SettingsContent` to conditionally display the save button based on the presence of `onSave`. - Added `SaveButtonRenderProps` interface to define props for the custom save button render function. - Updated Storybook examples to demonstrate the new save button functionality.
- Enhanced `renderSaveButton` prop to allow for more flexible custom save button rendering. - Updated `onChange` and `onSave` handlers to utilize `scopeId` for improved clarity. - Modified `SettingsContent` to conditionally render the save button based on the `onSave` prop. - Defined `SaveButtonRenderProps` interface for better type safety in custom save button implementations. - Updated Storybook examples to showcase the new save button capabilities.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a schema-driven Settings UI: Settings component + provider, navigation and sidebar, SettingsContent renderer, FieldRenderer and multiple field components, schema formatter/validation, dependency visibility and dirty-tracking, Storybook stories and MDX docs, and new public exports. Changes
Sequence DiagramsequenceDiagram
participant User
participant Settings as Settings (mount)
participant Provider as SettingsProvider (context)
participant Sidebar as SettingsSidebar
participant Content as SettingsContent
participant Renderer as FieldRenderer
participant Field as FieldComponent
User->>Settings: mount(schema, values, handlers)
Settings->>Provider: formatSettingsData(schema), init state
Provider-->>Settings: context ready
User->>Sidebar: search / select page
Sidebar->>Provider: setActivePage / setActiveSubpage
Provider-->>Sidebar: navigation state updated
User->>Content: open page/subpage
Content->>Provider: getActiveContent / tabs
Content->>Renderer: request render for fields
Renderer->>Provider: shouldDisplay, get value/errors
Renderer->>Field: render with props
User->>Field: change value
Field->>Renderer: onChange(key, value)
Renderer->>Provider: updateValue -> validate, mark dirty
Provider-->>User: call onChange / enable onSave
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 10
🤖 Fix all issues with AI agents
In `@src/components/settings/fields.tsx`:
- Around line 109-121: The TooltipTrigger currently renders a button and
contains another <button>, producing invalid nested buttons; update the
TooltipTrigger usage in the Tooltip block
(TooltipProvider/Tooltip/TooltipTrigger/TooltipContent) to use TooltipTrigger
with asChild so it forwards props to your inner button (remove the extra outer
button nesting), keeping the existing inner button (and Info icon) and
TooltipContent that displays element.tooltip; ensure the TooltipTrigger
component is the one using asChild so the rendered DOM has a single button
element.
- Around line 400-404: The JSX is injecting untrusted HTML with
dangerouslySetInnerHTML without sanitization; update the renderer in the
settings field component to check element.escape_html and, when true or when
html_content may be untrusted, run the content through a sanitizer (e.g., import
DOMPurify and call DOMPurify.sanitize(element.html_content)) and pass the
sanitized string to dangerouslySetInnerHTML; ensure you add the DOMPurify import
at the top of the file and only bypass sanitization if escape_html explicitly
indicates the content is safe.
In `@src/components/settings/settings-content.tsx`:
- Around line 80-103: The tab buttons are missing ARIA semantics; update the tab
container (the <nav> in settings-content) to include role="tablist" and for each
tab button produced by the tabs.map (the button whose onClick calls setActiveTab
and compares activeTab === tab.id) add role="tab", aria-selected (true when
activeTab === tab.id, otherwise false), and an appropriate aria-controls/id
pairing (e.g., button id referencing the tab panel id) and manage keyboard
focusability (tabIndex 0 for the active tab, -1 for inactive) so the WAI-ARIA
Tabs pattern is implemented.
- Around line 113-123: The sticky save area is shown whenever showSaveArea is
true but when renderSaveButton is undefined it renders an empty bar; implement
the behavior described in SettingsProps.renderSaveButton by providing a default
icon-only save button when renderSaveButton is not passed: inside the block that
currently does {renderSaveButton ? renderSaveButton({ scopeId, dirty, onSave:
handleSave }) : null} replace the null branch with a small default button
component that calls handleSave on click, reflects dirty (disabled/aria-disabled
when !dirty), includes a clear aria-label/testid (e.g.,
`settings-save-button-${scopeId}`), and uses the same props shape ({ scopeId,
dirty, onSave }) so custom renderers remain compatible; ensure the default
button styling matches the surrounding layout and remove the empty border-t only
when the save area is truly empty.
In `@src/components/settings/settings-context.tsx`:
- Around line 298-301: The current fallback that sets scopeId =
keyToScopeMap.get(key) || activeSubpage || activePage can attribute changes to
the wrong scope when keyToScopeMap lacks the key; update the handler to first
check keyToScopeMap.has(key) and if it does not, either return early (do not
call onChange) or in development emit a clear warning; specifically, replace the
unconditional fallback around keyToScopeMap.get(key) with a guard that uses
keyToScopeMap.has(key) and, when missing, calls console.warn (only when
process.env.NODE_ENV === 'development') and returns early instead of calling
onChange(scopeId, key, value) so consumers aren’t given a misleading scopeId
(refer to keyToScopeMap, activeSubpage, activePage, and onChange in that
handler).
- Around line 260-266: handleOnSave currently resets dirty state immediately
after calling onSave, so change handleOnSave to be async and await the result of
onSave before calling resetPageDirty(pageId); update the onSave prop/type to
allow returning Promise<void> (e.g. onSave?: (scopeId: string, values:
Record<string, any>) => void | Promise<void>), and ensure errors from awaiting
onSave are handled (do not call resetPageDirty on failure — log or rethrow as
appropriate) so the dirty indicator is only cleared on successful save.
In `@src/components/settings/settings-formatter.ts`:
- Around line 32-42: The early-return when isHierarchical is true skips all
enrichment/normalization; remove or modify that return so the existing
enrichment/normalization pass (the same logic used for flat inputs) runs for
every item and is applied recursively to item.children as well — ensure display
defaults, hook_key/dependency_key computation, validation/transforms and option
normalization are executed for both top-level items and nested children by
invoking the same enrichment functions on each node rather than bypassing
hierarchical data.
- Around line 195-198: The assignment to child.value in settings-formatter.ts
currently uses the `||` operator and thus converts valid falsy defaults (like 0
or false) to an empty string; update the expression that sets `child.value` (the
block handling `child.value = child.value !== undefined ? child.value :
child.default || ''`) to use nullish coalescing so `child.default` is only
replaced when it is null or undefined (preserve falsy-but-valid values such as 0
or false).
In `@src/components/settings/settings-sidebar.tsx`:
- Line 8: The current barrel import "import * as LucideIcons from
'lucide-react'" in settings-sidebar.tsx pulls in the entire icon library and
prevents tree-shaking; replace it with a curated map or dynamic import strategy
and update any icon lookup logic (e.g., getIcon) to use that map or an async
resolver. Specifically, remove the namespace import, create a small ICONS map of
only the icons used by SettingsSidebar/getIcon (or accept an iconResolver
prop/React.ComponentType map from the consumer) and switch runtime lookups to
reference that map or use dynamic import() to load icons on demand so bundlers
can tree-shake unused icons. Ensure references to LucideIcons in getIcon or the
component are replaced with the new ICONS map or resolver.
In `@src/components/settings/settings-types.ts`:
- Around line 150-152: The JSDoc for the renderSaveButton prop is incorrect: it
claims a default icon-only save button is rendered when onSave is set, but
settings-content.tsx currently renders an empty save area instead; either update
the comment or implement the default. Fix by editing the renderSaveButton
declaration's JSDoc in settings-types.ts to accurately state that no default
button is rendered (or conversely implement the default button in
settings-content.tsx by adding a fallback render that uses SaveButtonRenderProps
to render an icon-only button when renderSaveButton is undefined and onSave is
provided); reference the renderSaveButton prop, SaveButtonRenderProps type, and
the settings-content.tsx save-area rendering logic to locate and change the
behavior or documentation accordingly.
🧹 Nitpick comments (11)
src/components/settings/settings-sidebar.tsx (1)
172-187: Redundant array scan —somefollowed byfindon the same array.Minor:
schema.some(...)on Line 173 is immediately followed byschema.find(...)on Line 175. A singlefindsuffices since it returnsundefinedwhen no match exists.♻️ Proposed simplification
onItemClick={(item) => { - const isPage = schema.some((p) => p.id === item.id); - if (isPage) { - const page = schema.find((p) => p.id === item.id); - const hasSubpages = page?.children?.some( + const page = schema.find((p) => p.id === item.id); + if (page) { + const hasSubpages = page.children?.some( (c) => c.type === 'subpage' && c.display !== false ); if (!hasSubpages) {src/components/settings/Settings.stories.tsx (1)
86-88: Duplicate comment block.Lines 86–88 repeat the exact same comment from lines 82–84. Remove one occurrence.
src/components/settings/index.tsx (1)
171-181:usePrevioususing state-during-render triggers an extra re-render on every change.The current implementation calls
setPrevandsetCurrentduring the render phase whenvaluechanges. While React supports this pattern, it causes an extra synchronous re-render each timevaluechanges. The standarduseRef-based pattern avoids this overhead:♻️ Simpler ref-based usePrevious
-function usePrevious<T>(value: T): T | undefined { - const [prev, setPrev] = useState<T | undefined>(undefined); - const [current, setCurrent] = useState(value); - - if (value !== current) { - setPrev(current); - setCurrent(value); - } - - return prev; -} +function usePrevious<T>(value: T): T | undefined { + const ref = useRef<T | undefined>(undefined); + const prev = ref.current; + + useEffect(() => { + ref.current = value; + }); + + return prev; +}This also requires adding
useRefto the import on Line 1.src/components/settings/settings-types.ts (1)
35-110: Index signature[key: string]: anymakes all property names unchecked.This is a conscious extensibility tradeoff, but it means typos like
descrptioninstead ofdescriptionwill silently compile. If you're open to a stricter alternative, you could split the type:type SettingsElementBase = { id: string; type: ...; /* all known fields */ }; type SettingsElement = SettingsElementBase & Record<string, unknown>;Using
unknowninstead ofanywould at least force consumers to narrow custom properties before use. But given this is schema-driven from external JSON, the current approach is pragmatic. No action needed now, just noting for future consideration.src/components/settings/fields.tsx (2)
136-150: Non-null assertion onelement.dependency_key!is fragile across all field components.Every field component uses
element.dependency_key!(e.g., Lines 141, 170, 201, 227, 263–264, 295, 327, 424). If a field element is missingdependency_key(it's optional in the type), this silently passesundefinedtoonChangeat runtime — the!only suppresses TypeScript. Consider adding an early return or guard, or makingdependency_keyrequired inFieldComponentProps.
227-227: Replaceanytypes flagged by build checks.The build pipeline flags
anyat Lines 227, 293, 320, and 424. These are straightforward to type:
- Line 227:
val: any→val: string(Radix Select'sonValueChangepasses astring)- Line 293:
val: any→val: string[](ToggleGroup multiple) orval: string(single)- Line 320:
element.default as any[]→(element.default as Array<string | number>)(matches the union inSettingsElement)- Line 424:
val: any→val: string(Radix RadioGroup'sonValueChangepasses astring)Also applies to: 293-293, 320-320, 424-424
src/components/settings/settings-formatter.ts (2)
143-153: Non-page elements without a resolvable parent are silently dropped.Line 156–160 discards any non-page element whose parent can't be resolved. While the inline comment documents this, silently losing data can make debugging very difficult for consumers. Consider collecting orphan IDs and emitting a development-mode warning (e.g.,
console.warn) so schema misconfigurations surface early.
352-358: Sharedparamsobject across pipe-delimited rules may conflict.When a validation entry uses pipe-delimited rules like
"not_empty|min_value|max_value", a singleparamsobject is reused for all rules in the pipe. This meansparams.minandparams.maxmust coexist in the same object. If a consumer defines separate validation entries per rule (each with its own params), this works fine — but the pipe-delimited shorthand silently limits param namespacing. Worth documenting this constraint.src/components/settings/settings-context.tsx (3)
269-304: Recursive schema search on every keystroke is an O(n) cost that could be precomputed.
findElement(Lines 274–283) walks the entire schema tree on everyupdateValuecall to locate the element for validation. For large schemas with many fields this runs on every keystroke. Consider building a memoizedMap<string, SettingsElement>(keyed bydependency_key) alongsidescopeFieldKeysMapto make lookups O(1).♻️ Sketch — precomputed field map
Add a memoized map alongside the existing
scopeFieldKeysMap:const fieldByKeyMap = useMemo(() => { const map = new Map<string, SettingsElement>(); const walk = (elements: SettingsElement[]) => { for (const el of elements) { if (el.type === 'field' && el.dependency_key) { map.set(el.dependency_key, el); } if (el.children?.length) walk(el.children); } }; for (const page of schema) walk(page.children || []); return map; }, [schema]);Then in
updateValue:- const element = findElement(schema); + const element = fieldByKeyMap.get(key);
184-187: Effect syncsinternalValueson everyexternalValuesidentity change but never resetsinitialValues.The comment at Lines 179–183 correctly explains why
initialValuesisn't reset here. However, if the consumer replacesexternalValueswith a brand-new object reference (e.g., after a fetch), all fields will appear dirty becauseinitialValuesstill holds the mount-time snapshot. After a data refetch, consider exposing a way to resetinitialValues(e.g., aresetDirtyStatemethod on the context) or document that consumers must call save/reset after loading fresh data.
190-207: Auto-select effect depends only onschema, suppressing the exhaustive-deps rule.The
eslint-disableon Line 207 suppresses the missingactivePagedependency. This is intentional (as noted: "only on schema load"), but ifrawSchemais not memoized by the consumer, a newschemareference is produced on every render, causing this effect to re-evaluate. The!activePageguard prevents a reset, but the effect still runs. Consider documenting that consumers should memoizerawSchema, or wrap theformatSettingsDatacall with a deep-compare memo.
| {element.html_content && ( | ||
| <div | ||
| className="prose prose-sm max-w-none" | ||
| dangerouslySetInnerHTML={{ __html: element.html_content }} | ||
| /> |
There was a problem hiding this comment.
Sanitize HTML content before injecting into the DOM.
dangerouslySetInnerHTML bypasses React's XSS protection. The element.escape_html property exists in the type definition but is never checked here. If html_content comes from user input or an untrusted API, this is exploitable. Use a library like DOMPurify to sanitize before rendering.
🛡️ Proposed fix
+import DOMPurify from 'dompurify'; {element.html_content && (
<div
className="prose prose-sm max-w-none"
- dangerouslySetInnerHTML={{ __html: element.html_content }}
+ dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(element.html_content) }}
/>
)}🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 402-402: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.3.14)
[error] 403-403: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🤖 Prompt for AI Agents
In `@src/components/settings/fields.tsx` around lines 400 - 404, The JSX is
injecting untrusted HTML with dangerouslySetInnerHTML without sanitization;
update the renderer in the settings field component to check element.escape_html
and, when true or when html_content may be untrusted, run the content through a
sanitizer (e.g., import DOMPurify and call
DOMPurify.sanitize(element.html_content)) and pass the sanitized string to
dangerouslySetInnerHTML; ensure you add the DOMPurify import at the top of the
file and only bypass sanitization if escape_html explicitly indicates the
content is safe.
| {/* Per-scope save button — sticky at the bottom */} | ||
| {showSaveArea && ( | ||
| <div | ||
| className="sticky bottom-0 border-t border-border bg-background px-6 py-3 flex justify-end" | ||
| data-testid={`settings-save-${scopeId}`} | ||
| > | ||
| {renderSaveButton | ||
| ? renderSaveButton({ scopeId, dirty, onSave: handleSave }) | ||
| : null} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Empty save area rendered when onSave is set but renderSaveButton is not.
When onSave is provided (making showSaveArea true) but renderSaveButton is omitted, the code renders an empty sticky <div> with a visible border-t. This creates a blank bar at the bottom of the content.
Additionally, the JSDoc on SettingsProps.renderSaveButton (in settings-types.ts Line 150) states: "If not provided but onSave is set, a default icon-only save button is rendered." — but no default button is actually implemented here.
Either provide a default save button or gate the save area on renderSaveButton being defined.
🐛 Option A: Gate on renderSaveButton
- const showSaveArea = Boolean(onSave);
+ const showSaveArea = Boolean(onSave && renderSaveButton);🐛 Option B: Provide a default save button
{renderSaveButton
? renderSaveButton({ scopeId, dirty, onSave: handleSave })
- : null}
+ : (
+ <button
+ onClick={handleSave}
+ disabled={!dirty}
+ className="inline-flex items-center gap-2 rounded-md bg-primary px-4 py-2 text-sm font-medium text-primary-foreground disabled:opacity-50"
+ >
+ Save
+ </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.
| {/* Per-scope save button — sticky at the bottom */} | |
| {showSaveArea && ( | |
| <div | |
| className="sticky bottom-0 border-t border-border bg-background px-6 py-3 flex justify-end" | |
| data-testid={`settings-save-${scopeId}`} | |
| > | |
| {renderSaveButton | |
| ? renderSaveButton({ scopeId, dirty, onSave: handleSave }) | |
| : null} | |
| </div> | |
| )} | |
| {/* Per-scope save button — sticky at the bottom */} | |
| {showSaveArea && ( | |
| <div | |
| className="sticky bottom-0 border-t border-border bg-background px-6 py-3 flex justify-end" | |
| data-testid={`settings-save-${scopeId}`} | |
| > | |
| {renderSaveButton | |
| ? renderSaveButton({ scopeId, dirty, onSave: handleSave }) | |
| : ( | |
| <button | |
| onClick={handleSave} | |
| disabled={!dirty} | |
| className="inline-flex items-center gap-2 rounded-md bg-primary px-4 py-2 text-sm font-medium text-primary-foreground disabled:opacity-50" | |
| > | |
| Save | |
| </button> | |
| )} | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
In `@src/components/settings/settings-content.tsx` around lines 113 - 123, The
sticky save area is shown whenever showSaveArea is true but when
renderSaveButton is undefined it renders an empty bar; implement the behavior
described in SettingsProps.renderSaveButton by providing a default icon-only
save button when renderSaveButton is not passed: inside the block that currently
does {renderSaveButton ? renderSaveButton({ scopeId, dirty, onSave: handleSave
}) : null} replace the null branch with a small default button component that
calls handleSave on click, reflects dirty (disabled/aria-disabled when !dirty),
includes a clear aria-label/testid (e.g., `settings-save-button-${scopeId}`),
and uses the same props shape ({ scopeId, dirty, onSave }) so custom renderers
remain compatible; ensure the default button styling matches the surrounding
layout and remove the empty border-t only when the save area is truly empty.
|
|
||
| // Pass scopeId (subpage ID if exists, otherwise page ID) along with key and value | ||
| const scopeId = keyToScopeMap.get(key) || activeSubpage || activePage; | ||
| onChange?.(scopeId, key, value); |
There was a problem hiding this comment.
Scope fallback may attribute a change to the wrong page/subpage.
If keyToScopeMap doesn't contain the key (e.g., dynamically added field or orphaned field), Line 300 falls back to the currently active subpage/page. This sends an incorrect scopeId to the consumer's onChange, potentially causing data to be associated with the wrong scope silently.
Consider logging a dev-mode warning when the fallback is used, or returning early to avoid misleading the consumer.
🤖 Prompt for AI Agents
In `@src/components/settings/settings-context.tsx` around lines 298 - 301, The
current fallback that sets scopeId = keyToScopeMap.get(key) || activeSubpage ||
activePage can attribute changes to the wrong scope when keyToScopeMap lacks the
key; update the handler to first check keyToScopeMap.has(key) and if it does
not, either return early (do not call onChange) or in development emit a clear
warning; specifically, replace the unconditional fallback around
keyToScopeMap.get(key) with a guard that uses keyToScopeMap.has(key) and, when
missing, calls console.warn (only when process.env.NODE_ENV === 'development')
and returns early instead of calling onChange(scopeId, key, value) so consumers
aren’t given a misleading scopeId (refer to keyToScopeMap, activeSubpage,
activePage, and onChange in that handler).
| // Detect if data is already hierarchical (pages with children) | ||
| const isHierarchical = data.some( | ||
| (item) => | ||
| item.type === 'page' && | ||
| Array.isArray(item.children) && | ||
| item.children.length > 0 | ||
| ); | ||
|
|
||
| if (isHierarchical) { | ||
| return data; | ||
| } |
There was a problem hiding this comment.
Hierarchical data bypasses all enrichment and normalization.
When the heuristic at Line 33 detects hierarchical data (a page with children), the entire input is returned as-is. This means no display defaults, no hook_key/dependency_key computation, no validation/dependency transforms, and no option normalization are applied — creating a behavioral gap between flat and pre-hierarchical inputs. Consumers passing hierarchical data must manually ensure all these fields are populated, which is easy to miss and not documented.
Consider either: (a) running the enrichment pass on hierarchical data too, or (b) documenting this clearly as a contract for pre-hierarchical consumers.
🤖 Prompt for AI Agents
In `@src/components/settings/settings-formatter.ts` around lines 32 - 42, The
early-return when isHierarchical is true skips all enrichment/normalization;
remove or modify that return so the existing enrichment/normalization pass (the
same logic used for flat inputs) runs for every item and is applied recursively
to item.children as well — ensure display defaults, hook_key/dependency_key
computation, validation/transforms and option normalization are executed for
both top-level items and nested children by invoking the same enrichment
functions on each node rather than bypassing hierarchical data.
| child.value = | ||
| child.value !== undefined | ||
| ? child.value | ||
| : child.default || ''; |
There was a problem hiding this comment.
Falsy default values (e.g. 0, false) are silently replaced with ''.
Line 198 uses child.default || '' which coerces falsy-but-valid defaults like 0 or false to an empty string. Use nullish coalescing instead.
🐛 Proposed fix
child.value =
child.value !== undefined
? child.value
- : child.default || '';
+ : child.default ?? '';📝 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.
| child.value = | |
| child.value !== undefined | |
| ? child.value | |
| : child.default || ''; | |
| child.value = | |
| child.value !== undefined | |
| ? child.value | |
| : child.default ?? ''; |
🤖 Prompt for AI Agents
In `@src/components/settings/settings-formatter.ts` around lines 195 - 198, The
assignment to child.value in settings-formatter.ts currently uses the `||`
operator and thus converts valid falsy defaults (like 0 or false) to an empty
string; update the expression that sets `child.value` (the block handling
`child.value = child.value !== undefined ? child.value : child.default || ''`)
to use nullish coalescing so `child.default` is only replaced when it is null or
undefined (preserve falsy-but-valid values such as 0 or false).
| type LayoutMenuItemData, | ||
| } from '../ui/layout-menu'; | ||
| import type { SettingsElement } from './settings-types'; | ||
| import * as LucideIcons from 'lucide-react'; |
There was a problem hiding this comment.
Barrel import of lucide-react defeats tree-shaking and bloats the bundle.
import * as LucideIcons from 'lucide-react' pulls in every icon component (~1000+ icons). Since the namespace object is used as a runtime lookup, bundlers cannot tree-shake it. This can add hundreds of KB to the output.
Consider a lazy/dynamic approach or a curated map of icons actually used in the settings schemas.
♻️ Suggested approach: curated icon map or dynamic import
-import * as LucideIcons from 'lucide-react';
+import {
+ MapPin, Store, ShoppingCart, CreditCard, Wallet, Brush,
+ Settings as SettingsIcon, Palette, Mail, Info,
+ type LucideIcon,
+} from 'lucide-react';
+
+const iconMap: Record<string, LucideIcon> = {
+ MapPin, Store, ShoppingCart, CreditCard, Wallet, Brush,
+ Settings: SettingsIcon, Palette, Mail, Info,
+};Then in getIcon:
- const IconComponent = (LucideIcons as any)[iconName];
+ const IconComponent = iconMap[iconName];If the icon set must be open-ended (consumer-defined schemas), consider accepting an iconResolver prop or a React.ComponentType map from the consumer instead of embedding the entire library.
🤖 Prompt for AI Agents
In `@src/components/settings/settings-sidebar.tsx` at line 8, The current barrel
import "import * as LucideIcons from 'lucide-react'" in settings-sidebar.tsx
pulls in the entire icon library and prevents tree-shaking; replace it with a
curated map or dynamic import strategy and update any icon lookup logic (e.g.,
getIcon) to use that map or an async resolver. Specifically, remove the
namespace import, create a small ICONS map of only the icons used by
SettingsSidebar/getIcon (or accept an iconResolver prop/React.ComponentType map
from the consumer) and switch runtime lookups to reference that map or use
dynamic import() to load icons on demand so bundlers can tree-shake unused
icons. Ensure references to LucideIcons in getIcon or the component are replaced
with the new ICONS map or resolver.
| * If not provided but `onSave` is set, a default icon-only save button is rendered. | ||
| */ | ||
| renderSaveButton?: (props: SaveButtonRenderProps) => React.ReactNode; |
There was a problem hiding this comment.
Documentation on renderSaveButton contradicts implementation.
The JSDoc on Line 150 states: "If not provided but onSave is set, a default icon-only save button is rendered." However, as noted in the settings-content.tsx review, no default button is actually rendered — the save area shows an empty div. Update the doc to match the implementation or implement the default button.
🤖 Prompt for AI Agents
In `@src/components/settings/settings-types.ts` around lines 150 - 152, The JSDoc
for the renderSaveButton prop is incorrect: it claims a default icon-only save
button is rendered when onSave is set, but settings-content.tsx currently
renders an empty save area instead; either update the comment or implement the
default. Fix by editing the renderSaveButton declaration's JSDoc in
settings-types.ts to accurately state that no default button is rendered (or
conversely implement the default button in settings-content.tsx by adding a
fallback render that uses SaveButtonRenderProps to render an icon-only button
when renderSaveButton is undefined and onSave is provided); reference the
renderSaveButton prop, SaveButtonRenderProps type, and the settings-content.tsx
save-area rendering logic to locate and change the behavior or documentation
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 86: The project installed remark-gfm but Storybook's MDX compiler isn't
configured to use it; update the Storybook docs parameters for
`@storybook/addon-docs` to include remark-gfm by adding
mdxCompileOptions.docs.remarkPlugins (e.g., set
parameters.docs.mdxCompileOptions.remarkPlugins to include require('remark-gfm')
or an imported remarkGfm) in your Storybook config (use the docs param if using
preview config or the addon options if on Storybook 7.1+); ensure the symbol
names to change are parameters, docs, mdxCompileOptions, and remarkPlugins and
that require('remark-gfm') (or imported remarkGfm) is present so GFM features
(tables, strikethrough) are enabled in MDX.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 94-96: The package.json currently pins engines.node to ">=22"
which will block users on Node 20; update the engines.node field in package.json
(the "engines" object) to a less restrictive constraint such as ">=20" (or
">=18" if you want broader support) and, if this constraint is intended only for
contributors, move the strict requirement into contributor documentation or dev
tooling instead of the package.json engines field so installs from npm aren’t
blocked for Node 20 users.
- Around line 59-62: Update package.json peerDependencies to require React 18+
since the codebase uses React 18-only APIs (e.g., useId in
src/components/ui/currency-input.tsx). Change the "react" and "react-dom"
entries in the peerDependencies block from ">=17.0.0" to ">=18.0.0" so consumers
cannot install with React 17 and encounter runtime errors.
🧹 Nitpick comments (1)
package.json (1)
1-10: Missing"type"field — module format is ambiguous.Without
"type": "module"(or"type": "commonjs"), Node defaults to treating.jsfiles as CJS. If your build outputs ESM syntax (e.g.,import/exportstatements), the"."entry's"import"condition will fail under native Node ESM resolution. This compounds theimport/requireconcern above. Consider adding"type": "module"if the build output is ESM, or ensure the build produces CJS-compatible output.
| "engines": { | ||
| "node": ">=22" | ||
| }, |
There was a problem hiding this comment.
engines.node >= 22 may be unnecessarily restrictive for consumers.
Node 20 LTS is still in its maintenance window (until April 2026). Since consumers install this package via npm, the engine constraint will block installation on Node 20 unless --ignore-engines is used. If the requirement is only for local development (e.g., workspace features), consider either relaxing to >=18 or >=20, or documenting that this constraint applies to contributors only.
🤖 Prompt for AI Agents
In `@package.json` around lines 94 - 96, The package.json currently pins
engines.node to ">=22" which will block users on Node 20; update the
engines.node field in package.json (the "engines" object) to a less restrictive
constraint such as ">=20" (or ">=18" if you want broader support) and, if this
constraint is intended only for contributors, move the strict requirement into
contributor documentation or dev tooling instead of the package.json engines
field so installs from npm aren’t blocked for Node 20 users.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/components/settings/settings-content.tsx (1)
80-112: Tabs lack keyboard arrow-key navigation per WAI-ARIA Tabs pattern.The ARIA attributes (
role="tab",aria-selected,tabIndex) are correctly applied, which is a nice improvement. However, the WAI-ARIA Tabs pattern also recommends arrow-key navigation between tabs (Left/Right arrows move focus between tab buttons). Currently, only click handlers are wired. Consider adding anonKeyDownhandler that moves focus between sibling tab buttons on arrow keys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings/settings-content.tsx` around lines 80 - 112, Add keyboard arrow-key navigation to the tab buttons so they follow the WAI-ARIA Tabs pattern: update the button rendering (the map over tabs that uses tabId, activeTab, setActiveTab, scopeId) to attach an onKeyDown handler which listens for ArrowLeft/ArrowRight (and Home/End optionally), computes the previous/next tab index from the tabs array (skipping tabs with display === false), moves focus to that sibling button (using the generated tabId or DOM focus) and calls setActiveTab with the new tab.id; ensure tabIndex and aria-selected are updated accordingly so focus/selection remain in sync.src/components/settings/settings-context.tsx (2)
260-271: Thecatch (err) { throw err; }is a no-op — remove or add meaningful handling.Catching an error only to immediately rethrow it is functionally identical to not having the try/catch at all. The only effect is an extra stack frame. Either remove the catch block entirely, or add useful error handling (e.g., logging, setting an error state for the UI).
♻️ Simplified version
const handleOnSave = useCallback( async (pageId: string, pageValues: Record<string, any>) => { if (!onSave) return; - try { - await Promise.resolve(onSave(pageId, pageValues)); - resetPageDirty(pageId); - } catch (err) { - throw err; - } + await Promise.resolve(onSave(pageId, pageValues)); + resetPageDirty(pageId); }, [onSave, resetPageDirty] );Note: If the intent is to keep dirty state when save fails, the original structure from the previous review (try/await/resetPageDirty in try, empty catch to swallow) was more appropriate. The current code rethrows, so
resetPageDirtyis correctly skipped on failure, but the caller must handle the thrown error or it becomes an unhandled promise rejection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings/settings-context.tsx` around lines 260 - 271, The catch block in handleOnSave is a no-op (catch (err) { throw err; }) — either remove the try/catch entirely or implement meaningful error handling: keep the try/await(resetPageDirty) pattern but in the catch() for onSave errors either log the error via your logger or set an error state for the UI, and importantly decide whether to rethrow (propagate) or swallow the error to preserve dirty state; locate handleOnSave, onSave and resetPageDirty to apply the chosen change so resetPageDirty only runs on success and errors are handled consistently.
274-309:findElementdoes a full tree walk on every keystroke — consider pre-computing a lookup map.
updateValueis called on every field change. Each call triggers a recursivefindElementtraversal of the entire schema tree to locate the element for validation. For schemas with many fields, this is O(n) per keystroke.♻️ Pre-compute a dependency_key → element map
+ // Pre-compute field lookup for O(1) access during updateValue + const fieldByKey = useMemo(() => { + const map = new Map<string, SettingsElement>(); + const walk = (elements: SettingsElement[]) => { + for (const el of elements) { + if (el.type === 'field' && el.dependency_key) { + map.set(el.dependency_key, el); + } + if (el.children) walk(el.children); + } + }; + walk(schema); + return map; + }, [schema]); const updateValue = useCallback( (key: string, value: any) => { setInternalValues((prev) => ({ ...prev, [key]: value })); - const findElement = (elements: SettingsElement[]): SettingsElement | undefined => { - for (const el of elements) { - if (el.dependency_key === key) return el; - if (el.children) { - const found = findElement(el.children); - if (found) return found; - } - } - return undefined; - }; - - const element = findElement(schema); + const element = fieldByKey.get(key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings/settings-context.tsx` around lines 274 - 309, updateValue currently runs a full recursive findElement traversal of the schema on every keystroke (O(n) per change); instead precompute a lookup map from dependency_key → SettingsElement when the schema changes and use that map inside updateValue to get the element in O(1). Implement the map (e.g., buildDependencyMap or dependencyKeyMap) using useMemo or a useRef updated when schema changes, replace the inline findElement logic in updateValue with a direct lookup into that map, then call validateField(element, value) and update setErrors/setInternalValues as before; ensure the map keys match the schema elements’ dependency_key and that updateValue still uses keyToScopeMap, activeSubpage, activePage and onChange unchanged.src/components/settings/fields.tsx (1)
356-377:LabelFieldandHtmlFieldacceptFieldComponentPropsbut never useonChange.Both components destructure
FieldComponentProps(which requiresonChange) but don't use it. This is fine at runtime since unused params are harmless, but it signals an imprecise contract. Consider usingOmit<FieldComponentProps, 'onChange'>or a narrower type, or simply ignoringonChangeexplicitly if this is intentional for polymorphic dispatch.Also applies to: 383-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings/fields.tsx` around lines 356 - 377, LabelField and HtmlField currently accept FieldComponentProps but never use the required onChange prop; update their prop types to reflect they don't need onChange by changing the component signatures to use Omit<FieldComponentProps, 'onChange'> (e.g., function LabelField({ element }: Omit<FieldComponentProps,'onChange') ) or, if you prefer to keep the original type, explicitly ignore the prop by destructuring it as (_onChange) to make the intent clear; apply the same change to HtmlField so both components' typings match their actual usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/settings/fields.tsx`:
- Around line 136-150: The TextField and other field components (e.g., the
functions rendering Input / Checkbox / Select inside this file) are using the
non-null assertion element.dependency_key! which can pass undefined into
onChange and corrupt the values map; update each field component (starting with
TextField) to guard for element.dependency_key being undefined — either early
return/null render from the component or call onChange only when a valid string
key exists (or log/throw a clear error), and ensure the prop types
(FieldComponentProps / SettingsElement) are respected; locate usages of
element.dependency_key! in the file and replace them with a conditional check of
element.dependency_key before invoking onChange or render a safe fallback UI.
- Around line 108-121: The TooltipTrigger usage passes an unsupported prop
`asChild` causing a TypeScript error; remove the `asChild` prop from the
TooltipTrigger component and keep the button as its direct child (i.e., nest the
<button> inside TooltipTrigger without `asChild`) so TooltipProvider/Tooltip,
TooltipTrigger, TooltipContent and the Info icon still render the tooltip for
element.tooltip correctly. Ensure no other references to `asChild` remain in
this tooltip block.
In `@src/components/settings/settings-content.tsx`:
- Around line 34-38: handleSave currently calls the async onSave (from context)
without awaiting it, which can cause unhandled promise rejections; update the
handleSave function to be async, await the call to onSave(scopeId, scopeValues),
and wrap the await in a try/catch to handle or surface errors (e.g., log or
propagate) so rejections are handled—refer to the handleSave function, the
onSave callback, getPageValues and scopeId when making this change.
In `@src/components/settings/settings-context.tsx`:
- Around line 184-187: The current useEffect that merges defaultValues and
externalValues (useEffect -> merged = { ...defaultValues, ...(externalValues ||
{}) }; setInternalValues(merged)) runs on both changes and overwrites
internalValues when defaultValues (derived from schema) changes without
resetting initialValues, causing fields to appear dirty; modify by splitting
into two effects: one effect that watches defaultValues (or schema) to reset
both internalValues and initialValues to the merged defaults, and a second
effect that watches only externalValues to merge external updates into
internalValues without touching initialValues; reference the useEffect,
defaultValues, externalValues, setInternalValues, and initialValues identifiers
when implementing the split so schema-driven defaults properly reset
initialValues while external updates do not.
---
Duplicate comments:
In `@src/components/settings/fields.tsx`:
- Around line 400-404: The JSX is rendering element.html_content unsafely;
update the component in fields.tsx to sanitize before using
dangerouslySetInnerHTML: import a sanitizer like DOMPurify, compute a sanitized
string (e.g. const safeHtml = DOMPurify.sanitize(element.html_content)) and use
dangerouslySetInnerHTML={{ __html: safeHtml }}; ensure you check
element.escape_html (use it to decide whether to sanitize or, safer, always
sanitize regardless) so element.html_content is never injected raw.
In `@src/components/settings/settings-content.tsx`:
- Around line 127-137: The sticky save bar is shown when showSaveArea (computed
from onSave) is true even if renderSaveButton is not provided, causing an empty
bordered div; update the conditional to require both showSaveArea and
renderSaveButton (or alternatively provide a default button) so the container
only renders when there is a visible button. Locate the JSX block that uses
showSaveArea, renderSaveButton, scopeId and handleSave in settings-content.tsx
and change the surrounding conditional to check renderSaveButton (and onSave if
you prefer) before rendering the sticky <div>, or implement a default
renderSaveButton fallback that calls onSave.
- Line 83: Replace the semantic <nav> element used in SettingsContent's tab
container with a non-landmark element: change the element that currently reads
as the tab container (the one with className "flex gap-4 -mb-px",
role="tablist", and aria-label="Settings sections") to a plain <div> so you keep
role="tablist", aria-label, className and other props but avoid overriding the
implicit navigation landmark of <nav>; update the JSX in settings-content.tsx
where the tab list is rendered accordingly.
In `@src/components/settings/settings-context.tsx`:
- Around line 304-306: The current fallback in the settings-context (using
keyToScopeMap.get(key) || activeSubpage || activePage) can silently attribute
changes to the wrong scope; update the logic around keyToScopeMap,
activeSubpage, activePage and onChange so that when keyToScopeMap does not
contain the key (i.e., !keyToScopeMap.has(key)), you still call onChange but
also emit a dev-only warning (check process.env.NODE_ENV !== 'production' or an
existing __DEV__ flag) that includes the missing key and which fallback scope
was used (activeSubpage or activePage) to help debugging; keep the existing
onChange signature and behavior but add this warning before invoking onChange.
---
Nitpick comments:
In `@src/components/settings/fields.tsx`:
- Around line 356-377: LabelField and HtmlField currently accept
FieldComponentProps but never use the required onChange prop; update their prop
types to reflect they don't need onChange by changing the component signatures
to use Omit<FieldComponentProps, 'onChange'> (e.g., function LabelField({
element }: Omit<FieldComponentProps,'onChange') ) or, if you prefer to keep the
original type, explicitly ignore the prop by destructuring it as (_onChange) to
make the intent clear; apply the same change to HtmlField so both components'
typings match their actual usage.
In `@src/components/settings/settings-content.tsx`:
- Around line 80-112: Add keyboard arrow-key navigation to the tab buttons so
they follow the WAI-ARIA Tabs pattern: update the button rendering (the map over
tabs that uses tabId, activeTab, setActiveTab, scopeId) to attach an onKeyDown
handler which listens for ArrowLeft/ArrowRight (and Home/End optionally),
computes the previous/next tab index from the tabs array (skipping tabs with
display === false), moves focus to that sibling button (using the generated
tabId or DOM focus) and calls setActiveTab with the new tab.id; ensure tabIndex
and aria-selected are updated accordingly so focus/selection remain in sync.
In `@src/components/settings/settings-context.tsx`:
- Around line 260-271: The catch block in handleOnSave is a no-op (catch (err) {
throw err; }) — either remove the try/catch entirely or implement meaningful
error handling: keep the try/await(resetPageDirty) pattern but in the catch()
for onSave errors either log the error via your logger or set an error state for
the UI, and importantly decide whether to rethrow (propagate) or swallow the
error to preserve dirty state; locate handleOnSave, onSave and resetPageDirty to
apply the chosen change so resetPageDirty only runs on success and errors are
handled consistently.
- Around line 274-309: updateValue currently runs a full recursive findElement
traversal of the schema on every keystroke (O(n) per change); instead precompute
a lookup map from dependency_key → SettingsElement when the schema changes and
use that map inside updateValue to get the element in O(1). Implement the map
(e.g., buildDependencyMap or dependencyKeyMap) using useMemo or a useRef updated
when schema changes, replace the inline findElement logic in updateValue with a
direct lookup into that map, then call validateField(element, value) and update
setErrors/setInternalValues as before; ensure the map keys match the schema
elements’ dependency_key and that updateValue still uses keyToScopeMap,
activeSubpage, activePage and onChange unchanged.
| export function TextField({ element, onChange }: FieldComponentProps) { | ||
| return ( | ||
| <FieldWrapper element={element}> | ||
| <Input | ||
| value={String(element.value ?? element.default ?? "")} | ||
| onChange={(e) => onChange(element.dependency_key!, e.target.value)} | ||
| placeholder={ | ||
| element.placeholder ? String(element.placeholder) : undefined | ||
| } | ||
| disabled={element.disabled} | ||
| className="sm:max-w-56" | ||
| /> | ||
| </FieldWrapper> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Non-null assertion on element.dependency_key! will silently pass undefined to onChange.
Every field component uses element.dependency_key! (e.g., Lines 141, 170, 201, 227, 263, 295, 327, 424), but dependency_key is typed as string | undefined in SettingsElement. If a field element arrives without dependency_key set (e.g., misconfigured schema), the onChange handler receives undefined as the key, potentially corrupting the values map silently.
Consider a guard or early return:
♻️ Example guard (apply in each field component or in a shared wrapper)
export function TextField({ element, onChange }: FieldComponentProps) {
+ if (!element.dependency_key) return null;
return (
<FieldWrapper element={element}>
<Input
value={String(element.value ?? element.default ?? "")}
- onChange={(e) => onChange(element.dependency_key!, e.target.value)}
+ onChange={(e) => onChange(element.dependency_key, e.target.value)}Also applies to: 156-190, 196-210, 216-249, 255-280, 286-310, 316-350, 414-444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/settings/fields.tsx` around lines 136 - 150, The TextField and
other field components (e.g., the functions rendering Input / Checkbox / Select
inside this file) are using the non-null assertion element.dependency_key! which
can pass undefined into onChange and corrupt the values map; update each field
component (starting with TextField) to guard for element.dependency_key being
undefined — either early return/null render from the component or call onChange
only when a valid string key exists (or log/throw a clear error), and ensure the
prop types (FieldComponentProps / SettingsElement) are respected; locate usages
of element.dependency_key! in the file and replace them with a conditional check
of element.dependency_key before invoking onChange or render a safe fallback UI.
| const handleSave = () => { | ||
| if (!onSave) return; | ||
| const scopeValues = getPageValues(scopeId); | ||
| onSave(scopeId, scopeValues); | ||
| }; |
There was a problem hiding this comment.
handleSave doesn't await the async onSave, causing unhandled promise rejections on failure.
onSave from context is handleOnSave, which is async and may throw (the catch/rethrow in settings-context.tsx). Since handleSave calls it without await or .catch(), any rejection becomes an unhandled promise rejection, which can crash the app in strict environments.
🐛 Proposed fix
- const handleSave = () => {
+ const handleSave = async () => {
if (!onSave) return;
const scopeValues = getPageValues(scopeId);
- onSave(scopeId, scopeValues);
+ try {
+ await onSave(scopeId, scopeValues);
+ } catch {
+ // Error handling (e.g., toast notification) can be added here
+ }
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/settings/settings-content.tsx` around lines 34 - 38,
handleSave currently calls the async onSave (from context) without awaiting it,
which can cause unhandled promise rejections; update the handleSave function to
be async, await the call to onSave(scopeId, scopeValues), and wrap the await in
a try/catch to handle or surface errors (e.g., log or propagate) so rejections
are handled—refer to the handleSave function, the onSave callback, getPageValues
and scopeId when making this change.
| useEffect(() => { | ||
| const merged = { ...defaultValues, ...(externalValues || {}) }; | ||
| setInternalValues(merged); | ||
| }, [defaultValues, externalValues]); |
There was a problem hiding this comment.
External value sync also triggers when defaultValues (derived from schema) changes, potentially marking fields dirty.
This useEffect runs when either defaultValues or externalValues changes. If the schema prop changes (causing defaultValues to recompute), internalValues gets overwritten but initialValues is not reset (per the comment at Line 180). This means fields may appear dirty after a schema change even though the user made no edits.
If schema changes are expected at runtime, consider resetting initialValues when the schema changes, or splitting the effect to handle schema changes and external value changes separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/settings/settings-context.tsx` around lines 184 - 187, The
current useEffect that merges defaultValues and externalValues (useEffect ->
merged = { ...defaultValues, ...(externalValues || {}) };
setInternalValues(merged)) runs on both changes and overwrites internalValues
when defaultValues (derived from schema) changes without resetting
initialValues, causing fields to appear dirty; modify by splitting into two
effects: one effect that watches defaultValues (or schema) to reset both
internalValues and initialValues to the merged defaults, and a second effect
that watches only externalValues to merge external updates into internalValues
without touching initialValues; reference the useEffect, defaultValues,
externalValues, setInternalValues, and initialValues identifiers when
implementing the split so schema-driven defaults properly reset initialValues
while external updates do not.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/settings/fields.tsx (1)
227-227:(val: any)inonValueChangebypasses type safety — also applies to Line 424.Both
SelectField(Line 227) andCustomizeRadioField(Line 424) cast theonValueChangeargument toany. The localSelectandRadioGroupwrappers should expose a typedstringvalue; usinganysuppresses any mismatch between the expected callback signature and the actual emitted value.♻️ Replace `any` with the expected type
- onValueChange={(val: any) => onChange(element.dependency_key!, val)} + onValueChange={(val: string) => onChange(element.dependency_key!, val)}Apply the same change at Line 424.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings/fields.tsx` at line 227, Replace the use of the unsafely typed "(val: any)" in the onValueChange handlers with the correct typed signature (e.g., "(val: string)") so the callback matches the wrapped Select and RadioGroup value types; update the onValueChange in SelectField (where it calls onChange(element.dependency_key!, val)) and the analogous handler in CustomizeRadioField to accept a string (or the exact value type exposed by your Select/RadioGroup wrappers) instead of any, ensuring type-safe propagation to the onChange function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/settings/fields.tsx`:
- Around line 286-310: RadioCapsuleField's onValueChange treats the Base UI
ToggleGroup's array as if its first element is the newly selected value, so
selection never changes; update the handler in RadioCapsuleField to mirror
ButtonToggleGroup's single-select pattern by detecting the newly-added item from
the emitted array (e.g., if val is an array, pick the element that differs from
the currentValue, falling back to val[0] or the single string when not an array)
and pass that to onChange(element.dependency_key!, selected); keep using
ToggleGroup and ToggleGroupItem but replace the current Array.isArray(val) ?
val[0] : val logic with this new derivation.
---
Duplicate comments:
In `@src/components/settings/fields.tsx`:
- Line 141: Several interactive field components are using the non-null
assertion element.dependency_key! and can pass undefined into onChange; add a
guard to return null when dependency_key is missing. For each affected component
(NumberField, TextareaField, SelectField, SwitchField, RadioCapsuleField,
MulticheckField, CustomizeRadioField) in fields.tsx, add at the top of the
component render body: if (!element.dependency_key) return null; so
onChange(element.dependency_key, ...) never receives undefined and the values
map cannot be corrupted.
- Around line 400-404: element.html_content is being injected without
sanitization and element.escape_html is ignored; update the component in
fields.tsx to respect element.escape_html and to sanitize HTML before using
dangerouslySetInnerHTML: import DOMPurify (or your chosen sanitizer) at the top,
then if element.escape_html is truthy render the HTML as plain text (no
dangerouslySetInnerHTML) otherwise sanitize element.html_content with
DOMPurify.sanitize(...) and pass the sanitized output to
dangerouslySetInnerHTML; reference the element.html_content and
element.escape_html checks and the JSX node where dangerouslySetInnerHTML is
used.
---
Nitpick comments:
In `@src/components/settings/fields.tsx`:
- Line 227: Replace the use of the unsafely typed "(val: any)" in the
onValueChange handlers with the correct typed signature (e.g., "(val: string)")
so the callback matches the wrapped Select and RadioGroup value types; update
the onValueChange in SelectField (where it calls
onChange(element.dependency_key!, val)) and the analogous handler in
CustomizeRadioField to accept a string (or the exact value type exposed by your
Select/RadioGroup wrappers) instead of any, ensuring type-safe propagation to
the onChange function.
| export function RadioCapsuleField({ element, onChange }: FieldComponentProps) { | ||
| const currentValue = String(element.value ?? element.default ?? ""); | ||
|
|
||
| return ( | ||
| <FieldWrapper element={element}> | ||
| <ToggleGroup | ||
| value={[currentValue]} | ||
| onValueChange={(val: any) => { | ||
| const selected = Array.isArray(val) ? val[0] : val; | ||
| if (selected) onChange(element.dependency_key!, selected); | ||
| }} | ||
| > | ||
| {element.options?.map((option) => ( | ||
| <ToggleGroupItem | ||
| key={String(option.value)} | ||
| value={String(option.value)} | ||
| className="text-xs px-3" | ||
| > | ||
| {option.label ?? option.title} | ||
| </ToggleGroupItem> | ||
| ))} | ||
| </ToggleGroup> | ||
| </FieldWrapper> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "toggle-group" src/ --exec cat {}Repository: getdokan/plugin-ui
Length of output: 8098
RadioCapsuleField selection logic broken — val[0] returns the original item, not the newly clicked one.
The local ToggleGroup wrapper delegates directly to @base-ui/react's ToggleGroupPrimitive without overriding its multi-select behavior. When an option is clicked, Base UI emits the full array of selected values. For example, clicking option "B" while "A" is already selected yields val = ["A", "B"], so val[0] always returns the first (unchanged) element and the selection never updates. The (val: any) cast and the Array.isArray guard reveal the author's uncertainty about the API shape.
The project already demonstrates the correct single-select pattern in the ButtonToggleGroup component, which converts Base UI's multi-select behavior by wrapping the value in an array and extracting the first element from the callback. Apply the same approach here, or derive the newly-added item from the diff:
🐛 Proposed fix — derive the new item from the updated array
export function RadioCapsuleField({ element, onChange }: FieldComponentProps) {
const currentValue = String(element.value ?? element.default ?? "");
return (
<FieldWrapper element={element}>
<ToggleGroup
- value={[currentValue]}
- onValueChange={(val: any) => {
- const selected = Array.isArray(val) ? val[0] : val;
- if (selected) onChange(element.dependency_key!, selected);
- }}
+ value={currentValue ? [currentValue] : []}
+ onValueChange={(val: string[]) => {
+ // In multi-select mode, the newly pressed item is the one not in the previous selection
+ const next = val.find((v) => v !== currentValue);
+ if (next) onChange(element.dependency_key!, next);
+ }}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/settings/fields.tsx` around lines 286 - 310,
RadioCapsuleField's onValueChange treats the Base UI ToggleGroup's array as if
its first element is the newly selected value, so selection never changes;
update the handler in RadioCapsuleField to mirror ButtonToggleGroup's
single-select pattern by detecting the newly-added item from the emitted array
(e.g., if val is an array, pick the element that differs from the currentValue,
falling back to val[0] or the single string when not an array) and pass that to
onChange(element.dependency_key!, selected); keep using ToggleGroup and
ToggleGroupItem but replace the current Array.isArray(val) ? val[0] : val logic
with this new derivation.
2 participants
feat/settings-components — Reusable Schema-Driven Settings Component
Summary
Introduces a reusable Settings component that renders a complete settings page from a JSON schema (hierarchical or flat).
Supports:
Designed for use across plugins with optional WordPress filter extensibility.
Features
Schema-driven UI
Renders settings from a JSON array of
SettingsElementobjects (flat or hierarchical).Hierarchical structure
Pages → Subpages → Tabs → Sections → Subsections → Field Groups → Fields
Field variants
textnumbertextareaselectswitchradio_capsulecustomize_radio(RadioCard)multicheckbase_field_labelhtmlFlat data support
formatSettingsData()converts flat arrays using parent pointers(
page_id,subpage_id,tab_id,section_id, etc.) into a hierarchy.Per-scope save
Dirty tracking and save button per page/subpage.
onSave(scopeId, values)receives the active scope ID.Customizable save button
renderSaveButtonrender prop for custom UI and i18n:Summary by CodeRabbit
New Features
Documentation