Uh oh!
There was an error while loading. Please reload this page.
improvement(data-drains): move drains to the fullscreen list/detail pattern - #5954
Conversation
…attern Data drains was the last settings surface still on a Table plus a create modal. It now matches Skills and Custom Tools: - the list is SettingsResourceRow rows in a clickable button, with the source, destination, cadence, and last run on the row and a Disabled tag when a drain is paused - clicking a row opens a detail sub-view: the drain's actions (Run now / Test connection / Delete), its enabled toggle, resolved destination config, and recent run history — replacing the expanding table row - creating a drain is a fullscreen view instead of a modal, reusing the destination form registry so the two never fork - a drain's detail is deep-linkable via `data-drain-id`, pushed on open and replaced on close, matching custom-tool-id and custom-block-id The detail is read-only apart from the enabled toggle: destination credentials are never returned by the API, so changing one means recreating the drain. Alignment work the migration surfaced: - the destination registry rendered its 36 fields with ChipModalField, whose label is byte-identical to a SettingsSection header — on a page that made section titles and field labels indistinguishable. All of them, and the create view's own fields, now use SettingRow like every other page-level detail view - the shared source/destination/cadence label maps moved to labels.ts, now that three files need them - run status uses Badge with a dot rather than hand-coloured text, and byte counts use formatFileSize — the old /1024 math showed a 5 GB export as "5242880.0 KB" - copy follows the sibling surfaces: Create drain, Disabled, Run queued, Connection test passed, and "No drains found matching …" - seed the list cache on create so landing on the new drain's detail doesn't depend on the invalidation refetch succeeding
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview The list is clickable
The monolithic settings file shrinks accordingly; shared label maps live in Reviewed by Cursor Bugbot for commit f51d0e1. Configure here. |
Greptile SummaryThe PR migrates Data Drains to the fullscreen settings list/detail pattern.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains in the fixes associated with the previous accessibility threads.
|
| Filename | Overview |
|---|---|
| apps/sim/ee/data-drains/components/data-drain-create.tsx | Adds the fullscreen drain creation flow with accessible controls, validation, and unsaved-change handling. |
| apps/sim/ee/data-drains/components/data-drain-detail.tsx | Adds the read-only drain detail surface, enabled toggle, actions, resolved configuration, and run history. |
| apps/sim/ee/data-drains/components/data-drains-settings.tsx | Replaces the table and modal with searchable resource rows, URL-selected details, and fullscreen creation. |
| apps/sim/ee/data-drains/destinations/registry.tsx | Migrates destination fields to SettingRow and correctly associates id-capable controls while naming ChipSelect directly. |
| apps/sim/ee/data-drains/hooks/data-drains.ts | Updates data-drain query and mutation behavior to support the new list/detail flow. |
| apps/sim/app/workspace/[workspaceId]/settings/[section]/search-params.ts | Adds URL state configuration for deep-linking to an existing data drain. |
| apps/sim/ee/data-drains/labels.ts | Centralizes source, destination, and cadence labels and select options. |
Reviews (3): Last reviewed commit: "fix(data-drains): name the select fields..." | Re-trigger Greptile
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The ChipModalField -> SettingRow migration dropped the label/control association that ChipModalField generated for free, so screen readers announced the destination fields unnamed and clicking a label focused nothing. All 35 id-capable controls now carry an id with a matching htmlFor on their row; the one ChipSelect has no id prop, matching how the existing SettingRow + combobox rows render. Also pass includeBytes to formatFileSize — without it any run writing under 1 KB reported '0 Bytes'.
waleedlatif1
commented
Jul 25, 2026
waleedlatif1
commented
Jul 25, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 4848202. Configure here.
ChipSelect takes an aria-label that lands on its trigger button, so the four select fields no longer announce as unnamed buttons — the Datadog site plus the create view's source, cadence, and destination type. The visible SettingRow label stays; this only gives the trigger an accessible name, since ChipSelect exposes no id for htmlFor to point at.
waleedlatif1
commented
Jul 25, 2026
waleedlatif1
commented
Jul 25, 2026
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f51d0e1. Configure here.
Summary
Table+ create modal. It now matches Skills and Custom Tools.SettingsResourceRowrows in a clickable button — source, destination, cadence, and last run on the row, plus aDisabledtag when a drain is paused.ChipModal, reusingDESTINATION_FORM_REGISTRYso the two never fork.data-drain-id(push to open, replace to close), matchingcustom-tool-id/custom-block-id.The detail is read-only apart from the enabled toggle — destination credentials are never returned by the API, so changing one means recreating the drain.
Alignment work the migration surfaced:
ChipModalField, whose label is byte-identical to aSettingsSectionheader (text-small,--text-muted,pl-0.5) — on a page that made section titles and field labels indistinguishable. All of them, plus the create view's own fields, now useSettingRowlike every other page-level detail view. The registry had exactly one consumer, so no modal was left behind.labels.tsnow that three files need them (verified byte-identical to the originals).Badgewith a dot instead of hand-coloured text, and byte counts useformatFileSize— the old/1024math rendered a 5 GB export as5242880.0 KB.Create drain(notNew drain),Disabled(notPaused),Run queued,Connection test passed,No drains found matching ….Tradeoff worth a look
Enable/disable and the per-row Run now / Test / Delete actions previously sat inline on each table row; they now live in the detail, so toggling several drains costs more clicks. That's inherent to the row pattern — a
RowActionsMenuin the row'strailingslot would mean dropping the row-level<button>(a menu trigger nested in a button is invalid HTML). Flagging it as a deliberate choice rather than an oversight.Type of Change
Testing
Typecheck,
bun run lint:check(19/19 packages),check:react-query,check:api-validation, and 92 tests acrossee/data-drains,lib/data-drains, and settings all pass. Reviewed by five parallel audit passes (emcn design, hooks, React Query + URL state, comments/copy, and an adversarial regression pass) — findings applied. Not yet clicked through in a browser, so the visual pass is worth doing on the preview.Checklist