Skip to content

fix(poll): preserve cleared option identity without collapsing empty rows #1748

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
MartinCupela wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(poll): preserve cleared option identity without collapsing empty rows #1748

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
23 changes: 22 additions & 1 deletion src/messageComposer/middleware/pollComposer/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,27 @@ export const pollCompositionStateProcessors: Partial<
// For single option updates
const { index, text } = value;
const prevOptions = data.options || [];
const targetOption = prevOptions[index];

if (!targetOption) {
return { options: prevOptions };
}

const nextOptionsAfterTarget = prevOptions.slice(index + 1);
const shouldPreserveClearedOption =
!text &&
nextOptionsAfterTarget.length > 0 &&
nextOptionsAfterTarget.every((option) => !option.text);
Copy link
Copy Markdown
Contributor

@szuperaz szuperaz May 15, 2026

Choose a reason for hiding this comment

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

I'm just wondering if we need this condition here - to me it seems that if I have 5 options, clearing the text on the 3rd one, that option will still be removed. UX-wise it seems more logical to me to only delete this option when the remove button is clicked, and only automatically remove if the last option's text was cleared. In any case, the PR looks good to me, so feel free to dismiss the suggestion if it doesn't seem reasonable to you.


if (shouldPreserveClearedOption) {
return {
options: [
...prevOptions.slice(0, index),
{ ...targetOption, text },
...nextOptionsAfterTarget,
],
};
}

const shouldRemoveOption =
prevOptions && prevOptions.slice(index + 1).length > 0 && !text;
Expand All @@ -146,7 +167,7 @@ export const pollCompositionStateProcessors: Partial<

const newOptions = [
...optionListHead,
...(shouldRemoveOption ? [] : [{ ...prevOptions[index], text }]),
...(shouldRemoveOption ? [] : [{ ...targetOption, text }]),
...optionListTail,
];

Expand Down
71 changes: 66 additions & 5 deletions test/unit/MessageComposer/middleware/pollComposer/state.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,9 +369,8 @@ describe('PollComposerStateMiddleware', () => {
expect(result.status).toBeUndefined;
});

it('should remove an option when it is empty and there are more options after it', async () => {
it('should preserve the cleared option id while keeping following empty options', async () => {
const stateMiddleware = setup();
// Set up initial state with two options
const initialState = getInitialState();
initialState.data.options = [
{ id: 'option-1', text: 'Option 1' },
Expand All @@ -380,8 +379,8 @@ describe('PollComposerStateMiddleware', () => {

const result = await stateMiddleware.handlers.handleFieldChange(
setupHandlerParams({
nextState: { ...getInitialState() },
previousState: { ...getInitialState() },
nextState: { ...initialState },
previousState: { ...initialState },
targetFields: {
options: {
index: 0,
Expand All @@ -391,8 +390,70 @@ describe('PollComposerStateMiddleware', () => {
}),
);

expect(result.state.nextState.data.options.length).toBe(1);
expect(result.state.nextState.data.options.length).toBe(2);
expect(result.state.nextState.data.options[0].id).toBe('option-1');
expect(result.state.nextState.data.options[0].text).toBe('');
expect(result.state.nextState.data.options[1]).toEqual({
id: 'option-2',
text: '',
});
expect(result.status).toBeUndefined;
});

it('should not preserve the option when clearing it if a following option has text', async () => {
const stateMiddleware = setup();
const initialState = getInitialState();
initialState.data.options = [
{ id: 'option-1', text: 'Option 1' },
{ id: 'option-2', text: 'Option 2' },
{ id: 'option-3', text: '' },
];

const result = await stateMiddleware.handlers.handleFieldChange(
setupHandlerParams({
nextState: { ...initialState },
previousState: { ...initialState },
targetFields: {
options: {
index: 0,
text: '',
},
},
}),
);

expect(result.state.nextState.data.options).toEqual([
{ id: 'option-2', text: 'Option 2' },
{ id: 'option-3', text: '' },
]);
expect(result.status).toBeUndefined;
});

it('should not preserve trailing empty options for whitespace-only option text', async () => {
const stateMiddleware = setup();
const initialState = getInitialState();
initialState.data.options = [
{ id: 'option-1', text: 'Option 1' },
{ id: 'option-2', text: '' },
];

const result = await stateMiddleware.handlers.handleFieldChange(
setupHandlerParams({
nextState: { ...initialState },
previousState: { ...initialState },
targetFields: {
options: {
index: 0,
text: ' ',
},
},
}),
);

expect(result.state.nextState.data.options).toEqual([
{ id: 'option-1', text: ' ' },
{ id: 'option-2', text: '' },
]);
expect(result.status).toBeUndefined;
});

Expand Down
Loading