Conversation
…_variable dropdown fields by collections
📝 WalkthroughWalkthroughThis pull request refactors dropdown handling by introducing a centralized Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
webui/src/Helpers/CollectionGrouping.tsx (1)
44-64: The speculative child-check block (lines 44–61) is dead code.Hey! Nice work on this grouping helper — the overall design is clean. 🎉
One thing I noticed: when
groupOptions.length === 0at line 45, the code checks whether any child collections have items (lines 47–59). But regardless of the result, the function reaches line 64 and returnsnullbecausegroupOptionsis still empty. Both thehasChildGroups = trueandhasChildGroups = falsepaths lead to anullreturn.The speculative
buildGroupForCollectioncalls inside this block also add items toassignedItemIdsas a side effect, but sincecollectAllGroupsindependently recurses into children anyway (line 82–88), those items get re-processed (Set.add is idempotent, so no corruption — just redundant work).You could simplify by removing lines 44–61 entirely:
♻️ Suggested simplification
- // If this collection has no direct items and no valid child collections, hide it - if (groupOptions.length === 0) { - // Check if any child collections will have items - let hasChildGroups = false - if (collection.children) { - for (const child of collection.children) { - const childGroup = buildGroupForCollection(child, [ - ...parentPath, - collection.label || `Collection #${collection.id}`, - ]) - if (childGroup) { - hasChildGroups = true - break - } - } - } - if (!hasChildGroups) return null - } - // Only return a group if this collection has direct items if (groupOptions.length === 0) return nullwebui/src/Controls/InternalModuleField.tsx (1)
262-267: Consider moving inline interfaces outside theuseComputedcallback.Small style nit —
MinimalCustomVariable(and similarlyMinimalTriggerat line 465) is defined inside theuseComputedcallback. While this works fine at runtime, pulling these to module scope (or at least component scope) would improve readability and make them reusable if needed later. Totally optional though!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webui/src/DropDownInputFancy.tsx (1)
17-25: Pre-existing note:CustomSingleValueusesOptionPropsinstead ofSingleValuePropsThis isn't something introduced by your PR at all, so feel free to ignore — but just a friendly heads-up that
CustomSingleValueis typed withOptionProps<DropdownChoiceInt>while it rendersSelectComponents.SingleValue, which normally expectsSingleValueProps. If it's working fine today, no rush, but it might be worth a small follow-up cleanup sometime. 🙂
#3899
eg:
Summary by CodeRabbit
Release Notes
New Features
Improvements