Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sled instances prefetch #1802

Merged
merged 3 commits into from
Oct 31, 2023
Merged

Fix sled instances prefetch #1802

merged 3 commits into from
Oct 31, 2023

Conversation

david-crespo
Copy link
Collaborator

I noticed on rack3 that when navigating from sleds list to sled detail (instances tab), there was a delay before the instances table would show up. We were prefetching it, so I figured out it's because the sled detail link goes to /sleds/abc123, and then we do a little client-side redirect from there to /sleds/abc123/instances, and the loader is on the latter. Strictly speaking I think this might be a bug in React Router, but in the meantime we can fix this either by having the link from the sled list go directly to /sleds/abc123/instances or by adding the loader to that redirect route (as I've done here). I vaguely remember there being a problem with this when I tried it before on another page, so I'll leave this up while we mull it over.

@vercel
Copy link

vercel bot commented Oct 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Oct 31, 2023 5:27pm

@david-crespo
Copy link
Collaborator Author

david-crespo commented Oct 31, 2023

In my testing, the problems discussed in #1267 (comment) and #1125 don't seem to be a problem anymore, so I'm guessing they fixed the race conditions that were causing them. Of course it's always possible they're still there, but more likely to go how we prefer. In the short term I think that's ok, but eventually I'd like to understand why it was fixed.

So it seems like this should be considered ok now:

element={<Navigate to="abc" replace />
loader={AbcPage.loader}

The explicit redirect loader={() => redirect('abc')} is still bad because it leaves a history entry for the index route, which causes a navigation loop on back button.

@david-crespo david-crespo merged commit 57cc189 into main Oct 31, 2023
7 checks passed
@david-crespo david-crespo deleted the sled-instances-prefetch branch October 31, 2023 17:39
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Nov 4, 2023
oxidecomputer/console@bd65b9d...ae8218d

* [ae8218df](oxidecomputer/console@ae8218df) bump msw to 2.0.3 for the real safari fix
* [9a3f95a9](oxidecomputer/console@9a3f95a9) reduce dev console noise about local assets not served by MSW
* [a61ecf24](oxidecomputer/console@a61ecf24) oxidecomputer/console#1809
* [50f3a5b2](oxidecomputer/console@50f3a5b2) bump recharts and react query
* [1a2b5656](oxidecomputer/console@1a2b5656) oxidecomputer/console#1808
* [4c01cd68](oxidecomputer/console@4c01cd68) oxidecomputer/console#1800
* [57cc1892](oxidecomputer/console@57cc1892) oxidecomputer/console#1802
* [47d76dbf](oxidecomputer/console@47d76dbf) oxidecomputer/console#1806
* [eee0eb2e](oxidecomputer/console@eee0eb2e) oxidecomputer/console#1805
* [60c2285e](oxidecomputer/console@60c2285e) oxidecomputer/console#1804
* [d9cf1ef1](oxidecomputer/console@d9cf1ef1) bump sort imports plugin for vuln
* [ba3a383e](oxidecomputer/console@ba3a383e) bump playwright to 1.39 (fix issue with z-index test)
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Nov 4, 2023
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.

1 participant