Skip to content

fix: ensure onChange reports modified=true for file components #634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
prem22k wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: ensure onChange reports modified=true for file components #634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/components/Form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export type FormOptions = FormClass['options'] & {
export type FormType = PartialExcept<CoreFormType, 'components'>;
export type FormSource = string | FormType;
interface FormConstructor {
new (
new(
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
new(
new (

Copilot uses AI. Check for mistakes.
element: HTMLElement,
formSource: FormSource,
options: FormOptions,
Expand Down Expand Up @@ -136,12 +136,20 @@ const onAnyEvent = (
handlers.onCancelComponent(outputArgs[0]);
break;
case 'onChange':
if (handlers.onChange)
if (handlers.onChange) {
let modified = outputArgs[2];
const flags = outputArgs[1];
// 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') {
Comment on lines +142 to +144
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
// 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)
) {

Copilot uses AI. Check for mistakes.
modified = true;
}
handlers.onChange(
outputArgs[0],
outputArgs[1],
outputArgs[2],
modified,
);
}
break;
case 'onCustomEvent':
if (handlers.onCustomEvent)
Expand Down
49 changes: 49 additions & 0 deletions src/components/__tests__/FormFileChange.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@

import { render, screen, waitFor } from '@testing-library/react';
import '@testing-library/jest-dom';
import { Form } from '../Form';

const fileForm = {
display: 'form' as const,
components: [
{
label: 'Upload File',
key: 'file',
type: 'file',
input: true,
storage: 'base64',
},
],
};

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 });
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
// console.log('onChange called', { value, flags, modified });

Copilot uses AI. Check for mistakes.
});

let formInstance: any;
render(<Form src={fileForm} onChange={handleChange} onFormReady={(instance) => formInstance = instance} />);

await waitFor(() => {
expect(screen.getByText('Upload File')).toBeInTheDocument();
expect(formInstance).toBeDefined();
});

const fileComponent = formInstance.getComponent('file');
const fileValue = [{
name: 'test.png',
size: 100,
type: 'image/png',
data: 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M9QDwADhgGAWjR9awAAAABJRU5ErkJggg=='
}];

// Trigger change. Currently reproduces modified=false.
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
// Trigger change. Currently reproduces modified=false.
// Trigger change; this previously reproduced modified=false before the fix.

Copilot uses AI. Check for mistakes.
fileComponent.setValue(fileValue);

await waitFor(() => expect(handleChange).toHaveBeenCalledTimes(1));
const args = handleChange.mock.calls[0];
console.log('File Change Args:', { modified: args[2] });
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
console.log('File Change Args:', { modified: args[2] });

Copilot uses AI. Check for mistakes.

// This should PASS after the fix
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
// This should PASS after the fix
// This test verifies that the fix correctly reports modified=true

Copilot uses AI. Check for mistakes.
expect(args[2]).toBe(true);
}, 10000);
Loading