Add dirtyOnly option to getFieldInput and getInput for filtering dirty fields#96
Add dirtyOnly option to getFieldInput and getInput for filtering dirty fields#96fabian-hiller wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
size:M
WalkthroughThis pull request adds a 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6458822fa3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const child = internalFieldStore.children[key]; | ||
| if (!config?.dirtyOnly || child.isDirty.value) { | ||
| value[key] = getFieldInput(child, config); |
There was a problem hiding this comment.
When dirtyOnly is enabled, recursion is gated by child.isDirty.value, so traversal stops at any clean parent. In this codebase, parent isDirty often stays false for nested edits (for example, updating user.email in a non-null object), which means dirty descendants are dropped and getInput({ dirtyOnly: true }) can return {} even though fields changed. This breaks the documented "submit changed values" use case for common nested object/array updates.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/field/getFieldInput/getFieldInput.ts (1)
75-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
dirtyOnlyis ignored for value-field root calls.With
{ dirtyOnly: true }, this branch still returns clean scalar values. Path-scoped calls to primitive fields can therefore bypass the dirty filter.Proposed fix
// Return primitive value input - return internalFieldStore.input.value; + if (config?.dirtyOnly && !internalFieldStore.isDirty.value) { + return undefined; + } + return internalFieldStore.input.value;🤖 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 `@packages/core/src/field/getFieldInput/getFieldInput.ts` around lines 75 - 76, In getFieldInput, the root primitive return currently ignores the dirtyOnly flag; update the branch that returns internalFieldStore.input.value to first check the passed options.dirtyOnly and the field's dirty flag (e.g., internalFieldStore.meta.dirty or internalFieldStore.input.meta?.dirty) and only return the scalar when dirtyOnly is false OR the field is marked dirty; if dirtyOnly is true and the field is not dirty, return the "no value" sentinel used by getFieldInput (e.g., undefined/null) so path-scoped callers cannot bypass the dirty filter.
🤖 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.
Outside diff comments:
In `@packages/core/src/field/getFieldInput/getFieldInput.ts`:
- Around line 75-76: In getFieldInput, the root primitive return currently
ignores the dirtyOnly flag; update the branch that returns
internalFieldStore.input.value to first check the passed options.dirtyOnly and
the field's dirty flag (e.g., internalFieldStore.meta.dirty or
internalFieldStore.input.meta?.dirty) and only return the scalar when dirtyOnly
is false OR the field is marked dirty; if dirtyOnly is true and the field is not
dirty, return the "no value" sentinel used by getFieldInput (e.g.,
undefined/null) so path-scoped callers cannot bypass the dirty filter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78e68efe-98b4-4ca9-8274-0e73c9722dce
📒 Files selected for processing (11)
packages/core/CHANGELOG.mdpackages/core/src/field/getFieldInput/getFieldInput.test.tspackages/core/src/field/getFieldInput/getFieldInput.tspackages/methods/CHANGELOG.mdpackages/methods/src/getInput/getInput.test.tspackages/methods/src/getInput/getInput.tswebsite/src/routes/(docs)/methods/api/(methods)/getInput/index.mdxwebsite/src/routes/(docs)/methods/api/(types)/GetFieldInputConfig/index.mdxwebsite/src/routes/(docs)/methods/api/(types)/GetFieldInputConfig/properties.tswebsite/src/routes/(docs)/methods/api/(types)/GetFormInputConfig/index.mdxwebsite/src/routes/(docs)/methods/api/(types)/GetFormInputConfig/properties.ts
There was a problem hiding this comment.
Pull request overview
Adds a dirtyOnly option to input retrieval APIs so consumers can request only dirty values (based on isDirty) from getInput / getFieldInput, with accompanying docs, tests, and changelog entries.
Changes:
- Add
dirtyOnlytoGetFormInputConfig/GetFieldInputConfigand pass it throughgetInputto coregetFieldInput. - Extend core
getFieldInputwith a config parameter (GetFieldInputOptions) supporting dirty filtering. - Update docs + add tests + changelog entries for the new option.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| website/src/routes/(docs)/methods/api/(types)/GetFormInputConfig/properties.ts | Documents the new dirtyOnly config property for form-level input retrieval. |
| website/src/routes/(docs)/methods/api/(types)/GetFormInputConfig/index.mdx | Adds dirtyOnly to the rendered API docs list for GetFormInputConfig. |
| website/src/routes/(docs)/methods/api/(types)/GetFieldInputConfig/properties.ts | Documents the new dirtyOnly config property for field-level input retrieval. |
| website/src/routes/(docs)/methods/api/(types)/GetFieldInputConfig/index.mdx | Adds dirtyOnly to the rendered API docs list for GetFieldInputConfig. |
| website/src/routes/(docs)/methods/api/(methods)/getInput/index.mdx | Documents how to use dirtyOnly when calling getInput. |
| packages/methods/src/getInput/getInput.ts | Adds dirtyOnly to the public config types and forwards it to core getFieldInput. |
| packages/methods/src/getInput/getInput.test.ts | Adds test coverage for getInput(..., { dirtyOnly: true }). |
| packages/methods/CHANGELOG.md | Notes the new dirtyOnly option for getInput. |
| packages/core/src/field/getFieldInput/getFieldInput.ts | Adds GetFieldInputOptions and implements dirtyOnly filtering during traversal. |
| packages/core/src/field/getFieldInput/getFieldInput.test.ts | Adds tests for getFieldInput(..., { dirtyOnly: true }). |
| packages/core/CHANGELOG.md | Notes the new GetFieldInputOptions / config param for getFieldInput. |
Comments suppressed due to low confidence (1)
packages/core/src/field/getFieldInput/getFieldInput.ts:66
- With
dirtyOnly, this loop prunes recursion based onchild.isDirty.value. For object/array field stores,isDirtydoesn’t necessarily reflect dirty descendants (e.g.setFieldInputmarks ancestorinputtruthy but doesn’t recompute ancestorisDirty). This can cause nested dirty leaves to be omitted from form-level results. Consider always descending into object/array children and only including the subtree when it contains at least one dirty leaf (or usegetFieldBool(child, 'isDirty')as the gate).
// Collect input from each object property
for (const key in internalFieldStore.children) {
const child = internalFieldStore.children[key];
if (!config?.dirtyOnly || child.isDirty.value) {
value[key] = getFieldInput(child, config);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (internalFieldStore.input.value) { | ||
| // Create output array | ||
| const value = []; | ||
|
|
||
| // Collect input from each array item | ||
| for ( | ||
| let index = 0; | ||
| index < internalFieldStore.items.value.length; | ||
| index++ | ||
| ) { | ||
| value[index] = getFieldInput(internalFieldStore.children[index]); | ||
| const child = internalFieldStore.children[index]; | ||
| if (!config?.dirtyOnly || child.isDirty.value) { | ||
| value[index] = getFieldInput(child, config); | ||
| } | ||
| } | ||
| return value; |
| test('should miss dirty leaves under a clean object parent', () => { | ||
| const store = createTestStore( | ||
| v.object({ user: v.object({ email: v.string(), name: v.string() }) }), | ||
| { initialInput: { user: { email: 'a@x.com', name: 'John' } } } | ||
| ); | ||
| const user = store.children.user; | ||
| if (user.kind !== 'object') throw new Error('expected object'); | ||
| user.children.email.input.value = 'b@x.com'; | ||
| user.children.email.isDirty.value = true; | ||
| expect(getFieldInput(store, { dirtyOnly: true })).toStrictEqual({}); | ||
| }); |
| * The path to a field. Leave undefined to get the entire form input. | ||
| */ | ||
| readonly path?: undefined; | ||
| /** | ||
| * Whether to include only fields whose `isDirty` flag is set. Useful for | ||
| * submitting only the values that changed since the start input. | ||
| */ | ||
| readonly dirtyOnly?: boolean; | ||
| } |
| ### Explanation | ||
|
|
||
| The `form` parameter is the form store to retrieve input from. The optional `config` parameter specifies which field to get input from - if omitted, returns input from the entire form. | ||
| The `form` parameter is the form store to retrieve input from. The optional `config` parameter specifies which field to get input from - if omitted, returns input from the entire form. Set `dirtyOnly` to `true` to include only fields whose `isDirty` flag is `true`, which is useful for submitting only the values that changed since the start input. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/field/getFieldInput/getFieldInput.ts">
<violation number="1" location="packages/core/src/field/getFieldInput/getFieldInput.ts:43">
P1: `dirtyOnly` filtering can hide nested dirty fields because recursion is gated by direct child `isDirty` only. Dirty leaf updates under a clean parent are omitted from the returned input.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/field/getFieldInput/getFieldInput.ts (1)
49-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
dirtyOnlyis not applied to array children.Line 57 calls
getFieldInputwithoutconfig, so indirtyOnlymode clean indices are returned with their current values instead ofundefined. That breaks dirty-only array filtering semantics.Suggested fix
- // Collect input from each array item. Once we enter an array, every - // item is fully populated regardless of dirty state — arrays are - // atomic in `dirtyOnly` mode. + // Collect input from each array item. In `dirtyOnly` mode, clean + // indices resolve to `undefined`, preserving positional semantics. for ( let index = 0; index < internalFieldStore.items.value.length; index++ ) { - value[index] = getFieldInput(internalFieldStore.children[index]); + value[index] = getFieldInput(internalFieldStore.children[index], config); }🤖 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 `@packages/core/src/field/getFieldInput/getFieldInput.ts` around lines 49 - 58, When collecting array item inputs inside getFieldInput, the recursive call uses getFieldInput(internalFieldStore.children[index]) without passing the current config, so dirtyOnly semantics are lost; fix by passing the same config (including dirtyOnly) into the recursive call (i.e., call getFieldInput(internalFieldStore.children[index], config)) so clean indices return undefined when dirtyOnly is enabled and array children respect the parent's dirtyOnly behavior.
🤖 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.
Outside diff comments:
In `@packages/core/src/field/getFieldInput/getFieldInput.ts`:
- Around line 49-58: When collecting array item inputs inside getFieldInput, the
recursive call uses getFieldInput(internalFieldStore.children[index]) without
passing the current config, so dirtyOnly semantics are lost; fix by passing the
same config (including dirtyOnly) into the recursive call (i.e., call
getFieldInput(internalFieldStore.children[index], config)) so clean indices
return undefined when dirtyOnly is enabled and array children respect the
parent's dirtyOnly behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00473e7e-cb1e-46bc-a7d7-e711db5c66dc
📒 Files selected for processing (5)
packages/core/src/field/getFieldInput/getFieldInput.test.tspackages/core/src/field/getFieldInput/getFieldInput.tspackages/methods/src/getInput/getInput.test.tspackages/methods/src/getInput/getInput.tswebsite/src/routes/(docs)/methods/api/(methods)/getInput/index.mdx
✅ Files skipped from review due to trivial changes (1)
- website/src/routes/(docs)/methods/api/(methods)/getInput/index.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/methods/src/getInput/getInput.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d8e53e92b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return getFieldInput( | ||
| config?.path ? getFieldStore(form[INTERNAL], config.path) : form[INTERNAL] | ||
| config?.path ? getFieldStore(form[INTERNAL], config.path) : form[INTERNAL], | ||
| config?.dirtyOnly ? { dirtyOnly: true } : undefined |
There was a problem hiding this comment.
Enabling dirtyOnly now forwards { dirtyOnly: true } to getFieldInput, which returns undefined for clean forms/subtrees, but the public overloads still promise PartialValues<...> and do not include undefined. This makes TypeScript callers treat the result as always defined and can cause runtime errors on common code paths (for example, iterating keys of a clean result) when dirtyOnly is used.
Useful? React with 👍 / 👎.
Fix #21
Summary by cubic
Add a dirtyOnly option to
getInputandgetFieldInputto return only changed fields and reduce payloads. Arrays are now treated as atomic: if any item is dirty, the full array is returned.getFieldInput: acceptsconfigwith{ dirtyOnly }. Whentrue, clean subtrees returnundefined; objects include only dirty keys; arrays return the full current array when any descendant is dirty; includes explicitundefinedwhen a dirty value was cleared.getInput: acceptsdirtyOnlyin both config variants, forwards it togetFieldInput, and supports scoping viapath.dirtyOnlysemantics, including scoped paths, arrays-as-atomic, and nullish/undefined cases.Written for commit 0d8e53e. Summary will update on new commits. Review in cubic