Conversation
WalkthroughThis update introduces a modular and extensible autocomplete dropdown for template variable selection in the builder UI. It refactors the autocomplete logic in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BuilderTemplateInput
participant BuilderStateSelectorDropdown
participant Core/State
User->>BuilderTemplateInput: Type "@" in input field
BuilderTemplateInput->>BuilderStateSelectorDropdown: Show dropdown with query
BuilderStateSelectorDropdown->>Core/State: Fetch user state, secrets, bindings, results
BuilderStateSelectorDropdown-->>BuilderTemplateInput: Emit selection event on item click
BuilderTemplateInput->>BuilderTemplateInput: Insert selected variable into input
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![hackerone-code[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 This pull request has been sent to HackerOne's PullRequest review team because our automation detected one or more changes with potential security impact or requires further evaluation. Experts are now being assigned to this review based on relevant expertise and will validate or dismiss any security findings accordingly and post their feedback as comments within this pull request.
⏱️ Latest scan covered changes up to commit 0ab65d4 (latest)
Check the status or cancel this secure code review here.
There was a problem hiding this comment.
✅ Graham C reviewed all the included code changes and associated automation findings and determined that there were no immediately actionable security flaws. Note that they will continue to be notified of any new commits or comments and follow up as needed throughout the duration of this pull request's lifecycle.
Reviewed with ❤️ by PullRequest
14ba2b3 to
5bd644a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
src/ui/src/utils/template.spec.ts (1)
1-35: Well-structured tests with good coverage.The parameterized tests effectively cover the main scenarios for both functions. Consider adding test cases for:
- Empty variable name:
autocompleteTemplateVariable("@{", "")- Multiple consecutive templates:
"@{var1}@{var2"- Escaped braces if supported by the template syntax
src/ui/src/wds/WdsDropdownMenuItem.vue (1)
15-27: Consider simplifying the iconStyle computed property.The current implementation works well, but you could simplify it slightly:
const iconStyle = computed(() => { - const style: CSSProperties = {}; - - if (props.option.iconColor) { - style.color = props.option.iconColor; - } - - if (props.option.iconBgColor) { - style["background-color"] = props.option.iconBgColor; - } - - return Object.values(style).length > 0 ? style : undefined; + if (!props.option.iconColor && !props.option.iconBgColor) { + return undefined; + } + + return { + ...(props.option.iconColor && { color: props.option.iconColor }), + ...(props.option.iconBgColor && { 'background-color': props.option.iconBgColor }) + }; });src/ui/src/utils/template.ts (1)
1-71: Consider refactoring the complex autocomplete function for better maintainability.The
autocompleteTemplateVariablefunction handles multiple regex patterns and overlap detection, making it complex to understand and maintain. Consider breaking it down into smaller, focused functions.+// Helper function to find template matches +function findTemplateMatches(input: string): { start: number; end: number }[] { + const matches: { start: number; end: number }[] = []; + + // Extract pattern matching logic into separate functions + matches.push(...findBracedTemplateMatches(input)); + matches.push(...findDollarTemplateMatches(input)); + matches.push(...findStandaloneAtMatches(input, matches)); + + return matches; +} + export function autocompleteTemplateVariable(input: string, variable: string) { - const matches: { start: number; end: number }[] = []; - - // Pattern 2: @{...} or @{... - const pattern2 = /@\{[^}]*\}|@\{[^}\s]*/g; - // ... complex pattern matching logic + const matches = findTemplateMatches(input); if (matches.length === 0) { return input; }src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdownComponent.vue (1)
9-13: Consider adding prop validation for better type safety.The props definition could benefit from more specific validation, especially for the
pathprop which seems to be a key identifier.const props = defineProps({ - componentId: { type: String, required: true }, - path: { type: String, required: true }, + componentId: { + type: String, + required: true, + validator: (value: string) => value.length > 0 + }, + path: { + type: String, + required: true, + validator: (value: string) => value.length > 0 + }, selected: { type: Boolean, required: false }, });src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue (3)
27-39: Consider optimizing the filtering approach for better performance.The current implementation filters paths before creating option objects, which is good. However, for consistency with other sections that use Fuse.js for fuzzy search, consider applying the same pattern here. Alternatively, if exact substring matching is intentional here while fuzzy search is used elsewhere, document this design decision.
const userState = computed<WdsDropdownMenuOption[]>(() => { const options: WdsDropdownMenuOption[] = []; for (const path of extractObjectPaths(wf.userStateInitial.value)) { - if (props.query && !path.includes(props.query)) continue; options.push({ value: path, label: path, icon: "code", iconBgColor: WdsColor.Green2, }); } - return options.sort((a, b) => a.label.localeCompare(b.label)); + const sorted = options.sort((a, b) => a.label.localeCompare(b.label)); + return filterOptions(sorted); });
70-80: Optimize Fuse.js instance creation for better performance.Creating a new Fuse instance on every call to
filterOptionscould impact performance, especially with frequent recomputations. Consider caching Fuse instances or using a more efficient approach.+const fuseCache = new WeakMap<WdsDropdownMenuOption[], Fuse<WdsDropdownMenuOption>>(); + function filterOptions(options: WdsDropdownMenuOption[]) { if (!props.query) return options; - const fuse = new Fuse(options, { - findAllMatches: true, - includeMatches: true, - keys: ["value"], - }); + let fuse = fuseCache.get(options); + if (!fuse) { + fuse = new Fuse(options, { + findAllMatches: true, + includeMatches: true, + keys: ["value"], + }); + fuseCache.set(options, fuse); + } return fuse.search(props.query).map((res) => res.item); }
184-186: Capitalize section title for consistency.The section title should be capitalized to match other sections.
<p class="BuilderStateSelectorDropdown__section__title"> - block results + Block results </p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/ui/src/builder/stateDropdown/__snapshots__/BuilderStateSelectorDropdown.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
src/ui/src/builder/settings/BuilderFieldsText.vue(2 hunks)src/ui/src/builder/settings/BuilderSettingsBinding.vue(3 hunks)src/ui/src/builder/settings/BuilderTemplateInput.spec.ts(8 hunks)src/ui/src/builder/settings/BuilderTemplateInput.vue(5 hunks)src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.spec.ts(1 hunks)src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue(1 hunks)src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdownComponent.vue(1 hunks)src/ui/src/builder/useComponentActions.ts(1 hunks)src/ui/src/builder/useComponentDescription.ts(1 hunks)src/ui/src/builder/useDynamicUserState.spec.ts(1 hunks)src/ui/src/builder/useDynamicUserState.ts(1 hunks)src/ui/src/composables/useFocusWithin.ts(2 hunks)src/ui/src/core/index.ts(4 hunks)src/ui/src/tests/mocks.ts(3 hunks)src/ui/src/utils/object.ts(1 hunks)src/ui/src/utils/template.spec.ts(1 hunks)src/ui/src/utils/template.ts(1 hunks)src/ui/src/wds/WdsDropdownMenu.spec.ts(2 hunks)src/ui/src/wds/WdsDropdownMenu.vue(3 hunks)src/ui/src/wds/WdsDropdownMenuItem.vue(1 hunks)src/ui/src/wds/WdsTextInput.vue(1 hunks)src/ui/src/wds/tokens.ts(1 hunks)tests/e2e/tests/stateAutocompletion.spec.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/ui/src/builder/useComponentDescription.ts (1)
src/ui/src/utils/url.ts (1)
convertAbsolutePathtoFullURL(9-14)
src/ui/src/utils/template.spec.ts (1)
src/ui/src/utils/template.ts (2)
autocompleteTemplateVariable(1-71)getCurrentOpenedTemplate(73-104)
src/ui/src/builder/useDynamicUserState.ts (4)
src/ui/src/writerTypes.ts (1)
Core(9-9)src/ui/src/composables/useLogger.ts (1)
useLogger(6-16)src/ui/src/utils/object.ts (1)
extractObjectPaths(1-19)src/ui/src/constants/component.ts (2)
COMPONENT_TYPES_PAGE(2-2)COMPONENT_TYPES_ROOT(1-1)
src/ui/src/builder/useDynamicUserState.spec.ts (2)
src/ui/src/tests/mocks.ts (1)
buildMockComponent(24-33)src/ui/src/builder/useDynamicUserState.ts (1)
useDynamicUserState(16-49)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: tests (firefox)
- GitHub Check: build (3.13)
- GitHub Check: build (3.9)
- GitHub Check: build (3.11)
- GitHub Check: tests (webkit)
- GitHub Check: tests (chromium)
🔇 Additional comments (44)
src/ui/src/composables/useFocusWithin.ts (1)
8-8: LGTM! Enhanced null safety for element references.The type change to allow nullable element references and the corresponding null check using optional chaining improve the robustness of focus tracking. This prevents potential runtime errors when element references are null or undefined.
Also applies to: 49-49
src/ui/src/builder/useComponentDescription.ts (1)
42-52: Well-structured refactoring with improved readability.The extraction of
componentTypeandcategoryinto local constants improves reusability, and the conditional logic for blueprint components with proper filtering ensures no undefined values are passed to URL conversion. The array construction is clean and maintainable.src/ui/src/core/index.ts (2)
3-11: Clean import organization with new ShallowRef type.The import reorganization improves readability and includes the necessary
ShallowReftype for the newuserStateInitialfunctionality.
65-65: Well-implemented initial state tracking.The addition of
userStateInitialfollows the established pattern of reactive state management in the core. Proper initialization during session setup and readonly exposure maintain consistency with existing state handling.Also applies to: 129-129, 874-874
src/ui/src/wds/tokens.ts (1)
41-42: LGTM! Clean addition of new color token.The
Yellow2color token follows the established naming convention and color system pattern. The hex value#FFE999is appropriate for a light yellow color.src/ui/src/wds/WdsTextInput.vue (1)
22-22: Improved logic for right icon visibility.Removing the dependency on
modelvalue for right icon visibility is correct for toggle buttons (like dropdown arrows) that should be shown regardless of input content. This change supports the new autocomplete dropdown functionality mentioned in the PR objectives.src/ui/src/builder/settings/BuilderFieldsText.vue (1)
11-11: LGTM: Consistent prop passing for autocomplete functionality.The addition of
:component-id="componentId"to both BuilderTemplateInput instances correctly passes the component context needed for the new autocomplete dropdown functionality.Also applies to: 28-28
src/ui/src/wds/WdsDropdownMenu.spec.ts (1)
7-7: LGTM: Improved test maintainability with component-based queries.The refactor from DOM selectors to
findAllComponents(WdsDropdownMenuItem)makes the test more maintainable and less coupled to DOM implementation details while preserving the same test logic.Also applies to: 27-29
src/ui/src/builder/useComponentActions.ts (1)
1099-1101: LGTM: Enhanced function with useful return value.The modification to store and return the
pageIdmakes the function more useful for callers while maintaining backward compatibility. This supports the new navigation features in the autocomplete dropdown components.src/ui/src/tests/mocks.ts (1)
38-38: LGTM: Consistent mock implementation for new state functionality.The addition of
userStateInitialfollows the established pattern for reactive references in the mock core, properly initializing and exposing it for test usage.Also applies to: 53-53, 73-73
src/ui/src/builder/settings/BuilderSettingsBinding.vue (3)
4-23: LGTM! Good refactoring to simplify the template structure.The removal of the wrapper div and direct application of the class to
WdsFieldWrapperreduces DOM nesting. The new propscomponent-id,hide-dropdown-secrets, andhide-dropdown-blueprint-resultsalign well with the new dropdown functionality.
54-56: Good CSS refactoring following BEM convention.The rename from
.mainto.BuilderSettingsBinding__mainimproves CSS organization and prevents potential naming conflicts.
38-39: Non-null assertions are safe:coreandbuilderManagerare globally provided
The injections forinjectionKeys.coreandinjectionKeys.builderManagerare registered insrc/ui/src/main.ts(lines 56–57) viaapp.provide, so they cannot beundefinedat runtime. No changes needed.src/ui/src/wds/WdsDropdownMenu.vue (2)
68-76: Excellent component extraction for better modularity.The replacement of inline dropdown item rendering with the dedicated
WdsDropdownMenuItemcomponent improves code organization and reusability. The props and event handling are properly maintained.
90-91: Good addition of styling properties to the option type.The
iconColorandiconBgColorproperties provide flexibility for custom icon styling in dropdown items.tests/e2e/tests/stateAutocompletion.spec.ts (2)
31-48: Good refactoring to use component-based selectors.The updated approach of first locating the field container and then querying dropdown items within it is more maintainable and aligns with the new component structure.
147-161: Consistent selector updates across all field type tests.The uniform application of the new selector pattern across all test cases ensures consistency and maintainability.
src/ui/src/wds/WdsDropdownMenuItem.vue (1)
30-83: Well-structured template with good accessibility.The component template handles various configurations elegantly:
- Proper conditional rendering for icons
- Overflow tooltips for long text
- Flexible action area with slot support
- Appropriate data attributes for testing
src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.spec.ts (5)
15-28: Test setup looks comprehensive and well-structured.The beforeEach setup properly initializes mocks for core and secrets manager, ensuring isolated test execution.
30-47: Clean mount helper function with proper dependency injection.The mount helper correctly provides all necessary dependencies through injection keys and stubs shared components appropriately.
49-58: Good test coverage for initial state selection.The test properly sets up mock data and verifies the emitted event contains the expected state reference.
97-136: Comprehensive search functionality test with snapshot verification.The test sets up multiple data sources (user state, components, secrets) and verifies both the initial rendered state and filtered results. Using snapshot testing provides additional regression protection.
138-152: No changes needed for the add-functionality testThe implementation only shows the “create new” option when there are no exact matches and
allowCreateis true, so expecting a singleWdsDropdownMenuItemis correct. No update to the test is required.src/ui/src/builder/settings/BuilderTemplateInput.spec.ts (5)
7-8: Correct imports for new component architecture.Adding imports for
BuilderStateSelectorDropdownandWdsDropdownMenuItemaligns with the refactored component structure.
17-17: Appropriate mock update for initial user state.Changing from
mockCore.userState.valuetomockCore.userStateInitial.valuecorrectly reflects the new state management structure.
54-56: Clean refactor to component-based testing.The transition from DOM queries to component-based queries using
getComponent()andfindAllComponents()is more robust and maintainable.
72-77: Good addition of flushPromises for async operations.Adding
await flushPromises()after setting input values ensures async updates in the new dropdown component are properly awaited before assertions.
124-126: Improved assertion using data-automation-key attributes.Using
data-automation-keyattributes for assertions is more reliable than text-based matching and follows good testing practices.src/ui/src/builder/useDynamicUserState.spec.ts (4)
19-35: Good refactor to test component mappings instead of state values.The updated test correctly expects the new structure where bindings return a mapping of state references to component objects, rather than nested state values.
37-52: Proper test for initial state collision handling.The test correctly verifies that bindings are not returned when the key already exists in the initial user state, preventing conflicts.
68-85: Well-structured test for blueprint set states.The test properly validates that components with JSON content are mapped to their element keys, following the expected data structure.
87-101: Good edge case handling for empty element keys.Testing the behavior when the element key is empty ensures robustness and prevents invalid entries in the mapping.
src/ui/src/utils/template.ts (1)
73-104: Well-implemented stack-based template parsing.The
getCurrentOpenedTemplatefunction uses a clean stack-based approach to track nested template openers and closers. The logic is clear and handles the complexity well.src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdownComponent.vue (2)
36-49: Well-structured navigation function with proper state management.The
goToComponentfunction correctly handles selection state and mode switching based on parent page type. The async handling and state updates are appropriate.
64-64: Good use of click.prevent for action button.Using
@click.preventon the action icon prevents unintended side effects while allowing the custom navigation logic to execute.src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue (2)
105-116: Well-implemented creation validation logic.The logic correctly prevents creation of variables with trailing dots and ensures no duplicate entries across all state sources.
155-222: Template structure is clear and maintainable.The template sections are well-organized with consistent structure. While there's some repetition, each section has unique requirements that justify the current approach.
src/ui/src/builder/useDynamicUserState.ts (3)
88-92: Good use of nullish coalescing assignment.The pattern using
??=for initializing state entries is clean and efficient.
10-49: Excellent refactoring to structured state management.The transition from flat user state to the structured
DynamicUserStatetype improves code clarity and type safety. The consistent error handling and separation of concerns into dedicated compute functions makes the code more maintainable.
51-78: Remove unusedignorePathparameter.The
ignorePathparameter is declared but never used in the function. This appears to be inconsistent with the other compute functions that do use this parameter.function computeBlueprintsResults( wf: Core, - ignorePath = new Set<string>(), ): DynamicUserState {Likely an incorrect or invalid review comment.
src/ui/src/builder/settings/BuilderTemplateInput.vue (4)
203-206: Clarify the closing bracket merge behavior.The regex
/^(\})+/removes all consecutive closing brackets from the beginning of the text after cursor. This might be too aggressive if users need multiple closing brackets for nested templates.Consider documenting this behavior or making it more selective to only remove a single closing bracket when appropriate.
190-195: Well-implemented focus management.The use of
useFocusWithinto manage dropdown visibility when focus leaves the component is a good UX pattern.
130-163: Excellent floating UI integration.The floating UI setup with auto-update lifecycle management is well-implemented. The cleanup logic properly prevents memory leaks.
233-241: Regex pattern for template variable detection is validated by existing testsThe
/@\{([^}{@]*)$/check in BuilderTemplateInput.vue is already covered by:
src/ui/src/utils/template.spec.tstests for beginning ("@","@{"), nested ("before @{var1} ${var"), and multiple templates in one string.src/ui/src/builder/settings/BuilderTemplateInput.spec.tstests for inputs like"@{obj.","foo @{tex", and"@{vault.".- E2E specs (
tests/e2e/tests/stateAutocompletion.spec.ts) exercise nested paths ("@{nested.c.","@{nested.c.e}").No additional changes required.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ui/src/utils/object.ts (1)
1-22: LGTM! Circular reference protection implemented correctly.The function now includes proper circular reference detection using WeakSet, which addresses the previous review concern. The implementation is well-structured with appropriate type safety and null handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/ui/src/builder/stateDropdown/__snapshots__/BuilderStateSelectorDropdown.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
src/ui/src/builder/settings/BuilderTemplateInput.vue(6 hunks)src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue(1 hunks)src/ui/src/builder/useDynamicUserState.ts(1 hunks)src/ui/src/utils/object.ts(1 hunks)src/ui/src/utils/template.ts(1 hunks)src/ui/src/wds/WdsDropdownMenuItem.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/src/utils/template.ts
- src/ui/src/wds/WdsDropdownMenuItem.vue
- src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.13)
- GitHub Check: build (3.9)
- GitHub Check: build (3.12)
- GitHub Check: tests (firefox)
- GitHub Check: tests (chromium)
- GitHub Check: tests (webkit)
- GitHub Check: build (3.11)
- GitHub Check: build (3.13)
- GitHub Check: build (3.12)
- GitHub Check: build (3.9)
- GitHub Check: build (3.10)
🔇 Additional comments (4)
src/ui/src/builder/useDynamicUserState.ts (1)
21-48: Well-structured refactor with proper error handling.The refactor to use
DynamicUserStatetype and dedicated computation functions improves code organization. Error handling is consistent across all computed properties with appropriate logging.src/ui/src/builder/settings/BuilderTemplateInput.vue (3)
62-70: Excellent modularization with the new dropdown component.The refactor successfully delegates autocomplete functionality to
BuilderStateSelectorDropdown, improving code separation and reusability. The prop binding and event handling are implemented correctly.
190-195: Proper focus management implementation.The use of
useFocusWithinfor managing dropdown visibility provides good UX by automatically hiding the dropdown when focus leaves the component. The async handling withnextTickensures proper timing.
197-226: Well-implemented autocomplete selection logic.The
onSelectAutocompletefunction properly handles both template and state types, with appropriate cursor positioning and value updates. The template variable completion logic correctly handles bracket merging to avoid duplicates.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/ui/src/builder/settings/BuilderTemplateInput.vue (4)
77-92: Review import organization and potential tree-shaking impactThe imports look appropriate, but consider the bundle size impact of adding multiple new imports, especially
@floating-ui/vueutilities.Consider grouping Vue imports for better readability:
import { PropType, ref, useTemplateRef, nextTick, watch, computed, onUnmounted, } from "vue";
98-98: Consider making componentId required for better type safetyThe
componentIdprop is used in the dropdown but is optional. Consider if this should be required for proper component identification in the dropdown context.- componentId: { type: String, required: false, default: undefined }, + componentId: { type: String, required: true },
190-195: Potential race condition in focus handlingThe
nextTickusage in focus handling might create a race condition if focus changes rapidly. Consider debouncing or using a more robust focus management approach.-watch(hasFocusInRoot, () => { - if (!hasFocusInRoot.value) { - nextTick().then(() => (showAutocompletions.value = false)); - } -}); +watch(hasFocusInRoot, (newValue) => { + if (!newValue) { + // Use a small delay to prevent rapid toggling + setTimeout(() => { + if (!hasFocusInRoot.value) { + showAutocompletions.value = false; + } + }, 100); + } +});
228-242: Review regex pattern for template detectionThe regex pattern
/@\{([^}{@]*)$/on line 238 for detecting template variables could be more robust. Consider edge cases like escaped characters or nested structures.- showAutocompletions.value = - !!text.match(/@\{([^}{@]*)$/) || text.endsWith("@"); + // More robust template detection + const templateMatch = text.match(/@\{([^}{@]*)$/) || text.endsWith("@"); + showAutocompletions.value = !!templateMatch;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/ui/src/builder/stateDropdown/__snapshots__/BuilderStateSelectorDropdown.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
src/ui/src/builder/settings/BuilderTemplateInput.vue(6 hunks)src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue(1 hunks)src/ui/src/builder/useDynamicUserState.ts(1 hunks)src/ui/src/utils/object.ts(1 hunks)src/ui/src/utils/template.ts(1 hunks)src/ui/src/wds/WdsDropdownMenuItem.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/ui/src/wds/WdsDropdownMenuItem.vue
- src/ui/src/utils/object.ts
- src/ui/src/utils/template.ts
- src/ui/src/builder/useDynamicUserState.ts
- src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: tests
- GitHub Check: build (3.13)
- GitHub Check: build (3.12)
- GitHub Check: build (3.9)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.13)
- GitHub Check: build (3.12)
- GitHub Check: tests (webkit)
- GitHub Check: build (3.10)
- GitHub Check: build (3.9)
- GitHub Check: build (3.11)
- GitHub Check: tests (firefox)
- GitHub Check: tests (chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
src/ui/src/builder/settings/BuilderTemplateInput.vue (7)
2-2: Good: Root container reference for floating UI positioningUsing the root container as the floating UI reference instead of the input element is a good architectural choice that provides better control over dropdown positioning.
57-70: Excellent: Clean component composition with proper prop forwardingThe new
BuilderStateSelectorDropdownintegration is well-structured with appropriate prop forwarding and conditional visibility. The component API design allows for flexible configuration based on input type.
120-121: Good: Granular control over dropdown contentThe new props provide fine-grained control over dropdown content visibility, which aligns well with the component's flexible design.
169-175: Good: Dynamic icon based on dropdown stateThe computed property for
rightIconprovides clear visual feedback about dropdown state. The logic correctly excludes template and multiline types.
254-256: Good: Simplified dropdown stylingThe simplified CSS class name and z-index handling is clean and maintainable.
124-130: Verify floating UI positioning with new root referenceSwitching the anchor for
useFloatingfrom the input element to the root container can shift how the dropdown is positioned, especially in nested or overflow‐hidden layouts. Please manually test and ensure the autocomplete panel still lines up with the input in all scenarios:• File:
src/ui/src/builder/settings/BuilderTemplateInput.vue(around lines 124–130)
• Code under review:const root = useTemplateRef("root"); const dropdown = useTemplateRef("dropdown"); const { floatingStyles, update } = useFloating(root, dropdown, { // … });• Verify in:
– Scrollable or overflow‐hidden parent elements
– Absolutely or relatively positioned ancestors
– Responsive breakpoints and container resizes
– Nested layout components (modals, sidebars, etc.)If you spot any misalignment, adjust the reference element or update the floating middleware configuration. Consider adding visual/regression tests or storybook examples to lock in correct behavior.
177-188: Template extraction logic verifiedThe
getCurrentOpenedTemplateimplementation uses a stack-based parser to track both@{…}and${…}openers, correctly handling nested templates and malformed syntax (unmatched braces are ignored when the stack is empty). Existing unit tests cover incomplete and overlapping cases, and manual inspection confirms that:
- Nested openers push onto the stack and yield the innermost fragment as expected
- Unmatched or extra closing braces simply pop when possible or are skipped otherwise
- Incomplete prefixes (e.g. single “@” or “${” at end) return an empty string
No further changes are required here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/ui/src/builder/settings/BuilderTemplateInput.vue (1)
16-17: Ensure right-icon toggle is keyboard accessible.The accessibility concern from previous reviews still applies - the right-icon click handler needs keyboard support for users who navigate with keyboards or use screen readers.
Please refer to the previous review comment about making the right-icon focusable and responsive to keyboard events (Enter/Space keys) by updating the
WdsTextInputcomponent.
🧹 Nitpick comments (2)
src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue (2)
27-39: Consider using consistent search behavior across all option types.The
userStatecomputation uses simple substring matching withincludes(), while other option computations use Fuse.js fuzzy search. This inconsistency could lead to different search behaviors across option categories.Consider applying the same
filterOptionsfunction touserStatefor consistency:const userState = computed<WdsDropdownMenuOption[]>(() => { const options: WdsDropdownMenuOption[] = []; for (const path of extractObjectPaths(wf.userStateInitial.value)) { - if (props.query && !path.includes(props.query)) continue; options.push({ value: path, label: path, icon: "code", iconBgColor: WdsColor.Green2, }); } - return options.sort((a, b) => a.label.localeCompare(b.label)); + const sorted = options.sort((a, b) => a.label.localeCompare(b.label)); + return filterOptions(sorted); });
184-184: Fix capitalization inconsistency.The section title "block results" should be capitalized consistently with other section titles.
- block results + Block results
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/ui/src/builder/settings/BuilderSettingsBinding.vue(3 hunks)src/ui/src/builder/settings/BuilderTemplateInput.vue(6 hunks)src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue(1 hunks)src/ui/src/builder/useDynamicUserState.ts(1 hunks)src/ui/src/composables/useComponentBlueprints.ts(1 hunks)src/ui/src/composables/useComponentPage.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/src/builder/settings/BuilderSettingsBinding.vue
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: build (3.12)
- GitHub Check: tests (firefox)
- GitHub Check: build (3.9)
- GitHub Check: build (3.13)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.9)
- GitHub Check: build (3.13)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: tests (chromium)
- GitHub Check: build (3.10)
- GitHub Check: tests (webkit)
🔇 Additional comments (5)
src/ui/src/composables/useComponentPage.ts (1)
1-43: Well-implemented composable with proper cycle detection.The composable correctly handles component hierarchy traversal with cycle detection, reactive computations, and clear separation of concerns. The use of
MaybeRefprovides good flexibility for consumers.src/ui/src/composables/useComponentBlueprints.ts (1)
2-2: Good type flexibility improvement.Changing from
ComputedRef<string>toMaybeRef<string>broadens the accepted input types while maintaining the same functionality throughunref(). This aligns well with the broader pattern of usingMaybeRefthroughout the codebase.Also applies to: 6-7
src/ui/src/builder/useDynamicUserState.ts (1)
17-56: Well-structured refactoring with clear separation of concerns.The refactoring effectively separates different types of dynamic state (
blueprintsSetStates,bindings,blueprintsResults) with proper error handling and type safety. The integration withuseComponentPagefor scoping blueprint results to the current page is a good design choice.src/ui/src/builder/settings/BuilderTemplateInput.vue (2)
195-224: Template insertion logic handles edge cases well.The
onSelectAutocompletefunction correctly handles both template and state types, with proper cursor positioning and duplicate closing brace removal. The regex/^(\})+/appropriately prevents duplicate closing braces when inserting template variables.
62-69: Clean integration with the new dropdown component.The refactoring successfully delegates dropdown rendering to
BuilderStateSelectorDropdown, with proper prop passing and event handling. The conditional hiding of secrets and blueprint results based on type is a good design choice.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/ui/src/composables/useComponentPage.ts (1)
12-19: Make the early return explicit for clarity.The function logic is well-designed with proper cycle detection, but the early return on line 15 should be explicit.
Apply this diff to make the return explicit:
- if (!component) return; + if (!component) return undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdownComponent.vue(1 hunks)src/ui/src/builder/useComponentActions.ts(2 hunks)src/ui/src/composables/useComponentPage.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ui/src/builder/useComponentActions.ts
- src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdownComponent.vue
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: tests
- GitHub Check: build (3.12)
- GitHub Check: build (3.12)
- GitHub Check: build (3.9)
- GitHub Check: build (3.11)
- GitHub Check: build (3.13)
- GitHub Check: build (3.10)
- GitHub Check: build (3.10)
- GitHub Check: build (3.13)
- GitHub Check: build (3.11)
- GitHub Check: build (3.9)
- GitHub Check: tests (firefox)
- GitHub Check: tests (webkit)
- GitHub Check: tests (chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
src/ui/src/composables/useComponentPage.ts (1)
1-4: LGTM!Clean imports with appropriate types and Vue composables.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ui/src/builder/settings/BuilderTemplateInput.vue (1)
16-17: Right-icon keyboard accessibility still needs attention.
🧹 Nitpick comments (1)
src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue (1)
68-82: Optimize filtering logic for better performance.The
computeOptionsFromDynamicStatefunction could be optimized to avoid multiple iterations and improve readability.function computeOptionsFromDynamicState(dynamicState: DynamicUserState) { return Object.entries(dynamicState) - .map(([path, { components }]) => { - // get only the first component defining the state key - for (const component of components) { - if (component.id === props.componentId) continue; - return { componentId: component.id, path }; - } - }) - .filter((item) => { - if (!item) return false; - if (props.query && !item.path.includes(props.query)) return false; - return true; - }); + .flatMap(([path, { components }]) => { + if (props.query && !path.includes(props.query)) return []; + + const component = components.find(c => c.id !== props.componentId); + return component ? [{ componentId: component.id, path }] : []; + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/ui/src/builder/stateDropdown/__snapshots__/BuilderStateSelectorDropdown.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (29)
src/ui/package.json(0 hunks)src/ui/src/builder/settings/BuilderFieldsKeyValue.vue(1 hunks)src/ui/src/builder/settings/BuilderFieldsText.vue(2 hunks)src/ui/src/builder/settings/BuilderSettingsBinding.vue(3 hunks)src/ui/src/builder/settings/BuilderTemplateInput.spec.ts(8 hunks)src/ui/src/builder/settings/BuilderTemplateInput.vue(6 hunks)src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.spec.ts(1 hunks)src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue(1 hunks)src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdownComponent.vue(1 hunks)src/ui/src/builder/useComponentActions.ts(2 hunks)src/ui/src/builder/useComponentDescription.ts(1 hunks)src/ui/src/builder/useDynamicUserState.spec.ts(1 hunks)src/ui/src/builder/useDynamicUserState.ts(1 hunks)src/ui/src/composables/useComponentBlueprints.ts(1 hunks)src/ui/src/composables/useComponentPage.ts(1 hunks)src/ui/src/composables/useFocusWithin.ts(2 hunks)src/ui/src/core/index.ts(4 hunks)src/ui/src/renderer/useEvaluator.ts(1 hunks)src/ui/src/tests/mocks.ts(3 hunks)src/ui/src/utils/blueprints.ts(1 hunks)src/ui/src/utils/object.ts(1 hunks)src/ui/src/utils/template.spec.ts(1 hunks)src/ui/src/utils/template.ts(1 hunks)src/ui/src/wds/WdsDropdownMenu.spec.ts(2 hunks)src/ui/src/wds/WdsDropdownMenu.vue(3 hunks)src/ui/src/wds/WdsDropdownMenuItem.vue(1 hunks)src/ui/src/wds/WdsTextInput.vue(1 hunks)src/ui/src/wds/tokens.ts(1 hunks)tests/e2e/tests/stateAutocompletion.spec.ts(4 hunks)
💤 Files with no reviewable changes (1)
- src/ui/package.json
🚧 Files skipped from review as they are similar to previous changes (24)
- src/ui/src/builder/settings/BuilderFieldsText.vue
- src/ui/src/wds/tokens.ts
- src/ui/src/wds/WdsTextInput.vue
- src/ui/src/builder/settings/BuilderFieldsKeyValue.vue
- src/ui/src/renderer/useEvaluator.ts
- src/ui/src/wds/WdsDropdownMenu.spec.ts
- src/ui/src/tests/mocks.ts
- src/ui/src/builder/useComponentDescription.ts
- src/ui/src/utils/blueprints.ts
- src/ui/src/composables/useComponentBlueprints.ts
- src/ui/src/builder/settings/BuilderSettingsBinding.vue
- src/ui/src/composables/useFocusWithin.ts
- src/ui/src/utils/template.spec.ts
- src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.spec.ts
- src/ui/src/wds/WdsDropdownMenu.vue
- tests/e2e/tests/stateAutocompletion.spec.ts
- src/ui/src/builder/useComponentActions.ts
- src/ui/src/wds/WdsDropdownMenuItem.vue
- src/ui/src/utils/object.ts
- src/ui/src/builder/settings/BuilderTemplateInput.spec.ts
- src/ui/src/builder/useDynamicUserState.spec.ts
- src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdownComponent.vue
- src/ui/src/core/index.ts
- src/ui/src/composables/useComponentPage.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/ui/src/builder/useDynamicUserState.ts (5)
src/ui/src/writerTypes.ts (1)
Core(9-9)src/ui/src/composables/useLogger.ts (1)
useLogger(6-16)src/ui/src/utils/object.ts (1)
extractObjectPaths(1-22)src/ui/src/utils/blueprints.ts (1)
getDependentBlueprintsNodes(6-28)src/ui/src/constants/component.ts (2)
COMPONENT_TYPES_PAGE(2-2)COMPONENT_TYPES_ROOT(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build (3.9)
- GitHub Check: build (3.9)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.13)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.13)
- GitHub Check: tests
- GitHub Check: tests (webkit)
- GitHub Check: tests (chromium)
- GitHub Check: tests (firefox)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🔇 Additional comments (7)
src/ui/src/utils/template.ts (2)
1-30: Well-implemented template variable completion logic.The
autocompleteTemplateVariablefunction correctly handles the various cases for template variable insertion, including proper brace management and cursor positioning. The logic flow is clear and handles edge cases appropriately.
32-42: Clean template extraction with appropriate edge case handling.The
getCurrentOpenedTemplatefunction properly extracts the current template variable content while handling edge cases like closed templates and whitespace. The regex replacement is concise and effective.src/ui/src/builder/stateDropdown/BuilderStateSelectorDropdown.vue (1)
107-204: Excellent categorization and user experience design.The template structure provides clear categorization of different state types with appropriate icons and styling. The conditional rendering based on props and the "Add new variable" functionality enhance usability.
src/ui/src/builder/settings/BuilderTemplateInput.vue (2)
195-224: Robust template insertion logic with proper cursor management.The
onSelectAutocompletemethod correctly handles both template and state types, uses the utility functions appropriately, and manages cursor positioning well. The async/await pattern ensures proper DOM updates before setting selection.
226-242: Smart dropdown visibility control based on input context.The
handleInputmethod effectively controls when the dropdown should be visible, with appropriate logic for template patterns and different input types. The regex pattern correctly identifies template variable contexts.src/ui/src/builder/useDynamicUserState.ts (2)
86-103: Clean and efficient state computation with proper filtering.The
computeBindingsfunction efficiently processes components nested under root types, correctly filters byignorePath, and builds the state structure appropriately.
105-129: Well-structured blueprint set states computation.The
computeBlueprintsSetStatesfunction uses a generator for efficient iteration, properly validates the element content, and correctly applies the ignore path filtering.
| function computeBlueprintsResults( | ||
| wf: Core, | ||
| componentId: Component["id"], | ||
| ignorePath = new Set<string>(), | ||
| ): DynamicUserState { | ||
| const state: DynamicUserState = {}; | ||
|
|
||
| for (const component of getDependentBlueprintsNodes(wf, componentId)) { | ||
| const componentType = component.type; | ||
| const componentDefinition = wf.getComponentDefinition(component.type); | ||
| if (!componentDefinition) continue; | ||
|
|
||
| const hasResultId = | ||
| component.type && | ||
| !COMPONENT_TYPES_PAGE.has(componentType) && | ||
| component.type.startsWith("blueprints_") && | ||
| componentDefinition?.outs?.["success"] !== undefined; | ||
|
|
||
| if (!hasResultId) continue; | ||
|
|
||
| const key = `results.${component.id}`; | ||
| if (key && !ignorePath.has(key)) { | ||
| state[key] ??= { components: [] }; | ||
| state[key].components.push(component); | ||
| } | ||
| } | ||
|
|
||
| return state; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify componentId parameter handling for undefined values.
The computeBlueprintsResults function expects a Component["id"] but receives unref(componentId) which could be undefined. This might cause issues in getDependentBlueprintsNodes.
🏁 Script executed:
#!/bin/bash
# Check if getDependentBlueprintsNodes handles undefined componentId
ast-grep --pattern 'function* getDependentBlueprintsNodes($_, componentId: $_) {
$$$
}'Length of output: 91
🏁 Script executed:
#!/bin/bash
# Locate the definition and all references of getDependentBlueprintsNodes
rg "getDependentBlueprintsNodes" -n .Length of output: 350
🏁 Script executed:
#!/bin/bash
# Display the implementation of getDependentBlueprintsNodes to verify parameter handling.
sed -n '1,200p' src/ui/src/utils/blueprints.tsLength of output: 650
🏁 Script executed:
#!/bin/bash
# Display the beginning of useDynamicUserState.ts to inspect componentId handling
sed -n '1,200p' src/ui/src/builder/useDynamicUserState.tsLength of output: 3411
Add guard for undefined componentId in blueprintsResults
Currently, you’re calling
computeBlueprintsResults(wf, unref(componentId))even when componentId is undefined, which will pass undefined into getDependentBlueprintsNodes (which expects a string) and can cause runtime issues.
• File: src/ui/src/builder/useDynamicUserState.ts
• Location: the blueprintsResults computed block
Suggested change:
- const blueprintsResults = computed<DynamicUserState>(() => {
- try {
- return computeBlueprintsResults(wf, unref(componentId));
- } catch (e) {
- logger.error("Cannot compute blueprintsResults", e);
- return {};
- }
- });
+ const blueprintsResults = computed<DynamicUserState>(() => {
+ try {
+ const id = unref(componentId);
+ if (id === undefined) {
+ return {}; // no component selected, nothing to compute
+ }
+ return computeBlueprintsResults(wf, id);
+ } catch (e) {
+ logger.error("Cannot compute blueprintsResults", e);
+ return {};
+ }
+ });This ensures you never pass undefined into getDependentBlueprintsNodes.
🤖 Prompt for AI Agents
In src/ui/src/builder/useDynamicUserState.ts around lines 56 to 84, the function
computeBlueprintsResults is called with componentId that can be undefined, which
leads to passing undefined to getDependentBlueprintsNodes expecting a string. To
fix this, add a guard before calling computeBlueprintsResults to check if
componentId is defined, and only call it when componentId is a valid string,
preventing runtime errors from invalid arguments.
1 participant
Screen.Recording.2025-07-03.at.09.46.45.mov
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Tests
Chores