Conversation
WalkthroughThis pull request introduces multi-node support to handle duplicate nodes as grouped selections. The changes include adding a Changes
Sequence DiagramsequenceDiagram
participant User
participant Index
participant GroupingLogic as Grouping<br/>(useMemo)
participant ListItem
participant ConnectView
participant MutationAPI
User->>Index: Select nodes with duplicates
Index->>GroupingLogic: groupedSelection (group by chars + key/ns)
GroupingLogic-->>Index: {node, groupedNodes, duplicatesCount}
Index->>ListItem: items with groupedNodes
alt User modifies key/namespace
ListItem->>MutationAPI: Mutate key/ns for all effectiveNodes
MutationAPI-->>ListItem: Update complete
else User clicks connect
ListItem->>ConnectView: setRoute("connect", {nodes: effectiveNodes})
ConnectView->>MutationAPI: Update all nodes (translation, key, ns, connected)
MutationAPI-->>ConnectView: Batch update complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 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/ui/views/Index/Index.tsx`:
- Around line 97-103: The grouping uses node.characters.trim() which merges
distinct-whitespace text nodes; change groupedSelection's grouping to use the
raw node.characters (no trim) so groupKey is built from node.characters (and
existing connected branch `connected:${node.key}::${node.ns ??
""}::${node.characters}`) to ensure grouping matches the rest of the pipeline
that expects exact characters; update the useMemo grouping logic that references
selection and node.characters accordingly.
In `@src/ui/views/Index/ListItem.tsx`:
- Around line 132-144: handleNsChange currently mutates the objects in
effectiveNodes in place; instead, avoid in-place mutation by creating a new
array of node objects and passing that to setNodesDataMutation.mutate (use
effectiveNodes.map to return {...current, key: keyName, ns: value}) and call
setNamespace(value) as before; do not assign to current.key/current.ns, and if
you need to suppress any follow-up effect that reacts to effectiveNodes updates,
track the user-intended namespace change in local component state or a ref
(e.g., a pendingNs ref) so downstream grouping/effect logic ignores transient
UI-only changes until the mutation succeeds.
- Around line 36-42: The current useMemo that creates effectiveNodes from
groupedNodes and node causes silent bulk-overwrites of all members in a group
when debounced edits occur; change the logic in ListItem so that groupedNodes
are only expanded into per-row edits when the user explicitly edits this row
(e.g., an edit flag/state set in your onChange/onBlur handlers) or only
bulk-sync when every member in groupedNodes already has identical key and ns
values to node before applying the fan-out; update the effectiveNodes
computation and any code paths that currently write back to grouped members (the
groupedNodes, node and effectiveNodes logic and the edit handlers used in this
component) to gate the fan-out behind that explicit-edit flag or the equality
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 036d0780-09a9-4c4f-bd7f-fac4f51a7343
📒 Files selected for processing (5)
src/ui/components/NodeList/NodeRow.tsxsrc/ui/views/Connect/Connect.tsxsrc/ui/views/Index/Index.tsxsrc/ui/views/Index/ListItem.tsxsrc/ui/views/routes.ts
| const groupedSelection = useMemo(() => { | ||
| const groups = new Map<string, NodeInfo[]>(); | ||
| selection.forEach((node) => { | ||
| const textKey = node.characters.trim(); | ||
| const groupKey = node.connected | ||
| ? `connected:${node.key}::${node.ns ?? ""}::${textKey}` | ||
| : `unconnected:${textKey}`; |
There was a problem hiding this comment.
Don't group whitespace-distinct texts together.
Using trim() here makes "Hello" and "Hello " share one row, but the rest of the pipeline still treats characters as exact text. That lets bulk edits/connects fan out across nodes that can still conflict on push. Group on the raw characters value, or normalize whitespace consistently everywhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/views/Index/Index.tsx` around lines 97 - 103, The grouping uses
node.characters.trim() which merges distinct-whitespace text nodes; change
groupedSelection's grouping to use the raw node.characters (no trim) so groupKey
is built from node.characters (and existing connected branch
`connected:${node.key}::${node.ns ?? ""}::${node.characters}`) to ensure
grouping matches the rest of the pipeline that expects exact characters; update
the useMemo grouping logic that references selection and node.characters
accordingly.
| const effectiveNodes = useMemo( | ||
| () => | ||
| groupedNodes && groupedNodes.length > 0 | ||
| ? groupedNodes | ||
| : [node], | ||
| [groupedNodes, node] | ||
| ); |
There was a problem hiding this comment.
Avoid bulk-rewriting grouped nodes on initial render.
With the new grouping in src/ui/views/Index/Index.tsx, unconnected duplicates can share a row even when their draft key/ns values already differ. This effect will then overwrite every node in the group to match the first node's keyName after the debounce, without any user edit in this row. Please gate the fan-out behind an explicit edit, or only bulk-sync groups whose current key/ns already agree.
Also applies to: 75-92
🧰 Tools
🪛 GitHub Check: Static check 🪲
[failure] 37-37:
Replace ⏎······groupedNodes·&&·groupedNodes.length·>·0⏎········?·groupedNodes⏎········:·[node] with ·(groupedNodes·&&·groupedNodes.length·>·0·?·groupedNodes·:·[node])
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/views/Index/ListItem.tsx` around lines 36 - 42, The current useMemo
that creates effectiveNodes from groupedNodes and node causes silent
bulk-overwrites of all members in a group when debounced edits occur; change the
logic in ListItem so that groupedNodes are only expanded into per-row edits when
the user explicitly edits this row (e.g., an edit flag/state set in your
onChange/onBlur handlers) or only bulk-sync when every member in groupedNodes
already has identical key and ns values to node before applying the fan-out;
update the effectiveNodes computation and any code paths that currently write
back to grouped members (the groupedNodes, node and effectiveNodes logic and the
edit handlers used in this component) to gate the fan-out behind that
explicit-edit flag or the equality check.
| const handleNsChange = () => (value: string) => { | ||
| setNamespace(value); | ||
| setNodesDataMutation.mutate({ | ||
| nodes: [{ ...node, key: keyName, ns: value }], | ||
| nodes: effectiveNodes.map((current) => ({ | ||
| ...current, | ||
| key: keyName, | ||
| ns: value, | ||
| })), | ||
| }); | ||
| effectiveNodes.forEach((current) => { | ||
| current.key = keyName; | ||
| current.ns = value; | ||
| }); |
There was a problem hiding this comment.
Don't mutate effectiveNodes in place.
These objects come from props/selection state. Rewriting current.key and current.ns locally makes the UI look updated before setNodesDataMutation succeeds, and it can leak stale values into later grouping/effect logic. Keep the array immutable and suppress the follow-up effect with local state/ref instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/views/Index/ListItem.tsx` around lines 132 - 144, handleNsChange
currently mutates the objects in effectiveNodes in place; instead, avoid
in-place mutation by creating a new array of node objects and passing that to
setNodesDataMutation.mutate (use effectiveNodes.map to return {...current, key:
keyName, ns: value}) and call setNamespace(value) as before; do not assign to
current.key/current.ns, and if you need to suppress any follow-up effect that
reacts to effectiveNodes updates, track the user-intended namespace change in
local component state or a ref (e.g., a pendingNs ref) so downstream
grouping/effect logic ignores transient UI-only changes until the mutation
succeeds.
1 participant
Issue Source: #92
Summary by CodeRabbit
Release Notes