Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,17 @@ describe('createComponent', () => {
});
});

it('omits viewports when creating a viewport-free component', async () => {
const viewportFreeArgs = { ...args, viewports: undefined };
mockComponentCreate.mockResolvedValue(mockComponent);

await createComponentTool(mockConfig)(viewportFreeArgs);

expect(mockComponentCreate.mock.calls[0][1]).not.toHaveProperty(
'viewports',
);
});

it('rejects writes to a protected environment', async () => {
const protectedConfig = createMockConfig({
protectedEnvironments: ['test-environment'],
Expand Down
19 changes: 14 additions & 5 deletions packages/mcp-tools/src/tools/exo/components/createComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ import {
TreeNodeSchema,
ExoMetadataSchema,
} from '../../../types/exoSchemas.js';
import {
asViewportOptionalCmaPayload,
type ViewportOptionalPayload,
} from '../../../types/cmaViewportCompatibility.js';
import type { ContentfulConfig } from '../../../config/types.js';

export const CreateComponentToolParams = BaseToolSchema.extend({
Expand All @@ -29,6 +33,7 @@ export const CreateComponentToolParams = BaseToolSchema.extend({
description: z.string().describe('Description of the component'),
viewports: z
.array(ViewportSchema)
.optional()
Comment thread
Chaoste marked this conversation as resolved.
.describe('Viewport definitions for the component (may be empty)'),
contentProperties: z
.array(ContentPropertySchema)
Expand Down Expand Up @@ -63,13 +68,15 @@ export function createComponentTool(config: ContentfulConfig) {
const componentData = {
name: args.name,
description: args.description,
viewports: args.viewports,
...(args.viewports !== undefined && { viewports: args.viewports }),
contentProperties: args.contentProperties,
designProperties: args.designProperties,
...(args.componentTree && { componentTree: args.componentTree }),
...(args.slots && { slots: args.slots }),
...(args.metadata && { metadata: args.metadata }),
};
} satisfies ViewportOptionalPayload<
Parameters<typeof contentfulClient.component.create>[1]
>;

// Create the component with or without an explicit ID. Providing an ID
// uses upsert (PUT) with no sys.version, which the CMA treats as a create.
Expand All @@ -80,14 +87,16 @@ export function createComponentTool(config: ContentfulConfig) {
environmentId: args.environmentId,
componentId: args.componentId,
},
{
asViewportOptionalCmaPayload<
Comment on lines 90 to +92

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant generic, duplicated conversion

The upsert branch re-wraps the already-validated componentData with an explicit generic that type inference supplies anyway — the same helper is called without type arguments at line 99 and in upsertComponent.ts (line 124), which also passes a sys-bearing payload. Keeping one conversion style avoids divergence between the create and upsert paths.

Code Review Run #609b4f


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

Parameters<typeof contentfulClient.component.upsert>[1]
>({
sys: { id: args.componentId, type: 'Component' },
...componentData,
},
}),
)
: await contentfulClient.component.create(
{ spaceId: args.spaceId, environmentId: args.environmentId },
componentData,
asViewportOptionalCmaPayload(componentData),
);

return createSuccessResponse('Component created successfully', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,20 @@ describe('upsertComponent', () => {
expect(body.dataAssemblies).toEqual(dataAssemblies);
});

it('does not reintroduce viewports from a viewport-free component', async () => {
mockComponentGet.mockResolvedValue({
...mockComponent,
viewports: undefined,
});
mockComponentUpsert.mockResolvedValue(mockComponent);

await upsertComponentTool(mockConfig)({ ...mockArgs, version: 1 });

expect(mockComponentUpsert.mock.calls[0][1]).not.toHaveProperty(
'viewports',
);
});

it('rejects a stale version', async () => {
mockComponentGet.mockResolvedValue(mockComponent); // sys.version === 1

Expand Down
19 changes: 16 additions & 3 deletions packages/mcp-tools/src/tools/exo/components/upsertComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ import {
TreeNodeSchema,
ExoMetadataSchema,
} from '../../../types/exoSchemas.js';
import {
asViewportOptionalCmaPayload,
type ViewportOptionalPayload,
} from '../../../types/cmaViewportCompatibility.js';
import type { ContentfulConfig } from '../../../config/types.js';

export const UpsertComponentToolParams = BaseToolSchema.extend({
Expand Down Expand Up @@ -86,15 +90,17 @@ export function upsertComponentTool(config: ContentfulConfig) {
);
}

const component = await contentfulClient.component.upsert(params, {
const componentData = {
sys: {
id: current.sys.id,
type: 'Component',
version: current.sys.version,
},
name: args.name ?? current.name,
description: args.description ?? current.description,
viewports: args.viewports ?? current.viewports,
...((args.viewports ?? current.viewports) !== undefined && {
viewports: args.viewports ?? current.viewports,
}),
Comment thread
Chaoste marked this conversation as resolved.
Outdated
contentProperties: args.contentProperties ?? current.contentProperties,
designProperties: args.designProperties ?? current.designProperties,
...((args.componentTree ?? current.componentTree)
Expand All @@ -109,7 +115,14 @@ export function upsertComponentTool(config: ContentfulConfig) {
...(current.dataAssemblies
? { dataAssemblies: current.dataAssemblies }
: {}),
});
} satisfies ViewportOptionalPayload<
Parameters<typeof contentfulClient.component.upsert>[1]
>;

const component = await contentfulClient.component.upsert(
params,
asViewportOptionalCmaPayload(componentData),
);

return createSuccessResponse('Component updated successfully', {
component,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,27 @@ describe('createExperienceFragment', () => {
);
});

it('creates a viewport-free experience fragment with flattened design properties', async () => {
const viewportFreeArgs = { ...baseArgs, viewports: undefined };
mockExperienceFragmentCreate.mockResolvedValue(mockExperienceFragment);

await createExperienceFragmentTool(mockConfig)({
...viewportFreeArgs,
designProperties: {
color: { type: 'ManualDesignValue', value: 'red' },
},
});

expect(mockExperienceFragmentCreate.mock.calls[0][1]).toMatchObject({
designProperties: {
color: { type: 'ManualDesignValue', value: 'red' },
},
});
expect(mockExperienceFragmentCreate.mock.calls[0][1]).not.toHaveProperty(
'viewports',
);
});

it('rejects creates in a protected environment', async () => {
const tool = createExperienceFragmentTool(
createMockConfig({ protectedEnvironments: ['test-environment'] }),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,16 @@ import {
import {
ViewportSchema,
ExperienceMetadataSchema,
DesignPropertyValueSchema,
DimensionedDesignPropertyValueSchema,
ExperienceContentBindingsSchema,
ExperienceSlotNodeSchema,
ComponentResourceLinkSchema,
} from '../../../types/exoSchemas.js';
import {
asViewportOptionalCmaPayloadWithFlattenedDesignProperties,
type ViewportOptionalPayloadWithFlattenedDesignProperties,
} from '../../../types/cmaViewportCompatibility.js';
import type { ContentfulConfig } from '../../../config/types.js';

export const CreateExperienceFragmentToolParams = BaseToolSchema.extend({
Expand All @@ -26,9 +31,16 @@ export const CreateExperienceFragmentToolParams = BaseToolSchema.extend({
),
viewports: z
.array(ViewportSchema)
.optional()
Comment thread
Chaoste marked this conversation as resolved.
.describe('Viewport definitions (may be empty)'),
designProperties: z
.record(z.string(), DimensionedDesignPropertyValueSchema)
.record(
z.string(),
z.union([
DesignPropertyValueSchema,
DimensionedDesignPropertyValueSchema,
]),
)
.describe(
'Design property values keyed by property ID (may be empty object)',
),
Expand All @@ -55,18 +67,24 @@ export function createExperienceFragmentTool(config: ContentfulConfig) {

const contentfulClient = createExoToolClient(config, args);

const experienceFragmentData = {
name: args.name,
description: args.description,
component: args.component,
...(args.viewports !== undefined && { viewports: args.viewports }),
designProperties: args.designProperties,
...(args.contentBindings && { contentBindings: args.contentBindings }),
...(args.slots && { slots: args.slots }),
...(args.metadata && { metadata: args.metadata }),
} satisfies ViewportOptionalPayloadWithFlattenedDesignProperties<
Parameters<typeof contentfulClient.experienceFragment.create>[1]
>;

const experienceFragment = await contentfulClient.experienceFragment.create(
{ spaceId: args.spaceId, environmentId: args.environmentId },
{
name: args.name,
description: args.description,
component: args.component,
viewports: args.viewports,
designProperties: args.designProperties,
...(args.contentBindings && { contentBindings: args.contentBindings }),
...(args.slots && { slots: args.slots }),
...(args.metadata && { metadata: args.metadata }),
},
asViewportOptionalCmaPayloadWithFlattenedDesignProperties(
experienceFragmentData,
),
);

return createSuccessResponse('Experience fragment created successfully', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,25 @@ describe('updateExperienceFragment', () => {
);
});

it('preserves flattened design properties without reintroducing viewports', async () => {
mockExperienceFragmentGet.mockResolvedValue({
...mockExperienceFragment,
viewports: undefined,
designProperties: {
color: { type: 'ManualDesignValue', value: 'red' },
},
});
mockExperienceFragmentUpsert.mockResolvedValue(mockExperienceFragment);

await updateExperienceFragmentTool(mockConfig)({ ...mockArgs, version: 1 });

const [, body] = mockExperienceFragmentUpsert.mock.calls[0];
Comment thread
Chaoste marked this conversation as resolved.
expect(body).not.toHaveProperty('viewports');
expect(body.designProperties).toEqual({
color: { type: 'ManualDesignValue', value: 'red' },
});
});

it('rejects a stale version', async () => {
mockExperienceFragmentGet.mockResolvedValue(mockExperienceFragment); // sys.version === 1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,15 @@ import {
import {
ViewportSchema,
ExperienceMetadataSchema,
DesignPropertyValueSchema,
DimensionedDesignPropertyValueSchema,
ExperienceContentBindingsSchema,
ExperienceSlotNodeSchema,
} from '../../../types/exoSchemas.js';
import {
asViewportOptionalCmaPayloadWithFlattenedDesignProperties,
type ViewportOptionalPayloadWithFlattenedDesignProperties,
} from '../../../types/cmaViewportCompatibility.js';
import type { ContentfulConfig } from '../../../config/types.js';

export const UpdateExperienceFragmentToolParams = BaseToolSchema.extend({
Expand All @@ -39,7 +44,13 @@ export const UpdateExperienceFragmentToolParams = BaseToolSchema.extend({
.optional()
.describe('Viewport definitions; replaces existing viewports if provided'),
designProperties: z
.record(z.string(), DimensionedDesignPropertyValueSchema)
.record(
z.string(),
z.union([
DesignPropertyValueSchema,
DimensionedDesignPropertyValueSchema,
]),
)
.optional()
.describe('Design property values; replaces existing if provided'),
contentBindings: ExperienceContentBindingsSchema.optional().describe(
Expand Down Expand Up @@ -87,28 +98,36 @@ export function updateExperienceFragmentTool(config: ContentfulConfig) {
// call is upsert(): the read-before-write guard above means this only ever updates
// an existing fragment, and `update` is the verb the tool surface exposes for that.
// Do not "fix" the tool name to match the SDK method.
const experienceFragmentData = {
sys: {
id: current.sys.id,
type: 'ExperienceFragment',
version: current.sys.version,
},
name: args.name ?? current.name,
description: args.description ?? current.description,
...((args.viewports ?? current.viewports) !== undefined && {
viewports: args.viewports ?? current.viewports,
}),
designProperties: args.designProperties ?? current.designProperties,
...((args.contentBindings ?? current.contentBindings)
? { contentBindings: args.contentBindings ?? current.contentBindings }
: {}),
...((args.slots ?? current.slots)
? { slots: args.slots ?? current.slots }
: {}),
...((args.metadata ?? current.metadata)
? { metadata: args.metadata ?? current.metadata }
: {}),
} satisfies ViewportOptionalPayloadWithFlattenedDesignProperties<
Parameters<typeof contentfulClient.experienceFragment.upsert>[1]
>;

const experienceFragment = await contentfulClient.experienceFragment.upsert(
params,
{
sys: {
id: current.sys.id,
type: 'ExperienceFragment',
version: current.sys.version,
},
name: args.name ?? current.name,
description: args.description ?? current.description,
viewports: args.viewports ?? current.viewports,
designProperties: args.designProperties ?? current.designProperties,
...((args.contentBindings ?? current.contentBindings)
? { contentBindings: args.contentBindings ?? current.contentBindings }
: {}),
...((args.slots ?? current.slots)
? { slots: args.slots ?? current.slots }
: {}),
...((args.metadata ?? current.metadata)
? { metadata: args.metadata ?? current.metadata }
: {}),
},
asViewportOptionalCmaPayloadWithFlattenedDesignProperties(
experienceFragmentData,
),
);

return createSuccessResponse('Experience fragment updated successfully', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@ describe('createExperienceTemplate', () => {
);
});

it('omits viewports when creating a viewport-free experience template', async () => {
const viewportFreeArgs = { ...createArgs, viewports: undefined };
mockExperienceTemplateCreate.mockResolvedValue(mockExperienceTemplate);

await createExperienceTemplateTool(mockConfig)(viewportFreeArgs);

expect(mockExperienceTemplateCreate.mock.calls[0][1]).not.toHaveProperty(
'viewports',
);
});

it('rejects writes to a protected environment', async () => {
const tool = createExperienceTemplateTool(
createMockConfig({ protectedEnvironments: ['test-environment'] }),
Expand Down
Loading
Loading