Repository navigation
fix(app-admin): keep rejected-save errors in step with the form being saved - #5879
Conversation
… 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)
|
🚓 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. |
|
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change updates FormModel to replace errors from earlier ChangesContent-entry save handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue is established for this change; it is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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 thenextside of #5878, which fixes the same thing onrelease/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. SosetErrors([])reportedisValid: truewhileskustill 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.FormModelnow remembers which fieldssetErrors()marked, and resets them before applying the next set.validate(),setData()andreset()forget them, since they rebuild validation anyway.Saves wrote back to whatever form was current.
ContentEntryFormPresenter.saveRevision()andSingleEntryPresenter.save()readthis.formafter the request returned. If the form was reset, disposed or replaced by another entry in the meantime:this.formwasnull) instead of returningfalse, or put its errors on the new formsetData()on the new form, replacing what was loaded thereBoth 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 newsetErrorscases, 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 afterdispose()returnsfalse, a rejected save afterinit()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, usingreset()andnewEntry()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 buildpasses onnext.next's previousFormModeland presenters back in place, all 7 new race andsetErrorstests fail.Workspace: wby-next2 · 🧹 MERGE 6.5.0 => NEXT
🤖 Generated with Claude Code
Summary by CodeRabbit