Skip to content

Conversation

@paul-sachs
Copy link
Contributor

@paul-sachs paul-sachs commented Oct 30, 2025

Based off of #5002, this takes it a step further and only merges context from previous load (when preloaded is successful) and send that to the component. This ensures the context maintains it's proper type even when pending.

fixes #4998

Summary by CodeRabbit

  • Bug Fixes

    • Fixed context behavior during route preloading and navigation to prevent stale context from being incorrectly carried over to subsequent route transitions.
  • Tests

    • Added comprehensive tests validating proper context isolation during preload operations and subsequent navigation sequences, ensuring beforeLoad receives fresh context on each invocation.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

Walkthrough

This PR introduces isolated context handling for beforeLoad execution and adds test coverage for preload context scenarios. It modifies the context propagation logic to ensure beforeLoad receives fresh context on each invocation, while the pending component state preserves prior context from successful matches.

Changes

Cohort / File(s) Summary
Test Coverage for Context Isolation
packages/react-router/tests/routeContext.test.tsx
Adds two test cases: one verifying route component context behavior during preload with beforeLoad pending, and another ensuring beforeLoad receives fresh context on subsequent runs rather than stale preloaded context.
BeforeLoad Context Isolation
packages/router-core/src/load-matches.ts
Introduces beforeLoadFnInputContext (fresh context without prior __beforeLoadContext) for beforeLoad invocation and pendingContext (preserving prior __beforeLoadContext when match previously succeeded) for pending component state. Updates context propagation to merge __beforeLoadContext only when transitioning from beforeLoad to pending, isolating beforeLoad execution while maintaining component context continuity.

Sequence Diagram

sequenceDiagram
    participant App
    participant Router
    participant LoadMatches
    participant BeforeLoad
    participant Component

    App->>Router: Focus (preload)
    Router->>LoadMatches: Load with preload flag
    LoadMatches->>BeforeLoad: Call with beforeLoadFnInputContext (fresh)
    Note over BeforeLoad: Pending state marked<br/>with pendingContext<br/>(preserves prior context<br/>if match succeeded)
    
    App->>Router: Navigate (enter)
    Router->>LoadMatches: Load with enter flag
    LoadMatches->>BeforeLoad: Call with beforeLoadFnInputContext (fresh, not stale)
    Note over BeforeLoad: ✓ Fresh context,<br/>not preloaded context
    BeforeLoad-->>LoadMatches: beforeLoad resolves
    LoadMatches->>Component: Render with updated context
    Note over Component: __beforeLoadContext<br/>merged after beforeLoad<br/>completes
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • load-matches.ts: Multiple context flows with conditional logic (beforeLoadFnInputContext vs pendingContext, context merging timing) require careful verification of the isolation and propagation logic.
  • routeContext.test.tsx: Test assertions must be validated to ensure they properly capture the expected context values and invocation timing across preload and navigation phases.

Possibly related issues

Possibly related PRs

Suggested reviewers

  • schiller-manuel

Poem

🐰 Fresh contexts bloom, no stale preload tales,
BeforeLoad now runs pristine, untangling trails,
Pending preserves what came before,
Each navigation starts clean at the door! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Merge __beforeLoadContext for pending loading state" directly addresses the main change in the changeset. The PR introduces a mechanism to merge context from previous loads (using __beforeLoadContext) and apply it during pending loading states, which is precisely what the title conveys. The title is specific enough to distinguish this change from other context-related modifications, references the technical implementation detail (__beforeLoadContext), and accurately reflects the primary objective stated in the PR summary. The supporting test cases verify this context merging behavior during preload and navigation scenarios.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a62499 and 393b524.

📒 Files selected for processing (2)
  • packages/react-router/tests/routeContext.test.tsx (1 hunks)
  • packages/router-core/src/load-matches.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript in strict mode with extensive type safety across the codebase

Files:

  • packages/router-core/src/load-matches.ts
  • packages/react-router/tests/routeContext.test.tsx
packages/router-core/**

📄 CodeRabbit inference engine (AGENTS.md)

Keep framework-agnostic core router logic in packages/router-core/

Files:

  • packages/router-core/src/load-matches.ts
packages/{react-router,solid-router}/**

📄 CodeRabbit inference engine (AGENTS.md)

Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/

Files:

  • packages/react-router/tests/routeContext.test.tsx
🧠 Learnings (3)
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
PR: TanStack/router#0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/router-core/** : Keep framework-agnostic core router logic in packages/router-core/

Applied to files:

  • packages/router-core/src/load-matches.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
PR: TanStack/router#5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.

Applied to files:

  • packages/react-router/tests/routeContext.test.tsx
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
PR: TanStack/router#0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/

Applied to files:

  • packages/react-router/tests/routeContext.test.tsx
🧬 Code graph analysis (1)
packages/react-router/tests/routeContext.test.tsx (1)
packages/react-router/src/router.ts (1)
  • createRouter (80-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Test
🔇 Additional comments (2)
packages/router-core/src/load-matches.ts (1)

372-400: Context handoff stays type-safe.

The split between the fresh beforeLoadFnInputContext and the cached pendingContext keeps beforeLoad isolated while still letting components render against the last successful context during pending states. Nice guard rails to prevent stale type unions from leaking back into beforeLoad.

packages/react-router/tests/routeContext.test.tsx (1)

3132-3302: Great regression coverage for preload + enter.

These focused tests capture the “preload then enter” edge cases perfectly and ensure context consumers no longer see {} flashes or stale beforeLoad input. Thanks for locking that behavior down.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Oct 30, 2025

View your CI Pipeline Execution ↗ for commit 393b524

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 8m 1s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 25s View ↗

☁️ Nx Cloud last updated this comment at 2025-10-30 15:29:05 UTC

@paul-sachs
Copy link
Contributor Author

@coderabbitai generate docstrings

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

✅ Actions performed

Initiated docstring generation; will generate only if new commits exist.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

Caution

Docstrings generation - FAILED

No docstrings were generated.

@paul-sachs
Copy link
Contributor Author

Actually, this still doesn't fully solve the problem. The problem is that my component depends on context to such a degree that it cannot render at all until beforeLoad runs, and it seems that setting pendingComponent forces it to render to trigger suspense. This is gonna need some more work to figure out 🤔

@paul-sachs paul-sachs closed this Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Route component is rendered without route context from async beforeLoad with pendingComponent

1 participant