Add multi-element support to react-grab package review - #4
Conversation
- Enhanced element validation logic in agent.ts to ensure valid HTML tag names are checked before proceeding. - Updated session handling in core.tsx to support multiple elements, improving the follow-up session submission process.
- Adjusted session handling logic to ensure proper setting of session elements and status updates, enhancing overall session management.
|
Skipped: This PR was opened by one of your excluded authors: ( |
WalkthroughThis pull request adds multi-element support to the react-grab package and related provider packages. Callback signatures across multiple files are updated from accepting single Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Agent
participant Session as Session Manager
participant StateMachine as State Machine
participant Renderer
User->>Agent: Select multiple elements
activate Agent
Agent->>Session: startSession(elements: Element[])
activate Session
Session->>StateMachine: Send FREEZE_ELEMENTS event
activate StateMachine
StateMachine->>StateMachine: Update context.frozenElements
StateMachine->>StateMachine: Set frozenElement = first element
StateMachine-->>Session: State updated
deactivate StateMachine
Session->>Renderer: Pass selectionBoundsMultiple[]
activate Renderer
Renderer->>Renderer: Iterate bounds list
Renderer->>Renderer: Render SelectionBox for each bound
Renderer->>Renderer: Display "N elements" in SelectionLabel
Renderer-->>User: Show multiple selection boxes
deactivate Renderer
deactivate Session
User->>Agent: Complete/Abort action
activate Agent
Agent->>Session: Trigger onComplete(session, elements[])
Session->>Session: Pass full elements array to callback
Session-->>Agent: Callback executed
deactivate Agent
deactivate Agent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/provider-visual-edit/src/client/index.ts (1)
447-463: Same multi-element concern applies here.Similar to
onStart, this function acceptselements: Element[]but only processeselements[0]. The error handling on lines 460-463 appropriately handles the case where no element is found, but the same questions about multi-element support intent apply.This has the same pattern and concerns as the
onStartfunction reviewed above. Please address both consistently when clarifying the multi-element support strategy.
🧹 Nitpick comments (3)
packages/provider-visual-edit/src/client/index.ts (1)
136-140: Clarify the multi-element support intent and consider documenting the single-element behavior.The function signature now accepts
elements: Element[], but the implementation only useselements[0]. While the subsequent null check on line 140 handles the case where the array is empty, this pattern may confuse API consumers who might expect all elements to be processed.If this is intentional preparation for future multi-element support, consider adding a JSDoc comment explaining that only the first element is currently processed. If multiple elements should be handled now, the implementation needs to be updated accordingly.
Can you clarify whether:
- This is a partial implementation with full multi-element support planned for later?
- The API is being future-proofed while maintaining single-element behavior?
- All elements in the array should be processed in this PR?
If single-element behavior is intentional, consider adding documentation:
📝 Suggested documentation
+ /** + * Called when a visual edit session starts. + * Note: Currently only processes the first element from the array. + * @param session - The agent session + * @param elements - Array of target elements (only first element is used) + */ const onStart = (session: AgentSession, elements: Element[]) => {Optionally, make the single-element extraction more explicit:
- const element = elements[0]; + const element = elements[0]; // Currently only process first elementpackages/react-grab/src/components/renderer.tsx (1)
28-39: Consider using index-based keying if bounds can be reordered.Solid's
<For>uses referential identity for tracking. IfselectionBoundsListitems can be reordered or replaced with different objects representing the same element, theSelectionBoxanimations may behave unexpectedly. If bounds objects are stable or order doesn't change, this is fine.If needed, you could use
<Index>instead for index-based tracking, or add a stable identifier to each bounds object.packages/react-grab/src/core.tsx (1)
520-524: Redundant frozen element check in effectiveElement memo.The logic checks
isToggleFrozen()first (which returns early withfrozenElement), then immediately checksfrozenElementagain. The second check (lines 520-523) appears redundant ifisToggleFrozen()already covers the frozen state.#!/bin/bash # Check the state machine to understand when isToggleFrozen vs frozenElement differ rg -n "frozenElement|isToggleFrozen|frozen" packages/react-grab/src/state/machine.ts -C3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
packages/cli/src/utils/logger.tspackages/provider-amp/CHANGELOG.mdpackages/provider-amp/README.mdpackages/provider-amp/tsconfig.jsonpackages/provider-visual-edit/src/client/code-validation.tspackages/provider-visual-edit/src/client/index.tspackages/react-grab/src/agent.tspackages/react-grab/src/components/icon-retry.tsxpackages/react-grab/src/components/renderer.tsxpackages/react-grab/src/components/selection-label/index.tsxpackages/react-grab/src/core.tsxpackages/react-grab/src/core/copy.tspackages/react-grab/src/core/events.tspackages/react-grab/src/core/noop-api.tspackages/react-grab/src/hooks/use-animated-position.tspackages/react-grab/src/state/machine.tspackages/react-grab/src/types.tspackages/react-grab/src/utils/get-tag-name.tspackages/react-grab/src/utils/is-selection-backward.tspackages/react-grab/src/utils/is-target-key-combination.tspackages/react-grab/src/utils/key-matches-code.tspackages/utils/package.jsonpackages/utils/tsconfig.jsonpackages/utils/tsup.config.tspackages/website/app/blog/visual-edit/layout.tsxpackages/website/app/blog/visual-edit/page.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
packages/react-grab/src/components/renderer.tsx (1)
packages/react-grab/src/components/selection-box.tsx (1)
SelectionBox(21-156)
packages/react-grab/src/types.ts (7)
packages/react-grab/src/core.tsx (3)
AgentSession(2518-2518)AgentCompleteResult(2521-2521)OverlayBounds(2514-2514)packages/react-grab/src/index.ts (3)
AgentSession(24-24)AgentCompleteResult(28-28)OverlayBounds(14-14)packages/provider-visual-edit/src/client/index.ts (1)
AgentCompleteResult(14-14)packages/provider-ami/src/client.ts (1)
AgentCompleteResult(28-28)packages/provider-claude-code/src/client.ts (1)
AgentCompleteResult(16-16)packages/provider-cursor/src/client.ts (1)
AgentCompleteResult(16-16)packages/provider-opencode/src/client.ts (1)
AgentCompleteResult(16-16)
packages/provider-visual-edit/src/client/index.ts (1)
packages/react-grab/src/types.ts (1)
AgentSession(121-133)
packages/react-grab/src/agent.ts (5)
packages/react-grab/src/core.tsx (1)
generateSnippet(2526-2526)packages/react-grab/src/index.ts (1)
generateSnippet(8-8)packages/react-grab/src/utils/generate-snippet.ts (1)
generateSnippet(7-24)packages/react-grab/src/context.ts (1)
getNearestComponentName(70-84)packages/react-grab/src/utils/agent-session.ts (1)
saveSessionById(56-63)
🔇 Additional comments (40)
packages/react-grab/src/components/icon-retry.tsx (1)
1-27: Formatting fix approved.The trailing newline addition is a minor cleanup that aligns with standard file formatting conventions. The IconRetry component implementation itself is solid—proper TypeScript typing, clean Solid.js syntax, and efficient SVG rendering.
packages/react-grab/src/utils/is-target-key-combination.ts (1)
74-74: Formatting improvement applied.Adding a trailing newline improves file consistency and is a standard practice. The function logic remains unchanged and correct.
packages/react-grab/src/utils/is-selection-backward.ts (1)
1-10: LGTM!The selection backward detection logic is sound. The function correctly uses DOM
compareDocumentPosition()to determine node ordering, with appropriate null checks and a fallback to offset comparison when nodes are identical. The trailing newline at line 10 is a formatting adjustment with no functional impact.packages/website/app/blog/visual-edit/page.tsx (1)
504-505: Trailing newline formatting is a no-op change.packages/website/app/blog/visual-edit/layout.tsx (1)
51-52: Trailing newline formatting is a no-op change.packages/react-grab/src/utils/get-tag-name.ts (1)
1-3: No action required. The function signature is correct as-is.The utility is designed to operate on individual
Elementinstances, with callers handling array iteration where needed (as shown by.map((element) => getTagName(element))patterns in the codebase). This is a proper separation of concerns and aligns with the PR's architecture.Likely an incorrect or invalid review comment.
packages/cli/src/utils/logger.ts (1)
19-21: LGTM!The
break()method provides a useful utility for adding visual separation in logs, following the same pattern as existing logger methods.packages/provider-visual-edit/src/client/index.ts (1)
520-522: LGTM! Signature updated for API consistency.The signature update to accept
_sessionand_elementsparameters maintains consistency with the other callback signatures while correctly indicating these parameters are unused (via underscore prefix). The comment appropriately explains that the actual undo logic is handled elsewhere.packages/react-grab/src/utils/key-matches-code.ts (1)
1-11: LGTM!Formatting-only change adding trailing newline. The function logic is correct.
packages/react-grab/src/core/events.ts (1)
16-48: LGTM!Clean abstraction for event listener management with proper AbortController-based cleanup. The trailing newline addition is a formatting-only change.
packages/react-grab/src/hooks/use-animated-position.ts (1)
11-68: LGTM!Well-implemented animation hook with proper cleanup. The trailing newline is a formatting-only change.
packages/react-grab/src/core/noop-api.ts (1)
3-29: LGTM!Formatting-only change adding trailing newline.
packages/react-grab/src/components/renderer.tsx (2)
16-24: LGTM!The
selectionBoundsListmemo correctly handles the fallback logic: prioritizingselectionBoundsMultiplewhen available and non-empty, otherwise wrapping singleselectionBoundsor returning an empty array.
131-136: LGTM!Correctly passes
elementsCounttoSelectionLabelto support the multi-element display.packages/react-grab/src/core/copy.ts (1)
23-92: LGTM!Formatting-only change adding trailing newline. The multi-element copy logic with fallbacks is well-structured.
packages/react-grab/src/components/selection-label/index.tsx (2)
138-148: LGTM!Correctly added
props.elementsCountto the measurement effect dependencies to ensure the label re-measures when the element count changes.
245-253: LGTM!The
tagDisplaylogic correctly handles multi-element selection by showing "N elements" whenelementsCount > 1, while preserving existing behavior for single-element cases.packages/react-grab/src/state/machine.ts (6)
38-39: LGTM!Adding
frozenElements: Element[]to the context correctly extends the state to support multi-element freezing.
86-89: LGTM!Correctly initialized
frozenElementsas an empty array in the initial context.
156-157: LGTM!The new
FREEZE_ELEMENTSevent type is well-defined with anelements: Element[]payload.
286-303: LGTM!The actions maintain consistency between
frozenElementandfrozenElements:
setFrozenElementwraps the single element in an array forfrozenElementssetFrozenElementsextracts the first element forfrozenElement(backward compatibility)clearFrozenElementclears both fieldsThis ensures code relying on either the single or multi-element representation works correctly.
555-562: LGTM!
resetActivationStatecorrectly clearsfrozenElementsalongside the other activation state.
744-746: LGTM!The
FREEZE_ELEMENTSevent is correctly wired to thesetFrozenElementsaction at the activation state level.packages/react-grab/src/types.ts (3)
173-183: LGTM! Clean type signature updates for multi-element support.The callback signatures are consistently updated from
Element | undefinedtoElement[]. Using arrays is a better design choice as it handles both single and multiple element cases uniformly, and avoids undefined checks in favor of empty array checks.
288-289: LGTM! Additive props for multiple selection rendering.New optional props
selectionBoundsMultipleandselectionElementsCountextend the renderer capabilities without breaking existing consumers.
424-424: LGTM! Consistent addition of elementsCount prop.Aligns with the multi-element model and follows the same optional pattern as other props.
packages/react-grab/src/agent.ts (7)
23-24: LGTM! Clear parameter type update.
StartSessionParamsnow properly acceptselements: Element[]for multi-element support.
37-38: LGTM! Public API expanded with getElements accessor.Good addition maintaining backward compatibility with
getElementreturning the first element while exposing the full array viagetElements.
200-201: Good defensive check for multi-element tagName format.The
isValidHtmlTagNamecheck correctly identifies when the tagName contains a space (e.g., "3 elements"), preventing incorrect tag comparison during element reacquisition. This ensures reacquisition only occurs for single-element sessions with valid HTML tag names.
317-324: Well-designed tagName/componentName handling for multi-element sessions.
- For multiple elements: displays "n elements" as tagName, omits componentName
- For single element: uses actual tagName and attempts to resolve componentName
This provides clear user feedback while maintaining context accuracy.
465-466: Bounds update only considers first element for multi-element sessions.For sessions with multiple elements, only the first element's bounds are tracked/updated on viewport changes. This is consistent with the existing single-element behavior and reasonable for positioning the session UI, but may cause visual drift if the first element moves differently than others.
496-503: LGTM! Clean accessor pattern for backward compatibility.
getSessionElementreturns the first element (backward compatible), whilegetSessionElementsreturns the full array. The fallback to empty array (?? []) is consistent with the type signature.Also applies to: 517-517
373-376: Consider whether dismissSession should proceed when elements array is empty.The condition
elements.length > 0gates callingonDismiss, but the session cleanup (lines 377-384) still proceeds regardless. This is intentional—the session should be cleaned up even if no elements are found. Note thatonUndobehaves differently, calling its callback without checking elements (line 392), suggesting thatonDismissis specifically intended to fire only when elements exist.packages/react-grab/src/core.tsx (7)
547-556: LGTM! Clean memos for multi-element bounds tracking.
frozenElementsBoundscorrectly maps all frozen elements to their boundsfrozenElementsCountprovides efficient count access- Both react to viewport changes via
viewportVersiondependency
952-972: LGTM! Restore logic properly adapted for Element[].The function accepts the full array but correctly uses the first element for positioning and UI restoration. The
FREEZE_ELEMENTSmessage propagates the complete array to the state machine.
997-1007: LGTM! Robust elements array construction.Properly handles three cases:
- Multiple frozen elements → uses frozenElements array
- Single frozen/target element → wraps in array
- No element → empty array (triggers early return on line 1001-1004)
1208-1211: Good fix for race condition with clear documentation.The HACK comment explains why
dragRectmust be computed beforeDRAG_END—the send resetsdragStartin state. Pre-computing avoids stale/incorrect bounds.
1247-1247: Correctly uses FREEZE_ELEMENTS for drag selection.After drag-selecting multiple elements, they're all frozen together, enabling multi-element agent prompts.
2291-2292: LGTM! Renderer props and follow-up session properly wired.
selectionBoundsMultipleandselectionElementsCountpassed to renderer- Follow-up session creation uses
getElements()and validateselements.length > 0- Session start correctly receives the elements array
Also applies to: 2330-2332, 2340-2340
2462-2469: LGTM! setAgent API updated consistently.The
onAbortandonUndohandlers correctly receiveElement[]and pass them through to both the user callback andrestoreInputFromSession.
2 participants
Summary by CodeRabbit
Release Notes
New Features
getElements()API to retrieve all selected elements for a session.Refactor
✏️ Tip: You can customize this high-level summary in your review settings.