feat: Add missing configurations in app builder

[tristantr]

…ames with other components

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	6757825
Status:	✅  Deploy successful!
Preview URL:	https://7144ff50.windmill.pages.dev
Branch Preview URL:	https://tl-app-builder.windmill.pages.dev

View logs

[claude]

Claude finished @tristantr's task —— View job

---

PR Review Complete

I've completed a comprehensive review of PR #6922: "feat: Add missing configurations in app builder"

---

📊 Summary

This PR adds tooltip functionality to Tab components and Modal buttons in the app builder, aligning configuration names across components. The implementation follows the existing patterns in the codebase and adds a consistent tooltip feature across multiple component types.

Changed Files: 8 files
Lines Added: 138
Lines Removed: 38

---

✅ Positive Aspects

1. Consistent Pattern: The tooltip implementation follows the existing pattern used for disabledTabs and hiddenTabs, making it easy to understand and maintain.

2. Proper Svelte 5 Usage: Uses $state and $bindable runes correctly for reactive state management.

3. Component Reuse: Creates a reusable GridTabTooltip component following the same structure as GridTabDisabled and GridTabHidden.

4. Type Safety: Properly types the new tooltipTabs prop as RichConfiguration[].

5. Complete Implementation: The feature is implemented end-to-end, from the editor UI to the rendered components.

---

🐛 Issues Found & Recommendations

Critical Issues:

1. Accessibility Problem in Tab.svelte (frontend/src/lib/components/common/tabs/Tab.svelte:63-66)

• Issue: The tooltip is placed on a wrapper div instead of the interactive button element
• Impact: This breaks accessibility patterns - screen readers and keyboard navigation may not properly announce the tooltip
• Fix: Move the title attribute directly to the <button> element and remove the wrapper div
• See inline comment for details

High Priority:

2. Code Duplication in AppTabs.svelte

• Issue: Tooltip rendering logic is duplicated 3 times (lines 238-240, 258-260, 298-300)
• Impact: Maintenance burden; if the logic needs to change, it must be updated in 3 places
• Fix: Extract to a Svelte snippet or derived variable
• See inline comment for suggested implementation

3. Inconsistent Tooltip Application in AppModal.svelte

• Issue: Extra wrapper div created just for tooltip (line 143-146)
• Impact: Unnecessary DOM nesting; different pattern than Tab component
• Fix: Apply tooltip directly to Button component if supported, or document why the wrapper is necessary

Medium Priority:

4. Uninitialized State Array

• Issue: resolvedTooltipTabs initialized as empty array but populated by InputValue components
• Impact: Potential undefined access during initial render
• Fix: Initialize with proper default values based on tooltipTabs length

5. Empty String Default

• Issue: New tabs get empty string '' as tooltip default (GridTab.svelte:103)
• Impact: Unnecessary rendering of empty tooltip attributes
• Fix: Use undefined instead of empty string for better semantic meaning

---

🔍 Code Quality Observations

Svelte 5 Best Practices:

• ✅ Proper use of $state, $derived, and $bindable runes
• ✅ Reactive statements with $effect.pre()
• ⚠️ Consider using $derived for tooltip string conversion instead of inline logic repetition

Performance:

• ✅ No blocking operations detected
• ✅ Proper use of keyed {#each} blocks
• ⚠️ Minor: Triple string conversion (String(resolvedTooltipTabs[index])) could be optimized

Type Safety:

• ✅ Proper TypeScript interfaces
• ✅ Type annotations on props
• ⚠️ Could add more specific types for tooltip configuration

---

🔒 Security Implications

No security concerns identified. The tooltip content is:

• Rendered as plain text via HTML title attribute (no XSS risk)
• User-controlled but properly escaped by the browser
• Not executed as code

---

⚡ Performance Considerations

No significant performance concerns. The implementation:

• Uses standard Svelte reactivity patterns
• Doesn't introduce heavy computations
• Properly uses $derived for computed values
• Minor optimization opportunity: reduce duplicate string conversions

---

🧪 Testing Recommendations

To verify this implementation works correctly, test:

1. ✅ Tooltips display on hover over tabs
2. ✅ Tooltips display on Modal buttons
3. ✅ Empty tooltips don't show tooltip UI
4. ✅ Dynamic tooltip updates work properly
5. ⚠️ Accessibility: Test with screen readers (NVDA, JAWS, VoiceOver)
6. ⚠️ Keyboard Navigation: Ensure tooltips are announced when navigating with Tab key

---

📝 Reproduction Instructions for Testers

To test the changes added in this PR, navigate to the Windmill app builder and create or edit an app. Add a Tabs component to your app canvas. In the component settings panel on the right side, you'll see a new "Tooltips" configuration option for each tab alongside the existing "Disabled" and "Hidden" toggles. Enter tooltip text for one or more tabs, then preview the app - when you hover over the tab headers, the configured tooltips should appear. Similarly, add a Modal component to your app, and you'll find a new tooltip configuration field in its settings. Set a tooltip value and preview the app to verify that hovering over the modal's trigger button displays the tooltip. Test both static text and dynamic/eval-based tooltip values to ensure the feature works with all input types.

---

🎯 Final Recommendation

Approve with minor changes recommended.

This is a solid implementation that adds useful functionality. The main issues are:

1. Accessibility fix needed for Tab.svelte (wrapper div)
2. DRY principle - reduce code duplication in tooltip rendering
3. Minor improvements for initialization and consistency

These can be addressed in follow-up commits or accepted as-is with a follow-up issue created.

---

Review completed by Claude Code

[claude]

Type Annotation Missing: The tooltipTabs prop should have a more specific type annotation. Consider using RichConfiguration[] | undefined consistently with other similar props like disabledTabs and hiddenTabs for better type safety.

[claude]

Code Duplication: This tooltip rendering logic is duplicated in three places (lines 238-240, 258-260, 298-300). Consider extracting this into a helper function or derived variable to follow DRY principles:

{#snippet tooltipRenderer(index)}
  {resolvedTooltipTabs[index] && String(resolvedTooltipTabs[index]).length > 0
    ? String(resolvedTooltipTabs[index])
    : undefined}
{/snippet}

Then use: title={@render tooltipRenderer(index)}

[claude]

Potential Performance Issue: This state variable resolvedTooltipTabs is initialized as an empty array but is populated via InputValue components. Consider initializing it with proper default values based on tooltipTabs length to avoid potential index access issues during initial render.

[claude]

Inconsistent Pattern: The tooltip logic here differs from the Tab component. In AppModal, the tooltip is on a wrapper div, but the button is inside. This creates an extra wrapper element. Consider applying the title attribute directly to the Button component (if it supports it) for consistency and to avoid unnecessary DOM nesting.

[claude]

Accessibility Concern: Wrapping the button in a separate div just for the tooltip breaks some accessibility patterns. The title attribute should be on the interactive element (the button) itself, not on a wrapper div. This ensures screen readers and keyboard navigation work correctly.

Suggested fix:

<button
 
  use:triggerableByAI={{...}}
  ...
>

Remove the outer <div>.

[claude]

Initialization Issue: When adding a new tab, the tooltip is initialized with an empty string ''. Consider using a more descriptive default or undefined to be consistent with other optional configurations. An empty string could cause unnecessary rendering of empty tooltip attributes.