#9355: fix dirty flag on Formik reinitialization regression #9361
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main pixiebrix/pixiebrix-extension#9361 +/- ##
==========================================
+ Coverage 74.24% 75.44% +1.20%
==========================================
Files 1332 1386 +54
Lines 40817 42043 +1226
Branches 7634 7755 +121
==========================================
+ Hits 30306 31721 +1415
+ Misses 10511 10322 -189 ☔ View full report in Codecov by Sentry. |
Playwright test results
Details
Flaky testschrome › tests/pageEditor/copyMod.spec.ts › run a copied mod with a built-in integration Skipped testschrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options |
twschiller
changed the title
twschiller
changed the title
| dispatch(actions.checkActiveModComponentAvailability()); | ||
| const currentKey = getFormReinitializationKey(); | ||
|
|
||
| // Don't sync on the first call after the reinitialization key changes because the call would cause |
There was a problem hiding this comment.
See the failing CI test. I think our options are:
- Check for deep equality on the first update after reinitialization to determine whether or not the values have actually changed (so normalization would still cause the change icon to appear)
- Ignore the initial sync (current behavior of the PR) under the assumption that any normalization should never impact the functionality so there's no reason to show the change icon. (The only gotcha here is that the call is debounced -- so might want to ensure there's no actually human-made changes within the debounce window)
There was a problem hiding this comment.
I ended up going with approach 1. With approach 2, there's a corner case where normalizations get overwritten if the user's next change is not via Formik
There was a problem hiding this comment.
Latest changes have a blended approach - it updates the form state but does not update the dirty flag
|
@grahamlangford re-assigning to you for now because I believe this is related to the reinitialization PR: #9351 and I'll be partially OOO tomorrow |
| @@ -34,7 +34,7 @@ export const NotAvailableIcon: React.FunctionComponent = () => ( | |||
| ); | |||
|
|
|||
| export const UnsavedChangesIcon: React.FunctionComponent = () => ( | |||
| <Icon library="custom" icon="ic-unsaved" /> | |||
| <Icon library="custom" icon="ic-unsaved" title="Unsaved changes" /> | |||
There was a problem hiding this comment.
These should probably have role="status", but Playwright getByRole didn't seem to be working for that
twschiller
changed the title
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
Discussion
redux-loggermiddleware has an option for seeing the diff on each action: https://www.npmjs.com/package/redux-logger#options (You can use to quickly see where it's being set back todirty: trueRemaining Work
useEffectupdates the Formik values automatically on mount, do we want that to show as unsaved changes?Future Work
For more information on our expectations for the PR process, see the
code review principles doc