feat(settings): support plain field-id keys for dependency lookups#90
Conversation
Plugin-ui's formatSettingsData() rebuilds dependency_key as a dot-path
(parent.child.field) and the values map is keyed by that dot-path.
Dependencies declared with a plain field id (e.g., 'commission_type'
instead of 'commission.commission.commission_type') therefore failed to
resolve via values[dep.key].
Add an id-keyed fallback so both formats work side by side:
- Export buildIdIndex(schema) from settings-formatter — produces a
{ field_id: dependency_key } map from the hierarchical schema.
- evaluateDependencies() accepts an optional idIndex argument. When
values[dep.key] is undefined and an idIndex is supplied, the function
resolves dep.key as a field id and re-reads values via the index.
- SettingsProvider builds the idIndex (memoised on schema) and threads
it through shouldDisplay → evaluateDependencies.
Backwards compatible: callers that don't pass idIndex see identical
behavior. Consumers whose backend guarantees globally-unique field ids
(e.g., flat-storage schemas) can now use id-keyed dependencies, which
stay valid across structural moves (no parent path to update).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR enhances dependency evaluation in the settings system by introducing a schema-derived index for resolving field dependencies. A new ChangesSchema-driven dependency resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/settings/settings-formatter.ts`:
- Around line 312-327: The idIndex map should not be a plain {} because
prototype keys can collide; change its creation to a prototypeless map (use
Object.create(null)) and replace any "el.id in idIndex" checks and any direct
property reads with own-property checks (e.g.,
Object.prototype.hasOwnProperty.call(idIndex, el.id) or Object.hasOwn(idIndex,
el.id)) before treating or reading idIndex[el.id]; update the checks inside the
walk function and wherever idIndex is read (referencing idIndex and the walk /
SettingsElement logic) to use these own-property checks so prototype properties
like "toString" or "constructor" won't be treated as existing IDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25a5fb2c-0c0c-4f09-a903-e7bd8134220a
📒 Files selected for processing (2)
src/components/settings/settings-context.tsxsrc/components/settings/settings-formatter.ts
| const idIndex: Record<string, string> = {}; | ||
|
|
||
| const walk = (elements: SettingsElement[]) => { | ||
| for (const el of elements) { | ||
| if ( | ||
| el.type === 'field' && | ||
| el.id && | ||
| el.dependency_key && | ||
| el.id !== el.dependency_key | ||
| ) { | ||
| // First writer wins — if two fields share an id (a schema | ||
| // bug consumers should detect with their own validator), | ||
| // we don't silently clobber the earlier mapping. | ||
| if (!(el.id in idIndex)) { | ||
| idIndex[el.id] = el.dependency_key; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify prototype-sensitive dictionary usage in the touched file.
rg -n -C2 "Record<string, string> = \{\}|\\bin idIndex\\b|idIndex\\[dep\\.key\\]" src/components/settings/settings-formatter.tsRepository: getdokan/plugin-ui
Length of output: 1073
Use own-property checks for idIndex to avoid prototype-key collisions.
Using a plain object {} for the idIndex dictionary with the in operator (line 325) and direct property access (line 369) is unsafe. If an ID matches a prototype property like constructor, toString, or __proto__, the in check will incorrectly return true, skipping the mapping, and direct property reads will return inherited values instead of undefined.
Use Object.create(null) and Object.prototype.hasOwnProperty.call() for safe own-property checks.
Proposed fix
-export function buildIdIndex(
- schema: SettingsElement[]
-): Record<string, string> {
- const idIndex: Record<string, string> = {};
+export function buildIdIndex(
+ schema: SettingsElement[]
+): Record<string, string> {
+ const idIndex = Object.create(null) as Record<string, string>;
@@
- if (!(el.id in idIndex)) {
+ if (!Object.prototype.hasOwnProperty.call(idIndex, el.id)) {
idIndex[el.id] = el.dependency_key;
}
@@
- if (currentValue === undefined && idIndex) {
- const resolved = idIndex[dep.key];
- if (resolved !== undefined) {
+ if (
+ currentValue === undefined &&
+ idIndex &&
+ Object.prototype.hasOwnProperty.call(idIndex, dep.key)
+ ) {
+ const resolved = idIndex[dep.key];
+ if (resolved !== undefined) {
currentValue = values[resolved];
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/settings/settings-formatter.ts` around lines 312 - 327, The
idIndex map should not be a plain {} because prototype keys can collide; change
its creation to a prototypeless map (use Object.create(null)) and replace any
"el.id in idIndex" checks and any direct property reads with own-property checks
(e.g., Object.prototype.hasOwnProperty.call(idIndex, el.id) or
Object.hasOwn(idIndex, el.id)) before treating or reading idIndex[el.id]; update
the checks inside the walk function and wherever idIndex is read (referencing
idIndex and the walk / SettingsElement logic) to use these own-property checks
so prototype properties like "toString" or "constructor" won't be treated as
existing IDs.
1 participant
Summary
evaluateDependencies()so dependencies declared with a plain field id ('commission_type') resolve correctly alongside the existing dot-path format ('commission.commission.commission_type').buildIdIndex(schema)helper fromsettings-formatterthat maps each field'sidto its reconstructeddependency_key.SettingsProviderbuilds the index (memoised onschema) and threads it throughshouldDisplay → evaluateDependencies.Why
formatSettingsData()rebuilds every element'sdependency_keyas aparent.child.fielddot-path and uses that as the values-map key. Consumers whose backend declares dependencies with plain field ids (the schema is the source of truth for which field they reference) silently fail to resolve becausevalues[dep.key]returnsundefined.Dokan's settings refactor lands on a flat-storage model where every field id is globally unique; the natural way to reference fields is by id, not by structural path. Path-based keys also break on structural reorganization (moving a field changes its dot-path), while id-based keys remain stable.
Behavior
Resolution order in
evaluateDependencies():values[dep.key]direct lookup (existing behavior).undefinedand anidIndexis supplied,values[idIndex[dep.key]].Backwards Compatibility
evaluateDependencies()'s thirdidIndexparameter is optional. Callers that don't pass it see byte-identical behavior.extractValues(),updateValue, or the values-map shape.Test plan
target field's value).currentValue === undefined, preserving the legacy fail-closed behavior.Summary by CodeRabbit