Skip to content

fix: presentation material form reset - #1019

Open
santipalenque wants to merge 2 commits into
masterfrom
fix/presentation-material-form-reset
Open

fix: presentation material form reset#1019
santipalenque wants to merge 2 commits into
masterfrom
fix/presentation-material-form-reset

Conversation

@santipalenque

@santipalenque santipalenque commented Jul 22, 2026

Copy link
Copy Markdown

https://app.clickup.com/t/9014802374/86bb18pbh

Summary by CodeRabbit

  • Bug Fixes
    • Improved event material form behavior when existing data or validation errors change.
    • Fixed stale form values when switching from an existing material to a new one.
    • Improved file and media upload handling, including clearing removed files and updating uploaded file details.
    • Improved validation error display and automatic scrolling to errors.
    • Refined conditional fields and input states based on the selected material type.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a5ccc258-0369-4d36-9483-1e59260a304c

📥 Commits

Reviewing files that changed from the base of the PR and between 6520aa5 and 2d1ac8e.

📒 Files selected for processing (2)
  • src/components/forms/__tests__/event-material-form.test.js
  • src/components/forms/event-material-form.js

📝 Walkthrough

Walkthrough

EventMaterialForm was converted to a functional React component using hooks. State synchronization, form handlers, submission, conditional rendering, media uploads, and regression tests were updated.

Changes

Event material form

Layer / File(s) Summary
Hook state and prop synchronization
src/components/forms/event-material-form.js
React hooks replace class state wiring. Effects synchronize entity and validation errors. The form derives optional media configuration.
Form behavior and media rendering
src/components/forms/event-material-form.js, src/components/forms/__tests__/event-material-form.test.js
Handlers update fields and upload metadata, file removal clears metadata, submission passes entityState, and tests verify stale values are cleared when the entity changes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested reviewers: smarcet

🚥 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 clearly and concisely describes the form reset fix, which is the main change in the pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/presentation-material-form-reset

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/forms/event-material-form.js`:
- Around line 56-59: Update the mediaUploadsOpts mapping in the event material
form to safely handle an undefined event.type during initial render and
handleChangeMUType. Use the existing event data when available and fall back to
an empty list before calling map.
🪄 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: e7020f9e-94a8-4f9a-a171-d874adb218dc

📥 Commits

Reviewing files that changed from the base of the PR and between 53b15bf and 6520aa5.

📒 Files selected for processing (1)
  • src/components/forms/event-material-form.js

Comment thread src/components/forms/event-material-form.js Outdated
if (!shallowEqual(prevProps.errors, this.props.errors)) {
state.errors = { ...this.props.errors };
}
useEffect(() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@santipalenque This PR rewrites the component's entire state-reset mechanism (class componentDidUpdate/shallowEqualuseEffect([entity])/useEffect([errors])) to fix the "adding new material shows previous description" bug, but ships with no test exercising that reset path — event-material-form.js has never had its own test file, and edit-event-material-page.test.js fully mocks EventMaterialForm, so it can't catch a regression here.

Without a reproducing test, if this reset logic is touched again later, the exact bug this PR fixes (stale entity/description carrying over when switching from an existing material to "add new") can silently reappear with nothing in the suite to flag it.

Suggested fix: add src/components/forms/__tests__/event-material-form.test.js covering the reset case — render with a populated entity (non-empty description), then rerender with a fresh/default-shaped entity prop (as happens on navigating to "add new"), and assert the description/name fields no longer show the old value.

@smarcet smarcet left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@santipalenque please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants