Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions apps/meteor/client/navbar/NavBarNavigation.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,25 @@
import { NavBarGroup, NavBarItem, Box } from '@rocket.chat/fuselage';
import { useLayout, useRouter } from '@rocket.chat/ui-contexts';
import { FocusScope } from 'react-aria';
import { useEffect, useState } from 'react';
import { useTranslation } from 'react-i18next';

import NavBarSearch from './NavBarSearch';

const NavbarNavigation = () => {
const { t } = useTranslation();
const { navigate } = useRouter();
const router = useRouter();
const { isMobile } = useLayout();
const [, setRouteTick] = useState(0);

useEffect(() => {
return router.subscribeToRouteChange(() => {
setRouteTick((t) => t + 1);
});
}, [router]);

const canGoBack = router.getCanGoBack();
const canGoForward = router.getCanGoForward();

return (
<Box display='flex' flexGrow={1} justifyContent='center'>
Expand All @@ -18,8 +29,20 @@ const NavbarNavigation = () => {
{!isMobile && (
<Box mie={8}>
<NavBarGroup aria-label={t('History_navigation')}>
<NavBarItem title={t('Back_in_history')} onClick={() => navigate(-1)} icon='chevron-right' small />
<NavBarItem title={t('Forward_in_history')} onClick={() => navigate(1)} icon='chevron-left' small />
<NavBarItem
title={t('Back_in_history')}
onClick={() => router.navigate(-1)}
icon='chevron-right'
small
disabled={!canGoBack}
/>
<NavBarItem
title={t('Forward_in_history')}
onClick={() => router.navigate(1)}
icon='chevron-left'
small
disabled={!canGoForward}
/>
</NavBarGroup>
</Box>
)}
Expand Down
24 changes: 24 additions & 0 deletions apps/meteor/client/router/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@ export class Router implements RouterContextValue {

private readonly page = new Page();

private historyIndex = 0;

private historyMaxIndex = 0;

private pendingHistoryDelta = 0;

private pendingPush = false;

constructor() {
this.registerRoute('*', { id: 'not-found' });
this.updateCallbacks();
Expand Down Expand Up @@ -166,6 +174,16 @@ export class Router implements RouterContextValue {
}

private refresh() {
if (this.pendingHistoryDelta !== 0) {
this.historyIndex += this.pendingHistoryDelta;
this.pendingHistoryDelta = 0;
}
if (this.pendingPush) {
this.historyIndex += 1;
this.historyMaxIndex = this.historyIndex;
this.pendingPush = false;
}
Comment on lines +177 to +185
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.


if (!this.current?.route) return;

const currentContext = this.current;
Expand Down Expand Up @@ -273,6 +291,7 @@ export class Router implements RouterContextValue {
},
) => {
if (typeof toOrDelta === 'number') {
this.pendingHistoryDelta = toOrDelta;
history.go(toOrDelta);
return;
}
Expand All @@ -283,6 +302,7 @@ export class Router implements RouterContextValue {
if (options?.replace) {
history.replaceState(state, '', path);
} else {
this.pendingPush = true;
history.pushState(state, '', path);
}

Expand Down Expand Up @@ -315,6 +335,10 @@ export class Router implements RouterContextValue {
readonly getRoomRoute = (roomType: RoomType, routeData: RoomRouteData) => {
return { path: roomCoordinator.getRouteLink(roomType, routeData) || '/' };
};

readonly getCanGoBack = (): boolean => this.historyIndex > 0;

readonly getCanGoForward = (): boolean => this.historyIndex < this.historyMaxIndex;
}

type RouteOptions = {
Expand Down
2 changes: 2 additions & 0 deletions packages/mock-providers/src/MockedRouterContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ export const MockedRouterContext = ({ children, router }: { children: ReactNode;
navigate: () => undefined,
defineRoutes: () => () => undefined,
getRoomRoute: () => ({ path: '/' }),
getCanGoBack: () => false,
getCanGoForward: () => false,
...router,
}}
>
Expand Down
4 changes: 4 additions & 0 deletions packages/ui-contexts/src/RouterContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ export type RouterContextValue = {
): {
path: LocationPathname;
};
getCanGoBack(): boolean;
getCanGoForward(): boolean;
};

export const RouterContext = createContext<RouterContextValue>({
Expand Down Expand Up @@ -104,4 +106,6 @@ export const RouterContext = createContext<RouterContextValue>({
getRoomRoute: () => {
throw new Error('not implemented');
},
getCanGoBack: () => false,
getCanGoForward: () => false,
});