fix: undo color change functionality for elements not working

[Nithishvb]

Description

This PR addresses an issue where the undo functionality did not correctly revert color changes on UI elements.

Related Issues

closes #1763

Type of Change

• Bug fix
• New feature
• Documentation update
• Release
• Refactor
• Other (please describe):

Testing

Screenshots (if applicable)

Additional Notes

---

Important

Fixes undo functionality for color changes by storing and using original styles in HistoryManager.

• Behavior:
  • Fixes undo functionality for color changes in HistoryManager by storing original styles in originalStyleMap.
  • Updates push() to use original styles when undoing update-style actions.
• Data Structures:
  • Adds originalStyleMap to HistoryManager to track original styles for update-style actions.
• Analytics:
  • Modifies sendAnalytics call in push() to use updated action data.

^{This description was created by for 5312ca0. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 5312ca0 in 1 minute and 52 seconds. Click for details.

• Reviewed 78 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 7 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. apps/studio/src/lib/editor/engine/history/index.ts:76

• Draft comment:
  Using empty string as fallback for oid may hide issues if oid is missing; consider validating target structure.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

2. apps/studio/src/lib/editor/engine/history/index.ts:90

• Draft comment:
  Mapping targets by index assumes that the stored original action has targets with matching order; ensure this is always valid.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that the mapping of targets by index is always valid. This is a general request for validation and does not provide a specific suggestion or point out a specific issue. It violates the rule against asking the author to ensure behavior is intended.

3. apps/studio/src/lib/editor/engine/history/index.ts:115

• Draft comment:
  Clearing the originalStyleMap immediately after pushing updatedAction might discard needed state if multiple update-style actions occur in quick succession. Confirm that this behavior is intended.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

4. apps/studio/src/lib/editor/engine/history/index.ts:117

• Draft comment:
  Analytics is using updatedAction; ensure that analytics events reflect the correct state (original vs updated) for style undo operations.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

5. apps/studio/src/lib/editor/engine/history/index.ts:115

• Draft comment:
  Consider deleting only the used entry from originalStyleMap (using delete(oid)) instead of clearing the entire map. Using clear() may inadvertently remove original styles for other update-style actions if multiple occur.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The originalStyleMap is used to track original styles during transactions. Once an action is pushed (outside transaction), we've already used the original style data to create the updatedAction. The map is cleared because its data is no longer needed - it served its purpose for this update cycle. Multiple style updates would be handled through the transaction system, not through this map. The reviewer might be right that multiple style updates could occur, but they may be misunderstanding the lifecycle of the originalStyleMap - it's meant to be temporary for the current action processing. The clear() operation is actually correct here because the originalStyleMap is meant to be temporary storage for the current action processing cycle. The transaction system handles multiple related updates. The comment should be deleted because it suggests a change that would actually break the intended behavior of the originalStyleMap as temporary storage.

6. apps/studio/src/lib/editor/engine/history/index.ts:92

• Draft comment:
  Using action.targets[0].oid || '' may cause collisions if oid is missing. Consider validating the oid or handling empty strings explicitly to avoid unintended behavior.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code is actually handling the empty string case consistently - if oid is missing, it falls back to '' both when setting and getting from the map. This means there won't be collisions between missing oids since they'll all map to the same empty string key. While using empty string as a fallback isn't ideal, it's not actually causing the collision problem the comment suggests. The comment raises a valid concern about data validation, and empty string keys in maps can be problematic in some cases. However, the code is handling the edge case consistently and safely - missing oids are mapped to the same empty string key, which prevents the collision problem the comment warns about. The comment should be deleted because it warns about a collision problem that doesn't actually exist in this implementation, and the current handling of missing oids is consistent and safe.

7. apps/studio/src/lib/editor/engine/history/index.ts:98

• Draft comment:
  The mapping of original style uses array indices (originalValue?.targets[idx]?.change.original). Ensure that the order and count of targets remains consistent; otherwise, the wrong original styles might be applied on undo.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_MeYlwnw32aXElz1C

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[Kitenite]

Hello, I am migrating the desktop app to a new repository. This repository will now focus on the web version. This PR has been migrated to the new repository: onlook-dev#11. Sorry for the inconvenience, rest assure your work here will also be adapted for the web version (if it hasn't already).