Skip to content

fix(app-admin): keep rejected-save errors in step with the form being saved - #5879

Merged
adrians5j merged 1 commit into
nextfrom
claude/fix-form-rejected-save-errors-next
Oct 2, 2026
Merged

adrians5j merged 1 commit into
nextfrom
claude/fix-form-rejected-save-errors-next

Conversation

@adrians5j

@adrians5j adrians5j commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

What

Fixes two bugs in how a rejected save reports its errors in the entry form. Both came in with #5794, and CodeRabbit found them while reviewing the port to next (#5865). This is the next side of #5878, which fixes the same thing on release/6.5.0. It's the same commit, cherry-picked cleanly, so the two branches stay identical.

FormModel.setErrors() didn't clear the previous set. It marked fields invalid but never reset the fields it had marked on the previous call. So setErrors([]) reported isValid: true while sku still showed "Value must be unique.", and a second rejected save kept the field errors of the first one. saveRevision() skips validation by default, so nothing else cleared them either. FormModel now remembers which fields setErrors() marked, and resets them before applying the next set. validate(), setData() and reset() forget them, since they rebuild validation anyway.

Saves wrote back to whatever form was current. ContentEntryFormPresenter.saveRevision() and SingleEntryPresenter.save() read this.form after the request returned. If the form was reset, disposed or replaced by another entry in the meantime:

  • a rejected save threw (this.form was null) instead of returning false, or put its errors on the new form
  • a successful save called setData() on the new form, replacing what was loaded there

Both now keep hold of the form and model being saved, and only write back while that form is still the current one.

Tests

  • FormModel.test.ts: two new setErrors cases, one for an empty set after an error and one for a second rejected save with a different field. Both fail without the fix.
  • SingleEntryPresenter.test.ts (new): a rejected save puts its errors on the form, a save that finishes after dispose() returns false, a rejected save after init() rebuilt the form leaves both forms alone, and a successful save doesn't overwrite a rebuilt form. Against the previous presenter, the last three fail.
  • ContentEntryFormPresenter.test.ts (new): the same checks for the create path, using reset() and newEntry() mid-save. Against the previous presenter, the two race cases fail. It runs under @vitest-environment jsdom, because the presenter imports a dialog id from the list presenter, which pulls in DOM code.

Checks

  • yarn build passes on next.
  • Tests pass for app-admin (522) and app-headless-cms (74). With next's previous FormModel and presenters back in place, all 7 new race and setErrors tests fail.
  • adio, sync-dependencies, oxfmt and oxlint are clean.

Workspace: wby-next2 · 🧹 MERGE 6.5.0 => NEXT

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Form errors are cleared or replaced when new errors are applied, keeping the form’s validity state accurate.
    • Saves that finish after a form is reset, disposed, or replaced no longer update the wrong form or apply stale errors.
    • Failed saves resolve as unsuccessful and apply errors only to the form that initiated the save.

… saved

Two bugs in how a rejected save reports its errors (#5794), raised by
CodeRabbit on #5865.

FormModel.setErrors() marked fields invalid but never cleared the ones it
marked on the previous call. setErrors([]) then reported the form valid while
a field still showed its old error, and a second rejected save kept the
errors of the first. It now remembers the fields it marked and resets them
before applying a new set. validate(), setData() and reset() forget them,
since they rebuild validation anyway.

ContentEntryFormPresenter.saveRevision() and SingleEntryPresenter.save() read
this.form after the save request returned. If the form had been reset,
disposed or replaced by another entry in the meantime, a rejected save threw
instead of returning false, or put its errors on the new form, and a
successful one overwrote the new form's data. Both now keep hold of the form
being saved and only write back to it while it is still the current one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 05ec1d1)
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

🚓 Slop Cop

✅ Nothing worth flagging. The diff looks consistent with the PR's stated intent and the code-style rules.

Focused, well-tested bugfix PR matching its stated intent; no integrity or style issues found.

Automated, non-blocking heads-up from an LLM. It can be wrong — use your judgment. Regenerates on every push.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: bec11cf8-4ca0-4c06-bd99-3453793c0c60

📥 Commits

Reviewing files that changed from the base of the PR and between a140487 and 42d8745.

📒 Files selected for processing (6)
  • packages/app-admin/src/features/formModel/FormModel.test.ts
  • packages/app-admin/src/features/formModel/FormModel.ts
  • packages/app-headless-cms/src/presentation/contentEntries/form/ContentEntryFormPresenter.test.ts
  • packages/app-headless-cms/src/presentation/contentEntries/form/ContentEntryFormPresenter.ts
  • packages/app-headless-cms/src/presentation/contentEntries/singleEntry/SingleEntryPresenter.test.ts
  • packages/app-headless-cms/src/presentation/contentEntries/singleEntry/SingleEntryPresenter.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The change updates FormModel to replace errors from earlier setErrors calls and changes content-entry save presenters to apply asynchronous results only to the form captured when saving began. Tests cover error replacement, rejected saves, and form changes during pending saves.

Changes

Content-entry save handling

Layer / File(s) Summary
Form error replacement
packages/app-admin/src/features/formModel/FormModel.ts, packages/app-admin/src/features/formModel/FormModel.test.ts
FormModel tracks fields marked by setErrors. A later call clears those field errors before applying new errors. setData, reset, and validate clear the tracking state. Tests cover clearing all errors and replacing an earlier field error.
Revision save form guard
packages/app-headless-cms/src/presentation/contentEntries/form/ContentEntryFormPresenter.ts, packages/app-headless-cms/src/presentation/contentEntries/form/ContentEntryFormPresenter.test.ts
saveRevision captures the form and model before submission. It skips entry updates and error application if another form becomes active. Tests cover rejected saves, reset, and replacement forms.
Single-entry save form guard
packages/app-headless-cms/src/presentation/contentEntries/singleEntry/SingleEntryPresenter.ts, packages/app-headless-cms/src/presentation/contentEntries/singleEntry/SingleEntryPresenter.test.ts
save captures the form and model before submission. It skips updates to a replacement form after completion. Tests cover rejected saves, disposal, and reinitialization during a pending save.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested reviewers: pavel910

Merge Risk: ⚪ Minimal · up to 42d87

No actionable issue is established for this change; it is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 42d87

The changes improve isolation between a pending save and a replacement form. No introduced security issue was established. The protection applies to displaying save results, not to cancelling requests or isolating overlapping saves.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated change affects client-side validation state and result application for content-entry saves. The inspected request calls retain their previous model and entry selection behavior; no additional tenant, service, credential, or environment authority is established by these changes. Downstream authorization coverage remains unknown.

Trust Boundaries and Controls

  • inferred — The normal content-entry route keys the view by model ID and creates a child container containing the model context and presenters. The loader assigns the model before rendering its children. This limits ordinary navigation-based model substitution during an old presenter's save, but does not prove that every external context mutation is excluded.

Resilience and Maintainability Implications

  • observed — Active-form identity checks contain successful and rejected completion effects after form replacement. They do not provide transactional rollback of a successful save or prevent a request from proceeding after the submitting form becomes inactive.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: keeping rejected-save errors aligned with the form involved in the save. It is concise, specific, and related to the changes across the form model and …
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 6…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@adrians5j
adrians5j merged commit aee2044 into next Oct 2, 2026
20 checks passed
@adrians5j
adrians5j deleted the claude/fix-form-rejected-save-errors-next branch October 2, 2026 09:19
@adrians5j adrians5j added the port: 6.5.0 → next Brings release/6.5.0 work into next label Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

port: 6.5.0 → next Brings release/6.5.0 work into next

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant