fix: update onSave to pass nested tree values and handle save errors#65
Merged
fix: update onSave to pass nested tree values and handle save errors#65
Conversation
- Add dot-key-to-nested-object tree building in handleOnSave - Pass both treeValues and flatValues to onSave callback - Align onSave signature across all interfaces (SettingsContextValue, SettingsProviderProps, SettingsProps) - Only reset dirty state on successful save - Handle server-side validation errors by merging into errors state - Add setErrors to useCallback deps to prevent stale closure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughSettings save behavior was changed: the provider now constructs a nested Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Settings UI"
participant Provider as "SettingsProvider"
participant App as "Consumer onSave / App"
participant Server as "Server/API"
UI->>Provider: submit save (flat pageValues)
Provider->>Provider: build treeValues from dot-keys
Provider->>App: call onSave(pageId, treeValues, pageValues)
alt success
App->>Server: persist payload (treeValues / flatValues)
Server-->>App: success
App-->>Provider: resolve
Provider->>UI: clear dirty state
else error
App-->>Provider: throws with error.errors
Provider->>Provider: merge field-level errors into errors state
Provider->>UI: surface field errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Update examples and API reference tables to reflect the new onSave(scopeId, treeValues, flatValues) signature. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/DeveloperGuide.mdx (1)
915-917: Example shows only twoonSaveparameters but API accepts three.The example destructures only
(scopeId, treeValues), but the actual signature is(scopeId, treeValues, flatValues). While this works sinceflatValuesis optional to use, consider showing the full signature for completeness:- onSave={async (scopeId, treeValues) => { - await apiFetch({ path: `/my-plugin/v1/settings/${scopeId}`, method: 'POST', data: treeValues }); + onSave={async (scopeId, treeValues, flatValues) => { + // treeValues: nested object, flatValues: original dot-keyed values + await apiFetch({ path: `/my-plugin/v1/settings/${scopeId}`, method: 'POST', data: treeValues }); }}This aligns with the documented API in
Settings.mdxlines 80-84 which shows all three parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DeveloperGuide.mdx` around lines 915 - 917, Update the onSave handler example to match the real API signature by including the third parameter: change the parameter list from (scopeId, treeValues) to (scopeId, treeValues, flatValues) in the onSave callback so it reflects the actual signature used by Settings (refer to the onSave handler and Settings API docs), and adjust any example usage inside the handler to accept but not necessarily use flatValues.DEVELOPER_GUIDE.md (1)
1131-1136: Example shows only twoonSaveparameters for consistency with Storybook guide.Similar to
src/DeveloperGuide.mdx, this example omitsflatValues. While functionally correct, consider aligning with the full signature documented inSettings.mdxfor API completeness:- onSave={async (scopeId, treeValues) => { + onSave={async (scopeId, treeValues, _flatValues) => { await apiFetch({ path: `/my-plugin/v1/settings/${scopeId}`, method: 'POST', data: treeValues, }); }}Using
_flatValuessignals it's intentionally unused while showing the full signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEVELOPER_GUIDE.md` around lines 1131 - 1136, Update the example onSave handler to show the full signature by adding the unused flatValues parameter (use a named _flatValues to signal it's intentionally unused) so it matches the signature used in Settings.mdx and DeveloperGuide.mdx; locate the onSave arrow function (onSave={async (scopeId, treeValues) => { ... }}) and change its parameters to (scopeId, treeValues, _flatValues) and leave the body unchanged except for using the same apiFetch call.src/components/settings/settings-context.tsx (1)
270-271: Edge case:typeof null === 'object'in JavaScript.If an intermediate key has a
nullvalue, this check passes but the subsequent assignment fails. Consider adding a null check:- if (!(parts[i] in cursor) || typeof cursor[parts[i]] !== 'object') { + if (!(parts[i] in cursor) || cursor[parts[i]] === null || typeof cursor[parts[i]] !== 'object') {This is unlikely in practice for settings data, but worth hardening if keys could theoretically have null values.
🤖 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 270 - 271, The current traversal in the SettingsContext (the block using variables cursor and parts[]) checks typeof cursor[parts[i]] !== 'object' but misses the null case; update the condition in the loop that builds intermediate keys (the code that assigns cursor[parts[i]] = {}) to also verify cursor[parts[i]] !== null (i.e. only treat it as an object when not null) so that null values are replaced with a new object before continuing. Ensure the change is applied where cursor and parts are iterated in the settings-context.tsx function that mutates nested settings.
🤖 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/settings-types.ts`:
- Around line 139-140: The onSave callback in the settings types is currently
declared to return void but must allow async handlers; update the onSave
signature in settings-types (the onSave?: (scopeId: string, treeValues:
Record<string, any>, flatValues: Record<string, any>) => void) to return void |
Promise<void> so it matches how handleOnSave in settings-context.tsx awaits
Promise.resolve(onSave(...)) and avoids TypeScript mismatches.
---
Nitpick comments:
In `@DEVELOPER_GUIDE.md`:
- Around line 1131-1136: Update the example onSave handler to show the full
signature by adding the unused flatValues parameter (use a named _flatValues to
signal it's intentionally unused) so it matches the signature used in
Settings.mdx and DeveloperGuide.mdx; locate the onSave arrow function
(onSave={async (scopeId, treeValues) => { ... }}) and change its parameters to
(scopeId, treeValues, _flatValues) and leave the body unchanged except for using
the same apiFetch call.
In `@src/components/settings/settings-context.tsx`:
- Around line 270-271: The current traversal in the SettingsContext (the block
using variables cursor and parts[]) checks typeof cursor[parts[i]] !== 'object'
but misses the null case; update the condition in the loop that builds
intermediate keys (the code that assigns cursor[parts[i]] = {}) to also verify
cursor[parts[i]] !== null (i.e. only treat it as an object when not null) so
that null values are replaced with a new object before continuing. Ensure the
change is applied where cursor and parts are iterated in the
settings-context.tsx function that mutates nested settings.
In `@src/DeveloperGuide.mdx`:
- Around line 915-917: Update the onSave handler example to match the real API
signature by including the third parameter: change the parameter list from
(scopeId, treeValues) to (scopeId, treeValues, flatValues) in the onSave
callback so it reflects the actual signature used by Settings (refer to the
onSave handler and Settings API docs), and adjust any example usage inside the
handler to accept but not necessarily use flatValues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4e2a492-8900-4b6c-a043-606a00f8b0a2
📒 Files selected for processing (5)
DEVELOPER_GUIDE.mdsrc/DeveloperGuide.mdxsrc/components/settings/Settings.mdxsrc/components/settings/settings-context.tsxsrc/components/settings/settings-types.ts
Comment on lines
+139
to
+140
| /** Called when the save button is clicked. Receives the scope ID, nested tree values, and flat dot-keyed values. */ | ||
| onSave?: (scopeId: string, treeValues: Record<string, any>, flatValues: Record<string, any>) => void; |
There was a problem hiding this comment.
Return type mismatch: void vs void | Promise<void>.
The onSave return type here is void, but in settings-context.tsx (lines 72 and 91), the same callback is typed as void | Promise<void>. This inconsistency may cause TypeScript errors for consumers using async onSave handlers.
Since handleOnSave in the context uses await Promise.resolve(onSave(...)), the Promise return type should be supported:
/** Called when the save button is clicked. Receives the scope ID, nested tree values, and flat dot-keyed values. */
- onSave?: (scopeId: string, treeValues: Record<string, any>, flatValues: Record<string, any>) => void;
+ onSave?: (scopeId: string, treeValues: Record<string, any>, flatValues: Record<string, any>) => void | Promise<void>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Called when the save button is clicked. Receives the scope ID, nested tree values, and flat dot-keyed values. */ | |
| onSave?: (scopeId: string, treeValues: Record<string, any>, flatValues: Record<string, any>) => void; | |
| /** Called when the save button is clicked. Receives the scope ID, nested tree values, and flat dot-keyed values. */ | |
| onSave?: (scopeId: string, treeValues: Record<string, any>, flatValues: Record<string, any>) => void | Promise<void>; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/settings/settings-types.ts` around lines 139 - 140, The onSave
callback in the settings types is currently declared to return void but must
allow async handlers; update the onSave signature in settings-types (the
onSave?: (scopeId: string, treeValues: Record<string, any>, flatValues:
Record<string, any>) => void) to return void | Promise<void> so it matches how
handleOnSave in settings-context.tsx awaits Promise.resolve(onSave(...)) and
avoids TypeScript mismatches.
Provides architecture overview, component API surface, settings system usage, theme system, WordPress integration patterns, and CI checks. Not included in npm package (not in files array). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 195: The documentation text uses an incorrect casing for "GitHub" —
update the string in CLAUDE.md (the sentence referencing the CI pipeline
`.github/workflows/ci.yml`) to use the official casing "GitHub" so the reference
reads "The CI pipeline (`.github/workflows/ci.yml`) runs these on
`ubuntu-latest` with Node 24." ensuring consistent capitalization throughout the
file.
- Around line 7-17: The Markdown code fence containing the architecture tree
lacks a language tag (triggers markdownlint MD040); update the opening
triple-backticks to include a language (use ```text) so the block becomes fenced
with ```text and resolves the lint error while preserving the existing content.
Comment on lines
+7
to
+17
| ``` | ||
| src/ | ||
| ├── components/ | ||
| │ ├── ui/ # Core ShadCN-style components (150+ exports) | ||
| │ ├── settings/ # Schema-driven settings system | ||
| │ └── wordpress/ # WordPress integration (Layout, DataViews) | ||
| ├── providers/ # ThemeProvider (CSS variable injection) | ||
| ├── themes/ # Built-in theme presets | ||
| ├── hooks/ # useMobile, useWindowDimensions | ||
| └── lib/ # Utilities (cn, renderIcon, WpMedia, wordpress-date) | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the architecture code fence.
This block is missing a fence language, which will keep tripping markdownlint (MD040). text would be the right fit here.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 7 - 17, The Markdown code fence containing the
architecture tree lacks a language tag (triggers markdownlint MD040); update the
opening triple-backticks to include a language (use ```text) so the block
becomes fenced with ```text and resolves the lint error while preserving the
existing content.
| npm run typecheck # tsc --noEmit | ||
| ``` | ||
|
|
||
| Both must pass. The CI pipeline (`.github/workflows/ci.yml`) runs these on `ubuntu-latest` with Node 24. |
There was a problem hiding this comment.
Capitalize GitHub consistently.
Use the official casing here to avoid a docs/style nit slipping into a reference section.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~195-~195: The official name of this software platform is spelled with a capital “H”.
Context: ...t ``` Both must pass. The CI pipeline (.github/workflows/ci.yml) runs these on `ubunt...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 195, The documentation text uses an incorrect casing for
"GitHub" — update the string in CLAUDE.md (the sentence referencing the CI
pipeline `.github/workflows/ci.yml`) to use the official casing "GitHub" so the
reference reads "The CI pipeline (`.github/workflows/ci.yml`) runs these on
`ubuntu-latest` with Node 24." ensuring consistent capitalization throughout the
file.
The context's internal save wrapper (2 args) shared the same name as the consumer's onSave prop (3 args), causing a TS2554 error in settings-content. Rename to `save` in SettingsContextValue and update JSDoc across types and docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/settings/settings-context.tsx (1)
264-276: Consider defensive handling for tree-building edge cases.Two concerns with the current tree-building logic:
Outside try/catch: If
pageValuesis unexpectedly malformed, the loop would throw before reaching the try/catch block.Null intermediate values: If
typeof cursor[parts[i]] === 'object'passes fornull(sincetypeof null === 'object'), the subsequentcursor = cursor[parts[i]]assignsnull, causingcursor[parts[parts.length - 1]] = valto throw.🛡️ Proposed fix: Guard against null and wrap in try/catch
if (!onSave) return; + try { // Build nested tree from dot-separated keys const treeValues: Record<string, any> = {}; for (const [dotKey, val] of Object.entries(pageValues)) { const parts = dotKey.split('.'); let cursor: Record<string, any> = treeValues; for (let i = 0; i < parts.length - 1; i++) { - if (!(parts[i] in cursor) || typeof cursor[parts[i]] !== 'object') { + if (!(parts[i] in cursor) || cursor[parts[i]] === null || typeof cursor[parts[i]] !== 'object') { cursor[parts[i]] = {}; } cursor = cursor[parts[i]]; } cursor[parts[parts.length - 1]] = val; } - try { await Promise.resolve(onSave(pageId, treeValues, pageValues));🤖 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 264 - 276, The tree-building loop that converts pageValues into treeValues should be made defensive: wrap the entire for..of over Object.entries(pageValues) in a try/catch and on error log or rethrow with context; when iterating parts, guard against null by checking cursor[parts[i]] !== null && typeof cursor[parts[i]] === 'object' before assigning cursor = cursor[parts[i]] and if the existing value is missing or not a non-null object, replace it with an empty object (cursor[parts[i]] = {}) so traversal never sets cursor to null; reference the treeValues, pageValues, parts and cursor variables and ensure any caught error includes context about the offending dotKey.
🤖 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/settings-context.tsx`:
- Line 91: The onSave callback signature in settings-context.tsx changed to
onSave?: (scopeId: string, treeValues: Record<string, any>, flatValues:
Record<string, any>) => void | Promise<void>, so update all call sites that
still use two parameters (e.g., the Settings.stories usage and the Developer
Guide examples) to accept the third flatValues argument or explicitly ignore it
(for example change (scopeId, treeValues) => ... to (scopeId, treeValues,
flatValues) => ... or (scopeId, treeValues, ..._rest) => ...), ensuring any
handlers that call or implement onSave (named onSave) match the new
three-parameter signature.
---
Nitpick comments:
In `@src/components/settings/settings-context.tsx`:
- Around line 264-276: The tree-building loop that converts pageValues into
treeValues should be made defensive: wrap the entire for..of over
Object.entries(pageValues) in a try/catch and on error log or rethrow with
context; when iterating parts, guard against null by checking cursor[parts[i]]
!== null && typeof cursor[parts[i]] === 'object' before assigning cursor =
cursor[parts[i]] and if the existing value is missing or not a non-null object,
replace it with an empty object (cursor[parts[i]] = {}) so traversal never sets
cursor to null; reference the treeValues, pageValues, parts and cursor variables
and ensure any caught error includes context about the offending dotKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc6177cf-8b86-42fd-9304-505509463687
📒 Files selected for processing (4)
src/components/settings/Settings.mdxsrc/components/settings/settings-content.tsxsrc/components/settings/settings-context.tsxsrc/components/settings/settings-types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/settings/Settings.mdx
- src/components/settings/settings-types.ts
| values?: Record<string, any>; | ||
| onChange?: (scopeId: string, key: string, value: any) => void; | ||
| onSave?: (scopeId: string, values: Record<string, any>) => void | Promise<void>; | ||
| onSave?: (scopeId: string, treeValues: Record<string, any>, flatValues: Record<string, any>) => void | Promise<void>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the settings-context.tsx file to see the context around line 91
head -100 src/components/settings/settings-context.tsx | tail -20Repository: getdokan/plugin-ui
Length of output: 895
🏁 Script executed:
# Get more context - read the full file or at least the type/interface definition
wc -l src/components/settings/settings-context.tsxRepository: getdokan/plugin-ui
Length of output: 110
🏁 Script executed:
# Search for onSave usages in the codebase
rg -n 'onSave' --type=ts --type=tsx -B 2 -A 2 | head -100Repository: getdokan/plugin-ui
Length of output: 89
🏁 Script executed:
# Search for onSave usages without type filters
rg -n 'onSave' -B 2 -A 2Repository: getdokan/plugin-ui
Length of output: 15495
🏁 Script executed:
# Also check how onSave is called/invoked in the file
rg -n 'onSave\s*\(' -B 1 -A 1Repository: getdokan/plugin-ui
Length of output: 1625
🏁 Script executed:
# Verify the exact line in Settings.stories.tsx to understand the context better
sed -n '1080,1095p' src/components/settings/Settings.stories.tsxRepository: getdokan/plugin-ui
Length of output: 911
🏁 Script executed:
# Check if there are any other invocations of onSave callback we might have missed
rg -n 'onSave\s*\(\s*[^)]*\s*\)' -B 1 -A 1Repository: getdokan/plugin-ui
Length of output: 1625
Breaking change: onSave signature updated — some call sites still need updating.
The new signature (scopeId, treeValues, flatValues) is a breaking change. Several usages have not been updated:
src/components/settings/Settings.stories.tsxline 1084 still uses 2 parameterssrc/DeveloperGuide.mdxandDEVELOPER_GUIDE.mdexamples still use 2 parameters
Update these call sites to accept the new third parameter flatValues or ignore it with a rest parameter if unused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/settings/settings-context.tsx` at line 91, The onSave callback
signature in settings-context.tsx changed to onSave?: (scopeId: string,
treeValues: Record<string, any>, flatValues: Record<string, any>) => void |
Promise<void>, so update all call sites that still use two parameters (e.g., the
Settings.stories usage and the Developer Guide examples) to accept the third
flatValues argument or explicitly ignore it (for example change (scopeId,
treeValues) => ... to (scopeId, treeValues, flatValues) => ... or (scopeId,
treeValues, ..._rest) => ...), ensuring any handlers that call or implement
onSave (named onSave) match the new three-parameter signature.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
onSavenow receives a nested object built from dot-separated keys (e.g.{"dokan": {"general": {"store_name": "..."}}}) as the second argument, with the original flat dot-keyed values as the thirdonSavetype acrossSettingsContextValue,SettingsProviderProps, andSettingsPropsto(scopeId, treeValues, flatValues){ errors: {...} }) are merged into the errors statesetErrorstouseCallbackdependency array to prevent stale closureTest plan
onSavereceives(scopeId, treeValues, flatValues)with correct shapesonSavethrowsonSavethrows{ errors: { fieldKey: "message" } }🤖 Generated with Claude Code
Summary by CodeRabbit
Breaking Changes
Documentation