Skip to content

web/table: fetch on first render when already visible#22376

Open
GirlBossRush wants to merge 4 commits into
goauthentik:mainfrom
GirlBossRush:task-triage-rac-endpoint-modal
Open

web/table: fetch on first render when already visible#22376
GirlBossRush wants to merge 4 commits into
goauthentik:mainfrom
GirlBossRush:task-triage-rac-endpoint-modal

Conversation

@GirlBossRush
Copy link
Copy Markdown
Contributor

@GirlBossRush GirlBossRush commented May 15, 2026

Summary

Tables inside <ak-modal> rendered empty until the user clicked the refresh button — surfaced by a beta tester on the 2026.5 RC against the RAC endpoint launcher in the user library.

Root cause

The native-<dialog> modal migration (#21336) taught AKModal.updated() to force visible = true on its slotted child. Table.firstUpdated() was delegating to #synchronizeRefreshSchedule(), which only flushes a previously deferred refresh. With visibility forced on before the first update cycle, no deferred refresh was ever queued — so the synchronizer no-op'd and nothing fetched.

Fix

Call fetch() directly from Table.firstUpdated(). fetch() already handles both states correctly:

  • table visible → issue the request immediately.
  • table not yet visible → queue the deferred refresh that the synchronizer flushes on visibility flip.

The change therefore also covers any future caller that mounts a Table already-visible, not just the modal path.

 protected override firstUpdated(changedProperties: PropertyValues<this>): void {
     super.firstUpdated(changedProperties);
-    this.#synchronizeRefreshSchedule();
+    this.fetch();
 }

Test plan

  • Manually reproduced against the user-library RAC endpoint launcher in the playpen dev stack: modal opens empty on main, populates without a manual refresh with this change.
  • Playwright e2e regression test added at web/test/browser/rac-launch-modal.test.ts — seeds a RAC provider + two endpoints via the API, opens the launcher modal, asserts the endpoint rows appear without any user interaction. Fails on main, passes with this change.
  • make lint-fix clean.

Disclosure

Triaged and authored end-to-end by an agent running in a sandbox against goauthentik/authentik@main. Reviewed and shipped by @GirlBossRush.

@GirlBossRush GirlBossRush requested a review from a team as a code owner May 15, 2026 04:07
@netlify
Copy link
Copy Markdown

netlify Bot commented May 15, 2026

Deploy Preview for authentik-storybook ready!

Name Link
🔨 Latest commit 2eed554
🔍 Latest deploy log https://app.netlify.com/projects/authentik-storybook/deploys/6a070f46b8b0d20008b3dbfd
😎 Deploy Preview https://deploy-preview-22376--authentik-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link
Copy Markdown

netlify Bot commented May 15, 2026

Deploy Preview for authentik-docs ready!

Name Link
🔨 Latest commit 5ecdf74
🔍 Latest deploy log https://app.netlify.com/projects/authentik-docs/deploys/6a069c1b0db0570008d93bf8
😎 Deploy Preview https://deploy-preview-22376--authentik-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.08%. Comparing base (5cd1bf0) to head (2eed554).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #22376      +/-   ##
==========================================
+ Coverage   93.07%   93.08%   +0.01%     
==========================================
  Files        1032     1032              
  Lines       60055    60055              
  Branches      400      400              
==========================================
+ Hits        55895    55902       +7     
+ Misses       4160     4153       -7     
Flag Coverage Δ
conformance 36.61% <ø> (+<0.01%) ⬆️
e2e 40.97% <ø> (-0.27%) ⬇️
integration 32.55% <ø> (-0.50%) ⬇️
rust 0.00% <ø> (ø)
unit 91.95% <ø> (-0.26%) ⬇️
unit-migrate 91.02% <ø> (-1.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Tables inside `<ak-modal>` rendered empty until the user clicked the
refresh button. The 2026.5 RC native-`<dialog>` migration taught
`AKModal.updated()` to force `visible = true` on its slotted child, but
`Table.firstUpdated()` was delegating to `#synchronizeRefreshSchedule()`,
which only flushes a *previously deferred* refresh. With visibility
forced on before the first update cycle, no deferred refresh was ever
queued, so the synchronizer no-op'd and nothing fetched.

Switch the first-update hook to call `fetch()` directly. `fetch()`
already handles both states correctly: if the table is visible it
issues the request immediately, and if it isn't it queues the deferred
refresh that the synchronizer flushes when visibility flips on. Beyond
the modal case this also covers any future caller that mounts a Table
already-visible.

Reproduced and verified against the user-library RAC endpoint launcher
(the surface from the beta report). Added a Playwright e2e
(`rac-launch-modal.test.ts`) that seeds a RAC provider + two endpoints
via the API, opens the launcher, and asserts the endpoint rows appear
without a manual refresh — fails on `main`, passes with this change.

A 2026.5 backport will follow as a separate PR.

Co-Authored-By: Agent (authentik-m-triage-rac-proper-shared-lilac) <279763771+playpen-agent@users.noreply.github.com>
@GirlBossRush GirlBossRush force-pushed the task-triage-rac-endpoint-modal branch from 0036a40 to ba1cd93 Compare May 15, 2026 10:21
`changeme` in the playpen-specific default for `AK_TEST_BOOTSTRAP_TOKEN`
trips the spellcheck lint job. Add an inline `cspell:ignore` directive
so the fallback can stay (CI sets the env var so the default is only
used locally inside playpen sandboxes).
@GirlBossRush GirlBossRush force-pushed the task-triage-rac-endpoint-modal branch 2 times, most recently from e1c6528 to 2ec1794 Compare May 15, 2026 11:09
@GirlBossRush GirlBossRush force-pushed the task-triage-rac-endpoint-modal branch from 2ec1794 to 09a3360 Compare May 15, 2026 12:15
<ak-form-element-horizontal required name="protocol">
${AKLabel(

<ak-radio-input
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ARIA fixes for the e2e test.

includeScore: true,
shouldSort: true,
ignoreFieldNorm: true,
useExtendedSearch: true,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

AFAICT this is a vestigial setting and not something we've intended to support in this context. Disabling it here because programmatically generated application names can include search control characters that prevent results from matching.

// So we exclude self-presentation explicitly.
const control = context
.getByLabel(fieldName, { exact: true })
.and(context.locator(':not([role="presentation"])'))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feels like a hack.

// latter only flushes a *previously deferred* refresh and would no-op
// when a parent (e.g. `AKModal`) has already forced `visible = true`
// before the first update cycle, leaving the table empty on open.
this.fetch();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not connectedCallback() ?

I'm surprised Lit doesn't whine about a render being scheduled from an updated() command. It usually does. You can work around it by microtasking the fetch, but this still doesn't feel like the place to do that.

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

Labels

backport/version-2026.5 Add this label to PRs to backport changes to version-2026.5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants