fix(history): respect ignoreBlocker in history.go#7832
Conversation
`back` and `forward` set `skipBlockerNextPop` when called with
`ignoreBlocker: true`, so the resulting `popstate` bypasses navigation
blockers. `go` did not forward the option to the browser adapter, so
`history.go(n, { ignoreBlocker: true })` still triggered blockers.
Thread `ignoreBlocker` through the internal `go` adapter and set
`skipBlockerNextPop` in the browser history `go`, matching `back`/`forward`.
Adds a behavioural `createBrowserHistory` regression test that drives a real
`popstate` and asserts blockers run for `go(-1)` but are skipped for
`go(-1, { ignoreBlocker: true })`.
Closes TanStack#4867
📝 WalkthroughWalkthrough
ChangesHistory go blocker handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/history/src/index.ts`:
- Around line 504-508: Update the if statement in the go callback to wrap the
skipBlockerNextPop assignment in curly braces, preserving the existing behavior
and surrounding history navigation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 472908c1-fdf6-4889-8bc6-09745ce4a9fd
📒 Files selected for processing (2)
packages/history/src/index.tspackages/history/tests/createBrowserHistory.test.ts
| go: (n, ignoreBlocker) => { | ||
| if (ignoreBlocker) skipBlockerNextPop = true | ||
| nextPopIsGo = true | ||
| win.history.go(n) | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use curly braces for if statements.
The if statement has a one-line body without curly braces. As per coding guidelines, always use curly braces for if, else, loops, and similar control statements.
♻️ Proposed fix
- go: (n, ignoreBlocker) => {
- if (ignoreBlocker) skipBlockerNextPop = true
- nextPopIsGo = true
- win.history.go(n)
- },
+ go: (n, ignoreBlocker) => {
+ if (ignoreBlocker) {
+ skipBlockerNextPop = true
+ }
+ nextPopIsGo = true
+ win.history.go(n)
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go: (n, ignoreBlocker) => { | |
| if (ignoreBlocker) skipBlockerNextPop = true | |
| nextPopIsGo = true | |
| win.history.go(n) | |
| }, | |
| go: (n, ignoreBlocker) => { | |
| if (ignoreBlocker) { | |
| skipBlockerNextPop = true | |
| } | |
| nextPopIsGo = true | |
| win.history.go(n) | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/history/src/index.ts` around lines 504 - 508, Update the if
statement in the go callback to wrap the skipBlockerNextPop assignment in curly
braces, preserving the existing behavior and surrounding history navigation
logic.
Source: Coding guidelines
1 participant
What & why
history.go(n, { ignoreBlocker: true })still fires navigation blockers, unlikeback/forwardwhich honor the option (#4867). The internal history adapter'sgosignature was(n: number) => void, socreateHistory.godroppedignoreBlockerwhen callingopts.go(index), and the browsergoonly setnextPopIsGowithout settingskipBlockerNextPop. The resultingpopstatetherefore never bypassed blockers inonPushPopEvent.Fix (
packages/history/src/index.ts, 3 lines)go: (n: number, ignoreBlocker: boolean) => void.createHistory.goforwards the option:opts.go(index, navigateOpts?.ignoreBlocker ?? false).gosetsskipBlockerNextPop = truewhenignoreBlocker— mirroring the existingback/forwardlines exactly.Tests
New
packages/history/tests/createBrowserHistory.test.tssets up two real jsdom session entries, registers a blocker, and drives an actualgo(-1)popstate — asserting the blocker runs forgo(-1)and is skipped forgo(-1, { ignoreBlocker: true }). This deliberately reaches the browserpopstatehandler (the gap called out on the prior PR #4872). Verified red→green (without the fix: 1 failed; with: 2 passed); full@tanstack/historysuite 27 passed,tsc --noEmitclean.Closes #4867
Summary by CodeRabbit
New Features
Bug Fixes