From a85c4f263ad25e71b678d321b4822775f49861a0 Mon Sep 17 00:00:00 2001 From: Antoine Moreaux Date: Thu, 6 Feb 2025 17:07:19 +0100 Subject: [PATCH] fix(settings routing): handle trailing slashes in base paths (#10055) Adjusted URL construction to properly handle trailing slashes in base paths, ensuring consistent matching logic. Added logic for setting the hotkey scope when navigating to the domain settings path. --- .../__tests__/useIsMatchingLocation.test.tsx | 93 +++++++++++++------ ...sePageChangeEffectNavigateLocation.test.ts | 6 +- .../src/hooks/useIsMatchingLocation.ts | 33 +++++-- .../usePageChangeEffectNavigateLocation.ts | 2 +- .../modules/apollo/hooks/useApolloFactory.ts | 2 +- .../effect-components/PageChangeEffect.tsx | 9 +- .../auth/sign-in-up/hooks/useSignInUp.ts | 2 +- .../hooks/__tests__/useShowAuthModal.test.tsx | 6 +- .../ui/layout/hooks/useShowAuthModal.ts | 2 +- .../modules/users/components/UserProvider.tsx | 2 +- 10 files changed, 106 insertions(+), 51 deletions(-) diff --git a/packages/twenty-front/src/hooks/__tests__/useIsMatchingLocation.test.tsx b/packages/twenty-front/src/hooks/__tests__/useIsMatchingLocation.test.tsx index 92aac9b81..b775f8988 100644 --- a/packages/twenty-front/src/hooks/__tests__/useIsMatchingLocation.test.tsx +++ b/packages/twenty-front/src/hooks/__tests__/useIsMatchingLocation.test.tsx @@ -1,39 +1,74 @@ -import { MemoryRouter } from 'react-router-dom'; import { renderHook } from '@testing-library/react'; +import { useIsMatchingLocation } from '../useIsMatchingLocation'; +import { MemoryRouter } from 'react-router-dom'; +import { AppBasePath } from '@/types/AppBasePath'; -import { useIsMatchingLocation } from '~/hooks/useIsMatchingLocation'; - -const Wrapper = ({ children }: { children: React.ReactNode }) => ( - - {children} - -); +const Wrapper = + (initialIndex = 0) => + ({ children }: { children: React.ReactNode }) => ( + + {children} + + ); describe('useIsMatchingLocation', () => { - it('should return true for a matching location', () => { - const { result } = renderHook( - () => { - const checkMatchingLocation = useIsMatchingLocation(); - return checkMatchingLocation('/two'); - }, - { wrapper: Wrapper }, - ); + it('returns true when paths match with no basePath', () => { + const { result } = renderHook(() => useIsMatchingLocation(), { + wrapper: Wrapper(), + }); - expect(result.current).toBe(true); + expect(result.current.isMatchingLocation('/example')).toBe(true); }); - it('should return false for a non-matching location', () => { - const { result } = renderHook( - () => { - const checkMatchingLocation = useIsMatchingLocation(); - return checkMatchingLocation('/four'); - }, - { wrapper: Wrapper }, - ); + it('returns false when paths do not match with no basePath', () => { + const { result } = renderHook(() => useIsMatchingLocation(), { + wrapper: Wrapper(), + }); - expect(result.current).toBe(false); + expect(result.current.isMatchingLocation('/non-match')).toBe(false); + }); + + it('returns true when paths match with basePath', () => { + const { result } = renderHook(() => useIsMatchingLocation(), { + wrapper: Wrapper(2), + }); + + expect( + result.current.isMatchingLocation('example', AppBasePath.Settings), + ).toBe(true); + }); + + it('returns false when paths do not match with basePath', () => { + const { result } = renderHook(() => useIsMatchingLocation(), { + wrapper: Wrapper(), + }); + + expect( + result.current.isMatchingLocation('non-match', AppBasePath.Settings), + ).toBe(false); + }); + + it('handles trailing slashes in basePath correctly', () => { + const { result } = renderHook(() => useIsMatchingLocation(), { + wrapper: Wrapper(2), + }); + + expect( + result.current.isMatchingLocation( + 'example', + (AppBasePath.Settings + '/') as AppBasePath, + ), + ).toBe(true); + }); + + it('handles without basePath correctly', () => { + const { result } = renderHook(() => useIsMatchingLocation(), { + wrapper: Wrapper(), + }); + + expect(result.current.isMatchingLocation('example')).toBe(true); }); }); diff --git a/packages/twenty-front/src/hooks/__tests__/usePageChangeEffectNavigateLocation.test.ts b/packages/twenty-front/src/hooks/__tests__/usePageChangeEffectNavigateLocation.test.ts index 57a78a5d9..89fa8ac42 100644 --- a/packages/twenty-front/src/hooks/__tests__/usePageChangeEffectNavigateLocation.test.ts +++ b/packages/twenty-front/src/hooks/__tests__/usePageChangeEffectNavigateLocation.test.ts @@ -30,9 +30,9 @@ jest.mock('~/hooks/useIsMatchingLocation'); const mockUseIsMatchingLocation = jest.mocked(useIsMatchingLocation); const setupMockIsMatchingLocation = (pathname: string) => { - mockUseIsMatchingLocation.mockReturnValueOnce( - (path: string) => path === pathname, - ); + mockUseIsMatchingLocation.mockReturnValueOnce({ + isMatchingLocation: (path: string) => path === pathname, + }); }; jest.mock('@/auth/hooks/useIsLogged'); diff --git a/packages/twenty-front/src/hooks/useIsMatchingLocation.ts b/packages/twenty-front/src/hooks/useIsMatchingLocation.ts index aa57dbb43..da23ae39b 100644 --- a/packages/twenty-front/src/hooks/useIsMatchingLocation.ts +++ b/packages/twenty-front/src/hooks/useIsMatchingLocation.ts @@ -1,19 +1,32 @@ -import { useCallback } from 'react'; import { matchPath, useLocation } from 'react-router-dom'; import { AppBasePath } from '@/types/AppBasePath'; +import { isNonEmptyString } from '@sniptt/guards'; +import { isDefined } from 'twenty-shared'; export const useIsMatchingLocation = () => { const location = useLocation(); - return useCallback( - (path: string, basePath?: AppBasePath) => { - const constructedPath = basePath - ? (new URL(basePath + path, document.location.origin).pathname ?? '') - : path; + const addTrailingSlash = (path: string) => + path.endsWith('/') ? path : path + '/'; - return !!matchPath(constructedPath, location.pathname); - }, - [location.pathname], - ); + const getConstructedPath = (path: string, basePath?: AppBasePath) => { + if (!isNonEmptyString(basePath)) return path; + + return addTrailingSlash(basePath) + path; + }; + + const isMatchingLocation = (path: string, basePath?: AppBasePath) => { + const match = matchPath( + getConstructedPath(path, basePath), + location.pathname, + ); + const isMatching = isDefined(match); + + return isMatching; + }; + + return { + isMatchingLocation, + }; }; diff --git a/packages/twenty-front/src/hooks/usePageChangeEffectNavigateLocation.ts b/packages/twenty-front/src/hooks/usePageChangeEffectNavigateLocation.ts index 111b9c573..76858a93e 100644 --- a/packages/twenty-front/src/hooks/usePageChangeEffectNavigateLocation.ts +++ b/packages/twenty-front/src/hooks/usePageChangeEffectNavigateLocation.ts @@ -8,7 +8,7 @@ import { OnboardingStatus } from '~/generated/graphql'; import { useIsMatchingLocation } from '~/hooks/useIsMatchingLocation'; export const usePageChangeEffectNavigateLocation = () => { - const isMatchingLocation = useIsMatchingLocation(); + const { isMatchingLocation } = useIsMatchingLocation(); const isLoggedIn = useIsLogged(); const onboardingStatus = useOnboardingStatus(); const isWorkspaceSuspended = useIsWorkspaceActivationStatusSuspended(); diff --git a/packages/twenty-front/src/modules/apollo/hooks/useApolloFactory.ts b/packages/twenty-front/src/modules/apollo/hooks/useApolloFactory.ts index 21f47bccc..52995c715 100644 --- a/packages/twenty-front/src/modules/apollo/hooks/useApolloFactory.ts +++ b/packages/twenty-front/src/modules/apollo/hooks/useApolloFactory.ts @@ -24,7 +24,7 @@ export const useApolloFactory = (options: Partial> = {}) => { const [isDebugMode] = useRecoilState(isDebugModeState); const navigate = useNavigate(); - const isMatchingLocation = useIsMatchingLocation(); + const { isMatchingLocation } = useIsMatchingLocation(); const [tokenPair, setTokenPair] = useRecoilState(tokenPairState); const [currentWorkspace, setCurrentWorkspace] = useRecoilState( currentWorkspaceState, diff --git a/packages/twenty-front/src/modules/app/effect-components/PageChangeEffect.tsx b/packages/twenty-front/src/modules/app/effect-components/PageChangeEffect.tsx index 674ce12a6..67ccb250c 100644 --- a/packages/twenty-front/src/modules/app/effect-components/PageChangeEffect.tsx +++ b/packages/twenty-front/src/modules/app/effect-components/PageChangeEffect.tsx @@ -29,7 +29,7 @@ import { usePageChangeEffectNavigateLocation } from '~/hooks/usePageChangeEffect // - moved usePageChangeEffectNavigateLocation into dedicated hook export const PageChangeEffect = () => { const navigate = useNavigate(); - const isMatchingLocation = useIsMatchingLocation(); + const { isMatchingLocation } = useIsMatchingLocation(); const [previousLocation, setPreviousLocation] = useState(''); @@ -140,6 +140,13 @@ export const PageChangeEffect = () => { }); break; } + case isMatchingLocation(SettingsPath.Domain, AppBasePath.Settings): { + setHotkeyScope(PageHotkeyScope.Settings, { + goto: false, + keyboardShortcutMenu: true, + }); + break; + } case isMatchingLocation( SettingsPath.WorkspaceMembersPage, AppBasePath.Settings, diff --git a/packages/twenty-front/src/modules/auth/sign-in-up/hooks/useSignInUp.ts b/packages/twenty-front/src/modules/auth/sign-in-up/hooks/useSignInUp.ts index 090aee8e3..e54d10bcc 100644 --- a/packages/twenty-front/src/modules/auth/sign-in-up/hooks/useSignInUp.ts +++ b/packages/twenty-front/src/modules/auth/sign-in-up/hooks/useSignInUp.ts @@ -24,7 +24,7 @@ export const useSignInUp = (form: UseFormReturn
) => { const [signInUpStep, setSignInUpStep] = useRecoilState(signInUpStepState); const [signInUpMode, setSignInUpMode] = useRecoilState(signInUpModeState); - const isMatchingLocation = useIsMatchingLocation(); + const { isMatchingLocation } = useIsMatchingLocation(); const workspaceInviteHash = useParams().workspaceInviteHash; const [searchParams] = useSearchParams(); diff --git a/packages/twenty-front/src/modules/ui/layout/hooks/__tests__/useShowAuthModal.test.tsx b/packages/twenty-front/src/modules/ui/layout/hooks/__tests__/useShowAuthModal.test.tsx index 565141864..613192b41 100644 --- a/packages/twenty-front/src/modules/ui/layout/hooks/__tests__/useShowAuthModal.test.tsx +++ b/packages/twenty-front/src/modules/ui/layout/hooks/__tests__/useShowAuthModal.test.tsx @@ -34,9 +34,9 @@ jest.mock('~/hooks/useIsMatchingLocation'); const mockUseIsMatchingLocation = jest.mocked(useIsMatchingLocation); const setupMockIsMatchingLocation = (pathname: string) => { - mockUseIsMatchingLocation.mockReturnValueOnce( - (path: string) => path === pathname, - ); + mockUseIsMatchingLocation.mockReturnValueOnce({ + isMatchingLocation: (path: string) => path === pathname, + }); }; const getResult = (isDefaultLayoutAuthModalVisible = true) => diff --git a/packages/twenty-front/src/modules/ui/layout/hooks/useShowAuthModal.ts b/packages/twenty-front/src/modules/ui/layout/hooks/useShowAuthModal.ts index ee9af1da5..d0f18ce25 100644 --- a/packages/twenty-front/src/modules/ui/layout/hooks/useShowAuthModal.ts +++ b/packages/twenty-front/src/modules/ui/layout/hooks/useShowAuthModal.ts @@ -11,7 +11,7 @@ import { OnboardingStatus, SubscriptionStatus } from '~/generated/graphql'; import { useIsMatchingLocation } from '~/hooks/useIsMatchingLocation'; export const useShowAuthModal = () => { - const isMatchingLocation = useIsMatchingLocation(); + const { isMatchingLocation } = useIsMatchingLocation(); const isLoggedIn = useIsLogged(); const onboardingStatus = useOnboardingStatus(); const subscriptionStatus = useSubscriptionStatus(); diff --git a/packages/twenty-front/src/modules/users/components/UserProvider.tsx b/packages/twenty-front/src/modules/users/components/UserProvider.tsx index 4528a1ea1..e3cdede57 100644 --- a/packages/twenty-front/src/modules/users/components/UserProvider.tsx +++ b/packages/twenty-front/src/modules/users/components/UserProvider.tsx @@ -10,7 +10,7 @@ import { UserOrMetadataLoader } from '~/loading/components/UserOrMetadataLoader' export const UserProvider = ({ children }: React.PropsWithChildren) => { const isCurrentUserLoaded = useRecoilValue(isCurrentUserLoadedState); - const isMatchingLocation = useIsMatchingLocation(); + const { isMatchingLocation } = useIsMatchingLocation(); const dateTimeFormat = useRecoilValue(dateTimeFormatState);