Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

fix(step-details): Step's details panel is referencing an old value #1580

Merged
merged 1 commit into from
Mar 30, 2023
Merged

fix(step-details): Step's details panel is referencing an old value #1580

merged 1 commit into from
Mar 30, 2023

Conversation

lordrip
Copy link
Member

@lordrip lordrip commented Mar 29, 2023

Description

Currently, when opening the step’s details tab, it’s not referencing the most recent values but keeping the previous values from a previous step.

This generates unintentional changes to a different step than the user is intending to.

This situation was caused because of the addition of a debounce function with the intention to only save the form’s values after the user’s input stopped.

The problem was that the debounce function was causing to capture the form’s value from the previous render, hence saving the wrong values to the previous step.

To avoid this situation in the future, the saveConfig function now receives the original step and the new values.

Changes

  • Refactor saveConfig callback to receive the original step as a parameter
  • Replace lodash' debounce function with useDebounceCallback()
  • Increase the debounce timer from 500ms to 1s
  • Update onChangeModel signature to (configuration: Record<string, unknown>, isValid: boolean) to reflect more accurately the intended parameters.
  • Remove the lodash.debounce dependency since is no longer required

Before

Screencast.from.2023-03-29.07-46-54.webm

After

Screencast.from.2023-03-29.07-50-39.webm

fix: #1561

@lordrip lordrip requested a review from a team March 29, 2023 05:51
@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@a2bed61). Click here to learn what that means.
The diff coverage is 63.63%.

❗ Current head 0f8d0f2 differs from pull request most recent head 119fbc8. Consider uploading reports for the commit 119fbc8 to get more accurate results

@@           Coverage Diff           @@
##             main    #1580   +/-   ##
=======================================
  Coverage        ?   57.01%           
=======================================
  Files           ?       68           
  Lines           ?     2054           
  Branches        ?      470           
=======================================
  Hits            ?     1171           
  Misses          ?      839           
  Partials        ?       44           
Impacted Files Coverage Δ
src/components/JsonSchemaConfigurator.tsx 72.72% <0.00%> (ø)
src/services/stepsService.ts 33.77% <0.00%> (ø)
src/components/Visualization.tsx 65.07% <50.00%> (ø)
src/components/VisualizationStepViews.tsx 69.23% <85.71%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@unsortedhashsets unsortedhashsets left a comment

Choose a reason for hiding this comment

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

LGTM, + tested with blocked E2E test

apupier
apupier previously approved these changes Mar 29, 2023
Copy link
Member

@apupier apupier left a comment

Choose a reason for hiding this comment

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

From functional point of view, I tried with VS Code and it is working better. No more able to reproduce the data loss.

kahboom
kahboom previously approved these changes Mar 29, 2023
Copy link
Contributor

@kahboom kahboom left a comment

Choose a reason for hiding this comment

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

LGTM!

@kahboom
Copy link
Contributor

kahboom commented Mar 29, 2023

@lordrip only thing is that you have some dependency conflicts in package.json you'll have to resolve before merging

Currently, when opening the step’s details tab, it’s not referencing the
most recent values but keeping the previous values from a previous step.

This generates unintentional changes to a different step than the user is
intending to.

This situation was caused because of the addition of a `debounce` function
with the intention to only save the
[form’s values after the user’s input stopped](#1537).

The problem was that the `debounce` function was causing to capture the form’s
value from the previous render, hence saving the wrong values to the previous step.

To avoid this situation in the future, the saveConfig function now
receives the original step and the new values.

- Changes
* Refactor saveConfig callback to receive the original step as parameter
* Replace lodash' debounce function with useDebounceCallback()
* Increase the debounce timer from 500ms to 1s
* Update onChangeModel signature to (configuration: Record<string, unknown>, isValid: boolean)
  to reflect more accurately the intended parameters.
* Remove the lodash.debounce dependency since is no longer required

fix: #1561
@lordrip lordrip dismissed stale reviews from kahboom, apupier, and unsortedhashsets via 119fbc8 March 30, 2023 07:54
@sonarcloud
Copy link

sonarcloud bot commented Mar 30, 2023

Please retry analysis of this Pull-Request directly on SonarCloud.

@lordrip lordrip merged commit 9405caa into KaotoIO:main Mar 30, 2023
@lordrip lordrip deleted the fix/step-details-referencing-old-data branch May 6, 2023 17:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sometimes the provided data in config panel field are ignored
4 participants