refactor(tool-input): eliminate SyncWrappers, add canonical toggle and dependsOn gating #3169
Conversation
…d dependsOn gating Replace 17+ individual SyncWrapper components with a single centralized ToolSubBlockRenderer that bridges the subblock store with StoredTool.params via synthetic store keys. This reduces ~1000 lines of duplicated wrapper code and ensures tool-input renders subblock components identically to the standalone SubBlock path. - Add ToolSubBlockRenderer with bidirectional store sync - Add basic/advanced mode toggle (ArrowLeftRight) using collaborative functions - Add dependsOn gating via useDependsOnGate (fields disable instead of hiding) - Add paramVisibility field to SubBlockConfig for tool-input visibility control - Pass canonicalModeOverrides through getSubBlocksForToolInput - Show (optional) label for non-user-only fields (LLM can inject at runtime) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile OverviewGreptile SummaryThis PR refactors tool-input rendering for registry tools to be driven by the block’s The change fits into the existing architecture by reusing the same SubBlock definitions/visibility primitives already used elsewhere (conditions, canonical pairs, dependsOn), while keeping a legacy Blocking issues found are around (1) default visibility inference in Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as ToolInput
participant Params as getSubBlocksForToolInput
participant SubStore as SubBlockStore
participant Renderer as ToolSubBlockRenderer
participant Gate as useDependsOnGate
participant Collab as collaborativeSetBlockCanonicalMode
UI->>Params: getSubBlocksForToolInput(toolId, blockType, {operation, params}, canonicalOverrides)
Params-->>UI: filtered subBlocks (+ oauthConfig)
UI->>Renderer: render each SubBlock (syntheticId)
Renderer->>SubStore: useSubBlockValue(blockId, syntheticId)
Renderer->>Gate: compute finalDisabled(dependsOn + disabled)
Note over Renderer,SubStore: init effect syncs tool.params -> store
Renderer->>SubStore: setStoreValue(toolParamValue)
Note over Renderer,UI: sync effect propagates store -> tool.params
Renderer->>UI: onParamChange(toolIndex, effectiveParamId, stringValue)
UI->>Collab: toggle canonical mode (basic/advanced)
Collab-->>UI: updates block canonicalModes (real-time)
UI->>Params: re-filter/re-resolve canonical pair mode
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const toolParamIds = new Set(Object.keys(toolConfig.params || {})) | ||
| const toolParamVisibility: Record<string, ParameterVisibility> = {} | ||
| for (const [paramId, param] of Object.entries(toolConfig.params || {})) { | ||
| toolParamVisibility[paramId] = | ||
| param.visibility ?? (param.required ? 'user-or-llm' : 'user-only') | ||
| } |
There was a problem hiding this comment.
Wrong default visibility
In getSubBlocksForToolInput, the fallback assigns param.required ? 'user-or-llm' : 'user-only' as the default visibility. That makes optional params default to user-only, which will hide them from the tool-input UI even though they’re intended to be user-editable (previous behavior showed these with an “(optional)” label).
This should match the existing defaulting used elsewhere (optional → user-or-llm, required → user-only/user-or-llm depending on policy), or at least avoid excluding optional params by default. As-is, many optional tool params will stop rendering for registry tools that don’t set paramVisibility explicitly.
| // Initialize the store on first mount | ||
| const hasInitializedRef = useRef(false) | ||
| useEffect(() => { | ||
| if (!hasInitializedRef.current && toolParamValue) { | ||
| hasInitializedRef.current = true | ||
| setStoreValue(toolParamValue) | ||
| } | ||
| }, []) // eslint-disable-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Store never initializes empty
The “Initialize the store on first mount” effect only runs when toolParamValue is truthy. If the current param is an empty string (a common initial state), the synthetic subblock store key is never initialized, so store-driven components may render with stale/undefined state until the user types.
This should initialize even for empty values (e.g., set once on mount regardless), or otherwise ensure useSubBlockValue starts from '' for these synthetic IDs.
| return ( | ||
| <div key={paramId} className='relative min-w-0 space-y-[6px]'> | ||
| <div className='flex items-center justify-between gap-[6px] pl-[2px]'> | ||
| <Label className='flex items-center gap-[6px] whitespace-nowrap font-medium text-[13px] text-[var(--text-primary)]'> | ||
| {title} | ||
| {isRequired && visibility === 'user-only' && <span className='ml-0.5'>*</span>} | ||
| {visibility !== 'user-only' && ( | ||
| <span className='ml-[6px] text-[12px] text-[var(--text-tertiary)]'>(optional)</span> | ||
| )} |
There was a problem hiding this comment.
Optional label logic broken
ParameterWithLabel shows “(optional)” for any visibility !== 'user-only'. That means user-or-llm and hidden/llm-only would be labeled optional, and it ignores isRequired for non-user-only params.
This should key off isRequired (or the param’s required flag) rather than visibility, otherwise required-but-user-or-llm params will be incorrectly labeled optional in the UI.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| onToggle: () => { | ||
| const nextMode = | ||
| canonicalMode === 'advanced' ? 'basic' : 'advanced' | ||
| collaborativeSetBlockCanonicalMode(blockId, canonicalId, nextMode) |
There was a problem hiding this comment.
Canonical mode toggle shared across same-type tool instances
Medium Severity
The canonical mode override (basic/advanced toggle) is stored on the agent block via collaborativeSetBlockCanonicalMode(blockId, canonicalId, nextMode), keyed only by canonicalId (e.g., channelId). When two tools of the same type are added, they share the same canonicalId values, so toggling the mode on one instance changes the mode for all instances of that type. The canonicalModeOverrides read from the agent block is also passed to getSubBlocksForToolInput for each tool instance, causing both to render the same mode variant.
Additional Locations (2)
| </p> | ||
| </Tooltip.Content> | ||
| </Tooltip.Root> | ||
| )} |
There was a problem hiding this comment.
Wand search UI duplicated across two components
Low Severity
The ParameterWithLabel component in tool-input.tsx duplicates the wand "Generate" search UI (button, input field, submit/cancel handlers, streaming state display, and canonical toggle) that already exists in renderLabel within sub-block.tsx. Both render the same Generate button, search input with identical CSS classes, key handlers, blur logic, and ArrowLeftRight canonical toggle. A shared component or extracted render function would avoid divergence if the wand UI behavior changes.
1 participant
Summary
*SyncWrappercomponents with a single centralizedToolSubBlockRendererthat bridges the subblock store withStoredTool.paramsvia synthetic store keys (~1000 lines reduced)ArrowLeftRight) matching standalone SubBlock behavior, using collaborative functions for real-time syncdependsOngating viauseDependsOnGateso fields with unmet dependencies show as disabled rather than being hiddenparamVisibilityfield toSubBlockConfigfor explicit tool-input visibility control(optional)label for non-user-only fields since the LLM can inject them at runtimeTest plan
bun test --filter "tool-input"— 42 tests passbun test --filter "params"— 31 tests pass🤖 Generated with Claude Code