When switching themes switch the figma native theme mode selector if possible by ultraprinse · Pull Request #2336 · tokens-studio/figma-plugin

ultraprinse · 2023-10-25T17:38:50Z

20231025_133845.mp4

Closes #2187

…into 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

changeset-bot · 2023-10-25T17:38:53Z

🦋 Changeset detected

Latest commit: 21f2ae5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@tokens-studio/figma-plugin	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-10-25T17:40:42Z

PR Analysis

🎯 Main theme: The main theme of this PR is to switch the Figma native theme mode selector when switching themes in the plugin.
📝 PR summary: This PR introduces changes to the AsyncMessageChannel and setActiveTheme files. It adds functionality to switch the Figma native theme mode selector when switching themes in the plugin. It also updates the state when a new active theme is set.
📌 Type of PR: Enhancement
🧪 Relevant tests added: No
⏱️ Estimated effort to review [1-5]: 3, The PR introduces a new feature and modifies the state management, which requires a good understanding of the existing codebase and the new feature being introduced. However, the changes are not extensive and should be manageable to review.
🔒 Security concerns: No

PR Feedback

💡 General suggestions: The code changes seem to be well implemented, but it would be beneficial to add error handling for the new code blocks. Also, it would be good to consider the performance implications of the forEach loop on figma.currentPage.children.
🤖 Code feedback:
- relevant file: src/AsyncMessageChannel.ts
  suggestion: Consider adding error handling for the new code block. If there's an error while setting the explicit variable mode for a collection, it should be caught and handled appropriately. [important]
  relevant line: figma.currentPage.children.forEach((frame) => {
- relevant file: src/AsyncMessageChannel.ts
  suggestion: The forEach loop on figma.currentPage.children could potentially be expensive if there are many children. Consider if there's a more efficient way to achieve the same result. [medium]
  relevant line: figma.currentPage.children.forEach((frame) => {

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

SorsOps · 2023-10-26T10:21:49Z

          if ('message' in msg) {
+            if (msg.message.type === 'async/get-theme-info') {
+              if ('activeTheme' in msg.message && 'themes' in msg.message) {
+                const { activeTheme } = msg.message;


Suggested change

const { activeTheme } = msg.message;

const { activeTheme, themes } = msg.message;

SorsOps · 2023-10-26T10:21:58Z

+            if (msg.message.type === 'async/get-theme-info') {
+              if ('activeTheme' in msg.message && 'themes' in msg.message) {
+                const { activeTheme } = msg.message;
+                const { themes } = msg.message;


Suggested change

const { themes } = msg.message;

const themeToFind = Object.values(activeTheme)[0];

SorsOps · 2023-10-26T10:23:28Z

+              if ('activeTheme' in msg.message && 'themes' in msg.message) {
+                const { activeTheme } = msg.message;
+                const { themes } = msg.message;
+                const activeMode = themes.filter((theme) => theme.id === Object.values(activeTheme)[0])[0];


Suggested change

const activeMode = themes.filter((theme) => theme.id === Object.values(activeTheme)[0])[0];

const activeMode = themes.find((theme) => theme.id === themeToFind);

SorsOps · 2023-10-26T10:25:12Z

Can you add a changeset so we can inform the users about this change?

Check to see if the fixes for the lookup fix the OOM error, otherwise we'll have to double check why that's occurring

six7 · 2023-11-10T16:35:43Z

Tests are failing here

…into 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

github-actions · 2024-01-10T01:01:04Z

Commit SHA:4c507f93be36defe7933184695662c0f75d72629

Test coverage results 🧪

Code coverage diff between base branch:main and head branch: 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

Status	File	% Stmts	% Branch	% Funcs	% Lines
🔴	total	70.73 (0.01)	61.08 (-0.02)	68.84 (-0.02)	70.98 (0.01)
🔴	src/AsyncMessageChannel.ts	94.91 (-3.09)	82.6 (-14.27)	92.85 (-7.15)	94.64 (-3.27)

github-actions · 2024-01-10T01:01:06Z

Commit SHA:4c507f93be36defe7933184695662c0f75d72629
Current PR reduces the test coverage percentage by 100 for some tests

SorsOps · 2024-01-10T08:33:36Z

@@ -0,0 +1,5 @@
+---


This looks redundant?

SorsOps · 2024-01-10T08:35:45Z

+                const activeMode = themes.find((theme) => theme.id === themeToFind);
+                if (figma.currentPage.children && figma.currentPage.children.length > 0) {
+                  figma.currentPage.children.forEach((frame) => {
+                    frame.setExplicitVariableModeForCollection(activeMode?.$figmaCollectionId ?? '', activeMode?.$figmaModeId ?? '');


Shouldn't we be using an if around this and checking that the string id's exist? What happens when we these are invalid or null?

yeah it could be that the information is outdated and then this will error. they'll likely not be invalid or null but it could be that it doesnt exist anymore causing an error.

let's check for if a) those are set and b) wrap so that we dont crash

six7 · 2024-01-10T12:34:40Z

      this.attachMessageListener((msg: IncomingMessageEvent<AsyncMessageResults & { type: Message['type'] }>['data']['pluginMessage']) => {
        if (msg.id === messageId) {
          if ('message' in msg) {
+            if (msg.message.type === 'async/get-theme-info') {


i dont think this belongs here. this file doesnt have any of the other message handlers, this needs to go elsewhere where we define those.

figma-plugin/src/app/components/Initiator.tsx

Line 31 in 1424c9f

switch (pluginMessage.type) {

this feels like the right place

six7 · 2024-01-10T12:35:53Z

+                const activeMode = themes.find((theme) => theme.id === themeToFind);
+                if (figma.currentPage.children && figma.currentPage.children.length > 0) {
+                  figma.currentPage.children.forEach((frame) => {
+                    frame.setExplicitVariableModeForCollection(activeMode?.$figmaCollectionId ?? '', activeMode?.$figmaModeId ?? '');


yeah it could be that the information is outdated and then this will error. they'll likely not be invalid or null but it could be that it doesnt exist anymore causing an error.

let's check for if a) those are set and b) wrap so that we dont crash

six7 · 2024-01-10T12:38:44Z

+                const themeToFind = Object.values(activeTheme)[0];
+                const activeMode = themes.find((theme) => theme.id === themeToFind);
+                if (figma.currentPage.children && figma.currentPage.children.length > 0) {
+                  figma.currentPage.children.forEach((frame) => {


this ignores the issue requirement completely and sets an active variable mode for every frame on the current page. this is not what we need. we need to apply way more logic here than that.

#2187

the issue states that it should have different behavior depending on the apply mode

six7 · 2024-01-10T12:39:16Z

+            if (msg.message.type === 'async/get-theme-info') {
+              if ('activeTheme' in msg.message && 'themes' in msg.message) {
+                const { activeTheme, themes } = msg.message;
+                const themeToFind = Object.values(activeTheme)[0];


we do not just want to switch the first theme, but all theme groups that are active

hiroshi added 5 commits October 16, 2023 08:16

add console logs to figure out issues

11ec40d

clean console log

76fb981

Merge branch 'main' of https://github.com/tokens-studio/figma-plugin …

4fdd735

…into 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

apply style to figma check

a45930e

switching active theme in plugin

99d2004

ultraprinse self-assigned this Oct 25, 2023

ultraprinse requested review from SorsOps and six7 October 25, 2023 17:39

SorsOps suggested changes Oct 26, 2023

View reviewed changes

resolve comments

e8026a1

ultraprinse requested a review from SorsOps November 7, 2023 13:17

feat: add changeset

31099b0

hiroshi added 5 commits December 5, 2023 04:08

Merge branch 'main' of https://github.com/tokens-studio/figma-plugin …

64a4de2

…into 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

update: update the logic

253eca3

Merge branch 'main' of https://github.com/tokens-studio/figma-plugin …

c1b0204

…into 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

update to pass test case

3fbd4d7

feat: add changest

21f2ae5

SorsOps reviewed Jan 10, 2024

View reviewed changes

six7 requested changes Jan 10, 2024

View reviewed changes

six7 closed this Sep 16, 2025

	const { activeTheme } = msg.message;
	const { activeTheme, themes } = msg.message;

	const { themes } = msg.message;
	const themeToFind = Object.values(activeTheme)[0];

	const activeMode = themes.filter((theme) => theme.id === Object.values(activeTheme)[0])[0];
	const activeMode = themes.find((theme) => theme.id === themeToFind);

Uh oh!

Conversation

ultraprinse commented Oct 25, 2023 • edited by SorsOps Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot Bot commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot commented Oct 25, 2023

PR Analysis

PR Feedback

How to use

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SorsOps commented Oct 26, 2023

Uh oh!

six7 commented Nov 10, 2023

Uh oh!

github-actions Bot commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test coverage results 🧪

Uh oh!

github-actions Bot commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ultraprinse commented Oct 25, 2023 •

edited by SorsOps

Loading

changeset-bot Bot commented Oct 25, 2023 •

edited

Loading

github-actions Bot commented Jan 10, 2024 •

edited

Loading

github-actions Bot commented Jan 10, 2024 •

edited

Loading