Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughuseTags was changed to use a new local replaceEqualTags that compares RouterManagedTag entries via JSON.stringify and reuses matching previous object references. A test was added to ensure SSR stylesheet link elements remain mounted and not duplicated when navigating between routes with differing preload arrays. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
View your CI Pipeline Execution ↗ for commit 5ff1616
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 3 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/solid-router/tests/Scripts.test.tsx (1)
226-315: Test targets the bug scenario well.The asymmetric preload arrays (
['/a.js']vs['/b.js', '/b-child.js']) are exactly what shifts the stylesheet tag's index across the combined tags array, which was the scenario where the oldreplaceEqualDeeplost the reference. Asserting element identity (Line 309) plus dedup count (Lines 310–314) is the right pair of assertions.Optional: consider adding a
/b → /aclick in the same test (or a second iteration) to also assert the reference survives navigating back, similar to the 5× loop in the existing test at Lines 200–216. Not required for this fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-router/tests/Scripts.test.tsx` around lines 226 - 315, Add a back-navigation check to the test "keeps manifest stylesheet links mounted when preload counts change": after asserting the stylesheet link identity and dedupe count, simulate clicking the "Go to A" link (use screen.getByRole to find the link and fireEvent.click), wait for router.state.location.pathname to be '/a' (or use screen.findByRole for the "Go to B" link), then re-check that getStylesheetLink() returns the same initialLink and that only one link with href '/main.css' exists; this mirrors the existing 5× loop behavior and ensures the stylesheet reference survives navigating back.packages/solid-router/src/headContentUtils.tsx (1)
197-241: Correct fix; optional perf refactor to dedupe JSON.stringify work.Switching from the index-paired
replaceEqualDeepto a Map keyed byJSON.stringify(tag)is the right fix: when preload counts change across routes, the stylesheet tag's position in the combined tags array shifts, and the old index-based diff would replace the reference even though the tag is identical — which is exactly what triggers the FOUC. Keying by content lookups preserves the reference regardless of index. Duplicate-key collisions in the Map aren't a concern becauseprevwas already deduped by the same JSON key in the previous memo run viauniqBy(line 206–208).One optional optimization:
JSON.stringifyruns once per tag inuniqByon Line 207, then again for everyprevtag on Line 223 and everynexttag on Line 228. Head tags with largechildren(inline styles, JSON‑LD) make this noticeably hotter than it needs to be on every navigation. You can thread the keys through from the uniqBy step so each tag is stringified at most once:♻️ Proposed optional refactor
return Solid.createMemo((prev: Array<RouterManagedTag> | undefined) => { - const next = uniqBy( - [ + const combined = [ ...meta(), ...preloadLinks(), ...links(), ...styles(), ...headScripts(), - ] as Array<RouterManagedTag>, - (d) => { - return JSON.stringify(d) - }, - ) + ] as Array<RouterManagedTag> + const { items: next, keys: nextKeys } = uniqByWithKeys(combined, (d) => + JSON.stringify(d), + ) if (prev === undefined) { return next } - return replaceEqualTags(prev, next) + return replaceEqualTags(prev, next, nextKeys) }) } -function replaceEqualTags( - prev: Array<RouterManagedTag>, - next: Array<RouterManagedTag>, -) { - const prevByKey = new Map<string, RouterManagedTag>() - for (const tag of prev) { - prevByKey.set(JSON.stringify(tag), tag) - } - - let isEqual = prev.length === next.length - const result = next.map((tag, index) => { - const existing = prevByKey.get(JSON.stringify(tag)) +function replaceEqualTags( + prev: Array<RouterManagedTag>, + next: Array<RouterManagedTag>, + nextKeys: Array<string>, +) { + const prevByKey = new Map<string, RouterManagedTag>() + for (const tag of prev) { + prevByKey.set(JSON.stringify(tag), tag) + } + + let isEqual = prev.length === next.length + const result = next.map((tag, index) => { + const existing = prevByKey.get(nextKeys[index]!) if (existing) { if (existing !== prev[index]) { isEqual = false } return existing } isEqual = false return tag }) return isEqual ? prev : result }(With a small
uniqByWithKeyshelper that returns both the filtered items and their computed keys. Further: you could also cache the prev keys from the previous memo run to avoid re-stringifyingprevas well.)Not a blocker — the current implementation is correct and the fix itself is clean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-router/src/headContentUtils.tsx` around lines 197 - 241, The current fix is correct but we should avoid redundant JSON.stringify calls by threading computed keys through the memo: create a helper (e.g., uniqByWithKeys) that returns both the deduped items and their computed keys, replace the uniqBy(...) call in the Solid.createMemo callback with this helper to obtain next and nextKeys, build prevByKey using the corresponding prevKeys (cache prevKeys alongside prev in the memo closure), and in replaceEqualTags use the precomputed keys (instead of calling JSON.stringify again) to look up existing tags and to compare items when building result so each tag is stringified at most once; update code that references prev, next, prevByKey, and result accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/solid-router/src/headContentUtils.tsx`:
- Around line 197-241: The current fix is correct but we should avoid redundant
JSON.stringify calls by threading computed keys through the memo: create a
helper (e.g., uniqByWithKeys) that returns both the deduped items and their
computed keys, replace the uniqBy(...) call in the Solid.createMemo callback
with this helper to obtain next and nextKeys, build prevByKey using the
corresponding prevKeys (cache prevKeys alongside prev in the memo closure), and
in replaceEqualTags use the precomputed keys (instead of calling JSON.stringify
again) to look up existing tags and to compare items when building result so
each tag is stringified at most once; update code that references prev, next,
prevByKey, and result accordingly.
In `@packages/solid-router/tests/Scripts.test.tsx`:
- Around line 226-315: Add a back-navigation check to the test "keeps manifest
stylesheet links mounted when preload counts change": after asserting the
stylesheet link identity and dedupe count, simulate clicking the "Go to A" link
(use screen.getByRole to find the link and fireEvent.click), wait for
router.state.location.pathname to be '/a' (or use screen.findByRole for the "Go
to B" link), then re-check that getStylesheetLink() returns the same initialLink
and that only one link with href '/main.css' exists; this mirrors the existing
5× loop behavior and ensures the stylesheet reference survives navigating back.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e59af330-65c5-481e-96cf-f0c637eb1f9b
📒 Files selected for processing (2)
packages/solid-router/src/headContentUtils.tsxpackages/solid-router/tests/Scripts.test.tsx
Merging this PR will not alter performance
Comparing 
Footnotes
|
1 participant
Closes issue #7240
Summary by CodeRabbit
Refactor
Tests
Chores