fix: bridge SSR payload mismatch when client search provider differs from server default#2379
fix: bridge SSR payload mismatch when client search provider differs from server default#2379gusa4grr wants to merge 1 commit intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Fixes #2380 |
gusa4grr
force-pushed
the
search-ssr-mismatch-bridge
branch
2 times, most recently
from
2d26409 to
180d757
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis pull request removes route-query-based provider normalization and updates components and composables to use the reactive Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/interactions.spec.ts (1)
105-119: Consider adding explicit focus assertion for consistency.The other modified tests (lines 143, 170, 177) assert that
orgSuggestionis focused afterArrowDown, but this test relies solely on the final URL check. Adding an assertion after line 117 would make the test more consistent and easier to debug if it fails.🔧 Suggested improvement
// ArrowDown again, then Enter navigates to the suggestion // URL is /package/vue or /org/vue or /user/vue. Not /vue await page.keyboard.press('ArrowDown') + await expect(orgSuggestion).toBeFocused() await page.keyboard.press('Enter') await expect(page).toHaveURL(/\/(package|org|user)\/vue/)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: daf4de20-08e0-4555-b419-178494a8c1a0
📒 Files selected for processing (9)
app/components/SearchProviderToggle.client.vueapp/composables/npm/search-utils.tsapp/composables/npm/useOrgPackages.tsapp/composables/npm/useSearch.tsapp/composables/npm/useUserPackages.tsapp/composables/useGlobalSearch.tsapp/composables/useSettings.tstest/e2e/interactions.spec.tstest/fixtures/npm-registry/search/@vue.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/composables/npm/search-utils.ts (1)
1-26: Well-implemented SSR payload bridge.The logic correctly handles the hydration mismatch by copying SSR-cached data from the algolia-keyed entry to the client's provider-specific key. The guard conditions are comprehensive:
import.meta.clientensures client-only executionisHydratingensures this only runs during hydrationid &&prevents malformed keys from empty identifiers- The
!nuxtApp.payload.data[clientKey]check avoids overwriting existing dataConsider using the stricter
SearchProvidertype instead ofstringfor theproviderparameter:♻️ Optional: Stricter typing
+import type { SearchProvider } from '~/composables/useSettings' + export function bridgeSearchSSRPayload( prefix: string, identifier: MaybeRefOrGetter<string>, - provider: MaybeRefOrGetter<string>, + provider: MaybeRefOrGetter<SearchProvider>, ): void {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0bd3e3f-7b8b-48bb-b5b8-db13fd315b81
📒 Files selected for processing (9)
app/components/SearchProviderToggle.client.vueapp/composables/npm/search-utils.tsapp/composables/npm/useOrgPackages.tsapp/composables/npm/useSearch.tsapp/composables/npm/useUserPackages.tsapp/composables/useGlobalSearch.tsapp/composables/useSettings.tstest/e2e/interactions.spec.tstest/fixtures/npm-registry/search/@vue.json
✅ Files skipped from review due to trivial changes (2)
- app/composables/npm/useSearch.ts
- test/fixtures/npm-registry/search/@vue.json
🚧 Files skipped from review as they are similar to previous changes (4)
- app/composables/npm/useOrgPackages.ts
- test/e2e/interactions.spec.ts
- app/composables/npm/useUserPackages.ts
- app/composables/useGlobalSearch.ts
app/composables/useGlobalSearch.ts
Outdated
| return | ||
| } | ||
| router.push({ | ||
| void router.push({ |
There was a problem hiding this comment.
are these voids necessary?
There was a problem hiding this comment.
oh, my bad, I just "fixed" the errors in my IDE, but apparently that's my local oxc.typeAware: true config, which marks this as error.
I'll revert the changes
- Centralize `?p` query param merge into `useSearchProvider()` - Remove duplicated `searchProviderValue` computed from all consumers - Add `bridgeSearchSSRPayload()` to copy SSR cache to client's provider key during hydration - Destructure asyncData before spreading to avoid shadowing custom `data` ref - Update e2e tests for org suggestion cards in keyboard navigation
gusa4grr
force-pushed
the
search-ssr-mismatch-bridge
branch
from
180d757 to
7a77f97
Compare
2 participants
🔗 Linked issue
Resolves #2380
🧭 Context
When the user's
searchProvidervalue isnpm, there will be hydration errors and more. Routes affected aresearch?q=vueor/settingspage, but it may be more, like/~[username]routeMoreover, on
search?q=vue, the user is additionally presented with "no results screen" after flashing actual search results 😅In this MR I:
In order to see this behaviour yourself, you need to:
npmfrom algolia/search?q=vue. Observe hydration + no results/settings. Observe hydration errorMore details in "details" section below 😉
Images attached:
search page:
settings page:
?pquery param merge intouseSearchProvider()gettersearchProviderValuecomputed from all consumersbridgeSearchSSRPayload()to copy SSR cache to client's provider key during hydrationdataref📚 Description
During my adventures with the implementing server-synced user preferences (please check that one too if you fancy 🙂 ), I stumbled upon the e2e tests failing with hydration errors, which were:
Also, the
interactionsspec andurl-compatibilityspec have each failed tests 🤯So I went ahead and started digging into what "have I done" so it broke.
after bunch of hours of little to no success I (finally) decided to check how does the
mainbranch behave and just went ahead to npmx.dev prod app and I found that withnpmas searchProvider, th things are actually not working as they should.This became visible during my implementation, as preferences are now server-synced, which just uncovered those issues. And the combination of the fact that in the "test" environment, searchProvider
is defaulted tonpm`.I continued digging and found a misalignment in the searchProvider value during hydration. Basically the fetch was always happening with algolia, and then when NPM "results" were tried to be hydrated on client - it sees nothing due to cache key mismatch between providers.
so I dig more and more and I come up with the solution you see in this MR.
I'm happy to discuss further or implement a better approach.
NOTE: When navigating via the website to the search page, all is OK. like for instance if you're on the
/settingsand write in npmx search bar vue => it will render OK. The secret here is the?p=npmquery parameter, which is programmatically added, thereby enabling the search page to find results.