fix: disable navbar back and forward buttons at history boundaries#38947
fix: disable navbar back and forward buttons at history boundaries#38947schourasia750 wants to merge 5 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThe changes implement navigation history state tracking within the Router class and extend the RouterContext API to expose methods for querying back/forward navigation availability. The navbar component now subscribes to route changes, monitors history state, and conditionally disables back and forward buttons based on navigation capability. Changes
Sequence DiagramsequenceDiagram
participant User
participant NavBar as NavBar Component
participant Router
participant History as Browser History
User->>NavBar: Click Back Button
NavBar->>Router: router.navigate(-1)
Router->>History: history.go(-1)
History-->>Router: Route change event
Router->>Router: Update historyIndex state
Router-->>NavBar: subscribeToRouteChange callback
NavBar->>Router: getCanGoBack()
Router-->>NavBar: Return boolean (can navigate back)
NavBar->>NavBar: Update canGoBack state
NavBar->>NavBar: Disable/Enable Back button
NavBar-->>User: Render updated UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/navbar/NavBarNavigation.tsx`:
- Around line 32-45: The back/forward icons are inverted: in the
NavBarNavigation component swap the icon props on the two NavBarItem instances
so the Back button (onClick={() => router.navigate(-1)}, disabled={!canGoBack})
uses 'chevron-left' and the Forward button (onClick={() => router.navigate(1)},
disabled={!canGoForward}) uses 'chevron-right'; update the icon string values
passed to the NavBarItem components to match this mapping.
In `@apps/meteor/client/router/index.tsx`:
- Around line 177-185: The historyIndex can go negative when applying
pendingHistoryDelta; after applying pendingHistoryDelta in the router (symbols:
pendingHistoryDelta, historyIndex, historyMaxIndex) clamp historyIndex into the
valid range [0, historyMaxIndex] (e.g. set historyIndex = Math.max(0,
Math.min(historyIndex, historyMaxIndex))) before handling pendingPush so
getCanGoBack/getCanGoForward reflect the correct enabled state; leave the
pendingPush logic (increment and update historyMaxIndex) unchanged.
- Around line 293-296: The pendingHistoryDelta leak happens because
pendingHistoryDelta is set before calling history.go(delta) and never cleared if
history.go is a no-op; update the code in the branch that handles numeric
toOrDelta (the block that sets this.pendingHistoryDelta and calls history.go) to
only keep pendingHistoryDelta until navigation actually occurs: set a one-time
popstate listener (or reuse the existing refresh() trigger) to clear
this.pendingHistoryDelta when popstate fires and apply the delta to
historyIndex, and also register a short fallback timer to clear
pendingHistoryDelta if no popstate occurs within a small window (indicating
history.go was a no-op). This ensures pendingHistoryDelta, history.go,
refresh(), and historyIndex interactions are robust and prevents stale deltas
from being applied later.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/client/navbar/NavBarNavigation.tsxapps/meteor/client/router/index.tsxpackages/mock-providers/src/MockedRouterContext.tsxpackages/ui-contexts/src/RouterContext.ts
📜 Review details
⏰ 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: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/navbar/NavBarNavigation.tsxpackages/mock-providers/src/MockedRouterContext.tsxapps/meteor/client/router/index.tsxpackages/ui-contexts/src/RouterContext.ts
🧠 Learnings (1)
📚 Learning: 2025-09-15T13:10:30.049Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
Applied to files:
apps/meteor/client/router/index.tsx
🔇 Additional comments (3)
packages/ui-contexts/src/RouterContext.ts (1)
77-78: LGTM — clean API additions with safe defaults.Returning
false(instead of throwing as other unimplemented methods do) is the right choice here; it is a valid sentinel that prevents runtime crashes when the context is consumed outside a provider.Also applies to: 109-111
packages/mock-providers/src/MockedRouterContext.tsx (1)
19-21: LGTM — correctly placed before the spread.Declaring
getCanGoBack/getCanGoForwardbefore...routermeans consumers can still override them via therouterprop, matching the established pattern of all other mocked methods.apps/meteor/client/router/index.tsx (1)
339-341: LGTM — boundary conditions are correct.
historyIndex > 0andhistoryIndex < historyMaxIndexcorrectly model "has in-app back history" and "has in-app forward history" respectively.
| if (this.pendingHistoryDelta !== 0) { | ||
| this.historyIndex += this.pendingHistoryDelta; | ||
| this.pendingHistoryDelta = 0; | ||
| } | ||
| if (this.pendingPush) { | ||
| this.historyIndex += 1; | ||
| this.historyMaxIndex = this.historyIndex; | ||
| this.pendingPush = false; | ||
| } |
There was a problem hiding this comment.
historyIndex is not clamped after applying pendingHistoryDelta.
If pendingHistoryDelta pushes historyIndex below 0 (e.g., delta = -2, index = 1), historyIndex becomes -1. getCanGoBack() still correctly returns false (since -1 > 0 is false), but getCanGoForward() will return true as long as historyMaxIndex >= 0, producing an incorrect enabled state on the Forward button.
🛡️ Proposed fix: clamp after applying delta
if (this.pendingHistoryDelta !== 0) {
this.historyIndex += this.pendingHistoryDelta;
+ this.historyIndex = Math.max(0, Math.min(this.historyIndex, this.historyMaxIndex));
this.pendingHistoryDelta = 0;
}📝 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.
| if (this.pendingHistoryDelta !== 0) { | |
| this.historyIndex += this.pendingHistoryDelta; | |
| this.pendingHistoryDelta = 0; | |
| } | |
| if (this.pendingPush) { | |
| this.historyIndex += 1; | |
| this.historyMaxIndex = this.historyIndex; | |
| this.pendingPush = false; | |
| } | |
| if (this.pendingHistoryDelta !== 0) { | |
| this.historyIndex += this.pendingHistoryDelta; | |
| this.historyIndex = Math.max(0, Math.min(this.historyIndex, this.historyMaxIndex)); | |
| this.pendingHistoryDelta = 0; | |
| } | |
| if (this.pendingPush) { | |
| this.historyIndex += 1; | |
| this.historyMaxIndex = this.historyIndex; | |
| this.pendingPush = false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/router/index.tsx` around lines 177 - 185, The historyIndex
can go negative when applying pendingHistoryDelta; after applying
pendingHistoryDelta in the router (symbols: pendingHistoryDelta, historyIndex,
historyMaxIndex) clamp historyIndex into the valid range [0, historyMaxIndex]
(e.g. set historyIndex = Math.max(0, Math.min(historyIndex, historyMaxIndex)))
before handling pendingPush so getCanGoBack/getCanGoForward reflect the correct
enabled state; leave the pendingPush logic (increment and update
historyMaxIndex) unchanged.
ggazzo
added
the
stat: QA assured
2 participants
Summary
This PR fixes an issue where the Back and Forward navigation buttons in the navbar were not getting disabled when the user reached the beginning or end of the navigation history.
The buttons remained clickable even when no navigation was possible.
Changes Made
How to Test
Expected Result
Navigation buttons should reflect the correct enabled/disabled state based on available browser history.
Related Issue
Fixes #38945
Summary by CodeRabbit