Conversation
|
|
|
akshay-gupta7
changed the title
|
Commit SHA:44259dbe0a52b5458b7b4ce3e5706f07b17d7625 Test coverage results 🧪
|
|
Commit SHA:44259dbe0a52b5458b7b4ce3e5706f07b17d7625 |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #3707 where applying themes to selected frames fails to update all component layers in a single pass, requiring users to click "Apply to Selection" multiple times.
Changes:
- Enhanced node discovery algorithm to include component instance children that inherit token data from parent components
- Made
setValuesOnNodeproperly awaited to ensure synchronous completion of node updates - Refactored
SharedDataHandler.getAll()to check all known properties instead of only existing keys - Added filtering in
NodeManagerto exclude nodes without any token data - Added 'SECTION' to valid node types for token application
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tokens-studio-for-figma/src/utils/findAll.ts | Implements "Smart Discovery" mode that finds both nodes with local token data and instance children that may inherit token data, using Set for deduplication |
| packages/tokens-studio-for-figma/src/plugin/updateNodes.ts | Adds await keyword to properly wait for setValuesOnNode completion |
| packages/tokens-studio-for-figma/src/plugin/asyncMessageHandlers/update.ts | Enables Smart Discovery mode by passing nodesWithoutPluginData: true parameter |
| packages/tokens-studio-for-figma/src/plugin/SharedDataHandler.ts | Changes getAll() to iterate through all Properties enum values instead of only existing node keys |
| packages/tokens-studio-for-figma/src/plugin/NodeManager.ts | Adds filtering to exclude nodes with empty token objects from processing |
| packages/tokens-studio-for-figma/src/constants/ValidNodeTypes.ts | Adds 'SECTION' node type to support section nodes |
| Object.values(Properties).forEach((key) => { | ||
| const value = this.get(node, key); | ||
| if (value) { | ||
| try { | ||
| // make sure we catch JSON parse errors in case invalid property keys are set and found | ||
| // we're storing `none` as a string without quotes | ||
| const parsedValue = value === 'none' ? 'none' : JSON.parse(value); | ||
| result[key] = parsedValue; | ||
| } catch (err) { | ||
| console.warn(err); | ||
| } | ||
| } | ||
| return null; | ||
| }); |
There was a problem hiding this comment.
This refactoring changes the iteration logic from checking only the keys present on the node to checking all possible Properties enum values. While this ensures all known properties are checked (which may help with the incomplete theme application issue), it's less efficient as it makes up to 61 getSharedPluginData calls per node instead of only checking existing keys. Consider documenting why this change was necessary for fixing the theme application issue, as it's a significant performance trade-off.
| withLocalData.forEach(n => allNodesSet.add(n)); | ||
|
|
||
| // 2. Find all instances (their children have inherited token data) | ||
| const instances = node.findAllWithCriteria({ | ||
| types: ['INSTANCE'], | ||
| }); | ||
| instances.forEach(instance => { | ||
| allNodesSet.add(instance); | ||
| // For each instance, we must also check all its children | ||
| instance.findAllWithCriteria({ | ||
| types: ValidNodeTypes, | ||
| }).forEach(n => allNodesSet.add(n)); | ||
| }); | ||
|
|
||
|
|
||
| } else { | ||
| // Standard Discovery: Only nodes with local token data | ||
| node.findAllWithCriteria({ | ||
| types: ValidNodeTypes, | ||
| ...pluginDataOptions, | ||
| }), | ||
| ); | ||
| sharedPluginData: { namespace: 'tokens' }, | ||
| }).forEach(n => allNodesSet.add(n)); |
There was a problem hiding this comment.
Inconsistent arrow function parameter style. The codebase convention uses parentheses around single parameters (e.g., '(n) =>' not 'n =>'). Apply this consistently on lines 17, 23, 28, and 37 for better code consistency.
| @@ -1,23 +1,44 @@ | |||
| import { ValidNodeTypes } from '@/constants/ValidNodeTypes'; | |||
|
|
|||
| export function findAll(nodes: readonly BaseNode[], includeSelf = false, nodesWithoutPluginData = false): BaseNode[] { | |||
There was a problem hiding this comment.
The parameter name 'nodesWithoutPluginData' is misleading and creates confusion. When set to true, the function actually finds MORE nodes (including nodes without plugin data), not fewer. The name suggests it would exclude nodes with plugin data, but it actually enables a broader "Smart Discovery" mode that includes both nodes with plugin data AND instance children that may not have local plugin data. Consider renaming to something like 'includeInstanceChildren' or 'enableSmartDiscovery' to better reflect its actual behavior.
| import { ValidNodeTypes } from '@/constants/ValidNodeTypes'; | ||
|
|
||
| export function findAll(nodes: readonly BaseNode[], includeSelf = false, nodesWithoutPluginData = false): BaseNode[] { | ||
| let allNodes = includeSelf ? [...nodes] : []; | ||
| const pluginDataOptions = nodesWithoutPluginData | ||
| ? {} | ||
| : { | ||
| sharedPluginData: { | ||
| namespace: 'tokens', | ||
| }, | ||
| }; | ||
|
|
||
|
|
||
| const allNodesSet = new Set<BaseNode>(includeSelf ? nodes : []); | ||
|
|
||
| nodes.forEach((node) => { | ||
| if ('children' in node) { | ||
| allNodes = allNodes.concat( | ||
| if (nodesWithoutPluginData) { | ||
| // Smart Discovery: | ||
| // 1. Find all nodes with local token data (Figma optimized search) | ||
| const withLocalData = node.findAllWithCriteria({ | ||
| types: ValidNodeTypes, | ||
| sharedPluginData: { namespace: 'tokens' }, | ||
| }); | ||
| withLocalData.forEach(n => allNodesSet.add(n)); | ||
|
|
||
| // 2. Find all instances (their children have inherited token data) | ||
| const instances = node.findAllWithCriteria({ | ||
| types: ['INSTANCE'], | ||
| }); | ||
| instances.forEach(instance => { | ||
| allNodesSet.add(instance); | ||
| // For each instance, we must also check all its children | ||
| instance.findAllWithCriteria({ | ||
| types: ValidNodeTypes, | ||
| }).forEach(n => allNodesSet.add(n)); | ||
| }); | ||
|
|
||
|
|
||
| } else { | ||
| // Standard Discovery: Only nodes with local token data | ||
| node.findAllWithCriteria({ | ||
| types: ValidNodeTypes, | ||
| ...pluginDataOptions, | ||
| }), | ||
| ); | ||
| sharedPluginData: { namespace: 'tokens' }, | ||
| }).forEach(n => allNodesSet.add(n)); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| const allNodes = Array.from(allNodesSet); | ||
| return allNodes; | ||
| } |
There was a problem hiding this comment.
Missing mandatory changeset. According to the repository guidelines, every PR must include a changeset created by running 'yarn changeset'. The changeset should be a 'patch' version with a clear, user-facing description of the changes. This is required for proper versioning and release notes.
| // 2. Find all instances (their children have inherited token data) | ||
| const instances = node.findAllWithCriteria({ | ||
| types: ['INSTANCE'], | ||
| }); | ||
| instances.forEach(instance => { | ||
| allNodesSet.add(instance); | ||
| // For each instance, we must also check all its children |
There was a problem hiding this comment.
The instance children search (lines 26-28) finds ALL children of ValidNodeTypes without filtering for plugin data. While this is necessary to capture instance children that inherit token data from their parent component (and thus have no local plugin data), it creates performance overhead by collecting many nodes that have no token data at all. These are later filtered out in NodeManager.ts (lines 83-89), but it would be more efficient to avoid collecting them in the first place. Consider adding a comment explaining why this broad search is necessary for the fix, as it's not immediately obvious that instance children need to be included even without local plugin data.
| // 2. Find all instances (their children have inherited token data) | |
| const instances = node.findAllWithCriteria({ | |
| types: ['INSTANCE'], | |
| }); | |
| instances.forEach(instance => { | |
| allNodesSet.add(instance); | |
| // For each instance, we must also check all its children | |
| // 2. Find all instances (their children may have inherited token data) | |
| const instances = node.findAllWithCriteria({ | |
| types: ['INSTANCE'], | |
| }); | |
| instances.forEach(instance => { | |
| allNodesSet.add(instance); | |
| // IMPORTANT: | |
| // We intentionally search *all* children of instances, without filtering | |
| // by shared plugin data here. Children can inherit token data from their | |
| // parent component and therefore may not have any local plugin data set. | |
| // These nodes are later filtered in NodeManager based on actual token data. | |
| // Narrowing this search (e.g. by sharedPluginData) would cause us to miss | |
| // inherited tokens on instance children. |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Why does this PR exist?
Closes #3707
What does this pull request do?
Testing this change
Additional Notes (if any)
Note
Improves node discovery and token application to address incomplete theme updates.
SECTIONtoValidNodeTypesfindAllto include nodes without local plugin data by collecting nodes with token data plusINSTANCEnodes and their children; deduplicates via aSetNodeManager.findBaseNodesWithDatanow only returns nodes that actually have token data;updateenables this mode (nodesWithoutPluginData: true)SharedDataHandler.getAllnow reads only knownPropertieskeys and safely parses valuesupdateNodesawaits asyncsetValuesOnNodeto ensure complete applicationWritten by Cursor Bugbot for commit 5390708. Configure here.