Skip to content

design: Apply New Table Design in Rill Cloud#9181

Open
royendo wants to merge 35 commits intomainfrom
royendo/alert-report-table-migrate
Open

design: Apply New Table Design in Rill Cloud#9181
royendo wants to merge 35 commits intomainfrom
royendo/alert-report-table-migrate

Conversation

@royendo
Copy link
Copy Markdown
Contributor

@royendo royendo commented Apr 3, 2026

  • Extract shared ResourceListRow component in web-common that defines the row layout: icon + title + error tag + subtitle + action menu
  • Alerts and reports composite cells now consume ResourceListRow via props and slots
  • Update resource list styling to flat divider lines (no bordered cards, no rounded corners, no background) for all resource lists
  • Replace ResourceTypeBadge (icon + label) with icon-only in dashboard rows
  • Align subtitle text with title text across dashboards, alerts, and reports
  • Wire up working dropdown actions:
    • Alerts: Edit (navigates to detail page), Delete (calls admin mutation)
    • Reports: Run (triggers ad-hoc run), Edit (navigates to detail page), Delete (calls admin mutation)
  • Code-created resources hide Edit/Delete options
Screenshot 2026-04-28 at 18 43 31 Screenshot 2026-04-28 at 18 43 35

design: https://www.figma.com/design/JtG3sbaopjO0xQlyeCjmho/RILL-WIP?node-id=6857-271454&t=SrloSlXdGviOEwYN-4

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

royendo and others added 7 commits April 3, 2026 09:24
Extract common row layout (icon + title + error tag + three-dot menu + subtitle)
into a reusable `ResourceListRow` component. Alerts and reports composite cells
now consume this shared component via props and slots.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove bordered cards, rounded corners, and background color from resource
list items. Rows now use simple top/bottom divider lines matching the new design.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Alerts: Edit, Delete
Reports: Run, Edit, Delete

Updated ResourceListRow to use an actions slot instead of a hardcoded
three-dot button, allowing each consumer to provide its own dropdown menu.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace `ResourceTypeBadge` (icon + label) with just the icon in dashboard
rows. Keep raw AlertIcon/ReportIcon for alerts and reports.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add pl-[22px] to dashboard subtitle to match icon + gap offset, consistent
with alert and report rows.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Edit navigates to the resource detail page
- Delete calls the admin service mutation and refreshes the list
- Run (reports only) triggers an ad-hoc report run
- Code-created resources hide Edit/Delete (alerts hide entire menu)
- Dropdown opens down-right (align="start")

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@royendo royendo requested a review from Di7design April 3, 2026 14:46
@royendo
Copy link
Copy Markdown
Contributor Author

royendo commented Apr 3, 2026

@Di7design the one thing missing in the design is the file name, not sure we want to remove it yet its still there !

@royendo royendo changed the title Apply New Table Design in Rill Cloud design: Apply New Table Design in Rill Cloud Apr 3, 2026
Copy link
Copy Markdown

Screenshot 2026-04-06 at 2.54.43 PM.png

Can we keep this icon tag in the front as well, it's easy to tell which is canvas which is Explore. @royendo

Copy link
Copy Markdown
Contributor Author

royendo commented Apr 6, 2026

so Badge and Label instead of just Badge, correct?

royendo and others added 6 commits April 7, 2026 11:13
  - Alert/report table migration to ResourceListRow with dropdown actions
  - Delete confirmation dialogs for alerts and reports
  - ResourceListRow role fix
  - ResourceList deduplication (web-admin copy removed, imports point to web-common)
  - connector-metadata.ts → connector-icon-mapping.ts rename
  - ResourceList flat divider styling
- Migrate `ResourceListRow` to Svelte 5 ($props, snippets, Component type)
- Extract shared `DeleteResourceConfirmDialog` replacing duplicate alert/report dialogs
- Add `lastTrigger`/`lastRun` guard on error badge to avoid showing errors before first run
- Add try/catch error handling to `handleDelete` and `handleRun` with user notifications
- Migrate both `DashboardsTableCompositeCell` (web-admin + web-common) to use `ResourceListRow`
- Hide dropdown menu entirely for code-created reports (matching alert pattern)
- Document magic number `pl-[22px]` with comment

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@royendo
Copy link
Copy Markdown
Contributor Author

royendo commented Apr 7, 2026

Fix 2 — Svelte 5 migration:

  • ResourceListRow.svelte: Rewritten with $props(), Snippet types, {@render}, Component type for icon. Renamed icon to Icon for Svelte 5 component
    rendering.
  • DeleteResourceConfirmDialog.svelte (new): Uses $props(), $bindable() for open.

Fix 3 — Shared delete dialog:

  • Created web-common/src/features/resources/DeleteResourceConfirmDialog.svelte with resourceKind, title, description props.
  • Deleted DeleteAlertConfirmDialog.svelte and DeleteReportConfirmDialog.svelte.
  • Updated both composite cells to use the shared dialog.

Fix 4 — lastTrigger guard:

  • Alerts: errorMessage={lastTrigger ? lastTriggerErrorMessage : undefined}
  • Reports: errorMessage={lastRun ? lastRunErrorMessage : undefined}

Fix 5 — Error handling:

  • handleDelete in both alerts and reports wrapped in try/catch with eventBus.emit("notification", { type: "error" }).
  • handleRun in reports wrapped in try/catch.

Fix 6 — Dashboards migrated to ResourceListRow:

  • Both web-admin and web-common DashboardsTableCompositeCell now use ResourceListRow with ResourceTypeBadge in the tags snippet. Made icon optional,
    subtitle padding conditional.

Fix 7 — Hide dropdown for code-created reports:

  • Wrapped the entire DropdownMenu.Root in {#if !isCreatedByCode}, matching the alert pattern.

Fix 9 — Magic number comment:

  • Added .

Copy link
Copy Markdown

@Di7design Di7design left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error state colors for dark mode looks incorrect.

Image Image

See below the correct dark mode/light mode destructive colors.

Image
  1. Delete report action in report page dropdown is not red?
Image

@royendo
Copy link
Copy Markdown
Contributor Author

royendo commented Apr 7, 2026

Oh those were existing but will fix in this PR!

@royendo
Copy link
Copy Markdown
Contributor Author

royendo commented Apr 7, 2026

as discussed the color themeing extends past this PR and quite deeply into the first pass of dark mode, will take themes into a separate PR.

@ericpgreen2 ericpgreen2 self-requested a review April 8, 2026 14:34
Copy link
Copy Markdown
Contributor Author

royendo commented Apr 8, 2026

Eric, no need to review this yet, im waiting for #9173 then will make sure the filter bar is available for these tables

@ericpgreen2
Copy link
Copy Markdown
Contributor

Eric, no need to review this yet

Then I'll mark this as draft so I don't get pinged, if you don't mind.

@royendo
Copy link
Copy Markdown
Contributor Author

royendo commented Apr 25, 2026

PR #9181 Review — Apply New Table Design in Rill Cloud

▎ Design-against-Figma comparison was skipped — the design-reviewer agent hit a model-access error. Findings below were validated by reading PR-version
▎ files directly.


Critical / Warning

  1. Two near-duplicate delete-confirm dialogs
    web-common/src/features/resources/DeleteConfirmDialog.svelte and DeleteResourceConfirmDialog.svelte are both new in this PR, both in the same folder,
    both render an AlertDialog with a destructive button. The only difference is whether the caller hands in a fully formed message or three pieces. They
    are also stylistically inconsistent — one uses Svelte 4 export let, the other uses Svelte 5 runes.
    → Collapse into one component before merge.

  2. Interactive nested inside in ResourceListRow
    web-common/src/features/resources/ResourceListRow.svelte:30 wraps the row in <a {href}>, then :56-64 nests the DropdownMenu.Trigger button inside it.
    The onclick interceptor only catches primary clicks — middle-click and cmd/ctrl-click on the menu button still trigger the anchor's navigation, opening
    the row in a new tab when the user only meant to open the menu. Interactive content inside an anchor is also invalid HTML.
    → Restructure as siblings: anchor over the title region, action menu outside the anchor.

  3. DashboardsTable duplicated across web-admin/ and web-common/
    The two files share ~150 lines of identical filter/sort/column logic. Pre-existing pattern, but this PR expands both copies by ~100 lines each. Same
    issue with DashboardsTableCompositeCell.svelte.
    → The web-admin version should consume the web-common one, passing data={dashboardsData} and a derived getHref.

  4. Filtered-empty-state branch in ResourceList is unreachable
    ResourceList.svelte:78 keys the empty state on $table.getState().globalFilter?.length > 0. But every caller (AlertsTable, ReportsTable, dashboards) now
    pre-filters via processedData = (data ?? []).filter(...) and passes data={processedData}. TanStack's globalFilter is never set, so isFiltered is always
    false. Users searching with no results see "You don't have any {kind}s yet" instead of "No {kind}s match your search".
    → Accept an isFiltered prop from the caller, or push search into globalFilter instead of pre-filtering.

  5. New row-level actions have no test coverage
    tests/alerts.spec.ts and tests/reports.spec.ts only touch the pre-existing metadata-page Delete flow. Net-new listing-row actions — Alerts Edit/Delete,
    Reports Run/Edit/Delete, and the code-created-resource hide/show behavior — have no Playwright coverage.


Suggestions

  1. isCreatedByCode = !ownerId is the wrong predicate
    AlertsTableCompositeCell.svelte:31 and ReportsTableCompositeCell.svelte:40 derive "code-created" from missing ownerId. The backend's authoritative flag
    is admin_managed. A YAML resource with admin_owner_user_id set will wrongly show the menu (and the backend will reject with FailedPrecondition); a
    UI-created resource missing the annotation will wrongly hide it. Frontend gating is UX-only here, so not a security smell.

  2. {@html description} in DeleteConfirmDialog
    DeleteConfirmDialog.svelte:39 renders user-controlled display names through {@html ...} so callers can embed . Server-side validation likely
    keeps blast radius small, but this is an avoidable XSS primitive.
    → Pass the title as a separate prop and let Svelte auto-escape it (the way DeleteResourceConfirmDialog already does).

  3. ResourceListRow API: icon prop + tags snippet are mutually exclusive in practice
    Every caller uses one or the other. The subtitlePadding calc parseInt(iconSize) + GAP_PX silently breaks if iconSize isn't in px.
    → Consider a single leading snippet and drop the icon-only path.

  4. Toolbar/filter boilerplate copy-pasted across five tables
    Same searchText / matchesSearch / processedData / handleFilterChange / filterGroups shape repeated in AlertsTable, AlertHistoryTable, ReportsTable,
    ReportHistoryTable, and both DashboardsTables — ~50 lines per file. Worth a useTableFilters helper as the abstraction cost drops.


Rejected (false positives)

  • "Svelte 5 runes will break in Svelte 4 runtime" — package.json pins "svelte": "^5.0.0" across all workspaces; 44 files already use $props(). Project
    is on Svelte 5.
  • "Silent error handling in handleDelete" — the catch block does emit a "Failed to delete alert" notification via eventBus.emit. The reviewer's quoted
    line numbers were also stale (file is 121 lines, not ~300).

Developed in collaboration with Claude Code

- Fix dashboard sort: drop the TanStack `initialSorting` that was overriding the toolbar's Newest/Oldest selection.
- Restructure `ResourceListRow` so the action menu is a sibling of the anchor instead of nested inside it; fixes invalid HTML and middle-click/cmd-click navigation on the menu trigger.
- Collapse `DeleteResourceConfirmDialog` into `DeleteConfirmDialog`; the dialog now takes a default slot for body content (auto-escaped, removes the `{@html}` XSS surface).
- Add an `isFiltered` prop to `ResourceList` so the "no results match your search" empty state actually fires when callers pre-filter data externally.
- Refactor `web-admin` `DashboardsTable` to a thin wrapper around the `web-common` version (was duplicating ~150 lines); delete the duplicate composite cell.
- Add `ariaLabel` to row action menus and exercise the new listing-row Delete (alerts, reports) and Run (reports) actions in Playwright tests.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@royendo
Copy link
Copy Markdown
Contributor Author

royendo commented Apr 25, 2026

Code review fixes — pushed in 0793509ba

Bugs fixed

  • Dashboard "Newest" sort was a no-op (web-common and web-admin DashboardsTable.svelte). The table passed initialSorting=[{ id: "name", desc: false }] to TanStack, which re-sorted the already-sorted processedData by name ascending. Removed initialSorting so TanStack renders rows in the order the toolbar produced.
  • ResourceListRow nested a <button> inside the row's <a>. Restructured: the anchor wraps only the text region, and the action menu is now a sibling. Fixes invalid HTML and middle-click / cmd-click navigation on the menu trigger. The onclick preventDefault interceptor and role="presentation" shim are gone.
  • {@html description} on DeleteConfirmDialog was rendering caller-built HTML strings that interpolated user-controlled display names — an avoidable XSS primitive. Replaced with a default <slot /> so callers write Svelte markup (auto-escaped) for the body.
  • "No results match your search" empty state was unreachable. ResourceList keyed on TanStack's globalFilter, but every caller now pre-filters data externally so globalFilter was always empty. Added an isFiltered prop, wired through alerts/reports/dashboards.

Cleanup

  • Collapsed two delete-confirm dialogs into one. DeleteResourceConfirmDialog.svelte is gone; all six callers now use DeleteConfirmDialog with body content via the default slot.
  • De-duplicated DashboardsTable. web-admin/.../DashboardsTable.svelte shrank from 261 → 38 lines and now wraps the web-common version, fetching data and computing getHref based on isEmbedded. Deleted the duplicate web-admin/.../DashboardsTableCompositeCell.svelte (only difference vs. the web-common one was that it constructed href inline; that now happens in the wrapper).

Tests

  • Added ariaLabel={Actions for ${title}} to the row's IconButton in alerts and reports composite cells for stable Playwright selectors.
  • Updated tests/alerts.spec.ts "Should delete alert with schedule" to delete from the listing-row menu (covers the new wired-up Delete). The other delete test still covers the metadata-page path.
  • Updated tests/reports.spec.ts "Should run a report and receive an email" to trigger Run from the listing-row menu (covers the new wired-up Run); still asserts the email and landing pivot.
  • Updated tests/reports.spec.ts "Should delete report" to delete from the listing-row menu.

Net change

17 files, +134 / -452.

Knowingly not addressed

  • isCreatedByCode = !ownerId on the listing rows uses the wrong predicate — backend authoritative flag is admin_managed. UX-only mismatch (backend rejects with FailedPrecondition either way), kept for a follow-up.
  • Toolbar/filter boilerplate is still copy-pasted across the five listing tables. A useTableFilters helper would consolidate ~50 lines per file but is out of scope here.

Developed in collaboration with Claude Code

royendo and others added 5 commits April 25, 2026 10:01
Previously sorted by `state.dataRefreshedOn`, which made never-refreshed
dashboards appear at the bottom of "Newest" and didn't match what the label
implies. Switch to creation time so a brand-new dashboard appears at the
top of "Newest" immediately.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The five listing tables (alerts, alerts history, reports, reports history,
dashboards) were each copy-pasting the same `data.filter(...).slice().sort(...)`
pipeline plus the `selectedX = selectedX.includes(value) ? ... : [...]`
toggle. Pull both into `web-common/components/table-toolbar/filters.ts` so
future tables get them for free and the per-table files focus on their
domain-specific predicates.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Brings in #9296's table-toolbar simplifications, which moved the toolbar
to bindable props (`bind:searchText`, `bind:sortDirection`) and changed
`onFilterChange`'s second arg from a single value to the full new
selection (the dropdown now performs the toggle internally).

- Adopt main's TableToolbar + filter-dropdown + sort APIs.
- Re-add `showSearch` and `disabled` props that this branch had previously
  introduced; propagate `disabled` to `TableToolbarSort` and
  `TableToolbarFilterDropdown` so empty-data tables disable cleanly.
- Migrate the five caller tables (alerts listing, alerts history, reports
  listing, reports history, dashboards) to the bindable API and rewrite
  each `handleFilterChange` to consume the new array signature.
- Drop the now-unused `toggleArrayValue` helper since the toolbar owns
  the toggle.
- Take main's rewritten `ProjectLogsPage.svelte`, which no longer uses
  `TableToolbar` (uses `Search` + `DropdownMenu` directly).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces the hand-rolled `Search` + `DropdownMenu` + clear-button row
with the shared `TableToolbar`, so the logs page picks up the same
filter pills, dropdown selection logic, and clear-all behavior as the
rest of the Rill Cloud listing surfaces.

- Drops `selectedLevelLabel` (toolbar shows pills via
  `TableToolbarAppliedFilters`).
- Drops `toggleLevel` (the dropdown owns the toggle now).
- `showSort={false}` since logs are inherently time-ordered streaming.
- URL filter sync (`q`, `level`) is unchanged.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces the bespoke `Search` + `OrgUsersFilters` row with the shared
`TableToolbar` so the user management surfaces match the rest of Rill
Cloud (filter pills, single dropdown for selection, clear-all).

- Groups: search-only toolbar with the create-group button in the
  toolbar's children slot.
- Guests: single-select user-type filter (`all`/`pending`).
- Users: two single-select filters (user-type, role) — both default to
  `all`, so no chip is shown until the user picks a non-default value.
- Drops the now-orphaned `OrgUsersFilters.svelte` component.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@royendo royendo marked this pull request as ready for review April 27, 2026 17:34
@royendo royendo requested a review from a team as a code owner April 27, 2026 17:34
royendo and others added 6 commits April 27, 2026 14:09
- `ReportsTable` `isFiltered` now also accounts for the result-status
  filter, so an empty result set with the filter active shows the
  "no results match your search" empty state instead of the default
  "you don't have any reports yet" message.
- Alert and report execution-history tables drop the result-status
  filter and sort toggle entirely. The toolbar was overkill for two
  options on a fixed-size 25/10 row history; the data is already
  ordered most-recent-first by the backend, so just render it.
- Restore the focus ring + `data-[state=open]` styles on the
  `AlertDialog` close-X button. They were inadvertently removed in
  the dialog-consolidation commit, leaving the button with
  `focus:outline-none` and no replacement focus indicator —
  affecting every `AlertDialog` site, not just the new
  `DeleteConfirmDialog`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The leaderboard now renders the comparison value inline as
"previous →  current" before the delta and percent (e.g.
"My Little Universe 1.4k →  4.6k  3.2k  229%"). Update the regex on
the alert email-link assertion in `alerts.spec.ts` to match the new
format. The change came in via a main merge; this branch hadn't
touched the assertion.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The leaderboard row renders a hover-only "previous → current" inline
preview when `pointerover` fires. The previous regex update
(4b1a33a) accommodated the hover state, but that lets the test
drift if the hover preview format changes again, and it's
semantically asserting on a transient interactive state rather than
the canonical row data.

Force the cursor off the leaderboard before the assertion via
`mouse.move(0, 0)` and revert the regex to the tighter pre-hover
form. Mirrors the same idiom already used in `reports.spec.ts` to
dismiss tooltips before interacting with adjacent UI.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the changes in #9367, is it right that the toolbar implemented in this PR is no longer the intended design? CC @ericokuma

Copy link
Copy Markdown
Contributor

Yes, #9367 is an improvement on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants