Relevant Linear issue here. Also see @tomek's comment on D4416. This issue arose after we decided to leave ThreadSettingsGeneralTab open (Linear issue) after the user had clicked the Save button, since the Save button remained enabled even if there were no changes to be made.
Details
Details
Tested on Chrome/Safari and works as expected:
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/modals/threads/settings/thread-settings-general-tab.react.js | ||
|---|---|---|
| 131 ↗ | (On Diff #15891) | This is also used on line 113, and guarantees that !changesQueued is true, which will ensure that the disabled prop on line 189 is true. |
Comment Actions
What happens if the call to dispatchActionPromise(...) fails?
Are we sure that we want to throw away queued changes in that scenario?
This comment was removed by • abosh.
Comment Actions
Actually, I think this is exactly what needed to be done. Now, clearing the queued changes is explicitly the result of the promise being resolved, and it actually even fixes the bug that can be seen in the video in the Test Plan of this diff.
If you notice, every time the Save button is clicked, just before the button disables the old changes are shown again on the modal. Like if I change the color, and click save, the old selected color gets "reselected" before switching to the new color, and then the button disables since queuedChanges is cleared. This is because the promise resolves, but right after, queued changes (for a split second) is not cleared, so the value of the color selected or thread description reverts to queuedChanges.description or queuedChanges.color.
However, now that clearing queuedChanges is part of the promise resolving, queuedChanges clears, the button disables, and all the old changes are gone by the time this happens. Video:
Thanks for catching this!
| web/modals/threads/settings/thread-settings-general-tab.react.js | ||
|---|---|---|
| 130 ↗ | (On Diff #16020) | I'm not convinced that this is the right approach. In changeThreadSettingsAction we clear queued changes only when the request fails, but here we do it regardless. If we want to always clear we should do it in just one place. The current approach clears the changes when an error is detected, so we make it impossible to just retry. Maybe we should revisit it? |
Comment Actions
The current approach clears the changes when an error is detected, so we make it impossible to just retry. Maybe we should revisit it?
This makes sense, so I've updated this diff to incorporate this. Before, we were clearing the queued changes in case of an error in changeThreadSettingsAction. This makes sense since we report the error (unknown_error) and reset the modal to a default state (disabled submit button, etc.).
Now, we also want to clear the queued changes in case of successful changeThreadSettingsAction. This is because we also want to return the modal to a default state once the user has clicked submit (disabled save button, new changes selected, etc.).
Added a finally block in changeThreadSettingsAction and removed all code related to clearing queuedChanges from onSubmit. Now, in any case (error or success), after changeThreadSettingsAction is called, the changes will be cleared and the Save button will be disabled. This covers both cases.
Comment Actions
I'm still not sure if our approach is ideal.
What do you think about keeping the queued changes when a request fails? We would display an error message and show a button as enabled so that the user can retry - what do you think about it?
When everything goes well, clearing the queued changes is the right action.
Comment Actions
Going to land this and address @tomek's question in a Linear issue for a (potential) separate diff. For now, I think clearing the queued actions in all cases is a valid way forward.