Open
Conversation
Adds a UI for navigating and managing modifier nodes in the design tokens tree. This enables users to create and configure token modifiers with multiple contexts, switch between different views via tab navigation, and properly constrain drag-and-drop operations for the new node types. - Navigation: Added tab-based navigation in the right panel to switch between token sets and modifier contexts - Modifier Mode: Added toggle to switch between "Sets" and "Modifiers" configuration modes - Node Management: Added support for creating token-modifier and token-context nodes with proper parent constraints - Tree View: Updated drag-and-drop validation to handle modifier hierarchy constraints - Editor: Added editor support for modifier and context nodes, including default context selector - UI: Added appropriate icons (Layers, GitBranch) for modifier-related nodes
✅ Deploy Preview for engramma-website canceled.
|
✅ Deploy Preview for tools-for-web ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds modifier-context support and UI mode switching: new node types (token-modifier, token-context), context-aware selection and editing with context overrides, path/path-creation utilities in state, updated tree nesting/move rules, editor/Styleguide props for context selection, and a small CSS change for Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (UI)
participant Editor as Editor (UI)
participant State as TreeState (state.svelte.ts)
participant Style as Styleguide
App->>Editor: open token for edit (with selectedContextId)
Editor->>State: is token owned by context? (isTokenOwnedByContext)
alt token inherited from context
Editor->>State: createContextOverride(contextId, sourceTokenId, tokenData)
State-->>Editor: newOverrideId
Editor->>App: switch selection to newOverrideId
App->>Editor: re-render editor for override
else token not inherited
Editor->>State: transact updateMeta(tokenId, changes)
State-->>Editor: updated token
end
App->>Style: render with selectedContextId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app.svelte (1)
27-45:⚠️ Potential issue | 🟡 MinorSort lucide icon imports alphabetically.
The new specifiers break alphabetical ordering within the external import group.
As per coding guidelines, "Group imports as: external dependencies first, then relative imports. Sort alphabetically within each group."🧹 Suggested ordering
- import { - Settings, - Trash2, - Folder, - Square, - Ruler, - Clock, - Type, - Hash, - Bold, - Tangent, - CaseUpper, - ArrowRightLeft, - LineSquiggle, - Paintbrush, - Tags, - ListPlus, - Layers, - GitBranch, - } from "@lucide/svelte"; + import { + ArrowRightLeft, + Bold, + CaseUpper, + Clock, + Folder, + GitBranch, + Hash, + Layers, + LineSquiggle, + ListPlus, + Paintbrush, + Ruler, + Settings, + Square, + Tags, + Tangent, + Trash2, + Type, + } from "@lucide/svelte";
🤖 Fix all issues with AI agents
In `@src/app.svelte`:
- Around line 216-243: The handlers addSet, addGroup, addModifier, and
addContext are used directly as event handlers but don’t follow the on* naming
convention; rename them to onAddSet, onAddGroup, onAddModifier, and onAddContext
(and similarly rename the event handler functions in the other block referenced
around the later section) and update every place they’re passed as event
callbacks (e.g., button on:click or component props) to use the new names so
imports/uses remain consistent; ensure you update both the function declarations
and all references (including any exports or bindings) to avoid unused/undefined
identifiers.
- Around line 674-713: When toggling isConfiguringModifiers (the onclick on the
"Configure modifiers" button) or changing selectedContextId (the tab buttons),
ensure selectedItems and expandedItems are validated against the set of nodes
visible in the new mode and pruned if they reference hidden nodes; implement a
small sync routine (e.g., syncSelectionForMode) that computes the current
visible node IDs for the given selectedContextId/isConfiguringModifiers state
and filters selectedItems and expandedItems to only those IDs (and if none
remain, clear selection in a safe way), and call that routine from the onclick
handlers that set isConfiguringModifiers and selectedContextId so the Styleguide
receives consistent props.
Comment on lines
+25
to
+29
| const { | ||
| selectedItems, | ||
| selectedContextId, | ||
| }: { selectedItems: Set<string>; selectedContextId: string | undefined } = | ||
| $props(); |
There was a problem hiding this comment.
selectedContextId is accepted but never applied.
Context switching in the right panel won’t change the styleguide output unless this prop is used to scope visibleNodes and/or token resolution. Please thread selectedContextId into the rendering/filtering path so the tab selection has effect.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app.svelte`:
- Around line 184-196: The non-null assertion on treeState.getNode(...) is
unsafe; extract the parent lookup into a variable (e.g., const parentNode =
treeState.getNode(firstSelectedNode.parentId)) and explicitly check parentNode
is defined before accessing parentNode.meta.nodeType in the
config.parentTypes.includes(...) check; if parentNode is undefined handle it
(return early or choose a safe fallback) and then set parentId =
firstSelectedNode.parentId and insertAfterIndex =
generateKeyBetween(firstSelectedNode.index, null) as before.
---
Duplicate comments:
In `@src/app.svelte`:
- Around line 216-243: The event handler naming convention requires these
functions to be prefixed with "on" when used directly as onclick handlers;
rename addSet, addGroup, addModifier, and addContext to onAddSet, onAddGroup,
onAddModifier, and onAddContext respectively, keep their implementation
identical (they still call addNode with the same payload), and update all
references/usages (e.g., onclick bindings in the Svelte markup) to the new names
so callers and the addNode invocation remain correct.
Comment on lines
+184
to
+196
| } else if ( | ||
| firstSelectedNode.parentId && | ||
| config.parentTypes.includes( | ||
| treeState.getNode(firstSelectedNode.parentId)?.meta.nodeType!, | ||
| ) | ||
| ) { | ||
| // Add as sibling after selected node (same parent) | ||
| parentId = firstSelectedNode.parentId; | ||
| insertAfterIndex = generateKeyBetween(firstSelectedNode.index, null); | ||
| } else { | ||
| // Cannot add here | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Non-null assertion relies on tree consistency.
The ! assertion at line 187 assumes getNode(firstSelectedNode.parentId) always returns a defined node when parentId exists. If the tree state ever becomes inconsistent (e.g., orphaned node references), this could cause unexpected behavior. Consider adding an explicit check:
🛡️ Safer alternative
} else if (
firstSelectedNode.parentId &&
- config.parentTypes.includes(
- treeState.getNode(firstSelectedNode.parentId)?.meta.nodeType!,
- )
+ (() => {
+ const parentNode = treeState.getNode(firstSelectedNode.parentId);
+ return parentNode && config.parentTypes.includes(parentNode.meta.nodeType);
+ })()
) {Or extract the parent lookup before the condition for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app.svelte` around lines 184 - 196, The non-null assertion on
treeState.getNode(...) is unsafe; extract the parent lookup into a variable
(e.g., const parentNode = treeState.getNode(firstSelectedNode.parentId)) and
explicitly check parentNode is defined before accessing parentNode.meta.nodeType
in the config.parentTypes.includes(...) check; if parentNode is undefined handle
it (return early or choose a safe fallback) and then set parentId =
firstSelectedNode.parentId and insertAfterIndex =
generateKeyBetween(firstSelectedNode.index, null) as before.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/editor.svelte (1)
174-204:⚠️ Potential issue | 🟡 MinorMissing error handling around
createContextOverride.If
createContextOverridethrows (e.g., source token not found), the error is unhandled and the UI may end up in an inconsistent state. The coding guidelines recommend try-catch for operations that can fail. Consider wrapping the override path:🛡️ Suggested fix
if (isEditingInheritedToken) { - // Create a context override instead of editing the base token - const newTokenId = treeState.createContextOverride( - selectedContextId, - node.nodeId, - newMeta as Partial<TokenMeta>, - ); - - // Update selection to the new override token - selectedItems.clear(); - selectedItems.add(newTokenId); + try { + const newTokenId = treeState.createContextOverride( + selectedContextId, + node.nodeId, + newMeta as Partial<TokenMeta>, + ); + selectedItems.clear(); + selectedItems.add(newTokenId); + } catch (error) { + console.error("Failed to create context override:", error); + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/editor.svelte` around lines 174 - 204, The override branch in updateMeta calls treeState.createContextOverride without guarding for exceptions; wrap the createContextOverride call in a try-catch, only mutate selection (selectedItems.clear(); selectedItems.add(newTokenId)) after createContextOverride succeeds, and on error log or surface the error (e.g., via console.error or an app error handler) and avoid changing state; ensure the normal treeState.transact path remains unchanged and reference updateMeta, createContextOverride, selectedItems, and treeState.transact when making the change.
♻️ Duplicate comments (2)
src/app.svelte (2)
291-317: Rename click handlers to follow theon*convention.
addSet,addGroup,addModifier, andaddContextare bound directly toonclickbut don't follow theon*naming convention. As per coding guidelines, "Prefix event handlers with on* (onMount, onClick)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.svelte` around lines 291 - 317, Rename the click handler functions to follow the on* convention: change addSet → onAddSet, addGroup → onAddGroup, addModifier → onAddModifier, and addContext → onAddContext; update their definitions (the functions that call addNode with nodeType/token defaults) and replace all usages/bindings (e.g., any onclick attributes or references) to the new names so event wiring continues to work; ensure any other references (props, tests, or passed callbacks) are updated to the new identifiers to avoid unresolved symbol errors.
259-263: Non-null assertion on parent node lookup is still present.
treeState.getNode(firstSelectedNode.parentId)?.meta.nodeType!— the!assertion assumesgetNodealways returns a defined node whenparentIdexists. If the tree state is ever inconsistent, this will silently passundefinedintoincludes().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.svelte` around lines 259 - 263, The code uses a non-null assertion on treeState.getNode(...) inside the includes call which can pass undefined into config.parentTypes.includes; instead, fetch the parent node into a local (e.g., parentNode = treeState.getNode(firstSelectedNode.parentId)), then guard that parentNode and parentNode.meta.nodeType exist before calling config.parentTypes.includes(parentNode.meta.nodeType). Update the conditional around firstSelectedNode, parentId, and the parent node lookup (referencing firstSelectedNode, treeState.getNode, parentId, parentNode.meta.nodeType, and config.parentTypes) to avoid using the `!` operator and short-circuit if the parent node is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Around line 1-157: The file index.html lacks the SPA bootstrap script so the
Svelte app never mounts; add the module loader that imports your app entry
(e.g., the equivalent of <script type="module" src="/src/main.ts"></script>)
into index.html (place it after the existing markup or before </body>) so the
Svelte/Vite entrypoint (src/main.ts) is executed; if this page is only a
standalone demo, instead move this demo to a separate HTML file and restore the
bootstrap script in the main index.html used by the app.
In `@src/app.svelte`:
- Around line 128-177: The treeData computation duplicates logic by manually
computing childPath/getNodePath and matching against contextTokens for token
children instead of reusing buildMergedTreeItem; refactor the children mapping
inside treeData to call buildMergedTreeItem(child, contextTokens) for every
child (tokens and groups) and let buildMergedTreeItem handle path resolution and
ownership, removing the inline getNodePath/contextTokens lookup and the isOwned
conditional in the returned child objects; ensure buildMergedTreeItem accepts
the same contextTokens Map and returns objects with
id,parentId,name,children,isOwned so treeData can simply map children =>
buildMergedTreeItem(child, contextTokens).
- Around line 106-126: In buildMergedTreeItem, the code incorrectly looks up
contextTokens by node.nodeId (UUID) so context overrides never match; change the
lookup to use the node's path key (e.g. contextTokens.get(node.meta.path) or
contextTokens.get(node.path) depending on your TreeNodeMeta shape), update the
isOwned and displayNode assignments to use that path-based lookup, and ensure
recursive calls keep passing the same contextTokens map unchanged.
In `@src/editor.svelte`:
- Around line 470-484: Replace the five independent Svelte {`#if`} blocks that
check node?.meta.nodeType with a single conditional chain using {`#if`} ... {:else
if} ... {:else} so evaluation short-circuits after the first match; locate the
fragment that reads node?.meta.nodeType and convert the separate checks for
"token-set", "token-modifier", "token-context", "token-group", and "token" into
an ordered {`#if` node?.meta.nodeType === "token-set"} / {:else if
node?.meta.nodeType === "token-modifier"} / {:else if node?.meta.nodeType ===
"token-context"} / {:else if node?.meta.nodeType === "token-group"} / {:else if
node?.meta.nodeType === "token"} chain to improve performance and ensure only
one branch renders.
In `@src/state.svelte.ts`:
- Around line 326-345: findNodeAtPath is inefficient because it re-scans the
entire nodes Map for every path segment; similarly createPathInContext repeats
that pattern. Fix by looking up children by parentId instead of filtering
nodes.values() each iteration: build or reuse a parent→children index (e.g. a
Map<string, TreeNode[]> or call the existing TreeStore.getChildren method) and
use that to find the matching child for each segment, then traverse via
match.nodeId; update both findNodeAtPath and createPathInContext to accept or
access the children index/getChildren to avoid O(n × depth) scans.
- Around line 563-599: The code calls this.#store.getChildren(parentId) inside
the same transact block after createPathInContext, which won't see child nodes
staged via tx.set; replace the fragile store read with a local sibling lookup
from the transaction-scoped nodes map used earlier (the nodes: Map<string,
TreeNode<TreeNodeMeta>> passed into createPathInContext) — e.g., compute
siblings by filtering/collecting entries in nodes whose parentId === parentId
and sort by index (or use nodes.get(parentId) if a children list is stored
there), then derive lastIndex from that local collection before calling
generateKeyBetween; retain the rest of the newToken creation and
tx.set(newToken) unchanged.
---
Outside diff comments:
In `@src/editor.svelte`:
- Around line 174-204: The override branch in updateMeta calls
treeState.createContextOverride without guarding for exceptions; wrap the
createContextOverride call in a try-catch, only mutate selection
(selectedItems.clear(); selectedItems.add(newTokenId)) after
createContextOverride succeeds, and on error log or surface the error (e.g., via
console.error or an app error handler) and avoid changing state; ensure the
normal treeState.transact path remains unchanged and reference updateMeta,
createContextOverride, selectedItems, and treeState.transact when making the
change.
---
Duplicate comments:
In `@src/app.svelte`:
- Around line 291-317: Rename the click handler functions to follow the on*
convention: change addSet → onAddSet, addGroup → onAddGroup, addModifier →
onAddModifier, and addContext → onAddContext; update their definitions (the
functions that call addNode with nodeType/token defaults) and replace all
usages/bindings (e.g., any onclick attributes or references) to the new names so
event wiring continues to work; ensure any other references (props, tests, or
passed callbacks) are updated to the new identifiers to avoid unresolved symbol
errors.
- Around line 259-263: The code uses a non-null assertion on
treeState.getNode(...) inside the includes call which can pass undefined into
config.parentTypes.includes; instead, fetch the parent node into a local (e.g.,
parentNode = treeState.getNode(firstSelectedNode.parentId)), then guard that
parentNode and parentNode.meta.nodeType exist before calling
config.parentTypes.includes(parentNode.meta.nodeType). Update the conditional
around firstSelectedNode, parentId, and the parent node lookup (referencing
firstSelectedNode, treeState.getNode, parentId, parentNode.meta.nodeType, and
config.parentTypes) to avoid using the `!` operator and short-circuit if the
parent node is missing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
index.htmlsrc/app.sveltesrc/editor.sveltesrc/state.svelte.tssrc/tree-view.svelte
Comment on lines
+1
to
+157
| <style> | ||
| body { | ||
| background-color: black; | ||
| font-family: sans-serif; | ||
| display: grid; | ||
| height: 100%; | ||
| height: stretch; | ||
| justify-content: center; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .nav { | ||
| border: 1px solid #585a5e; | ||
| width: 60dvw; | ||
| margin: 0 auto; | ||
| border-radius: 2rem; | ||
| } | ||
|
|
||
| .menu { | ||
| color: gray; | ||
| list-style: none; | ||
| margin: 0; | ||
| padding: 0; | ||
| display: flex; | ||
| justify-content: end; | ||
| padding: 0 1rem; | ||
| } | ||
|
|
||
| .item { | ||
| position: relative; | ||
| } | ||
|
|
||
| @keyframes bounce { | ||
| from { | ||
| translate: 0 30px; | ||
| } | ||
| 80% { | ||
| scale: 1.2; | ||
| } | ||
| to { | ||
| scale: 1; | ||
| translate: 0 0px; | ||
| } | ||
| } | ||
|
|
||
| @keyframes bounceIn { | ||
| 0% { | ||
| scale: 1; | ||
| translate: 0 30px; | ||
| } | ||
| 40% { | ||
| scale: 1.2; | ||
| translate: 0 0; | ||
| } | ||
| 60% { | ||
| scale: 0.95; | ||
| } | ||
| 80% { | ||
| scale: 1.05; | ||
| } | ||
| 100% { | ||
| scale: 1; | ||
| translate: 0 0; | ||
| } | ||
| } | ||
|
|
||
| @keyframes bounceOut { | ||
| 0% { | ||
| scale: 1; | ||
| translate: 0 0; | ||
| } | ||
| 40% { | ||
| scale: 0.9; | ||
| translate: 0 30px; | ||
| } | ||
| 60% { | ||
| scale: 1.02; | ||
| } | ||
| 80% { | ||
| scale: 0.98; | ||
| } | ||
| 100% { | ||
| scale: 1; | ||
| translate: 0 30px; | ||
| } | ||
| } | ||
|
|
||
| .item::before { | ||
| content: ""; | ||
| position: absolute; | ||
| left: 0; | ||
| right: 0; | ||
| bottom: -1px; | ||
| width: 100px; | ||
| height: 30px; | ||
| margin: 0 auto; | ||
| background-color: white; | ||
| pointer-events: none; | ||
| clip-path: path("M0,30 S-20,30,0,30 S30,15,50,15 S80,30,100,30 Z"); | ||
| translate: 0 30px; | ||
| transform-origin: center bottom; | ||
| } | ||
|
|
||
| .item:has(.link:target)::before { | ||
| translate: 0 0; | ||
| } | ||
|
|
||
| .item:has(.link.ready)::before { | ||
| animation: bounceOut 0.8s ease forwards; | ||
| } | ||
|
|
||
| .item:has(.link.ready:target)::before { | ||
| animation: bounceIn 0.8s ease forwards; | ||
| } | ||
|
|
||
| .link { | ||
| color: inherit; | ||
| text-decoration: none; | ||
| display: block; | ||
| padding: 2rem 1.5rem; | ||
| } | ||
|
|
||
| .link:hover, | ||
| .link:focus-visible, | ||
| .link:target { | ||
| color: #2b75ed; | ||
| } | ||
|
|
||
| .content { | ||
| background-color: white; | ||
| border-radius: 2rem; | ||
| height: 300px; | ||
| } | ||
| </style> | ||
|
|
||
| <nav class="nav"> | ||
| <ul class="menu"> | ||
| <li class="item"> | ||
| <a class="link" id="item-1" href="#item-1">Menu Item</a> | ||
| </li> | ||
| <li class="item"> | ||
| <a class="link" id="item-2" href="#item-2">Menu Item</a> | ||
| </li> | ||
| </ul> | ||
| <div class="content"></div> | ||
| </nav> | ||
|
|
||
| <script> | ||
| // activate | ||
| document.addEventListener("click", (event) => { | ||
| if (event.target.closest(".link")) { | ||
| for (const link of document.querySelectorAll(".link")) { | ||
| link.classList.add("ready"); | ||
| } | ||
| } | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check git history for index.html to see previous content
echo "=== Git history for index.html ==="
git log --oneline -10 -- index.html 2>/dev/null || echo "No git history available"
echo ""
echo "=== Previous version of index.html (last 5 commits) ==="
git show HEAD~1:index.html 2>/dev/null | head -30 || echo "Cannot access previous version"
echo ""
echo "=== Vite config files ==="
fd -t f "vite.config\." 2>/dev/null | while read f; do
echo "File: $f"
cat "$f"
echo ""
done
echo ""
echo "=== All HTML files in repository ==="
fd -e html -t f 2>/dev/null
echo ""
echo "=== Directory structure (first 2 levels) ==="
tree -L 2 -I 'node_modules' 2>/dev/null || find . -maxdepth 2 -type d | head -20Repository: TrySound/engramma
Length of output: 2294
🏁 Script executed:
#!/bin/bash
echo "=== Content of src/main.ts ==="
head -50 src/main.ts
echo ""
echo "=== Content of packages/engramma/index.html ==="
cat packages/engramma/index.html
echo ""
echo "=== Recent commits (all) ==="
git log --oneline | head -20
echo ""
echo "=== Current status of index.html ==="
git status --short index.html || echo "No git status available"
echo ""
echo "=== Check if index.html exists in git ==="
git ls-files | grep index.htmlRepository: TrySound/engramma
Length of output: 3173
index.html is missing the app bootstrap script — application won't load.
The current index.html contains only a CSS animation demo and is missing the <script> tag needed to load the Svelte app entry point (src/main.ts). The root vite.config.ts uses the default SPA configuration (entry defaults to index.html), so this file is responsible for bootstrapping the application. Without the script loader, the app will not mount.
This needs the app bootstrap script (e.g., <script type="module" src="/src/main.ts"></script>) to be added back, unless this demo is intentionally meant for a different purpose and should be moved to a separate file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 1 - 157, The file index.html lacks the SPA bootstrap
script so the Svelte app never mounts; add the module loader that imports your
app entry (e.g., the equivalent of <script type="module"
src="/src/main.ts"></script>) into index.html (place it after the existing
markup or before </body>) so the Svelte/Vite entrypoint (src/main.ts) is
executed; if this page is only a standalone demo, instead move this demo to a
separate HTML file and restore the bootstrap script in the main index.html used
by the app.
Comment on lines
+106
to
+126
| const buildMergedTreeItem = ( | ||
| node: TreeNode<TreeNodeMeta>, | ||
| contextTokens: Map<string, TreeNode<TreeNodeMeta>>, | ||
| ): TreeItem => { | ||
| const children = treeState.getChildren(node.nodeId); | ||
| const contextToken = contextTokens.get(node.nodeId); | ||
| const isOwned = contextToken !== undefined; | ||
|
|
||
| // Use context token if available, otherwise use base token | ||
| const displayNode = contextToken ?? node; | ||
|
|
||
| return { | ||
| id: displayNode.nodeId, | ||
| parentId: displayNode.parentId, | ||
| name: displayNode.meta.name, | ||
| children: children.map((child) => | ||
| buildMergedTreeItem(child, contextTokens), | ||
| ), | ||
| isOwned, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Bug: buildMergedTreeItem looks up context tokens by nodeId instead of by path.
contextTokens is keyed by path strings (e.g. "GroupName/TokenName" — see lines 139-140), but line 111 looks up by node.nodeId (a UUID). This will never match, so isOwned will always be false for any recursively processed child, and context overrides in nested groups will not be displayed correctly.
The lookup needs to use the node's path, consistent with how the map was built:
🐛 Proposed fix
const buildMergedTreeItem = (
node: TreeNode<TreeNodeMeta>,
contextTokens: Map<string, TreeNode<TreeNodeMeta>>,
): TreeItem => {
const children = treeState.getChildren(node.nodeId);
- const contextToken = contextTokens.get(node.nodeId);
- const isOwned = contextToken !== undefined;
-
- // Use context token if available, otherwise use base token
- const displayNode = contextToken ?? node;
+ const nodePath = getNodePath(node, treeState.nodes()).slice(1).join("/");
+ const contextToken = contextTokens.get(nodePath);
+ const isOwned = contextToken !== undefined;
+ const displayNode = contextToken ?? node;
return {
id: displayNode.nodeId,
parentId: displayNode.parentId,
name: displayNode.meta.name,
children: children.map((child) =>
buildMergedTreeItem(child, contextTokens),
),
isOwned,
};
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const buildMergedTreeItem = ( | |
| node: TreeNode<TreeNodeMeta>, | |
| contextTokens: Map<string, TreeNode<TreeNodeMeta>>, | |
| ): TreeItem => { | |
| const children = treeState.getChildren(node.nodeId); | |
| const contextToken = contextTokens.get(node.nodeId); | |
| const isOwned = contextToken !== undefined; | |
| // Use context token if available, otherwise use base token | |
| const displayNode = contextToken ?? node; | |
| return { | |
| id: displayNode.nodeId, | |
| parentId: displayNode.parentId, | |
| name: displayNode.meta.name, | |
| children: children.map((child) => | |
| buildMergedTreeItem(child, contextTokens), | |
| ), | |
| isOwned, | |
| }; | |
| }; | |
| const buildMergedTreeItem = ( | |
| node: TreeNode<TreeNodeMeta>, | |
| contextTokens: Map<string, TreeNode<TreeNodeMeta>>, | |
| ): TreeItem => { | |
| const children = treeState.getChildren(node.nodeId); | |
| const nodePath = getNodePath(node, treeState.nodes()).slice(1).join("/"); | |
| const contextToken = contextTokens.get(nodePath); | |
| const isOwned = contextToken !== undefined; | |
| const displayNode = contextToken ?? node; | |
| return { | |
| id: displayNode.nodeId, | |
| parentId: displayNode.parentId, | |
| name: displayNode.meta.name, | |
| children: children.map((child) => | |
| buildMergedTreeItem(child, contextTokens), | |
| ), | |
| isOwned, | |
| }; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app.svelte` around lines 106 - 126, In buildMergedTreeItem, the code
incorrectly looks up contextTokens by node.nodeId (UUID) so context overrides
never match; change the lookup to use the node's path key (e.g.
contextTokens.get(node.meta.path) or contextTokens.get(node.path) depending on
your TreeNodeMeta shape), update the isOwned and displayNode assignments to use
that path-based lookup, and ensure recursive calls keep passing the same
contextTokens map unchanged.
Comment on lines
+128
to
+177
| const treeData = $derived.by(() => { | ||
| if (selectedContextId) { | ||
| // Get all base set tokens | ||
| const baseNodes = treeState.getBaseNodes(); | ||
|
|
||
| // Get all tokens in the selected context | ||
| const contextTokens = new Map<string, TreeNode<TreeNodeMeta>>(); | ||
| const contextTokenList = treeState.getAllTokensUnder(selectedContextId); | ||
|
|
||
| // Build a map of paths to context tokens | ||
| for (const token of contextTokenList) { | ||
| const path = getNodePath(token, treeState.nodes()).slice(1).join("/"); | ||
| contextTokens.set(path, token); | ||
| } | ||
|
|
||
| // Merge: show base sets but indicate which tokens are owned by context | ||
| return baseNodes.map((setNode) => { | ||
| const children = treeState.getChildren(setNode.nodeId); | ||
| return { | ||
| id: setNode.nodeId, | ||
| parentId: setNode.parentId, | ||
| name: setNode.meta.name, | ||
| children: children.map((child) => { | ||
| // For each child, check if it's in the context | ||
| const childPath = getNodePath(child, treeState.nodes()) | ||
| .slice(1) | ||
| .join("/"); | ||
| const contextToken = contextTokens.get(childPath); | ||
|
|
||
| if (child.meta.nodeType === "token") { | ||
| const isOwned = contextToken !== undefined; | ||
| return { | ||
| id: isOwned ? contextToken.nodeId : child.nodeId, | ||
| parentId: isOwned ? contextToken.parentId : child.parentId, | ||
| name: child.meta.name, | ||
| children: [], | ||
| isOwned, | ||
| }; | ||
| } | ||
|
|
||
| // For groups, recursively build with context awareness | ||
| return buildMergedTreeItem(child, contextTokens); | ||
| }), | ||
| isOwned: true, | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| return rootNodes.map(buildTreeItem); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant path computation in treeData — inline code duplicates buildMergedTreeItem logic.
Lines 150-166 manually handle context-token matching for direct children of a set, then delegate to buildMergedTreeItem for groups (line 169). Once buildMergedTreeItem is fixed (per the bug above), you can simplify by using it for all children uniformly:
♻️ Suggested simplification
return baseNodes.map((setNode) => {
const children = treeState.getChildren(setNode.nodeId);
return {
id: setNode.nodeId,
parentId: setNode.parentId,
name: setNode.meta.name,
- children: children.map((child) => {
- const childPath = getNodePath(child, treeState.nodes())
- .slice(1)
- .join("/");
- const contextToken = contextTokens.get(childPath);
-
- if (child.meta.nodeType === "token") {
- const isOwned = contextToken !== undefined;
- return {
- id: isOwned ? contextToken.nodeId : child.nodeId,
- parentId: isOwned ? contextToken.parentId : child.parentId,
- name: child.meta.name,
- children: [],
- isOwned,
- };
- }
-
- return buildMergedTreeItem(child, contextTokens);
- }),
+ children: children.map((child) =>
+ buildMergedTreeItem(child, contextTokens),
+ ),
isOwned: true,
};
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app.svelte` around lines 128 - 177, The treeData computation duplicates
logic by manually computing childPath/getNodePath and matching against
contextTokens for token children instead of reusing buildMergedTreeItem;
refactor the children mapping inside treeData to call buildMergedTreeItem(child,
contextTokens) for every child (tokens and groups) and let buildMergedTreeItem
handle path resolution and ownership, removing the inline
getNodePath/contextTokens lookup and the isOwned conditional in the returned
child objects; ensure buildMergedTreeItem accepts the same contextTokens Map and
returns objects with id,parentId,name,children,isOwned so treeData can simply
map children => buildMergedTreeItem(child, contextTokens).
Comment on lines
+470
to
+484
| {#if node?.meta.nodeType === "token-set"} | ||
| Set | ||
| {/if} | ||
| {#if node?.meta.nodeType === "token-modifier"} | ||
| Modifier | ||
| {/if} | ||
| {#if node?.meta.nodeType === "token-context"} | ||
| Modifier Context | ||
| {/if} | ||
| {#if node?.meta.nodeType === "token-group"} | ||
| Group | ||
| {/if} | ||
| {#if node?.meta.nodeType === "token"} | ||
| Token | ||
| {/if} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider {:else if} chain instead of independent {#if} blocks.
Since nodeType is always exactly one value, these five independent {#if} blocks evaluate every condition even after a match. An {:else if} chain short-circuits after the first match.
♻️ Suggested refactor
- {`#if` node?.meta.nodeType === "token-set"}
+ {`#if` node?.meta.nodeType === "token-set"}
Set
- {/if}
- {`#if` node?.meta.nodeType === "token-modifier"}
+ {:else if node?.meta.nodeType === "token-modifier"}
Modifier
- {/if}
- {`#if` node?.meta.nodeType === "token-context"}
+ {:else if node?.meta.nodeType === "token-context"}
Modifier Context
- {/if}
- {`#if` node?.meta.nodeType === "token-group"}
+ {:else if node?.meta.nodeType === "token-group"}
Group
- {/if}
- {`#if` node?.meta.nodeType === "token"}
+ {:else if node?.meta.nodeType === "token"}
Token
{/if}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/editor.svelte` around lines 470 - 484, Replace the five independent
Svelte {`#if`} blocks that check node?.meta.nodeType with a single conditional
chain using {`#if`} ... {:else if} ... {:else} so evaluation short-circuits after
the first match; locate the fragment that reads node?.meta.nodeType and convert
the separate checks for "token-set", "token-modifier", "token-context",
"token-group", and "token" into an ordered {`#if` node?.meta.nodeType ===
"token-set"} / {:else if node?.meta.nodeType === "token-modifier"} / {:else if
node?.meta.nodeType === "token-context"} / {:else if node?.meta.nodeType ===
"token-group"} / {:else if node?.meta.nodeType === "token"} chain to improve
performance and ensure only one branch renders.
Comment on lines
+326
to
+345
| export const findNodeAtPath = ( | ||
| contextId: string, | ||
| path: string[], | ||
| nodes: Map<string, TreeNode<TreeNodeMeta>>, | ||
| ): TreeNode<TreeNodeMeta> | undefined => { | ||
| let currentParentId: string | undefined = contextId; | ||
|
|
||
| for (const segment of path) { | ||
| const children = Array.from(nodes.values()).filter( | ||
| (n) => n.parentId === currentParentId, | ||
| ); | ||
| const match = children.find((n) => n.meta.name === segment); | ||
| if (!match) { | ||
| return undefined; | ||
| } | ||
| currentParentId = match.nodeId; | ||
| } | ||
|
|
||
| return currentParentId ? nodes.get(currentParentId) : undefined; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
findNodeAtPath scans all nodes for every path segment — O(n × depth).
Each segment calls Array.from(nodes.values()).filter(n => n.parentId === currentParentId), iterating the entire node map. createPathInContext (line 412) has the same pattern. For typical design-token trees this is fine, but if the tree grows large, consider building a parent→children index or reusing TreeStore.getChildren.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state.svelte.ts` around lines 326 - 345, findNodeAtPath is inefficient
because it re-scans the entire nodes Map for every path segment; similarly
createPathInContext repeats that pattern. Fix by looking up children by parentId
instead of filtering nodes.values() each iteration: build or reuse a
parent→children index (e.g. a Map<string, TreeNode[]> or call the existing
TreeStore.getChildren method) and use that to find the matching child for each
segment, then traverse via match.nodeId; update both findNodeAtPath and
createPathInContext to accept or access the children index/getChildren to avoid
O(n × depth) scans.
Comment on lines
+563
to
+599
| let newTokenId: string | undefined; | ||
|
|
||
| this.transact((tx) => { | ||
| // Create the path in the context | ||
| const parentId = createPathInContext( | ||
| contextId, | ||
| pathWithoutRoot, | ||
| tx as Transaction<TreeNodeMeta>, | ||
| nodes as Map<string, TreeNode<TreeNodeMeta>>, | ||
| ); | ||
|
|
||
| // Generate index for the new token | ||
| const siblings = this.#store.getChildren(parentId); | ||
| const lastIndex = | ||
| siblings.at(-1)?.index ?? generateKeyBetween(null, null); | ||
| const newIndex = generateKeyBetween(lastIndex, null); | ||
|
|
||
| // Create the token override | ||
| const newToken: TreeNode<TreeNodeMeta> = { | ||
| nodeId: crypto.randomUUID(), | ||
| parentId, | ||
| index: newIndex, | ||
| meta: { | ||
| nodeType: "token", | ||
| name: sourceMeta.name, | ||
| description: tokenData.description ?? sourceMeta.description, | ||
| deprecated: tokenData.deprecated ?? sourceMeta.deprecated, | ||
| type: tokenData.type ?? sourceMeta.type, | ||
| value: tokenData.value ?? sourceMeta.value, | ||
| } as TokenMeta, | ||
| }; | ||
|
|
||
| tx.set(newToken as TreeNode<Meta>); | ||
| newTokenId = newToken.nodeId; | ||
| }); | ||
|
|
||
| return newTokenId!; |
There was a problem hiding this comment.
getChildren inside the transaction won't see groups created by createPathInContext.
this.#store.getChildren(parentId) at line 575 reads from the store, which hasn't committed the groups staged via tx.set() in createPathInContext. When the parent was just created in the same transaction, getChildren returns an empty list, so the fallback index from generateKeyBetween(null, null) is used and the token is placed correctly as the first child.
This works today but is fragile — if future code adds other children to a newly-created parent within the same transaction, they could collide on ordering. Consider documenting this constraint or using the local nodes map for sibling lookups:
🛡️ Safer sibling lookup
// Generate index for the new token
- const siblings = this.#store.getChildren(parentId);
+ const siblings = Array.from(nodes.values())
+ .filter((n) => n.parentId === parentId)
+ .sort((a, b) => (a.index < b.index ? -1 : 1));
const lastIndex =
siblings.at(-1)?.index ?? generateKeyBetween(null, null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state.svelte.ts` around lines 563 - 599, The code calls
this.#store.getChildren(parentId) inside the same transact block after
createPathInContext, which won't see child nodes staged via tx.set; replace the
fragile store read with a local sibling lookup from the transaction-scoped nodes
map used earlier (the nodes: Map<string, TreeNode<TreeNodeMeta>> passed into
createPathInContext) — e.g., compute siblings by filtering/collecting entries in
nodes whose parentId === parentId and sort by index (or use nodes.get(parentId)
if a children list is stored there), then derive lastIndex from that local
collection before calling generateKeyBetween; retain the rest of the newToken
creation and tx.set(newToken) unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a UI for navigating and managing modifier nodes in the design tokens tree. This enables users to create and configure token modifiers with multiple contexts, switch between different views via tab navigation, and properly constrain drag-and-drop operations for the new node types.
Summary by CodeRabbit
New Features
Style
Chores