KIL-688 Polish Available Tools/Skills lists with "+N more" modal#1445
KIL-688 Polish Available Tools/Skills lists with "+N more" modal#1445sfierro wants to merge 2 commits into
Conversation
Available Tools / Available Skills lists overflowed when a run config had many tools or skills. Cap these lists at one badge plus a "+N more" badge that opens a wide modal with the full set. - property_list.svelte: add opt-in collapse_badges; tools/skills rows on the run-config and run detail pages now collapse, opening a wide modal titled by the property name. Modal pills match the inline pills (badge-outline + hover). - badge_list.svelte: add opt-in collapse + modal_title; the Optimize run-config table tools/skills columns now collapse (modal titled "Tools"/"Skills"). - dialog.svelte: give modal-box its own text-base-content so a dialog title no longer inherits color from its trigger context (e.g. a gray table cell). - Unify "+N more" copy: tag truncation on the specs/dataset tables now reads "+N more" to match the tools/skills wording. Adds component tests for both collapse behaviors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds optional collapse behavior to badge lists: components show only the first badge plus a "+N more" control when enabled, opening a modal that lists all items (with links). Includes types, tests, minor dialog styling, and page-level enablement. ChangesBadge collapse feature with modal expansion
Sequence Diagram(s)sequenceDiagram
participant User
participant BadgeList
participant Dialog
User->>BadgeList: render with collapse=true
BadgeList->>User: show first visible_item + "+N more" button
User->>BadgeList: click "+N more"
BadgeList->>Dialog: dialog.show()
Dialog->>User: display all_items (with optional links)
User->>Dialog: click close
Dialog->>BadgeList: dialog closed
sequenceDiagram
participant User
participant PropertyList
participant Dialog
participant Navigation
User->>PropertyList: render with collapse_badges=true
PropertyList->>User: show first badge + "+N more"
User->>PropertyList: click "+N more"
PropertyList->>Dialog: open with modal_title and modal_values
Dialog->>User: display badge pills for all items
User->>Dialog: click badge pill with link
Dialog->>Navigation: navigate to link
Navigation->>User: arrive at target
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a badge collapsing feature to the BadgeList and PropertyList components, which displays only the first badge alongside a '+N more' button that opens a modal containing the full list. This helps prevent layout overflow for long lists of tools or skills. Unit tests have been added to verify this behavior, and various pages have been updated to utilize the new collapse option. The review feedback suggests improving accessibility and semantic HTML inside the PropertyList modal by using standard <a> and <span> tags instead of <button> elements, and updating the corresponding tests to assert the href attributes directly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| <button | ||
| class="badge badge-outline h-auto hover:bg-base-200" | ||
| on:click={() => { | ||
| if (link) { | ||
| goto(link) | ||
| } | ||
| }} | ||
| > | ||
| {value} | ||
| </button> |
There was a problem hiding this comment.
Similarly to the inline badges, the badges inside the modal should also use semantic <a> tags for links and <span> tags for non-clickable items to ensure proper accessibility and standard browser navigation behaviors.
{#if link}
<a
href={link}
class="badge badge-outline h-auto hover:bg-base-200"
>
{value}
</a>
{:else}
<span class="badge badge-outline h-auto">
{value}
</span>
{/if}
There was a problem hiding this comment.
Good call. Done in bba15a4 — but I also applied the same change to the inline badges (not just the modal), so links render as semantic <a href> and non-links as <span> in both places. Changing only the modal would have made it diverge from the inline pills (which were <button on:click={goto}>); now both use anchors and match badge_list.svelte.
| const pills = container.querySelectorAll( | ||
| "dialog button.badge.badge-outline", | ||
| ) | ||
| expect(pills.length).toBe(2) | ||
| expect(pills[1].textContent?.trim()).toBe("beta") | ||
| // Clicking a pill navigates to its tool link. | ||
| await fireEvent.click(pills[1]) | ||
| expect(goto).toHaveBeenCalledWith("/tools/b") |
There was a problem hiding this comment.
Since we are refactoring the badges in the modal to use semantic <a> tags instead of <button> elements, we should update the test to query for a.badge.badge-outline and assert the href attribute directly. This is more robust and standard than mocking and asserting goto calls.
| const pills = container.querySelectorAll( | |
| "dialog button.badge.badge-outline", | |
| ) | |
| expect(pills.length).toBe(2) | |
| expect(pills[1].textContent?.trim()).toBe("beta") | |
| // Clicking a pill navigates to its tool link. | |
| await fireEvent.click(pills[1]) | |
| expect(goto).toHaveBeenCalledWith("/tools/b") | |
| const pills = container.querySelectorAll( | |
| "dialog a.badge.badge-outline", | |
| ) | |
| expect(pills.length).toBe(2) | |
| expect(pills[1].textContent?.trim()).toBe("beta") | |
| expect(pills[1].getAttribute("href")).toBe("/tools/b") |
There was a problem hiding this comment.
Updated in bba15a4 — the test now queries dialog a.badge.badge-outline and asserts href directly, and I dropped the goto mock since it's no longer needed.
📊 Coverage ReportOverall Coverage: 92% Diff: origin/main...HEADNo lines with coverage information in this diff.
|
Address Gemini review: render badge links as <a href> and non-links as
<span> instead of <button on:click={goto}>, for both the inline list and the
"+N more" modal. Keeps inline and modal pills identical and aligns
property_list with badge_list's markup.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sfierro
requested review from
chiang-daniel,
leonardmq,
scosman and
tawnymanticore
1 participant
BEFORE:
AFTER:
Linear ticket: https://linear.app/kiln_ai/issue/KIL-688/available-tools-in-property-list-needs-polish-for-many-tools
Description
The Available Tools and Available Skills lists overflowed and looked messy when a run config had many tools/skills. This caps each list at one badge plus a
+N morebadge that opens awidth="wide"modal with the full set — the same paradigm we already use for tags in tables.Changes
property_list.svelte/property_list.ts— add opt-incollapse_badges. The tools/skills rows on the run-config detail and run detail pages now collapse to the first pill ++N more, opening a wide modal titled by the property name (Available Tools/Available Skills). Modal pills use the exact samebadge badge-outline hover:bg-base-200markup as the inline pills, and stay individually clickable links.badge_list.svelte— add opt-incollapse+modal_title. The Optimize run-config table Tools/Skills columns now collapse, opening a modal titledTools/Skillsto match the column headers.+N moreand the dialog stop click propagation so they don't trigger the row's navigate-on-click.dialog.svelte— givemodal-boxits owntext-base-contentso a dialog title no longer inherits color from its trigger context (the tools/skills modals open from insidetext-gray-500table cells, which previously washed out their titles).+N more(was+N other(s)) to match the tools/skills wording. Distinct pill styles are intentionally kept (tag = single filled pill; tools/skills = outline link-pills).Audit
Audited the app for other long inline badge/pill lists in constrained spaces. The only actionable spot beyond the above was the Optimize table (fixed here). The kiln-task-tools list in
tools/[project_id]/+page.svelteis already hidden behind anInfoTooltip, so it was left as-is.Tests
property_list.test.ts(5) andbadge_list.test.ts(5) covering: all-badges default, collapse to+N more, single-item no-collapse, modal opens with the full list + title, and link navigation.🤖 Generated with Claude Code