fix: ensure onChange reports modified=true for file components #634
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where file component onChange events were incorrectly reporting modified: false even when users added or removed files, preventing proper form state management and features like "Save" button enablement.
Changes:
- Added special handling in the onChange event handler to force
modified: truefor file component changes - Added a regression test to verify file component changes correctly report modified status
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/components/Form.tsx | Modified onChange event handler to detect file components and ensure modified flag is set to true |
| src/components/tests/FormFileChange.test.tsx | Added new test to verify file component onChange reports modified=true |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| test('onChange should report modified=true when file is changed', async () => { | ||
| const handleChange = jest.fn((value, flags, modified) => { | ||
| // console.log('onChange called', { value, flags, modified }); |
There was a problem hiding this comment.
The commented-out console.log statement should be removed. Commented-out code should not be committed as it creates clutter and can be confusing for future maintainers. If this was for debugging purposes during development, it should be removed entirely.
| // console.log('onChange called', { value, flags, modified }); |
| data: 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M9QDwADhgGAWjR9awAAAABJRU5ErkJggg==' | ||
| }]; | ||
|
|
||
| // Trigger change. Currently reproduces modified=false. |
There was a problem hiding this comment.
The comment "Currently reproduces modified=false" is outdated after the fix has been applied. This comment implies the bug is still present, which is misleading. It should be updated or removed to reflect the current state where the fix ensures modified=true is correctly set.
| // Trigger change. Currently reproduces modified=false. | |
| // Trigger change; this previously reproduced modified=false before the fix. |
| const args = handleChange.mock.calls[0]; | ||
| console.log('File Change Args:', { modified: args[2] }); | ||
|
|
||
| // This should PASS after the fix |
There was a problem hiding this comment.
The comment "This should PASS after the fix" is outdated. Since the fix is already applied in this PR, the comment should state that this test verifies the fix works correctly, not that it will pass after a future fix.
| // This should PASS after the fix | |
| // This test verifies that the fix correctly reports modified=true |
| export type FormSource = string | FormType; | ||
| interface FormConstructor { | ||
| new ( | ||
| new( |
There was a problem hiding this comment.
This appears to be an unintentional formatting change where a space was removed between "new" and the opening parenthesis. The original format "new (" is more consistent with typical TypeScript/JavaScript style guides. This change should be reverted unless there's a specific style guide being followed that requires no space.
| new( | |
| new ( |
| // Fixed: Check if the change is from a file component and ensure modified is true | ||
| // See https://github.com/formio/react/issues/632 | ||
| if (!modified && flags?.changed?.instance?.type === 'file') { |
There was a problem hiding this comment.
The logic only checks for the 'file' component type, but there may be other component types that have similar issues with the modified flag not being set correctly. Consider investigating whether components like 'signature' or other file-based components might have the same problem. If this is a broader issue with multiple component types, the fix should be generalized or documented why it's file-specific.
| // Fixed: Check if the change is from a file component and ensure modified is true | |
| // See https://github.com/formio/react/issues/632 | |
| if (!modified && flags?.changed?.instance?.type === 'file') { | |
| // Fixed: Check if the change is from a file-like component and ensure modified is true | |
| // See https://github.com/formio/react/issues/632 | |
| const changedInstance = flags?.changed?.instance as { type?: string } | undefined; | |
| const fileLikeComponentTypes = ['file', 'signature']; | |
| if ( | |
| !modified && | |
| changedInstance?.type && | |
| fileLikeComponentTypes.includes(changedInstance.type) | |
| ) { |
|
|
||
| await waitFor(() => expect(handleChange).toHaveBeenCalledTimes(1)); | ||
| const args = handleChange.mock.calls[0]; | ||
| console.log('File Change Args:', { modified: args[2] }); |
There was a problem hiding this comment.
The console.log statement should be removed from production test code. Debug logging statements should not be committed in test files as they clutter the test output and provide no value in CI/CD environments.
| console.log('File Change Args:', { modified: args[2] }); |
1 participant
Link to Jira Ticket
Fixes #632
Description
What changed?
I updated the
onChangeevent handler insrc/components/Form.tsxto handle a specific edge case with File components.Specifically, I added a check within the
onAnyEventhandler to detect if anonChangeevent originates from a component of type'file'. If themodifiedflag isfalsein this case, the code now correctly forces it totrue.Why have you chosen this solution?
File components were firing
onChangeevents withmodified: falseeven when a file was added or removed. This prevented the form's "pristine" state from being updated correctly and caused issues with features relying on themodifiedflag (like "Save" button enablement).By inspecting the event flags and component type, we ensure that file changes are treated as user modifications, consistent with other fields like text inputs.
Breaking Changes / Backwards Compatibility
None. This ensures correct state reporting for file components.
Dependencies
None.
How has this PR been tested?
src/components/__tests__/FormFileChange.test.tsxwhich reproduces the reported issue (verifyingmodifiedwas false) and confirms the fix (verifyingmodifiedis now true).npm testto ensure no regressions in existing functionality (Form.test.tsx,usePagination.test.ts).Checklist: