Remove last SelectLegacy

[diegoimbert]

• Replace SelectLegacy in AppPicker
• Better DynSelect
• remove select legacy

---

Important

Remove SelectLegacy and replace it with Select, updating related components and utilities for improved functionality.

• Component Replacement:
  • Replace SelectLegacy with Select in DynSelect.svelte and AppPicker.svelte.
  • Remove SelectLegacy import and usage in DynSelect.svelte.
• Component Deletion:
  • Delete SelectLegacy.svelte and its index file.
• Component Enhancements:
  • Update Select and MultiSelect components to support new features and handle previous SelectLegacy functionalities.
  • Modify SelectDropdown.svelte to improve dropdown behavior and styling.
• Utility Updates:
  • Enhance readFieldsRecursively in utils.ts to support exclusion of specific fields.

^{This description was created by for ffe5bd8. You can customize this summary. It will automatically update as commits are pushed.}

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	ffe5bd8
Status:	✅  Deploy successful!
Preview URL:	https://2afb5744.windmill.pages.dev
Branch Preview URL:	https://di-remove-last-legacy-select.windmill.pages.dev

View logs

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to d71208c in 2 minutes and 10 seconds. Click for details.

• Reviewed 1394 lines of code in 5 files
• Skipped 1 files when reviewing.
• Skipped posting 4 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. frontend/src/lib/components/select/SelectDropdown.svelte:140

• Draft comment:
  Using 'item.label || '\xa0'' provides a fallback to prevent an empty button. Confirm that this fallback value meets design expectations.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to confirm if the fallback value meets design expectations, which is against the rules. It doesn't provide a specific code suggestion or point out a clear issue with the code.

2. frontend/src/lib/components/wizards/AppPicker.svelte:50

• Draft comment:
  Ensure that replacing the legacy select with the new component using 'disablePortal' replicates the previous behavior (portal={false}). Reason this comment was not posted: Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to ensure that the behavior of the new component matches the old one, which violates the rule against asking the author to ensure behavior. It doesn't provide a specific suggestion or ask for a test to be written. 3. frontend/src/lib/utils.ts:1388 Draft comment: In the array branch of readFieldsRecursively, pass the options parameter to the recursive call to ensure exclusions (excludeField) propagate into nested array objects. Reason this comment was not posted: Comment was not on a location in the diff, so it can't be submitted as a review comment. 4. frontend/src/lib/utils.ts:1388 Draft comment: Consider using 'i < obj.length' instead of 'i <= obj.length' when iterating arrays to avoid an extra unnecessary iteration. Reason this comment was not posted: Comment was on unchanged code. Workflow ID: wflow_JEI3IISI9AwR6mtM You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

[ellipsis-dev]

Avoid duplicate calls to validSelectObject. Consider caching its result in a variable to prevent recomputation when checking and then rejecting.

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed 1962d27 in 2 minutes and 0 seconds. Click for details.

• Reviewed 83 lines of code in 1 files
• Skipped 1 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. frontend/src/lib/components/DynSelect.svelte:57

• Draft comment:
  Cache the result of validSelectObject instead of calling it twice.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_ejZxs6PbS3TlTCsI

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Add a return after calling resolve([]) to prevent further processing in the callback.

Suggested change

	if (res.length == 0) resolve([])
	if (res.length == 0) return resolve([])

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed b0fc501 in 34 seconds. Click for details.

• Reviewed 5 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. frontend/src/lib/components/apps/svelte-select/lib/index.js:1

• Draft comment:
  Removed legacy export of SelectLegacy. Ensure that all usages are updated and that the component file is removed if no longer needed.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_UsI4nGdkXx1cLzbT

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 80a95d6 in 2 minutes and 24 seconds. Click for details.

• Reviewed 76 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 4 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. frontend/src/lib/components/DynSelect.svelte:106

• Draft comment:
  Check the loading logic: using loading={!open && _items.status === 'loading'} may suppress the loading state when the dropdown is open. Confirm this is intended.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The loading logic is clearly intentional - it's a specific UX pattern to only show loading when closed. The comment isn't pointing out a bug or suggesting an improvement, it's just asking for confirmation. This falls under the rule about not asking authors to confirm their intentions. Perhaps there could be a legitimate performance or UX issue with suppressing the loading state when open that I'm not considering. Even if there were edge case issues, the comment isn't raising specific problems - it's just asking for confirmation of intent, which our rules explicitly say not to do. Delete this comment as it merely asks for confirmation of an intentional design choice rather than pointing out a specific issue or suggesting an improvement.

2. frontend/src/lib/components/select/Select.svelte:77

• Draft comment:
  The two $effect blocks syncing the internal 'open' state with '_open' may be redundant or risk circular updates. Consider consolidating this binding.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The pattern appears intentional and matches how filterText binding is implemented. Two-way binding often requires this kind of setup in Svelte. The comment suggests consolidation but doesn't consider that this might be a necessary pattern. The code is following a consistent pattern used elsewhere in the file. I might be missing some Svelte-specific optimization that could make this more efficient. There could be a better way to handle two-way binding that I'm not aware of. The consistent pattern usage and the fact that it matches filterText binding suggests this is an intentional, working implementation rather than a mistake. The comment should be deleted as it questions an intentional pattern that's consistently used in the codebase for two-way binding.

3. frontend/src/lib/components/select/Select.svelte:139

• Draft comment:
  Verify that calling getLabel({ value }) (instead of getLabel(value)) matches the intended API of getLabel.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify their intention, which violates the rules. It does not provide a specific suggestion or point out a clear issue with the code. Therefore, it should be removed.

4. frontend/src/lib/components/select/Select.svelte:144

• Draft comment:
  Changing the condition for applying '!placeholder-primary' from valueEntry && !loading to value && !loading may affect styling if the selected value is not found in processed items. Confirm this change is deliberate.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_wPr64G8tlVf3NKkN

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed ffe5bd8 in 1 minute and 25 seconds. Click for details.

• Reviewed 51 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. frontend/src/lib/components/select/Select.svelte:17

• Draft comment:
  Refactored filterText binding: now initialized with an empty string via $bindable('') instead of using a legacy _filterTextBind. This improves consistency.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining a change that was made. It does not provide any actionable feedback or suggestions for improvement. According to the rules, purely informative comments should be removed.

2. frontend/src/lib/components/select/Select.svelte:30

• Draft comment:
  Simplified open binding by directly using $bindable(false) rather than a legacy _open variable.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment seems to be purely informative, explaining a change that was made. It doesn't ask for any confirmation or suggest any improvements. According to the rules, purely informative comments should be removed.

3. frontend/src/lib/components/select/Select.svelte:62

• Draft comment:
  Removed redundant $effect blocks syncing legacy bindable variables. The direct use of $bindable for filterText and open makes these obsolete.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining what was done in the code without suggesting any changes or asking for confirmation. It doesn't align with the rules for useful comments.

Workflow ID: wflow_dNVwdhSCuPNPEcrs

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[diegoimbert]

/ai review my PR

[claude]

Claude finished @diegoimbert's task —— View job

---

I'll review your PR to analyze the SelectLegacy removal and replacement with the modern Select component.

Todo List:

• Read and analyze all changed files
• Review component replacement strategy
• Check for potential bugs or issues
• Verify best practices adherence
• Provide comprehensive review feedback

---

Code Review

✅ Overall Assessment: LGTM

This is an excellent refactoring that successfully removes the legacy SelectLegacy component and modernizes the select components across the codebase. The changes align well with Svelte 5 best practices and improve code maintainability.

Strengths:

1. Clean Component Migration

• DynSelect.svelte: Successfully migrated from SelectLegacy to the modern Select component. The new implementation is cleaner and properly uses Svelte 5 runes.
• AppPicker.svelte: Proper migration to use disablePortal prop instead of the legacy portal={false} pattern.

2. Excellent Svelte 5 Usage

• Proper use of $bindable() for two-way binding in Select.svelte:17 and Select.svelte:30
• Correct implementation of $derived for computed values
• Clean $effect usage for side effects

3. Code Quality Improvements

• utils.ts:1385: Added excludeField option to readFieldsRecursively - good design that allows excluding specific fields during recursive reading
• Select.svelte: Simplified binding logic by removing redundant $effect blocks and using direct $bindable

Issues & Suggestions:

🔧 Minor Issues:

1. DynSelect.svelte:54 - Missing early return could cause unnecessary processing:

   if (res.length == 0) resolve([])  // Should be: return resolve([])

2. DynSelect.svelte:57-58 - validSelectObject is called twice:

   // Current:
   } else if (res.find((x) => validSelectObject(x) != undefined)) {
       reject(validSelectObject(res.find((x) => validSelectObject(x) != undefined)))
   
   // Better:
   const invalid = res.find((x) => validSelectObject(x) != undefined)
   } else if (invalid) {
       reject(validSelectObject(invalid))

3. utils.ts:1390 - Recursive call missing options parameter:

   // In array branch, should pass options to maintain excludeField functionality
   readFieldsRecursively(obj[i], options)  // Currently missing options

🎯 Performance Notes:

• DynSelect.svelte:106 - The loading logic loading={!open && _items.status === 'loading'} is intentional UX design (only show loading when closed) - this is good.
• Select.svelte - The two-way binding pattern for filterText and open is correctly implemented and follows Svelte 5 best practices.

Security & Best Practices:

• ✅ No security concerns identified
• ✅ Proper input sanitization maintained
• ✅ Error handling preserved from legacy implementation
• ✅ Accessibility attributes maintained (autofocus, disabled, etc.)

Testing Recommendations:

1. Test the migration in AppPicker to ensure disablePortal works as expected
2. Verify DynSelect behavior with empty results and error states
3. Test the new excludeField functionality in utils

Summary:

This PR successfully removes 1186 lines of legacy code while maintaining all functionality. The new Select component is more maintainable, follows Svelte 5 best practices, and provides better developer experience. The few minor issues identified are easily addressable and don't block the PR.

Recommendation: Approve with minor fixes 🚀

---