Skip to content

feat: replace old settings form with the new implementation#4058

Merged
tpoisseau merged 6 commits intomainfrom
4032-replace-previous-generic-setting-dialog-with-new-tanstack-implementation
Mar 16, 2026
Merged

feat: replace old settings form with the new implementation#4058
tpoisseau merged 6 commits intomainfrom
4032-replace-previous-generic-setting-dialog-with-new-tanstack-implementation

Conversation

@tpoisseau
Copy link
Copy Markdown
Contributor

Closes: #4032
Closes: #3932

@tpoisseau tpoisseau changed the title feat: replace old general settings by the new implementation feat: replace old general settings form by the new implementation Mar 13, 2026
@tpoisseau tpoisseau changed the title feat: replace old general settings form by the new implementation feat: replace old settings form with the new implementation Mar 13, 2026
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages bot commented Mar 13, 2026

Deploying nmrium with  Cloudflare Pages  Cloudflare Pages

Latest commit: bea3a26
Status: ✅  Deploy successful!
Preview URL: https://f7d70c0e.nmrium.pages.dev
Branch Preview URL: https://4032-replace-previous-generi.nmrium.pages.dev

View logs

@tpoisseau
Copy link
Copy Markdown
Contributor Author

tpoisseau commented Mar 13, 2026

If you find any issues that were not present in the previous implementation, now is the time to report them. It needs to be fixed, before merge this PR.

If you find issues but they already here in previous implementation this should not block this PR and open a new issue.

@targos
Copy link
Copy Markdown
Member

targos commented Mar 13, 2026

The "Reset workspace preferences" button's is not enabled based on the right condition. I guess it's based on the form being dirty, but it should be based on the workspace being dirty and reset to the last saved state of the workspace (or to the initial value for read-only workspaces)

@tpoisseau
Copy link
Copy Markdown
Contributor Author

tpoisseau commented Mar 13, 2026

Ok, I re-checked the previous check. adapt and fixed.

  const isResetDisabled = useMemo(() => {
    if (!isPristine) return false;

    const original = getPreferencesByWorkspace(currentName, originalWorkspaces);

    return isEqualLodash(current, original);
  }, [current, currentName, isPristine, originalWorkspaces]);

Method with JSON.stringify is heavy and leads to false-positive. (sensible to property declaration order), isEqualLodash works better.

There is still an edge-case. If the form is not pristine but all values match with original: reset is allowed, but it will reset to the same values. I think it's OK, if not we could remove the first check.

@targos
Copy link
Copy Markdown
Member

targos commented Mar 13, 2026

There is still an edge-case. If the form is not pristine but all values match with original: reset is allowed, but it will reset to the same values. I think it's OK, if not we could remove the first check.

That sounds OK, but then is there a way for it to become pristine when the click is done? It's confusing that the read-only workspace stays "unsaved" even after clicking on reset.

@targos
Copy link
Copy Markdown
Member

targos commented Mar 13, 2026

To reproduce the case:

  • Open the page fresh -> Simple NMR analysis workspace is loaded and pristine
  • Open the settings
  • Save the settings (without changes)
  • Workspace is not pristine anymore (I can accept that, since the form maybe initialized some fields with default values)
  • Open the settings
  • Click the reset button -> Why is it not considered pristine now?

@Sebastien-Ahkrin Sebastien-Ahkrin self-requested a review March 13, 2026 12:07
@tpoisseau
Copy link
Copy Markdown
Contributor Author

tpoisseau commented Mar 13, 2026

left: current after save the form. right: original
CleanShot 2026-03-13 at 13 44 14@2x
I found the issue. The new form change the order of filters.

I'll fix that.

(reset set the form pristine, it changes the default values)

@targos
Copy link
Copy Markdown
Member

targos commented Mar 13, 2026

Wow it's nice that we found it. It's a real bug (filter order is important)!

@tpoisseau
Copy link
Copy Markdown
Contributor Author

I fixed it, (not yet pushed) but form save apply more props than workspaceDefaultProperties (the source of workspace normalization) so isEqual return false.
Ex: workspaceDefaultProperties.display.panels.matrixGenerationPanel does not exists. The form saves it. so comparaison is broken.

Final solution is:

* refactor AutoProcessingTab
  * no codec to transform filters array to record by name because it can reorder the final filters and the order is important.
  * simplify the components with `withFieldGroup`
* to check current form values (transformed to workspace) with the original one. isPristine is not needed anymore.
* fix `getPreferencesByWorkspace` with `mergeReplaceArray` strategy
* complete `workspaceDefaultProperties` to match with concrete save of the form.
  * add `satisfies` constraints when properties can be missing (lot of `Partial` and optional props usage in `WorkspacePreferences`)
@tpoisseau
Copy link
Copy Markdown
Contributor Author

When comparing form behavior, pay attention to the experimental mode. Check it add more fields in the form

@targos

This comment was marked as resolved.

@targos
Copy link
Copy Markdown
Member

targos commented Mar 13, 2026

I'll finish the review with @lpatiny on Monday

@tpoisseau tpoisseau merged commit fe2f032 into main Mar 16, 2026
12 checks passed
@tpoisseau tpoisseau deleted the 4032-replace-previous-generic-setting-dialog-with-new-tanstack-implementation branch March 16, 2026 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace previous generic setting dialog with new tanstack implementation Use tanstack form and react-science in general preferences

3 participants