Glm/improve tabs

[Guilhem-lm]

Important

Add TabFlex component for flexible tab creation and update DetailPageTriggerPanel.svelte to use it.

• Components:
  • Add TabFlex.svelte to common/tabs for flexible tab creation with icons and labels.
  • Export TabFlex in index.ts.
• DetailPageTriggerPanel.svelte:
  • Replace multiple Tab instances with TabFlex for dynamic tab creation.
  • Use tabs array to define tab properties like value, label, icon, and otherValues.
  • Bind selected state to triggerSelected for tab selection.

^{This description was created by for 83138d9. It will automatically update as commits are pushed.}

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	83138d9
Status:	✅  Deploy successful!
Preview URL:	https://6667b525.windmill.pages.dev
Branch Preview URL:	https://glm-improve-tabs.windmill.pages.dev

View logs

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 4f91972 in 1 minute and 55 seconds

More details

• Looked at 140 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 8 drafted comments based on config settings.

1. frontend/src/lib/components/common/tabs/TabFlex.svelte:10

• Draft comment:
  Consider initializing 'spanWidth' to 0 to avoid potential undefined state on first render.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  In Svelte, when using bind:clientWidth, the value will be automatically set when the element mounts. The initial undefined state won't cause issues because the binding happens immediately. The conditional at line 23 will work correctly regardless of initial value since undefined > 54 is false, which is the desired behavior before the width is known.
  I might be overlooking potential edge cases where the binding is delayed or fails. There could be a brief flash of incorrect rendering.
  The binding happens synchronously during mount, and even if there was a brief undefined state, initializing to 0 would actually give the same visual result since 0 > 54 is also false.
  The comment should be deleted as it suggests an unnecessary initialization that doesn't improve the code's behavior or safety.

2. frontend/src/lib/components/common/tabs/TabFlex.svelte:23

• Draft comment:
  Consider defining a named constant for the magic number 54 used in the condition for clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  This is a UI-specific value that represents a pixel width threshold. In UI components, specific pixel values are common and their meaning is usually clear from context. Here it's used to determine when to show/hide the label text in a responsive tab. Creating a constant for this wouldn't significantly improve readability since its purpose is already clear from context.
  The number 54 does seem arbitrary at first glance, and magic numbers generally should be named constants. Maybe this value has special significance that future maintainers need to understand?
  While magic numbers should generally be named constants, UI-specific pixel values are an accepted exception when their purpose is clear from context, as it is here. The value is clearly used for responsive behavior breakpoints.
  Delete this comment. While generally good practice, creating a constant for this UI-specific pixel value wouldn't meaningfully improve code quality, and the purpose is already clear from context.

3. frontend/src/lib/components/details/DetailPageTriggerPanel.svelte:57

• Draft comment:
  The new {#each} loop using TabFlex improves consistency. Verify that all required props (like 'otherValues') are correctly handled.
• Reason this comment was not posted:
  Confidence changes required: 20%
  None

4. frontend/src/lib/components/common/index.ts:18

• Draft comment:
  ✅ Export for TabFlex added correctly.
• Reason this comment was not posted:
  Confidence changes required: 0%
  None

5. frontend/src/lib/components/common/tabs/TabFlex.svelte:10

• Draft comment:
  Consider initializing spanWidth (e.g. let spanWidth: number = 0) to avoid using an undefined value in the if condition.
• Reason this comment was not posted:
  Marked as duplicate.

6. frontend/src/lib/components/common/tabs/TabFlex.svelte:23

• Draft comment:
  Avoid magic numbers: consider defining a constant (e.g. const LABEL_THRESHOLD = 54) instead of using 54 directly.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

7. frontend/src/lib/components/common/tabs/TabFlex.svelte:6

• Draft comment:
  For better type safety, consider using a more specific type for the 'icon' prop instead of 'any'.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  While using 'any' is generally not ideal for type safety, we don't have enough context to know what valid types could be used here. The icon seems to be a component that accepts size and class props, but without knowing the full set of possible icons or their common interface, suggesting a more specific type could be misleading or incorrect. The comment doesn't provide a specific alternative type to use.
  The comment points out a legitimate TypeScript best practice. Using a more specific type would indeed improve type safety.
  However, without knowing the full context of what icons can be passed or their shared interface, we can't be confident that this comment is actionable or helpful.
  While the suggestion is technically correct, it's not specific enough to be actionable without more context about the icon system. The comment should be removed.

8. frontend/src/lib/components/details/DetailPageTriggerPanel.svelte:30

• Draft comment:
  Typo: 'simplfiedPoll' seems misspelled; consider renaming it to 'simplifiedPoll' for clarity.
• 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_nF30nmyfi6x5RxDT

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

When passing 'selected' to TabFlex, include otherValues in the check (e.g. selected={triggerSelected === tab.value || tab.otherValues.includes(triggerSelected)}) to ensure tabs with alternative values are styled correctly.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 83138d9 in 20 seconds

More details

• Looked at 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. frontend/src/lib/components/details/DetailPageTriggerPanel.svelte:63

• Draft comment:
  Good improvement: now the tab is marked as selected if triggerSelected matches the value or any of the alternate values. Ensure that tab.otherValues is always an array to avoid unexpected runtime errors.
• Reason this comment was not posted:
  Confidence changes required: 0%
  None

2. frontend/src/lib/components/details/DetailPageTriggerPanel.svelte:63

• Draft comment:
  Improved condition now checks for alternate values. Ensure tab.otherValues is always defined (or default to an empty array) to avoid potential runtime issues.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

Workflow ID: wflow_oGN1DhdMmuxhdIlr

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[Guilhem-lm]

A better solution will be implemented