Conversation
📝 WalkthroughWalkthroughThis PR adds two new test routes to the Solid Start selective SSR application: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 1m 54s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-08 09:41:04 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/solid-start/selective-ssr/tests/app.spec.ts (1)
24-25: Drop the fixed sleeps from these two assertions.
await expect(...).toBeVisible()already does the waiting here, so the extrawaitForTimeout(100)just adds fixed delay to every run without making the repro stronger. Please verify the grep still reproduces as intended after removing them.Proposed cleanup
- await page.waitForTimeout(100) await expect(page.getByTestId('pending-inherit-fallback')).toBeVisible() @@ - await page.waitForTimeout(100) await expect(page.getByTestId('pending-data-only-fallback')).toBeVisible()Also applies to: 38-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/solid-start/selective-ssr/tests/app.spec.ts` around lines 24 - 25, Remove the fixed 100ms sleeps before the visibility assertions: delete the await page.waitForTimeout(100) call that precedes the await expect(page.getByTestId('pending-inherit-fallback')).toBeVisible() assertion, and likewise remove the analogous waitForTimeout call at the other occurrence (the pair around lines 38-39). Rely on await expect(...).toBeVisible() to perform the waiting, then run the grep to confirm the repro still works.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/solid-start/selective-ssr/tests/app.spec.ts`:
- Around line 24-25: Remove the fixed 100ms sleeps before the visibility
assertions: delete the await page.waitForTimeout(100) call that precedes the
await expect(page.getByTestId('pending-inherit-fallback')).toBeVisible()
assertion, and likewise remove the analogous waitForTimeout call at the other
occurrence (the pair around lines 38-39). Rely on await
expect(...).toBeVisible() to perform the waiting, then run the grep to confirm
the repro still works.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32ea6349-2b95-40ec-8310-1ad4b7b62a97
📒 Files selected for processing (5)
e2e/solid-start/selective-ssr/src/routeTree.gen.tse2e/solid-start/selective-ssr/src/routes/index.tsxe2e/solid-start/selective-ssr/src/routes/posts.pending-data-only-component.tsxe2e/solid-start/selective-ssr/src/routes/posts.pending-inherit.tsxe2e/solid-start/selective-ssr/tests/app.spec.ts
1 participant
Note
Nobody seems to have complained about this yet, so we can wait for #6704 to be merged. It fixes the Solid errors. And then we'll make a separate PR for the Vue data-only one.
This PR adds two focused browser repros for a gap between
mainandrefactor-signals: during nested client navigation under inherited selective SSR,mainnever renders the leafpendingComponent, whilerefactor-signalsdoes.These tests do not try to prove that
mainis definitively wrong. They are meant to make the discrepancy easy to evaluate.What was already covered
The existing selective-SSR suite already covers the inheritance matrix for final SSR/data behavior on full document navigations (
reloadDocument={true}). In particular, it already covers these inherited cases:root: true,posts: false, leaf inheritspostsselective-ssrtest matrix (testcase-3)root: true,posts: 'data-only', leaf inheritspostsselective-ssrtest matrix (testcase-4)What was not covered before this PR: whether a nested suspending leaf under those same inherited configs should show its configured
pendingComponentduring a client-side navigation.Cases in one view
mainrefactor-signalsroot: true,posts: false, leaf inheritsposts/posts/$postIdroot: true,posts: 'data-only', leaf inheritsposts/posts/$postIdroot: true,posts: false, leaf inheritsposts/posts/pending-inheritpending, but leaf fallback never appearsroot: true,posts: 'data-only', leaf inheritsposts/posts/pending-data-only-componentpending, but leaf fallback never appearsInterpretation
The inheritance behavior itself is already intentional enough to have coverage. The open question is narrower: in these nested inherited selective-SSR cases, should a suspending leaf's configured
pendingComponentbe allowed to render during client navigation?This PR exists so that question can be answered against a concrete repro instead of inferred from the broader selective-SSR matrix.
Notes
There are neighboring transition tests in other Solid e2e apps that prefer keeping previous content visible during navigation, but they do not cover this selective-SSR nested leaf case, so they do not resolve the expected behavior here.
Testing
pnpm test:eslintpnpm test:typespnpm test:unitpnpm test:e2e tests/app.spec.ts --grep \"nested inherited\"(fails onmain, passes onrefactor-signals)