feat(dashboard): Send only changed fields on detail page updates#4916
Conversation
The form engine (useGeneratedForm) submitted the entire mapped payload on every save, so replace-semantics fields (facetValueIds, assetIds, scalar stock config, etc.) resent their stale page-load value and could silently overwrite a concurrent edit made by another admin or via the API. Compute the set of top-level fields the user actually changed by deep-comparing the submitted values against the form baseline (plus any non-nullable input field, which is always sent), and prune update-mutation payloads to those fields in useDetailPage. RHF dirtyFields is not used as the source (array-item removal reads as clean; setValue-driven fields have no Controller). Update inputs are patch-style, so omitting untouched fields leaves them unchanged. Adds a sendAllFieldsOnUpdate opt-out for update mutations that need the full payload. Relates to OSS-567
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds ChangesSend only changed fields on update
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant GeneratedForm as useGeneratedForm
participant Utils as getChangedTopLevelFields
participant DetailPage as useDetailPage
participant API as GraphQL API
User->>GeneratedForm: edit single field, submit
GeneratedForm->>Utils: diff rawValues vs baseline
Utils-->>GeneratedForm: changedFields set
GeneratedForm->>DetailPage: onSubmit(processed, meta)
DetailPage->>DetailPage: prune filteredValues to changedFields
DetailPage->>API: send Update mutation (id + changed fields only)
API-->>DetailPage: success response
DetailPage-->>User: show updated toast
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Adds an e2e payload assertion on the collection detail page (configurable- operation filters array, no update transform) confirming that editing only the name submits just id + translations and omits the filters replace-array. Relates to OSS-567
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/dashboard/e2e/tests/catalog/product-variants.spec.ts (1)
380-384: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment format deviates from required
#ISSUE_NUMBERconvention.Uses
OSS-567instead of#ISSUE_NUMBER, inconsistent with the#4478/#4608style already used elsewhere in this same file. As per path instructions, comments above Dashboard E2E tests should be formatted//#ISSUE_NUMBER— description.🤖 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/dashboard/e2e/tests/catalog/product-variants.spec.ts` around lines 380 - 384, The inline comment above the variant update test uses the wrong issue tag format, so update the leading marker in the catalog product-variants E2E test to match the required `#ISSUE_NUMBER` convention used elsewhere in this file. Locate the comment near the variant update describe block and change the OSS-567 style prefix to the Dashboard test comment format while keeping the descriptive text intact.Source: Path instructions
packages/dashboard/e2e/tests/catalog/products.spec.ts (1)
124-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment format deviates from required
#ISSUE_NUMBERconvention.The comment uses
OSS-567instead of#ISSUE_NUMBER. As per path instructions, Dashboard E2E test comments should be formatted as//#ISSUE_NUMBER— description.🤖 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/dashboard/e2e/tests/catalog/products.spec.ts` around lines 124 - 128, The test comment in products.spec.ts uses the wrong issue tag format; update the leading identifier in the describe block comment from OSS-567 to the required `#ISSUE_NUMBER` convention. Locate the comment above test.describe('product update sends only changed fields (OSS-567)') and change only the issue reference so it matches the Dashboard E2E format while keeping the rest of the description unchanged.Source: Path instructions
packages/dashboard/e2e/tests/catalog/collections.spec.ts (1)
370-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment format deviates from required
#ISSUE_NUMBERconvention.As per path instructions, comments above Dashboard E2E tests should be formatted
//#ISSUE_NUMBER— description; this usesOSS-567without the#prefix.🤖 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/dashboard/e2e/tests/catalog/collections.spec.ts` around lines 370 - 374, The test description comment in collections.spec.ts uses the wrong issue-tag format; update the OSS-567 marker to the required Dashboard E2E convention with a leading #. Keep the surrounding explanatory text the same, and ensure the comment above the test.describe block follows the `// `#ISSUE_NUMBER` — description` style consistently.Source: Path instructions
🤖 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.
Nitpick comments:
In `@packages/dashboard/e2e/tests/catalog/collections.spec.ts`:
- Around line 370-374: The test description comment in collections.spec.ts uses
the wrong issue-tag format; update the OSS-567 marker to the required Dashboard
E2E convention with a leading #. Keep the surrounding explanatory text the same,
and ensure the comment above the test.describe block follows the `//
`#ISSUE_NUMBER` — description` style consistently.
In `@packages/dashboard/e2e/tests/catalog/product-variants.spec.ts`:
- Around line 380-384: The inline comment above the variant update test uses the
wrong issue tag format, so update the leading marker in the catalog
product-variants E2E test to match the required `#ISSUE_NUMBER` convention used
elsewhere in this file. Locate the comment near the variant update describe
block and change the OSS-567 style prefix to the Dashboard test comment format
while keeping the descriptive text intact.
In `@packages/dashboard/e2e/tests/catalog/products.spec.ts`:
- Around line 124-128: The test comment in products.spec.ts uses the wrong issue
tag format; update the leading identifier in the describe block comment from
OSS-567 to the required `#ISSUE_NUMBER` convention. Locate the comment above
test.describe('product update sends only changed fields (OSS-567)') and change
only the issue reference so it matches the Dashboard E2E format while keeping
the rest of the description unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4879d6c-3cb9-4567-b790-b322df1524fe
📒 Files selected for processing (7)
packages/dashboard/e2e/tests/catalog/collections.spec.tspackages/dashboard/e2e/tests/catalog/product-variants.spec.tspackages/dashboard/e2e/tests/catalog/products.spec.tspackages/dashboard/src/lib/framework/form-engine/use-generated-form.tsxpackages/dashboard/src/lib/framework/form-engine/utils.spec.tspackages/dashboard/src/lib/framework/form-engine/utils.tspackages/dashboard/src/lib/framework/page/use-detail-page.ts
…hboard-form-engine-resubmits-full-payload-allowing
… data Relates to OSS-567
CI fix:
|
1 participant
Summary
The dashboard detail-page form engine now submits only the fields the user actually changed on update, instead of resending the entire form payload. This closes a silent data-loss vector: previously, a field left untouched by admin A would be resubmitted at its stale page-load value and could overwrite a concurrent change made by admin B (or an API integration) — no error, success toast.
Design was signed off by @michaelbromley on OSS-567 (targeting
minor). This is Tier 1 of his framing (don't send untouched fields); Tier 2 (touched replace-set element conflicts, beyond the merged stock fix) and Tier 3 (optimistic concurrency) remain separate.Root cause
useGeneratedFormmapped the whole entity into the form and calledonSubmitwith the entire payload on every save;useDetailPageforwarded it to the update mutation. For replace-semantics fields (facetValueIds,assetIds, scalar stock config,channelIds, prices, promotion conditions/actions, …) the stale value replaced whatever was there.Change
form-engine/utils.ts—deepEqual(primitives incl. NaN, Date, arrays, plain objects) andgetChangedTopLevelFields(submitted, baseline, fields): the top-level keys that differ from the baseline, plus any input field that is non-nullable (FieldInfo.nullable === false, e.g.id,UpdateShippingMethodInput.translations) which is always sent.useGeneratedForm— computeschangedFieldsby deep-comparing the raw submitted values against the form baseline (thevaluesmemo) and passes it via a new, backward-compatible 2ndmetaarg toonSubmit. RHFdirtyFieldsis deliberately not used (array-item removal reads as clean → would drop the user's own deletion;assetIds/featuredAssetIdare written viasetValuewith no Controller).useDetailPage— on the update path only, prunes the payload tochangedFieldsbeforetransformUpdateInput. Create path is unchanged. Adds asendAllFieldsOnUpdate?: booleanopt-out. Composes with the existinggetChangedStockLevelstransform (fix(dashboard): only send edited stock levels on variant update #4834).transformUpdateInput's signature (avoids the generic-inference hazard from the OSS-554 fix).Whole-key granularity: a changed top-level key is sent in full (the entire
translationsarray,facetValueIds,customFieldsobject), preserving the "full translation objects per language" invariant.Behaviour change / extensions
Update mutations move from full-snapshot to patch-of-changed-fields for all detail pages, including third-party dashboard extensions built on
useDetailPage. Core update inputs are patch-style by design, so this is safe for core. Extensions whose custom update mutation relies on receiving unchanged fields can setsendAllFieldsOnUpdate: true.Test plan
Unit (
form-engine/utils.spec.ts, 48 pass): deepEqual (NaN, Date, nested arrays/objects); getChangedTopLevelFields — changed scalar kept, untouched pruned, array add/remove, key deletion, nested translation/customFields, non-nullable always kept, undefined-baseline↔value transitions, missing non-nullable field doesn't throw.E2E (payload assertions, the strongest proof):
catalog/product-variants.spec.ts— edit only the SKU → update input is exactly{ id, sku }(facetValueIds/assetIds/trackInventory/translations/price omitted). 11/11.catalog/products.spec.ts— edit only the name → update input is exactly{ id, translations }. 16/16.Typecheck clean; existing catalog suites green (no regression).
Relates to OSS-567.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
Tests
Chores