Skip to content

reproduction for fix-match-loading edge-cases - #7713

Closed
Sheraff wants to merge 3 commits into
fix-match-loadingfrom
fix-match-loading-edge-cases
Closed

reproduction for fix-match-loading edge-cases#7713
Sheraff wants to merge 3 commits into
fix-match-loadingfrom
fix-match-loading-edge-cases

Conversation

@Sheraff

Copy link
Copy Markdown
Collaborator

No description provided.

@coderabbitai

coderabbitaiBot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0f823661-109e-4b58-ae6e-8c5e27df6770

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-match-loading-edge-cases

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

@nx-cloud

nx-cloudBot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

View your CI Pipeline Execution ↗ for commit 4d218c3

CommandStatusDurationResult
nx affected --targets=test:eslint,test:unit,tes...❌ Failed6m 31sView ↗
nx run-many --target=build --exclude=examples/*...✅ Succeeded26sView ↗

☁️ Nx Cloud last updated this comment at 2026-06-29 17:40:33 UTC

*
* This test navigates through the public router API with object-form
* viewTransition options. The mocked browser reports no typed-transition
* support, but still provides document.startViewTransition, so navigation should

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this behavior exist in actual browsers?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to be true that the case exists in actual browsers, but it may be a somewhat narrow band

  • Document.startViewTransition: Chrome 111, Safari 18, Firefox 144
  • :active-view-transition-type(): Chrome 125, Safari 18.2, Firefox 147

i'm not 100% convinced it's worth jumping through hoops to support this. I'll leave that up to you

Comment threadpackages/router-core/tests/repro-issue-5-server-redirect-leak.test.ts Outdated
* loading can behave differently from other route failures.
*
* This test uses a real router navigation to a lazy route whose chunk rejects.
* The match reaches error state with the chunk error, but the route's onError

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it? why?

@SheraffSheraffJun 29, 2026

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it weird if onError isn't called in the specific case of "failed to load a chunk", but called in all other error cases?

import { createTestRouter } from './routerTestUtils'

/**
* Issue 8: route headers are response behavior and should not be dropped just

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should a head throwing on the server be swallowed in general? or is the whole response than a 500 with no data?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this issue here is about the inconsistency: if we don't fail the request on head() failure, we shouldn't drop the headers() on head() failure.

But I feel like a fail is a fail, and it should 500 instead of delivering partial content.

Comment threadpackages/router-core/tests/repro-issue-9-redirect-status-overwrite.test.ts Outdated
@TanStackTanStack deleted a comment from codspeed-hqBotJun 29, 2026
@TanStackTanStack deleted a comment from github-actionsBotJun 29, 2026
@TanStackTanStack deleted a comment from pkg-pr-newBotJun 29, 2026
@TanStackTanStack deleted a comment from github-actionsBotJun 29, 2026
@TanStackTanStack deleted a comment from codspeed-hqBotJun 29, 2026
@TanStackTanStack deleted a comment from github-actionsBotJun 29, 2026
@TanStackTanStack deleted a comment from pkg-pr-newBotJun 29, 2026
@TanStackTanStack deleted a comment from github-actionsBotJun 29, 2026
@Sheraff

Copy link
Copy Markdown
CollaboratorAuthor

superseded #7805

@SheraffSheraff closed this Aug 4, 2026
Sign up for freeto 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.

2 participants

@Sheraff@schiller-manuel