feat(shared-ui): add variable picker affordance to config fields#1276
Draft
dsapandora wants to merge 1 commit into
Draft
feat(shared-ui): add variable picker affordance to config fields#1276dsapandora wants to merge 1 commit into
dsapandora wants to merge 1 commit into
Conversation
dsapandora
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID. Do not edit or delete. |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a ChangesVariable Picker Affordance
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/shared-ui/src/components/canvas/components/rjsf-widgets/base-input-template/BaseInputTemplate.tsx`:
- Around line 214-220: The picker IconButton components are set to
tabIndex={-1}, which removes them from keyboard navigation and makes them
inaccessible to keyboard-only users. Remove the tabIndex={-1} attribute from the
picker IconButton in
packages/shared-ui/src/components/canvas/components/rjsf-widgets/base-input-template/BaseInputTemplate.tsx
(lines 214-220) with the aria-label "Insert variable", and apply the same fix by
removing tabIndex={-1} from the picker IconButton in
packages/shared-ui/src/components/canvas/components/rjsf-widgets/textarea-widget/TextareaWidget.tsx
(lines 110-116). This will allow both picker buttons to participate in the
normal tab order and be discoverable to keyboard users.
In
`@packages/shared-ui/src/components/canvas/components/rjsf-widgets/hooks/useEnvVarAutocomplete.ts`:
- Around line 115-128: The new openAll callback function in the
useEnvVarAutocomplete hook introduces a second code path for handling
environment variable autocomplete insertion (cursor-position based insertion
without requiring the ${ prefix). Add test cases in the hook tests or widget
tests that specifically validate this openAll behavior, including tests that
confirm the triggerStartRef is correctly set to the cursor position, that
setSuggestions populates with envKeys, that setIsOpen sets the popover to open
state, and that setHighlightedIndex initializes to 0. These tests should cover
the insertion semantics when openAll is called with a valid anchorElement and
cursorPos to ensure this alternative entry path works correctly alongside the
existing trigger-based path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05b88e22-d948-46e0-8c5c-09f934c3a9c8
📒 Files selected for processing (3)
packages/shared-ui/src/components/canvas/components/rjsf-widgets/base-input-template/BaseInputTemplate.tsxpackages/shared-ui/src/components/canvas/components/rjsf-widgets/hooks/useEnvVarAutocomplete.tspackages/shared-ui/src/components/canvas/components/rjsf-widgets/textarea-widget/TextareaWidget.tsx
Comment on lines
+214
to
+220
| <IconButton | ||
| size="small" | ||
| edge="end" | ||
| tabIndex={-1} | ||
| aria-label="Insert variable" | ||
| title="Insert variable" | ||
| onClick={handleOpenPicker} |
Contributor
There was a problem hiding this comment.
Shared root cause: picker affordance is excluded from keyboard navigation via tabIndex={-1}. This defeats the discoverability fix for keyboard-only users in both widgets.
packages/shared-ui/src/components/canvas/components/rjsf-widgets/base-input-template/BaseInputTemplate.tsx#L214-L220: removetabIndex={-1}from the pickerIconButtonso it participates in tab order.packages/shared-ui/src/components/canvas/components/rjsf-widgets/textarea-widget/TextareaWidget.tsx#L110-L116: apply the same change to the textarea pickerIconButton.
📍 Affects 2 files
packages/shared-ui/src/components/canvas/components/rjsf-widgets/base-input-template/BaseInputTemplate.tsx#L214-L220(this comment)packages/shared-ui/src/components/canvas/components/rjsf-widgets/textarea-widget/TextareaWidget.tsx#L110-L116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/shared-ui/src/components/canvas/components/rjsf-widgets/base-input-template/BaseInputTemplate.tsx`
around lines 214 - 220, The picker IconButton components are set to
tabIndex={-1}, which removes them from keyboard navigation and makes them
inaccessible to keyboard-only users. Remove the tabIndex={-1} attribute from the
picker IconButton in
packages/shared-ui/src/components/canvas/components/rjsf-widgets/base-input-template/BaseInputTemplate.tsx
(lines 214-220) with the aria-label "Insert variable", and apply the same fix by
removing tabIndex={-1} from the picker IconButton in
packages/shared-ui/src/components/canvas/components/rjsf-widgets/textarea-widget/TextareaWidget.tsx
(lines 110-116). This will allow both picker buttons to participate in the
normal tab order and be discoverable to keyboard users.
Comment on lines
+115
to
+128
| const openAll = useCallback( | ||
| (anchorElement: HTMLElement | null, cursorPos: number) => { | ||
| if (!anchorElement || !envKeys.length) return; | ||
| // No `${` was typed — insert at the current cursor position. handleSelect | ||
| // replaces from triggerStart to the cursor, so a zero-width range there | ||
| // just inserts `${KEY}` without clobbering surrounding text. | ||
| triggerStartRef.current = cursorPos; | ||
| setSuggestions(envKeys); | ||
| setAnchorEl(anchorElement); | ||
| setHighlightedIndex(0); | ||
| setIsOpen(true); | ||
| }, | ||
| [envKeys], | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add focused tests for the new openAll insertion path.
openAll introduces a second entry path into selection/insertion semantics. Add hook/widget tests that validate cursor-position insertion and popover open state so regressions don’t slip in.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/shared-ui/src/components/canvas/components/rjsf-widgets/hooks/useEnvVarAutocomplete.ts`
around lines 115 - 128, The new openAll callback function in the
useEnvVarAutocomplete hook introduces a second code path for handling
environment variable autocomplete insertion (cursor-position based insertion
without requiring the ${ prefix). Add test cases in the hook tests or widget
tests that specifically validate this openAll behavior, including tests that
confirm the triggerStartRef is correctly set to the cursor position, that
setSuggestions populates with envKeys, that setIsOpen sets the popover to open
state, and that setHighlightedIndex initializes to 0. These tests should cover
the insertion semantics when openAll is called with a valid anchorElement and
cursorPos to ensure this alternative entry path works correctly alongside the
existing trigger-based path.
03730e8 to
563271d
Compare
Saved variables were only discoverable by typing ${ — the env-var suggestion popover never appeared otherwise. Add a small ${} button to the config field widgets (BaseInputTemplate, TextareaWidget, ApiKeyWidget) that opens the full list of saved variables and inserts ${ROCKETRIDE_NAME} on select, reusing the existing suggestion list and insert logic.
On ApiKeyWidget the button shows only while the field is editable (an existing key stays masked + read-only until cleared via the trash button). Shown only when variables exist (envKeys). Works in web and VS Code since the widgets are shared.
dsapandora
force-pushed
the
feat/config-field-variable-picker
branch
from
563271d to
4010043
Compare
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.
Summary
${}button to the config field widgets (BaseInputTemplate,TextareaWidget) that opens the list of saved variables — no need to know the${...}syntax orROCKETRIDE_prefix.${ROCKETRIDE_NAME}at the cursor, reusing the existing suggestion list and insert logic (newopenAllonuseEnvVarAutocomplete).envKeys) and the field is editable. Works in web and VS Code (shared widgets).Type
feat
Testing
./builder testpassesChecklist
Linked Issue
Fixes #1275
Summary by CodeRabbit
Release Notes