Skip to content

fix(frontend): ensure onboarding checks are only for production namespace, improve caching#5077

Merged
jog1t merged 1 commit into
mainfrom
05-20-fix_frontend_ensure_onboarding_checks_are_only_for_production_namespace_improve_caching
May 21, 2026
Merged

fix(frontend): ensure onboarding checks are only for production namespace, improve caching#5077
jog1t merged 1 commit into
mainfrom
05-20-fix_frontend_ensure_onboarding_checks_are_only_for_production_namespace_improve_caching

Conversation

@jog1t
Copy link
Copy Markdown
Contributor

@jog1t jog1t commented May 20, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 20, 2026

Code Review

Overview

This PR improves the onboarding flow in the namespace route loader and related frontend infrastructure. The changes look well-structured, but there are a few issues worth addressing.


Bugs

1. Inverted undefined guard silently suppresses onboarding (frontend/src/routes/_context/ns.$namespace.tsx)

namespaceQueryOptions returns data.namespaces[0], which is undefined when the API returns an empty list. Since undefined !== "Default" evaluates to true, the loader exits early with displayOnboarding: false — hiding onboarding for a namespace whose state is actually unknown. The guard should be:

namespace !== undefined && namespace.displayName !== "Default"

2. hasRunnerNames/hasRunnerConfigs only check pages[0] (frontend/src/lib/data.ts)

deriveOnboardingState uses pages[0] for the boolean checks but correctly uses pages.flatMap(...) for provider derivation just above. If the first page of a paginated result is empty but later pages contain data, hasBackendConfigured is false and onboarding incorrectly appears. Use .some() across all pages, consistent with the provider scan:

const hasRunnerNames = pages.some(p => p.runners.length > 0);

Minor Issues

3. persist: true applies to all runnerConfigsQueryOptions invocations (frontend/src/app/data-providers/engine-data-provider.tsx)

all-runner-select.tsx calls runnerConfigsQueryOptions({ variant: "serverless" }), which gets a different query key but shares the same persist: true meta. Serverless-variant configs are persisted to localStorage unintentionally, potentially bloating storage. Consider scoping persist: true to the base (no-variant) call only.


4. 24-hour maxAge creates a wide stale-data flash window (frontend/src/queries/global.ts)

If a user deleted all runner configs and revisits within 24 hours, the cached stale data causes the cache-first gate to skip the fetch, showing no onboarding until the background refetch fires. A shorter maxAge (e.g. 1 hour) bounds this window without hurting the page-refresh use case.


Summary

The two bug-level issues (#1 and #2) are the most actionable — both can cause the onboarding flow to be incorrectly suppressed in real production scenarios. The minor issues are lower priority but worth a follow-up.

Copy link
Copy Markdown
Contributor Author

jog1t commented May 21, 2026

Merge activity

  • May 21, 8:51 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • May 21, 8:53 PM UTC: Graphite rebased this pull request as part of a merge.
  • May 21, 8:53 PM UTC: @jog1t merged this pull request with Graphite.

@jog1t jog1t changed the base branch from 05-20-fix_frontend_fix_changelog_display to graphite-base/5077 May 21, 2026 20:51
@jog1t jog1t changed the base branch from graphite-base/5077 to main May 21, 2026 20:52
@jog1t jog1t force-pushed the 05-20-fix_frontend_ensure_onboarding_checks_are_only_for_production_namespace_improve_caching branch from 7c3c044 to 3a1e78a Compare May 21, 2026 20:52
@jog1t jog1t merged commit 164fc7a into main May 21, 2026
8 of 11 checks passed
@jog1t jog1t deleted the 05-20-fix_frontend_ensure_onboarding_checks_are_only_for_production_namespace_improve_caching branch May 21, 2026 20:53
const namespace = await context.queryClient.fetchQuery(
context.dataProvider.currentNamespaceQueryOptions(),
);
if (namespace?.displayName !== "Default") {
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.

Bug: the undefined-namespace guard is inverted. When the API returns an empty list, namespaceQueryOptions returns data.namespaces[0] which is undefined. Then namespace?.displayName is undefined, and undefined !== 'Default' is true, so the loader exits early with displayOnboarding: false — silently suppressing onboarding for a namespace whose state is unknown. The correct guard is: only skip onboarding when namespace is defined AND its displayName is not 'Default'. An unresolvable namespace should fall through to the onboarding fetch rather than hiding it.

Comment thread frontend/src/lib/data.ts
)
.find((p) => p !== undefined);

const hasRunnerNames = (runnerNames?.pages[0]?.names.length ?? 0) > 0;
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.

Bug: only pages[0] is checked but deriveOnboardingState scans all pages for the provider. When an infinite query spans more than one page, pages[0] could be empty while later pages hold names or configs, causing hasBackendConfigured to be false and incorrectly showing the onboarding screen. The provider lookup just above this line already correctly uses (runnerConfigs?.pages ?? []).flatMap — apply the same pattern here: runnerNames?.pages.some((p) => p.names.length > 0) and runnerConfigs?.pages.some((p) => Object.keys(p.runnerConfigs).length > 0).

retry: shouldRetryAllExpect403,
meta: {
mightRequireAuth,
persist: true,
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.

Bug: persist:true is attached to every invocation of runnerConfigsQueryOptions, including the variant-filtered call in all-runner-select.tsx (runnerConfigsQueryOptions({ variant: 'serverless' })). That call gets a different query key ([{namespace}, 'runners', 'configs', {variant:'serverless'}]) but still carries persist:true, so serverless-variant configs are also written to localStorage. The persist comment says only a curated allowlist should be stored. Fix: make persist conditional on the no-opts path: persist: opts === undefined.


const persistOptions: Omit<PersistQueryClientOptions, "queryClient"> = {
persister: queryCachePersister,
maxAge: 24 * 60 * 60 * 1000,
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.

PLAUSIBLE: a 24-hour maxAge creates a wide window where deleted configs appear to still exist. If a user removed all runner configs in a previous session, the persisted cache entry keeps showing hasBackendConfigured=true for up to 24 hours. On reload the cache-first gate skips the blocking fetch, deriveOnboardingState derives displayFrontendOnboarding=false (backend configured, no actors), and no onboarding is shown — until the background refetch fires and invalidates the loader, causing a flash. A 1-hour maxAge is sufficient to survive typical page refreshes while bounding the stale window.

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.

1 participant