Fixes 30704: Gate agent wizard Next on form readiness and clear the stuck dashboard service loader - #30705
Conversation
…e loader Fixes #30704 The Add/Edit agent wizard's footer `next-button` had no disabled state, so pressing it before the lazily loaded RJSF templates resolved called `submit()` against a null form ref and silently did nothing. Report step readiness from inside the Suspense boundary and disable the button until the form is mounted. Also suppress the stray RJSF default submit button that appeared because a falsy `children` expression reads as "no children". `fetchDashboardsDataModel` never cleared `isServiceLoading`, so switching tabs on a dashboard service left the still-mounted entity tab's table spinner running for the rest of the page's life. On the Playwright side, add `waitForIngestionWorkflowForm` and use it at every step-1 advance, scope the agent-tab loader waits the way `addIngestionPipeline` already does, move the dbt config source onto the react-aria `oneOf` select, and replace the two `submit-btn` clicks that no longer resolve. `selectOneOfOption` now opens the select by clicking its wrapper instead of react-aria's visually hidden native combobox, which never opened the listbox. Co-Authored-By: Claude <noreply@anthropic.com>
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
`fetchDashboardsDataModel` only feeds the Data Model tab's count label, but it was also driving `isServiceLoading` and, on error, the entity list paging — both of which belong to `getOtherDetails`. Sharing the flag meant whichever request settled first decided the entity table's spinner, so a fast count query could clear it while the entity list was still in flight. Dropping the writes fixes the stuck spinner without introducing that race, and is simpler than clearing the flag in a `finally`. Co-Authored-By: Claude <noreply@anthropic.com>
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
…ock the merge queue (#30784) * ci(playwright): raise the chromium shard budget to 21 minutes The chromium lane outgrew a 19-minute shard. At the COMMON_MAX_SHARDS ceiling of 24 the heaviest shard is predicted at 19.2m, so assign_lane_within_budget() raises SystemExit and full-mode planning aborts before a single test runs. Every merge_group run today failed this way (PRs #30705, #30768, #30458, #30725, #30754), while pull_request_target runs pass because targeted selection is far smaller. Raise COMMON_SHARD_BUDGET_MS from 19m to 21m. At 24 shards the heaviest is 19.2m, so the loop is guaranteed to converge at or before the ceiling. 21m stays inside the 25m `timeout` wrapper around `npx playwright test` and the 35m playwright-ci-postgresql job clock, leaving ~4m of headroom. Note the common lane now sits 1m above the dedicated lanes rather than 1m below. The strict 20-minute TARGET_MS ceiling is unaffected: it bounds a single atomic unit, not a shard, so a 21m shard built from units each under 20m does not trip it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(playwright): cover full-mode allocator convergence at the shard ceiling Addresses review on #30784. assign_lane_within_budget() was only exercised in "targeted" mode, so neither the full-mode convergence path nor the SystemExit at COMMON_MAX_SHARDS had coverage -- the exact code path that took the merge queue down. Add both: - test_full_mode_chromium_converges_at_the_shard_ceiling builds a lane that needs the window above 19m and asserts the allocator converges at or before the ceiling. Verified as a real guard: with the budget reverted to 19m it fails with "needs more than 24 shards ... heaviest shard is predicted at 20.4m". - test_full_mode_chromium_reports_a_lane_the_ceiling_cannot_hold pins the SystemExit path, which had no coverage at all. Also reword the budget comment: ~4m of headroom is relative to the 25m playwright timeout wrapper specifically, not to the 35m job clock, which is looser and additionally absorbs setup/teardown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
|
Changes have been cherry-picked to the 2.0 branch. |
…tuck dashboard service loader (#30705) * fix(ui): gate agent wizard Next on form readiness, clear stuck service loader Fixes #30704 The Add/Edit agent wizard's footer `next-button` had no disabled state, so pressing it before the lazily loaded RJSF templates resolved called `submit()` against a null form ref and silently did nothing. Report step readiness from inside the Suspense boundary and disable the button until the form is mounted. Also suppress the stray RJSF default submit button that appeared because a falsy `children` expression reads as "no children". `fetchDashboardsDataModel` never cleared `isServiceLoading`, so switching tabs on a dashboard service left the still-mounted entity tab's table spinner running for the rest of the page's life. On the Playwright side, add `waitForIngestionWorkflowForm` and use it at every step-1 advance, scope the agent-tab loader waits the way `addIngestionPipeline` already does, move the dbt config source onto the react-aria `oneOf` select, and replace the two `submit-btn` clicks that no longer resolve. `selectOneOfOption` now opens the select by clicking its wrapper instead of react-aria's visually hidden native combobox, which never opened the listbox. Co-Authored-By: Claude <noreply@anthropic.com> * Fix checkstyle * fix(ui): stop the data model count fetch from owning entity tab state `fetchDashboardsDataModel` only feeds the Data Model tab's count label, but it was also driving `isServiceLoading` and, on error, the entity list paging — both of which belong to `getOtherDetails`. Sharing the flag meant whichever request settled first decided the entity table's spinner, so a fast count query could clear it while the entity list was still in flight. Dropping the writes fixes the stuck spinner without introducing that race, and is simpler than clearing the flag in a `finally`. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit 749d226)
|
Failed to cherry-pick changes to the 1.13 branch. |
Code Review ✅ ApprovedGates the agent wizard Next button on lazy-loaded form readiness and ensures dashboard service loading states clear correctly when switching tabs. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
Describe your changes:
Fixes #30704
ServiceIngestion.spec.tswas failing on the nightly AUT run. I pulled the retained traces (16 zips) and found four distinct failure signatures, not one flake — two genuine product bugs plus two stale selectors. Because the affected blocks aretest.describe.serial, one root cause takes out a whole service suite (e.g. Superset never reachesUpdate schedule optionsbecauseUpdate description…dies first).getByTestId('schedular-schedule')times out (60 s)next-buttonsilently no-ops while the RJSF form is suspendedexpect(loader).toHaveCount(0)stuck at 1 (30 s)fetchDashboardsDataModelnever clearsisServiceLoading#root/dbtConfigSource__oneof_selecttimes outoneOfselect replaced by a react-aria SelectDonetimes out (180 s)CollateSaaSrunner never completes theRestworkflow — infra, out of scope1.
next-buttonis a dead click while the Configure Ingestion form lazy-loads.AddIngestionPage/EditIngestionPagerender the footer'snext-buttonwith no disabled state; pressing it callsaddIngestionRef.current?.submit()→workflowFormRef.current?.submit()→formRef.current?.submit().IngestionWorkflowFormwraps its RJSF<Form>in<Suspense>and passes bareReact.lazytemplates, so until those chunks resolveformRef.currentisnullandsubmit()does nothing — no toast, no validation error, the wizard just stays on step 1. The trace is unambiguous:edit-buttonat t=391905,next-buttonat t=392080 (175 ms later), wizard data fetches landing at t=392102 — after the click. Regressed in #28569, which moved the advance button out of the form (it used to be a nativehtmlType="submit", which could not race a ref).IngestionWorkflowFormnow reports readiness from a smallFormReadyNotifierrendered inside the Suspense boundary (so its effect can only fire once every lazy template has resolved),AddIngestionforwards that up asonStepReadyChange, and both host pages disablenext-buttonuntil the active step is ready — mirroring thefooterNextDisabledpatternEmbeddedAddServicePagealready uses. Step 2 (ScheduleIntervalStep) is a static import, so it reports ready immediately.While in there:
{!hideFooter && (…)}evaluates tofalse, and RJSF treats falsy children as "no children" (children ? children : <SubmitButton/>), so it was injecting its own stray "Submit" button into the wizard next to the real footer. Suppressed viaui:submitButtonOptions: { norender: true }.2. Dashboard services leave a table spinner running forever.
ServiceDetailsPage.fetchDashboardsDataModelsetisServiceLoading(true)with nofinally, unlike its siblinggetOtherDetails. Its effect re-runs onactiveTab, andgetOtherDetailsearly-returns on a non-entity tab, so switching Dashboards → Agents left the flag stuck on. antdTabskeeps the visited pane mounted, so the Dashboards<Table loading>spinner stayed DOM-attached for the rest of the page's life. Invisible to the user, but fatal to any page-wide[data-testid="loader"]assertion — and dashboard-service-specific, which is exactly why only Superset and Metabase failed.3. Playwright fixes. New
waitForIngestionWorkflowFormhelper (mirrors the existingwaitForServiceConnectionForm) used at every step-1 advance;openAgentScheduleStepextracted from the four duplicated blocks inupdateScheduleOptions; the agent-tab loader waits inupdateDescriptionForIngestedTablesscoped the wayaddIngestionPipelinealready does it; the dbt config source moved onto the existingselectOneOfOptionhelper; and the two[data-testid="submit-btn"]clicks replaced withnext-button— that testid does not exist in this wizard (verified against@rjsf/core5.24.13Form.render; the button rendered there is RJSF's default and carries no testid).selectOneOfOptionitself had a latent bug found while dry-running: its Select branch preferred clicking react-aria's visually hidden nativecombobox(withforce: true), which never opens the listbox. Only the tabs branch was exercised by passing tests. It now clicks the wrapper — the patternServiceBaseClass.createServicealready uses successfully for the ingestion-runner select — and scopes options to the visible popover.Type of change:
High-level design:
The wizard footer lives in the host page while the form it submits lives three levels down behind a Suspense boundary, so "is this step submittable" has to travel upwards. Rather than reach into the ref, readiness is reported as a plain callback chain:
FormReadyNotifieris a sibling of<Form>rather than one of its children on purpose — RJSF'schildrenslot doubles as the submit-button override, and putting anything there changes button rendering. Being inside the boundary is what makes the signal correct: any suspending template defers the whole subtree, so the effect cannot fire early.Alternatives rejected:
d82b7cf0cc).No schema, API, or migration impact. The new props are optional, so the third
AddIngestion-shaped consumer (EmbeddedAddServicePage, which already has its own gate) is unaffected.Tests:
Use cases covered
oneOfselect.Unit tests
src/components/Settings/Services/AddIngestion/AddIngestion.test.tsx— the configure step reports ready only after the workflow form mounts; the schedule step reports ready immediately.src/pages/AddIngestionPage/AddIngestionPage.test.tsx—next-buttonstays disabled until the active step reports ready.src/pages/ServiceDetailsPage/ServiceDetailsPage.test.tsx— the entity tab is not left loading after moving off it, on both the resolved and rejected data-model-count paths.yarn test src/components/Settings/Services src/pages/AddIngestionPage src/pages/ServiceDetailsPage→ 28 suites, 322 tests passing.ServiceDetailsPagetest was confirmed to actually fail without the fix (data-service-loading="true") before being kept.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
playwright/e2e/nightly/ServiceIngestion.spec.ts— newEdit agent wizard step navigationdescribe: creates a MySQL service + deployed metadata pipeline via API, opens the edit wizard, and asserts Next reaches the schedule step. No ingestion runtime needed, so it is a cheap regression guard for the 60 s timeout.playwright/utils/serviceIngestion.ts,playwright/utils/serviceFormUtils.ts,playwright/support/entity/ingestion/{ServiceBaseClass,MySqlIngestionClass,PostgresIngestionClass,RedshiftWithDBTIngestionClass}.ts— helper and selector fixes described above.Manual testing performed
The Playwright changes target the nightly AUT environment (Redshift / Superset / BigQuery credentials), which I do not have locally, so the E2E suite has not been executed on this branch — it needs a nightly run to confirm. What was verified locally:
yarn test …— 28 suites / 322 tests green, and the loader test verified RED without thefinally.npx tsc --noEmit— no new errors on the changed files (the one pre-existing AJV validator variance error inIngestionWorkflowForm.tsxreproduces on a stashed tree).make ui-checkstyle-changed— exit 0, no reformat diff.next-button(playwright/utils/autoClassification.ts,playwright/utils/profilerForm.ts,playwright/e2e/Features/StorageMetadataAgentForm.spec.ts) to confirm each already waits on the form before advancing, so the new disabled state cannot hang them.UI screen recording / screenshots:
No visual change — the only user-visible difference is that the wizard's Next button is briefly disabled instead of dead while the form loads, and a stray RJSF "Submit" button no longer appears next to the footer. Trace evidence for the original failure, from the nightly run:
Checklist:
I have read the CONTRIBUTING document.
My PR title is
Fixes <issue-number>: <short explanation>My PR is linked to a GitHub issue via
Fixes #<issue-number>above.I have commented on my code, particularly in hard-to-understand areas.
For JSON Schema changes: I updated the migration scripts or explained why it is not needed. (No schema changes.)
For UI changes: I attached a screen recording and/or screenshots above. (Explained above — no visual change.)
I have added tests (unit / integration / Playwright as applicable) and listed them above.
I have added a test that covers the exact scenario we are fixing. For complex issues, comment the issue number in the test for future reference.
🤖 Generated with Claude Code
Greptile Summary
The PR fixes ingestion-wizard readiness and dashboard-service loading behavior.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the shared-loader issue is resolved by giving the entity fetch exclusive ownership of its loading state, and the previously questioned remount path is synchronous after the lazy modules have resolved.
Important Files Changed
Reviews (4): Last reviewed commit: "Merge branch 'main' into fix-serviceinge..." | Re-trigger Greptile