fix(navigation-menu-item): reject PAGE_LAYOUT items that don't reference a STANDALONE_PAGE layout (#22343)
## Issue A `PAGE_LAYOUT` navigation menu item can be created pointing at a page layout whose type is **not** `STANDALONE_PAGE` (e.g. a `DASHBOARD`). The sidebar always links such an item to `/page/<pageLayoutId>`, but that route only renders `STANDALONE_PAGE` layouts, anything else is redirected to 404. Result: a silently broken sidebar link (cc: https://discord.com/channels/1130383047699738754/1519045990047285288). ## Root cause - `/page/:pageLayoutId` is standalone-only by design (route guard in `usePageChangeEffectNavigateLocation`, and `StandalonePageLayoutPage` hardcodes `layoutType: STANDALONE_PAGE`). Dashboards/record pages are reached elsewhere (record show page). - A `PAGE_LAYOUT` nav item unconditionally computes `/page/<pageLayoutId>`. - No validation ensured the referenced layout is `STANDALONE_PAGE`: the migration/manifest validator only checked that `pageLayoutId` was present, the DB constraint only checked `NOT NULL`, and the runtime tool description even suggested pinning dashboards this way. So an app manifest pairing a `DASHBOARD` layout with a `PAGE_LAYOUT` nav item installed cleanly and produced a dead link. ## Fix (treat as invalid config — fail fast) - Cross-entity validation in `FlatNavigationMenuItemValidatorService` (both create and update): when `type === PAGE_LAYOUT`, resolve the referenced page layout from the optimistic page-layout maps and raise `INVALID_NAVIGATION_MENU_ITEM_INPUT` if its `type !== STANDALONE_PAGE`. Existence keeps being enforced by foreign-key resolution, so the type check only fires when the layout resolves. - Corrected the misleading `create_navigation_menu_item` tool description (no longer says "e.g. a dashboard"; states the target must be a `STANDALONE_PAGE`). - Added unit tests covering: `STANDALONE_PAGE` accepted; `DASHBOARD` rejected; `RECORD_PAGE` rejected; unresolved reference not flagged as a type error. ## Files changed - `flat-navigation-menu-item-validator.service.ts` — new `validatePageLayoutReference` + wired into create/update. - `create-navigation-menu-item.tool.ts` — tool description fix. - `__tests__/flat-navigation-menu-item-validator.service.spec.ts` — new tests (4 passing). ## Out of scope / follow-up - To open discussion, check https://github.com/twentyhq/twenty/pull/22255 <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/twentyhq/twenty/pull/22343?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
This commit is contained in:
Etienne
+230
@@ -0,0 +1,230 @@
|
||||
import { NavigationMenuItemType } from 'twenty-shared/types';
|
||||
|
||||
import { createEmptyFlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/constant/create-empty-flat-entity-maps.constant';
|
||||
import { validateNavigationMenuItemPageLayoutReferenceCrossEntity } from 'src/engine/metadata-modules/flat-navigation-menu-item/validators/utils/validate-navigation-menu-item-page-layout-reference-cross-entity.util';
|
||||
import { NavigationMenuItemExceptionCode } from 'src/engine/metadata-modules/navigation-menu-item/navigation-menu-item.exception';
|
||||
import { PageLayoutType } from 'src/engine/metadata-modules/page-layout/enums/page-layout-type.enum';
|
||||
|
||||
const NAVIGATION_MENU_ITEM_UNIVERSAL_IDENTIFIER =
|
||||
'00000000-0000-0000-0000-000000000001';
|
||||
const PAGE_LAYOUT_UNIVERSAL_IDENTIFIER = '00000000-0000-0000-0000-0000000000aa';
|
||||
const MISSING_PAGE_LAYOUT_UNIVERSAL_IDENTIFIER =
|
||||
'00000000-0000-0000-0000-0000000000bb';
|
||||
|
||||
const mapsFrom = (entities: { universalIdentifier: string }[]): any => {
|
||||
const maps = createEmptyFlatEntityMaps() as any;
|
||||
|
||||
for (const entity of entities) {
|
||||
maps.byUniversalIdentifier[entity.universalIdentifier] = entity;
|
||||
}
|
||||
|
||||
return maps;
|
||||
};
|
||||
|
||||
const pageLayout = (
|
||||
type: PageLayoutType,
|
||||
universalIdentifier = PAGE_LAYOUT_UNIVERSAL_IDENTIFIER,
|
||||
) => ({
|
||||
universalIdentifier,
|
||||
type,
|
||||
});
|
||||
|
||||
const navigationMenuItem = (
|
||||
pageLayoutUniversalIdentifier: string | null,
|
||||
universalIdentifier = NAVIGATION_MENU_ITEM_UNIVERSAL_IDENTIFIER,
|
||||
) => ({
|
||||
universalIdentifier,
|
||||
type: NavigationMenuItemType.PAGE_LAYOUT,
|
||||
pageLayoutUniversalIdentifier,
|
||||
});
|
||||
|
||||
const emptyActions = () => ({ create: [], update: [], delete: [] });
|
||||
|
||||
const reportFrom = ({
|
||||
createdNavigationMenuItemUniversalIdentifiers = [],
|
||||
updatedNavigationMenuItemUniversalIdentifiers = [],
|
||||
createdPageLayoutUniversalIdentifiers = [],
|
||||
updatedPageLayoutUniversalIdentifiers = [],
|
||||
}: {
|
||||
createdNavigationMenuItemUniversalIdentifiers?: string[];
|
||||
updatedNavigationMenuItemUniversalIdentifiers?: string[];
|
||||
createdPageLayoutUniversalIdentifiers?: string[];
|
||||
updatedPageLayoutUniversalIdentifiers?: string[];
|
||||
}): any => ({
|
||||
navigationMenuItem: {
|
||||
...emptyActions(),
|
||||
create: createdNavigationMenuItemUniversalIdentifiers.map(
|
||||
(universalIdentifier) => ({ flatEntity: { universalIdentifier } }),
|
||||
),
|
||||
update: updatedNavigationMenuItemUniversalIdentifiers.map(
|
||||
(universalIdentifier) => ({ universalIdentifier }),
|
||||
),
|
||||
},
|
||||
pageLayout: {
|
||||
...emptyActions(),
|
||||
create: createdPageLayoutUniversalIdentifiers.map(
|
||||
(universalIdentifier) => ({ flatEntity: { universalIdentifier } }),
|
||||
),
|
||||
update: updatedPageLayoutUniversalIdentifiers.map(
|
||||
(universalIdentifier) => ({
|
||||
universalIdentifier,
|
||||
}),
|
||||
),
|
||||
},
|
||||
});
|
||||
|
||||
const run = ({
|
||||
navigationMenuItems,
|
||||
pageLayouts,
|
||||
report,
|
||||
}: {
|
||||
navigationMenuItems: { universalIdentifier: string }[];
|
||||
pageLayouts: { universalIdentifier: string }[];
|
||||
report: any;
|
||||
}) =>
|
||||
validateNavigationMenuItemPageLayoutReferenceCrossEntity({
|
||||
optimisticUniversalFlatMaps: {
|
||||
flatNavigationMenuItemMaps: mapsFrom(navigationMenuItems),
|
||||
flatPageLayoutMaps: mapsFrom(pageLayouts),
|
||||
},
|
||||
orchestratorActionsReport: report,
|
||||
});
|
||||
|
||||
const errorCodes = (result: ReturnType<typeof run>) =>
|
||||
result.navigationMenuItem.flatMap((failed) =>
|
||||
failed.errors.map((error) => error.code),
|
||||
);
|
||||
|
||||
describe('validateNavigationMenuItemPageLayoutReferenceCrossEntity', () => {
|
||||
it('accepts a created PAGE_LAYOUT item referencing a same-migration STANDALONE_PAGE layout', () => {
|
||||
const result = run({
|
||||
navigationMenuItems: [
|
||||
navigationMenuItem(PAGE_LAYOUT_UNIVERSAL_IDENTIFIER),
|
||||
],
|
||||
pageLayouts: [pageLayout(PageLayoutType.STANDALONE_PAGE)],
|
||||
report: reportFrom({
|
||||
createdNavigationMenuItemUniversalIdentifiers: [
|
||||
NAVIGATION_MENU_ITEM_UNIVERSAL_IDENTIFIER,
|
||||
],
|
||||
createdPageLayoutUniversalIdentifiers: [
|
||||
PAGE_LAYOUT_UNIVERSAL_IDENTIFIER,
|
||||
],
|
||||
}),
|
||||
});
|
||||
|
||||
expect(errorCodes(result)).not.toContain(
|
||||
NavigationMenuItemExceptionCode.INVALID_NAVIGATION_MENU_ITEM_INPUT,
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects a created PAGE_LAYOUT item referencing a same-migration DASHBOARD layout', () => {
|
||||
const result = run({
|
||||
navigationMenuItems: [
|
||||
navigationMenuItem(PAGE_LAYOUT_UNIVERSAL_IDENTIFIER),
|
||||
],
|
||||
pageLayouts: [pageLayout(PageLayoutType.DASHBOARD)],
|
||||
report: reportFrom({
|
||||
createdNavigationMenuItemUniversalIdentifiers: [
|
||||
NAVIGATION_MENU_ITEM_UNIVERSAL_IDENTIFIER,
|
||||
],
|
||||
createdPageLayoutUniversalIdentifiers: [
|
||||
PAGE_LAYOUT_UNIVERSAL_IDENTIFIER,
|
||||
],
|
||||
}),
|
||||
});
|
||||
|
||||
expect(errorCodes(result)).toContain(
|
||||
NavigationMenuItemExceptionCode.INVALID_NAVIGATION_MENU_ITEM_INPUT,
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects a created PAGE_LAYOUT item referencing a same-migration RECORD_PAGE layout', () => {
|
||||
const result = run({
|
||||
navigationMenuItems: [
|
||||
navigationMenuItem(PAGE_LAYOUT_UNIVERSAL_IDENTIFIER),
|
||||
],
|
||||
pageLayouts: [pageLayout(PageLayoutType.RECORD_PAGE)],
|
||||
report: reportFrom({
|
||||
createdNavigationMenuItemUniversalIdentifiers: [
|
||||
NAVIGATION_MENU_ITEM_UNIVERSAL_IDENTIFIER,
|
||||
],
|
||||
createdPageLayoutUniversalIdentifiers: [
|
||||
PAGE_LAYOUT_UNIVERSAL_IDENTIFIER,
|
||||
],
|
||||
}),
|
||||
});
|
||||
|
||||
expect(errorCodes(result)).toContain(
|
||||
NavigationMenuItemExceptionCode.INVALID_NAVIGATION_MENU_ITEM_INPUT,
|
||||
);
|
||||
});
|
||||
|
||||
it('does not raise a type error when the referenced layout cannot be resolved', () => {
|
||||
const result = run({
|
||||
navigationMenuItems: [
|
||||
navigationMenuItem(MISSING_PAGE_LAYOUT_UNIVERSAL_IDENTIFIER),
|
||||
],
|
||||
pageLayouts: [pageLayout(PageLayoutType.STANDALONE_PAGE)],
|
||||
report: reportFrom({
|
||||
createdNavigationMenuItemUniversalIdentifiers: [
|
||||
NAVIGATION_MENU_ITEM_UNIVERSAL_IDENTIFIER,
|
||||
],
|
||||
}),
|
||||
});
|
||||
|
||||
expect(errorCodes(result)).not.toContain(
|
||||
NavigationMenuItemExceptionCode.INVALID_NAVIGATION_MENU_ITEM_INPUT,
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects an untouched item when its layout type is updated to a non-standalone type in the same migration', () => {
|
||||
const result = run({
|
||||
navigationMenuItems: [
|
||||
navigationMenuItem(PAGE_LAYOUT_UNIVERSAL_IDENTIFIER),
|
||||
],
|
||||
pageLayouts: [pageLayout(PageLayoutType.RECORD_PAGE)],
|
||||
report: reportFrom({
|
||||
updatedPageLayoutUniversalIdentifiers: [
|
||||
PAGE_LAYOUT_UNIVERSAL_IDENTIFIER,
|
||||
],
|
||||
}),
|
||||
});
|
||||
|
||||
expect(errorCodes(result)).toContain(
|
||||
NavigationMenuItemExceptionCode.INVALID_NAVIGATION_MENU_ITEM_INPUT,
|
||||
);
|
||||
});
|
||||
|
||||
it('accepts an updated item whose layout is concurrently updated to STANDALONE_PAGE (no false positive)', () => {
|
||||
const result = run({
|
||||
navigationMenuItems: [
|
||||
navigationMenuItem(PAGE_LAYOUT_UNIVERSAL_IDENTIFIER),
|
||||
],
|
||||
pageLayouts: [pageLayout(PageLayoutType.STANDALONE_PAGE)],
|
||||
report: reportFrom({
|
||||
updatedNavigationMenuItemUniversalIdentifiers: [
|
||||
NAVIGATION_MENU_ITEM_UNIVERSAL_IDENTIFIER,
|
||||
],
|
||||
updatedPageLayoutUniversalIdentifiers: [
|
||||
PAGE_LAYOUT_UNIVERSAL_IDENTIFIER,
|
||||
],
|
||||
}),
|
||||
});
|
||||
|
||||
expect(errorCodes(result)).not.toContain(
|
||||
NavigationMenuItemExceptionCode.INVALID_NAVIGATION_MENU_ITEM_INPUT,
|
||||
);
|
||||
});
|
||||
|
||||
it('ignores items that are neither touched nor pointing to a touched layout', () => {
|
||||
const result = run({
|
||||
navigationMenuItems: [
|
||||
navigationMenuItem(PAGE_LAYOUT_UNIVERSAL_IDENTIFIER),
|
||||
],
|
||||
pageLayouts: [pageLayout(PageLayoutType.DASHBOARD)],
|
||||
report: reportFrom({}),
|
||||
});
|
||||
|
||||
expect(errorCodes(result)).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
+128
@@ -0,0 +1,128 @@
|
||||
import { msg, t } from '@lingui/core/macro';
|
||||
import { NavigationMenuItemType } from 'twenty-shared/types';
|
||||
import { isDefined } from 'twenty-shared/utils';
|
||||
|
||||
import { findFlatEntityByUniversalIdentifier } from 'src/engine/metadata-modules/flat-entity/utils/find-flat-entity-by-universal-identifier.util';
|
||||
import { NavigationMenuItemExceptionCode } from 'src/engine/metadata-modules/navigation-menu-item/navigation-menu-item.exception';
|
||||
import { PageLayoutType } from 'src/engine/metadata-modules/page-layout/enums/page-layout-type.enum';
|
||||
import {
|
||||
type OrchestratorActionsReport,
|
||||
type OrchestratorFailureReport,
|
||||
} from 'src/engine/workspace-manager/workspace-migration/types/workspace-migration-orchestrator.type';
|
||||
import { type AllUniversalFlatEntityMaps } from 'src/engine/workspace-manager/workspace-migration/universal-flat-entity/types/all-universal-flat-entity-maps.type';
|
||||
import { getEmptyFlatEntityValidationError } from 'src/engine/workspace-manager/workspace-migration/workspace-migration-builder/builders/utils/get-flat-entity-validation-error.util';
|
||||
|
||||
export const validateNavigationMenuItemPageLayoutReferenceCrossEntity = ({
|
||||
optimisticUniversalFlatMaps,
|
||||
orchestratorActionsReport,
|
||||
}: {
|
||||
optimisticUniversalFlatMaps: Pick<
|
||||
AllUniversalFlatEntityMaps,
|
||||
'flatNavigationMenuItemMaps' | 'flatPageLayoutMaps'
|
||||
>;
|
||||
orchestratorActionsReport: Pick<
|
||||
OrchestratorActionsReport,
|
||||
'navigationMenuItem' | 'pageLayout'
|
||||
>;
|
||||
}): Pick<OrchestratorFailureReport, 'navigationMenuItem'> => {
|
||||
const validationErrors: Pick<
|
||||
OrchestratorFailureReport,
|
||||
'navigationMenuItem'
|
||||
> = {
|
||||
navigationMenuItem: [],
|
||||
};
|
||||
|
||||
const createdNavigationMenuItemUniversalIdentifiers = new Set(
|
||||
orchestratorActionsReport.navigationMenuItem.create.map(
|
||||
(action) => action.flatEntity.universalIdentifier,
|
||||
),
|
||||
);
|
||||
|
||||
const updatedNavigationMenuItemUniversalIdentifiers = new Set<string>(
|
||||
orchestratorActionsReport.navigationMenuItem.update.map(
|
||||
(action) => action.universalIdentifier,
|
||||
),
|
||||
);
|
||||
|
||||
const touchedPageLayoutUniversalIdentifiers = new Set<string>([
|
||||
...orchestratorActionsReport.pageLayout.create.map(
|
||||
(action) => action.flatEntity.universalIdentifier,
|
||||
),
|
||||
...orchestratorActionsReport.pageLayout.update.map(
|
||||
(action) => action.universalIdentifier,
|
||||
),
|
||||
]);
|
||||
|
||||
const navigationMenuItemUniversalIdentifiersToValidate = new Set<string>([
|
||||
...createdNavigationMenuItemUniversalIdentifiers,
|
||||
...updatedNavigationMenuItemUniversalIdentifiers,
|
||||
]);
|
||||
|
||||
if (touchedPageLayoutUniversalIdentifiers.size > 0) {
|
||||
for (const navigationMenuItem of Object.values(
|
||||
optimisticUniversalFlatMaps.flatNavigationMenuItemMaps
|
||||
.byUniversalIdentifier,
|
||||
)) {
|
||||
if (
|
||||
isDefined(navigationMenuItem) &&
|
||||
isDefined(navigationMenuItem.pageLayoutUniversalIdentifier) &&
|
||||
touchedPageLayoutUniversalIdentifiers.has(
|
||||
navigationMenuItem.pageLayoutUniversalIdentifier,
|
||||
)
|
||||
) {
|
||||
navigationMenuItemUniversalIdentifiersToValidate.add(
|
||||
navigationMenuItem.universalIdentifier,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
for (const universalIdentifier of navigationMenuItemUniversalIdentifiersToValidate) {
|
||||
const navigationMenuItem = findFlatEntityByUniversalIdentifier({
|
||||
universalIdentifier,
|
||||
flatEntityMaps: optimisticUniversalFlatMaps.flatNavigationMenuItemMaps,
|
||||
});
|
||||
|
||||
if (
|
||||
!isDefined(navigationMenuItem) ||
|
||||
navigationMenuItem.type !== NavigationMenuItemType.PAGE_LAYOUT ||
|
||||
!isDefined(navigationMenuItem.pageLayoutUniversalIdentifier)
|
||||
) {
|
||||
continue;
|
||||
}
|
||||
|
||||
const referencedPageLayout = findFlatEntityByUniversalIdentifier({
|
||||
universalIdentifier: navigationMenuItem.pageLayoutUniversalIdentifier,
|
||||
flatEntityMaps: optimisticUniversalFlatMaps.flatPageLayoutMaps,
|
||||
});
|
||||
|
||||
if (
|
||||
!isDefined(referencedPageLayout) ||
|
||||
referencedPageLayout.type === PageLayoutType.STANDALONE_PAGE
|
||||
) {
|
||||
continue;
|
||||
}
|
||||
|
||||
const failedValidation = getEmptyFlatEntityValidationError({
|
||||
flatEntityMinimalInformation: {
|
||||
universalIdentifier: navigationMenuItem.universalIdentifier,
|
||||
},
|
||||
metadataName: 'navigationMenuItem',
|
||||
type: createdNavigationMenuItemUniversalIdentifiers.has(
|
||||
universalIdentifier,
|
||||
)
|
||||
? 'create'
|
||||
: 'update',
|
||||
});
|
||||
|
||||
failedValidation.errors.push({
|
||||
code: NavigationMenuItemExceptionCode.INVALID_NAVIGATION_MENU_ITEM_INPUT,
|
||||
message: t`PAGE_LAYOUT navigation menu item must reference a STANDALONE_PAGE page layout`,
|
||||
userFriendlyMessage: msg`A page layout navigation menu item can only point to a standalone page`,
|
||||
});
|
||||
|
||||
validationErrors.navigationMenuItem.push(failedValidation);
|
||||
}
|
||||
|
||||
return validationErrors;
|
||||
};
|
||||
Reference in New Issue
Block a user