Skip to content

Conversation

@KevinDavilaDotCMS
Copy link
Contributor

@KevinDavilaDotCMS KevinDavilaDotCMS commented Dec 30, 2025

style.editor.mechanis.mov
…chine functionality and code cleanup

- Added time machine capabilities to manage state history for optimistic updates in the style editor.
- Cleaned up code formatting and ensured consistent use of semicolons and newlines across various files.

These changes improve the maintainability and functionality of the style editor, allowing for better state management and user experience.
…gement

- Refactored the style editor form component to utilize a dedicated form builder service, enhancing form creation and management.
- Removed unused imports and cleaned up code for better readability.
- Updated methods for extracting and updating style properties to streamline functionality and improve performance.
- Ensured consistent handling of form values during rollback operations.

These changes enhance the maintainability and efficiency of the style editor, providing a more robust user experience.
…nger import

- Removed the test button from the edit-ema editor component to streamline the UI.
- Updated the import path for the UveIframeMessengerService to reflect its new location.
- Cleaned up console log statements in the contentlet tools component for better code clarity.
- Adjusted style properties handling in the GraphQL utility functions for consistency.

These changes enhance the maintainability of the code and improve the overall user experience in the edit-ema editor.
- Removed unused imports from the edit-ema editor component and style editor form builder service for improved clarity.
- Updated import paths for consistency across components, ensuring better maintainability.
- Streamlined the GraphQL utility functions by adjusting import statements, enhancing code readability.

These changes contribute to a cleaner codebase and facilitate easier future modifications.
…tyle-editor-for-real-time-headless-rendering
@KevinDavilaDotCMS KevinDavilaDotCMS changed the title 34085 task implement optimistic updates in style editor for real time headless rendering Dec 31, 2025
@KevinDavilaDotCMS KevinDavilaDotCMS marked this pull request as ready for review December 31, 2025 03:08
@nicobytes nicobytes requested a review from Copilot December 31, 2025 19:44
…tyle-editor-for-real-time-headless-rendering
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements optimistic updates for real-time headless rendering in the style editor. The implementation adds immediate visual feedback when users modify styles by updating the iframe instantly while debouncing the actual API save calls. A time machine feature provides history tracking and automatic rollback capabilities when save operations fail.

Key Changes:

  • Time machine feature for undo/rollback with deep cloning support for state snapshots
  • New iframe messenger service centralizing postMessage communication
  • Optimistic update pattern with automatic rollback on API failure
  • Form builder service extracting reactive form creation logic

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
public.ts Adds styleProperties field to DotCMSBasicContentlet interface
withTimeMachine.ts New time machine feature providing history tracking, undo/redo, and state navigation
withTimeMachine.spec.ts Comprehensive tests for time machine functionality
withLoad.ts Adds history tracking on page load
withEditor.ts Updates type signatures from ContentletPayload to ActionPayload
withEditor.spec.ts Updates test mocks to match new ActionPayload structure
withSave.ts Adds saveStyleEditor method with optimistic update rollback on failure
models.ts Adds StyleEditorProperties type and SaveStylePropertiesPayload interface
withClient.ts Integrates time machine for graphqlResponse with optimistic update methods
uve-iframe-messenger.service.ts New service centralizing iframe communication
uve-iframe-messenger.service.spec.ts Tests for iframe messenger service
dot-page-api.service.ts Adds saveStyleProperties API method
dot-page-api.service.spec.ts Tests for saveStyleProperties endpoint
edit-ema-editor.component.ts Refactors to use iframe messenger service throughout
style-editor-graphql.utils.ts Utilities for updating/extracting style properties from GraphQL responses
style-editor-form-builder.service.ts Service for building reactive forms from schemas
dot-uve-style-editor-form.component.ts Implements optimistic updates with immediate iframe sync and debounced saves
dot-uve-style-editor-form.component.spec.ts Updates tests with mocked dependencies
dot-uve-style-editor-form.component.scss Adds form actions styling
uve-style-editor-field-input.component.spec.ts Adds jest-dom import
dot-uve-contentlet-tools.component.ts Updates to emit ActionPayload instead of ContentletPayload
dot-uve-contentlet-tools.component.spec.ts Updates test expectations for ActionPayload
dot-uve-contentlet-tools.component.html Calls new setSelectedContent method
// Debounced subscription: Save to API
formValueChanges$
.pipe(
debounceTime(3000),
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

The debounce time is hardcoded to 3000ms (3 seconds). Consider extracting this as a configurable constant at the class or module level to make it easier to adjust based on testing and user feedback. For example, defining a constant like STYLE_SAVE_DEBOUNCE_MS = 3000 would improve maintainability.

Copilot uses AI. Check for mistakes.
Comment on lines +155 to +165
distinctUntilChanged((prev, curr) => JSON.stringify(prev) === JSON.stringify(curr))
)
.subscribe((formValues) => {
this.#updateIframeImmediately(formValues);
});

// Debounced subscription: Save to API
formValueChanges$
.pipe(
debounceTime(3000),
distinctUntilChanged((prev, curr) => JSON.stringify(prev) === JSON.stringify(curr))
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

Using JSON.stringify for deep equality comparison can be inefficient and unreliable (object key ordering issues). Consider using a proper deep equality function from a utility library or the store's built-in comparison. Additionally, the same comparison is performed twice in two different subscription chains - consider extracting this logic to avoid duplication.

Copilot uses AI. Check for mistakes.
imports: [AccordionModule, ButtonModule]
imports: [AccordionModule, ButtonModule],
providers: [
// mockProvider(UVEStore),
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

The commented-out code // mockProvider(UVEStore), should be removed. If the UVEStore mock provider is not needed, it should be deleted rather than left commented. Commented-out code reduces code clarity and maintainability.

Suggested change
// mockProvider(UVEStore),
Copilot uses AI. Check for mistakes.
Comment on lines +108 to +130
error: (error) => {
console.error('Error saving style properties:', error);

// Rollback the optimistic update
const rolledBack = store.rollbackGraphqlResponse();

if (!rolledBack) {
console.error(
'Failed to rollback optimistic update - no history available'
);

return;
}

// Update iframe with rolled back state
const rolledBackResponse = store.$customGraphqlResponse();
if (rolledBackResponse) {
iframeMessenger.sendPageData(rolledBackResponse);
}
console.warn(
'[TEST] Rolled back optimistic style update due to save failure'
);
}
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

The error handler in the tapResponse only logs to console but doesn't update the UI state or show any user-facing error message. When a style save fails and rollback occurs, the user should be notified through the UI (e.g., via a toast message) so they understand their changes weren't persisted. Consider adding a user-facing error notification using the MessageService or similar.

Copilot uses AI. Check for mistakes.
pageId: 'test-page-123',
containerIdentifier: 'container-id-456',
containerUUID: 'container-uuid-789',
contentledIdentifier: 'contentlet-id-abc',
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

There's a spelling error in the property name. The property is named "contentledIdentifier" but should be "contentletIdentifier" (missing 't'). This matches the typo in the interface definition.

Suggested change
contentledIdentifier: 'contentlet-id-abc',
contentletIdentifier: 'contentlet-id-abc',
Copilot uses AI. Check for mistakes.
Comment on lines 80 to 87
$reloadSchemaEffect = effect(() => {
const schema = this.$schema();
// This allow to preserve the current value on the form when the schema is reloaded.
// TODO: Remove untracked when we have the styleProperties in PageAPI response, also ensure that the form is rebuilt correctly.
const schema = untracked(() => this.$schema());
if (schema) {
this.#buildForm(schema);
this.#listenToFormChanges();
}
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

The TODO comment indicates that the untracked wrapper and this special handling should be removed once styleProperties are included in the PageAPI response. However, the corresponding test on line 232 is skipped (xdescribe) but the actual functionality in the component still runs. This creates a mismatch where untested code is active in production. Consider either enabling the test with the current behavior or adding a different test that validates the current untracked behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +153 to +154
store.addHistory({ pageAsset });

Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

The history is being added to the store in withLoad.ts, but the pageAsset data doesn't match the type expected by the time machine. The time machine in withClient is configured for ClientConfigState['graphqlResponse'] type, but here you're adding pageAsset which is of type DotCMSPageAsset. This type mismatch could cause issues. The addHistory call should be removed from here or the correct type should be used - likely the graphqlResponse should be added instead of pageAsset.

Suggested change
store.addHistory({ pageAsset });
Copilot uses AI. Check for mistakes.
pageId: string;
containerIdentifier: string;
containerUUID: string;
contentledIdentifier: string;
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

There's a spelling error in the parameter name. The parameter is named "contentledIdentifier" but should be "contentletIdentifier" (missing 't'). This typo appears in both the interface definition and JSDoc.

Suggested change
contentledIdentifier: string;
contentletIdentifier: string;
Copilot uses AI. Check for mistakes.
*/
saveStyleProperties({
containerIdentifier,
contentledIdentifier,
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

There's a spelling error in the parameter name. The parameter is named "contentledIdentifier" but should be "contentletIdentifier" (missing 't'). This typo is also used in the JSDoc comment on line 98.

Copilot uses AI. Check for mistakes.
// Use the store's saveStyleEditor method which handles API call and rollback on failure
this.#uveStore.saveStyleEditor({
containerIdentifier: activeContentlet.container.identifier,
contentledIdentifier: activeContentlet.contentlet.identifier,
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

There's a spelling error in the variable name. The variable is named "contentledIdentifier" but should be "contentletIdentifier" (missing 't'). This typo propagates throughout the payload construction.

Suggested change
contentledIdentifier: activeContentlet.contentlet.identifier,
contentletIdentifier: activeContentlet.contentlet.identifier,
Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants