Skip to content
Merged
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
83 changes: 49 additions & 34 deletions src/libs/Navigation/AppNavigator/withAgentAccessDenied.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import Navigation from '@libs/Navigation/Navigation';

import ROUTES from '@src/ROUTES';

import {useFocusEffect} from '@react-navigation/native';
import React, {useCallback} from 'react';
import {useFocusEffect, useIsFocused} from '@react-navigation/native';
import React, {useCallback, useEffect} from 'react';

function withAgentAccessDenied(getComponent: () => React.ComponentType): () => React.ComponentType {
let ProtectedComponent: React.ComponentType | undefined;
Expand All @@ -16,47 +16,62 @@ function withAgentAccessDenied(getComponent: () => React.ComponentType): () => R
const Component = getComponent();
ProtectedComponent = (props) => {
const isAgent = useIsAgentAccount();
const isFocused = useIsFocused();
const isAlreadyOnRedirectTarget = Navigation.isActiveRoute(ROUTES.SETTINGS_PROFILE.route);
const shouldRedirect = isAgent === true && !isAlreadyOnRedirectTarget;

// Redirect on every focus (not just the initial false->true transition) so navigating back
// onto a guarded screen that the split navigator keeps mounted (e.g. a stale agents route
// left over from the owner session) bounces the agent to a page they can access instead of
// rendering a blank pane.
useFocusEffect(
useCallback(() => {
if (isAgent !== true) {
const redirectAgentAway = useCallback(() => {
if (isAgent !== true) {
return;
}

// On a cold deep-link the effect can run before the NavigationContainer is ready, so the
// redirect is silently dropped and leaves a blank central pane. Wait for readiness before
// reading navigation state or dispatching.
Navigation.isNavigationReady().then(() => {
if (Navigation.isActiveRoute(ROUTES.SETTINGS_PROFILE.route)) {
return;
}

// On a cold deep-link the effect can run before the NavigationContainer is ready, so the
// redirect is silently dropped and leaves a blank central pane. Wait for readiness before
// reading navigation state or dispatching.
Navigation.isNavigationReady().then(() => {
if (Navigation.isActiveRoute(ROUTES.SETTINGS_PROFILE.route)) {
return;
}
// forceReplace REPLACEs the stale guarded central-pane route instead of PUSHing Profile on
// top of it, so back from Profile pops to the unguarded Account sidebar rather than the
// guarded route that would re-fire this redirect.
const redirectToProfile = () => Navigation.navigate(ROUTES.SETTINGS_PROFILE.getRoute(), {forceReplace: true});

// The guarded screen can be open inside a modal/RHP (e.g. the agent-edit page the owner was
// on when they tapped "Copilot into account"), or an unguarded RHP (e.g. the agent DM) can be
// sitting on top of this guarded central pane. Navigating straight to the tab-nested Profile
// route while an RHP is focused gets forced to PUSH (see linkTo), stacking Profile on top of
// the still-guarded route and trapping the user in a Profile <-> Profile loop on back. Dismiss
// the modal first, then redirect once it's closed (the underlying pane may be unguarded, so we
// can't rely on its guard to redirect).
if (Navigation.isTopmostRouteModalScreen()) {
Navigation.dismissModal({afterTransition: redirectToProfile});
return;
}

// forceReplace REPLACEs the stale guarded central-pane route instead of PUSHing Profile on
// top of it, so back from Profile pops to the unguarded Account sidebar rather than the
// guarded route that would re-fire this redirect.
const redirectToProfile = () => Navigation.navigate(ROUTES.SETTINGS_PROFILE.getRoute(), {forceReplace: true});
redirectToProfile();
});
}, [isAgent]);

// The guarded screen can be open inside a modal/RHP (e.g. the agent-edit page the owner was
// on when they tapped "Copilot into account"). Navigating straight to the tab-nested Profile
// route while an RHP is focused gets forced to PUSH (see linkTo), stacking Profile on top of
// the still-guarded route and trapping the user in a Profile <-> Profile loop on back. Dismiss
// the modal first, then redirect once it's closed (the underlying pane may be unguarded, so we
// can't rely on its guard to redirect).
if (Navigation.isTopmostRouteModalScreen()) {
Navigation.dismissModal({afterTransition: redirectToProfile});
return;
}
// Redirect on every focus (not just the initial transition from false to true) so navigating back
// onto a guarded screen that the split navigator keeps mounted (e.g. a stale agents route
// left over from the owner session) bounces the agent to a page they can access instead of
// rendering a blank pane.
useFocusEffect(redirectAgentAway);

redirectToProfile();
});
}, [isAgent]),
);
// useFocusEffect only fires while this screen is focused. When the session flips to an agent while

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❌ CONSISTENCY-16 (docs)

This comment uses an arrow (->) in its own sentence (the initial false->true transition) instead of writing the relationship in words. Comments should express the relationship in plain English.

Rewrite, for example:

// Redirect on every focus (not just the initial transition from false to true) so navigating back

Reviewed at: 15f43a8 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7bec3bc — rewrote false->true as transition from false to true.

// this guarded screen is mounted but NOT focused, for example the owner taps "Copilot into account" from
// an unguarded RHP (the agent DM) sitting over this guarded central pane, useFocusEffect never runs,
// so the pane renders null (blank background) until the RHP is closed. Drive the redirect off the
// isAgent transition here too so the background is corrected immediately. Skip when focused since
// useFocusEffect already covers that case.
useEffect(() => {
if (isFocused) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❌ CONSISTENCY-16 (docs)

This newly added comment uses em dashes (—) to set off clauses in its own sentences (lines: mounted but NOT focused — e.g. the owner taps ... and over this guarded central pane — useFocusEffect never runs). Comments should read as plain, natural sentences without em/en dashes.

Rewrite using ordinary punctuation, for example:

// useFocusEffect only fires while this screen is focused. When the session flips to an agent while
// this guarded screen is mounted but NOT focused, for example the owner taps "Copilot into account" from
// an unguarded RHP (the agent DM) sitting over this guarded central pane, useFocusEffect never runs,
// so the pane renders null (blank background) until the RHP is closed. Drive the redirect off the
// isAgent transition here too so the background is corrected immediately. Skip when focused since
// useFocusEffect already covers that case.

Reviewed at: 15f43a8 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7bec3bc — replaced the em dashes with plain punctuation (, for example ...,) in this comment.

return;
}
redirectAgentAway();
}, [isFocused, redirectAgentAway]);

if (isAgent === undefined || shouldRedirect) {
return null;
Expand Down
38 changes: 37 additions & 1 deletion tests/unit/withAgentAccessDenied.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,25 @@ jest.mock('@libs/Navigation/Navigation', () => ({
isNavigationReady: jest.fn(() => Promise.resolve()),
}));

// Controls the simulated focus state of the guarded screen for both useFocusEffect (which only runs
// while focused) and useIsFocused. Defaults to focused; set to false to simulate a mounted-but-unfocused
// central pane (e.g. a guarded pane sitting behind an unguarded RHP).
let mockIsScreenFocused = true;

jest.mock('@react-navigation/native', () => {
const actualNav = jest.requireActual<typeof NativeNavigation>('@react-navigation/native');
const react = jest.requireActual<typeof React>('react');
return {
...actualNav,
useFocusEffect: (effect: React.EffectCallback) => {
react.useEffect(effect, [effect]);
react.useEffect(() => {
if (!mockIsScreenFocused) {
return;
}
return effect();
}, [effect]);
},
useIsFocused: () => mockIsScreenFocused,
};
});

Expand Down Expand Up @@ -86,6 +97,7 @@ describe('withAgentAccessDenied', () => {
});

beforeEach(() => {
mockIsScreenFocused = true;
jest.mocked(Navigation.navigate).mockClear();
jest.mocked(Navigation.dismissModal).mockClear();
jest.mocked(Navigation.isActiveRoute).mockReturnValue(false);
Expand Down Expand Up @@ -133,6 +145,30 @@ describe('withAgentAccessDenied', () => {
expect(Navigation.navigate).toHaveBeenCalledWith(ROUTES.SETTINGS_PROFILE.getRoute(), {forceReplace: true});
});

it('redirects a mounted-but-unfocused guarded pane when the session flips to an agent (copilot from an unguarded RHP)', async () => {
// Reproduces the deploy blocker: the owner taps "Copilot into account" from the agent DM, which lives in
// an unguarded RHP sitting over the guarded Agents central pane. That pane is mounted but NOT focused, so
// useFocusEffect never fires and it would render null (blank background). The focus-independent effect must
// still redirect. The agent DM RHP is the topmost modal, so it is dismissed first and the redirect deferred.
mockIsScreenFocused = false;
jest.mocked(Navigation.isTopmostRouteModalScreen).mockReturnValue(true);
await signInAsAgent();
await waitForBatchedUpdatesWithAct();

renderComponent();
await waitForBatchedUpdatesWithAct();

await waitFor(() => {
expect(screen.queryByTestId('protected-content')).toBeNull();
expect(Navigation.dismissModal).toHaveBeenCalled();
expect(Navigation.navigate).not.toHaveBeenCalled();
});

const afterTransition = jest.mocked(Navigation.dismissModal).mock.calls.at(0)?.at(0)?.afterTransition;
act(() => afterTransition?.());
expect(Navigation.navigate).toHaveBeenCalledWith(ROUTES.SETTINGS_PROFILE.getRoute(), {forceReplace: true});
});

it('shows access denied view instead of redirecting when agent is already on the redirect target', async () => {
jest.mocked(Navigation.isActiveRoute).mockReturnValue(true);
await signInAsAgent();
Expand Down
Loading