Fix vue router regression #13040

farmaazon · 2025-05-06T10:43:39Z

Pull Request Description

A workaround for #13039

The problems were caused by two factors:

I missed a place where we already navigated in a react transition;
The searchParams were modified "in place" in useSearchParamsState what caused unpredictable effects.

See PR comments to see what should be improved to close the task.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
~~- [ ] Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.~~
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
~~- [ ] Unit tests have been written where possible.~~
- [ ] If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
or the Snowflake database integration, a run of the Extra Tests has been scheduled.
- If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

github-actions · 2025-05-06T10:48:07Z

🧪 Storybook is successfully deployed!

📊 Dashboard:

👀 Review changes: https://www.chromatic.com/build?appId=673738989fc3e8430364197e&number=2790
👨‍🎨 Preview storybook: https://673738989fc3e8430364197e-pnvszdyjxr.chromatic.com/

farmaazon · 2025-05-06T11:38:13Z

The integration tests discovered a regression with pre-populate email fields in forms. Added a fix, and re-requested review to just make sure you're ok with it.

MrFlashAccount · 2025-05-06T12:22:46Z

app/gui/src/dashboard/pages/authentication/Registration.tsx

@@ -89,6 +89,10 @@ export default function Registration() {
    },
  })

+  useEffect(() => {


This is clearly a bad thing

When you add use 'useEffect' to set initial values - you better ask yourself, where the root cause, and solve it first.

The root cause:

defaultValues are cached. To reset them, use the reset API.

with the fact that query search is deferred now, so we initialize it with old value

So I assumed this fix is just "a way of doing" in react forms. Additionally, it has an effect that query navigation inside page will also update the email field (not sure if desirable, but I think it is neat).

Do you prefer making navigating to login page non-transitional? Or have another solution in mind?

Initial value is supposed to be present by the moment of initial rendering. Otherwise you'll keep facing issues like this one. Also, React transitions should be preserved as well, as you claimed in the vue router PR.

Changed the fix. Now no useEffects are needed.

Also, React transitions should be preserved as well, as you claimed in the vue router PR.

This is not what I've written, BTW. I said that these use cases for react transitions may be implemented differenttly

Second point is not an option for us

MrFlashAccount · 2025-05-06T12:25:05Z

app/gui/src/dashboard/hooks/searchParamsStateHooks.ts

+      // would be keeping the "intermediate" state by ourselves and leave window.location.search
+      // management to router.
+      if (options.replace ?? false) {
+        window.history.replaceState(null, '', `?${nextSearchParams.toString()}`)


This can't be a solution.

What exactly cannot be a solution?

If "using window.location.search" to keep state of current query, then it is how it worked before:

enso/app/gui/src/dashboard/hooks/searchParamsStateHooks.ts

Line 56 in 7b7890d

const params = new URLSearchParams(window.location.search)

It just relied on navigate updating window.location.search synchronously, while now it is asynchronous.

I may implement better solution, not relying on window.location.search (as in TODO), but wanted to merge a quick fix first.

Could you add a reference to a code where we update window.location directly?

Could you add a reference to a code where we update window.location directly?

There is no such code. Please, re-read my message carefully: I said keeping state of current query. Previous code was relying on navigate setting window.location.search synchronously. This is not something guaranteed, according to my research (but I might be wrong).

In react-router it's always synchronous, same true for window.replaceState(...). In the code you mentioned it serves a different purpose - in some edge cases we can have an outdated value in react (because react update states in batch), see: https://react.dev/reference/react/useState#updating-state-based-on-the-previous-state

react-router doesn't provide an option to get the current pending value so we need to read the location directly.

My initial comment was about the workaround with window.history.replaceState and it's obviously not a solution and I won't let it merged that way.

In react-router it's always synchronous

I wasn't sure, when reading documentation. navigate may return Promise<void>: https://reactrouter.com/api/hooks/useNavigate#signature

in some edge cases we can have an outdated value in react (because react update states in batch),

Yes, I know. This is exactly what I meant by "we want window.location.href to be updated immediately, so any subsequent query changes won't override this one."

My initial comment was about the workaround with window.history.replaceState and it's obviously not a solution and I won't let it merged that way.

For me is no so obvious. Why reading window.location.search is ok, but writing window.location.search is not ok?

It's indirect.

It happens outside of the router context

It runs states out-of-sync

It's not ideomatic.

This is clearly a hack

I spent the last year getting rid of code like this.

Reading is fine because we use it only as a pending state.

MrFlashAccount · 2025-05-06T13:39:35Z

app/gui/src/dashboard/hooks/searchParamsStateHooks.ts

@@ -40,7 +40,8 @@ export function useSearchParamsState<T = unknown>(
  defaultValue: T | (() => T),
  predicate: (unknown: unknown) => unknown is T = (unknown): unknown is T => true,
 ): SearchParamsStateReturnType<T> {
-  const { router, searchParams } = useRouterInReact()
+  const { router, searchParams: searchParamsRaw } = useRouterInReact()
+  const searchParams = React.useDeferredValue(searchParamsRaw)


Why did you add 'useDefferedValue' here? It exists for a different purpose

Different purpose than what?

You clearly don't need it here because the only purpose for this hook is deferring rendering for the parts of the UI. Why do you want to defer here?

When changing category or current id (the state being kept in search params) we want to defer updating asset table. I thought that it should be the default for every query update. But perhaps you're right, I should use it near actual suspense queries.

In that case we should wrap category change in 'startTransition' (I'm sure we do this already)

Deferred state has completely different purpose compared to 'startTransition'

Deferred state has completely different purpose compared to 'startTransition'

Didn't know that. I was just reading in startTransition docs, that:

You can wrap an update into a Transition only if you have access to the set function of that state. If you want to start a Transition in response to some prop or a custom Hook return value, try useDeferredValue instead.

ATM the category is based on the context change coming from vue.

MrFlashAccount · 2025-05-06T15:02:24Z

I'm not even talking about 2 heavy rerenders instead of one previously

…/vue-router-regression

github-actions · 2025-05-29T13:23:10Z

✨ GUI Checks Results

Summary

🧹 Prettier: ✅ Passed
🧰 GUI Checks: ✅ Passed
📚 Storybook: ✅ Deployed

✅ All checks passed! Your changes look good to go.

See individual check results for more details.

ℹ️ Chromatic Tests Skipped

Chromatic visual regression tests were skipped for this PR.

To run the tests and deploy Storybook, add the CI: Run Chromatic label to this PR.

Note: We skip tests by default to optimize CI costs. Only run them when your UI changes are ready for review.

👮 Lint GUI Results

Check Results

🧠 Typecheck: ✅ Passed
🧹 Lint: ✅ Passed
🧪 Unit Tests: ✅ Passed

🎭 Playwright Test Results

Summary

⏳ Duration: 5m 44s
✅ Passed: 344 tests
🔴 Failed: 0 tests
⚠️ Flaky: 0 tests

📥 Download Detailed Report

✅ All tests passed! Your changes look good.

Fix vue router regression

c75dc8f

farmaazon self-assigned this May 6, 2025

farmaazon requested review from PabloBuchu, indiv0, somebody1234, MrFlashAccount, Frizi, vitvakatu, kazcw and AdRiley as code owners May 6, 2025 10:43

farmaazon added the CI: No changelog needed Do not require a changelog entry for this PR. label May 6, 2025

somebody1234 approved these changes May 6, 2025

View reviewed changes

Test fixes

892e5b7

farmaazon requested a review from somebody1234 May 6, 2025 11:37

MrFlashAccount suggested changes May 6, 2025

View reviewed changes

Change fix for emails in login page

606fd51

MrFlashAccount reviewed May 6, 2025

View reviewed changes

MrFlashAccount approved these changes May 15, 2025

View reviewed changes

PabloBuchu approved these changes May 29, 2025

View reviewed changes

Merge branch 'develop' of github.com:enso-org/enso into wip/farmaazon…

1d10bb9

…/vue-router-regression

farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label May 29, 2025

mergify bot merged commit e691cf4 into develop May 29, 2025
66 of 68 checks passed

mergify bot deleted the wip/farmaazon/vue-router-regression branch May 29, 2025 14:29

Fix vue router regression #13040

Fix vue router regression #13040

Uh oh!

Conversation

farmaazon commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Description

Important Notes

Checklist

Uh oh!

github-actions bot commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧪 Storybook is successfully deployed!

📊 Dashboard:

Uh oh!

farmaazon commented May 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farmaazon May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrFlashAccount May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrFlashAccount May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrFlashAccount May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrFlashAccount commented May 6, 2025

Uh oh!

github-actions bot commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ GUI Checks Results

Summary

ℹ️ Chromatic Tests Skipped

👮 Lint GUI Results

Check Results

🎭 Playwright Test Results

Summary

Uh oh!

Uh oh!

Uh oh!

farmaazon commented May 6, 2025 •

edited

Loading

github-actions bot commented May 6, 2025 •

edited

Loading

farmaazon May 6, 2025 •

edited

Loading

MrFlashAccount May 6, 2025 •

edited

Loading

MrFlashAccount May 6, 2025 •

edited

Loading

MrFlashAccount May 6, 2025 •

edited

Loading

github-actions bot commented May 29, 2025 •

edited

Loading