[perf] Paint the playground from disk on reload; cut boot render + request cost#5433
Open
ardaerzin wants to merge 29 commits into
Open
[perf] Paint the playground from disk on reload; cut boot render + request cost#5433ardaerzin wants to merge 29 commits into
ardaerzin wants to merge 29 commits into
Conversation
…ow entities and catalogs Adds @agenta/shared/api/persist: idb-keyval-backed per-query persistence via TanStack's experimental_createQueryPersister, with two policy classes: - immutablePersister (revision bodies): restore from disk, never revalidate, schema-version buster; revision detail gets staleTime Infinity + 1h gcTime, and commit/list primes persist through primeWorkflowRevisionDetailCache (full bodies only) - catalogPersister (inspect, harness catalog, ag-type schemas, tool/trigger catalogs incl. infinite queries, evaluator/app/application templates, service schemas): paint-from-disk + one background revalidate when stale Replaces the localStorage disk-SWR layers (persistedInspect LRU/quota machinery, persistedCatalog) — persistedAgentType and last-selection stay in localStorage as sync-critical first-frame signals. Logout clears the IDB store; an idle GC sweep drops expired/stale-version entries. Safety and diagnostics: - nullish-data guard: null queryFn results are never persisted and pre-existing nullish entries are evicted on read (an immutable-restored null would suppress refetch forever) - agenta:persist:debug console logging (HIT/MISS/WRITE/SKIP/EVICT/GC) and agenta:persist:disable kill switch at the storage boundary - query-core unified at 5.100.9 via pnpm-workspace override (persist-client- core exact-pins its own query-core, which split the workspace into two nominally-incompatible type universes) Design doc with inventory, mutability-class policy, and build notes in docs/design/playground-query-persistence/plan.md. 290 unit tests green in @agenta/shared (15 new persistence behavioral tests over fake-indexeddb).
…etch Two follow-ups from measuring the persistence work (dev, 3 runs/condition: persisted requests absent as designed, but felt reload time dominated by the front of the waterfall, not the data chain): - profile + projects paint-fast: profileQueryAtom and projectsQueryAtom now restore from IndexedDB and revalidate in the background (profile has no staleTime, so restore always triggers exactly one refetch). profile gates the entire level-2 fanout on !!user, so warm reloads start that tree ~0.5-1s earlier. Logout clears the store; nullish results never persist. - eager revisions-list mount removed: workflowLatestRevisionIdAtomFamily's fallback branch get()-mounted the full revisions-by-workflow query whenever the dedicated latest-revision query was still in flight — the entire cold-load window — via the two always-mounted header consumers (PlaygroundVariantConfigHeader, SelectVariant). The fallback is now a passive queryClient.getQueryData peek; reactivity flows through the dedicated latest query, whose cache both writers prime. Pickers still mount the list on open; the no-selection default path keeps its list fallback. Replaces a full-body list fetch (0.5-1.7s observed) with the lightweight latest-revision batch on cold loads with a known selection.
…riven) A captured reload waterfall showed the agent playground firing requests for collapsed empty sections, hidden dropdown data, and one guaranteed-null query. This trims the burst without changing what any other page fetches: - idleReadyAtom moves to @agenta/shared/state so package queries can gate on it (oss entitlements consumer re-pointed; no re-export shim) - tools/connections, triggers/subscriptions, triggers/schedules gain enabled: idleReady — they render collapsed-section counts and no longer spend 1.2-2s backend slots inside the first-paint burst - the environments list stays untouched (render-critical on the deployments dashboard); instead the playground's always-mounted deployment badge (revisionDeploymentAtomFamily) returns empty until idle, so it no longer drags environments/query into the burst - variants list defers to dropdown-open: new workflowVariantsCachedListAtomFamily passive cache peek + sticky activation latches in SelectVariant and DeployVariantButton (which double-mounted the list via useAppEnvironments); the collapsed label falls back to revision name, then the variant slug carried by the revision body - fix a guaranteed-null request: DeployVariantButton read the workflow-level is_evaluator flag through the revision-keyed molecule with a WORKFLOW id, firing a null /workflows/revisions/query on every playground load; it now reads workflowDetailQueryAtomFamily, cache-shared with the router - the __ag__build_kit overlay persists via catalogPersister (staleTime Infinity -> 5m so platform updates are picked up) Known same-pattern sites on other views (EvaluatorPlaygroundHeader, WorkflowRevisionDrawer MetadataSidebar, evaluatorColumns) are documented in docs/design/playground-query-persistence/plan.md as a follow-up sweep.
…s fetched 3x Two cold-load defects found via HAR captures after the persistence work: - inspect: local draft ids are minted fresh on every reload (generateLocalId -> local-<ts>-<rand>) and snapshot hydration always re-creates drafts, so the inspect queryKey's revisionId component changed every reload -> the IndexedDB persister missed every time and the full /inspect round-trip (5.3s under contention) fired on every draft reload. The request body never contains a revision id (inspectWorkflow keys on uri + serviceUrl), so the queryKey is now service-scoped: ["workflows","inspect", uri, serviceUrl, projectId]. Restores the old persistedInspect per-service sharing and dedupes in-memory inspect entries across drafts/revisions of one service. Sole invalidation site is prefix-based and unaffected; no per-revision invalidation exists. - session records: loadSessionMessages called querySessionRecords directly, outside TanStack, from two AgentConversation effects (cache-miss hydration + revalidate-on-open). The effects' cancelled flag discards results but not requests, so StrictMode double-mounts / ancestor-key remounts fired two simultaneous 226KB fetches on top of the Inspector's atom-based one (which also fetches while closed - it subscribes before its active guard). The imperative path now goes through queryClient.fetchQuery with the shared sessionRecordsQueryOptions (new fetchSessionRecordsAtom), so all paths join one flight per 15s stale window.
…null-fetch stragglers Continues the HAR-driven playground load work: - trace summaries (the 1.9MB /spans/query transcript enrichment) are immutable once ingested (the query already had staleTime Infinity) and now persist via immutablePersister: the payload downloads once per session ever; reloads restore from IndexedDB with zero network. The response was already roots+errors-filtered — it is fat because root spans embed attributes.ag.data, which the playground result chips genuinely need, and /spans/query supports no field projection (a server-side projection option is the follow-up if first-load cost matters) - billing queries (subscription/catalog/pricing) now gate on isBillingEnabled() (NEXT_PUBLIC_AGENTA_BILLING_ENABLED) in addition to isEE(): deployments without the billing service no longer burn a guaranteed 502 on every load; entitlement logic already treats billing-disabled as unrestricted - a closed agent-chat Inspector no longer fetches session records (226KB): it subscribed the records query before its active guard; the query now gets an empty key until the inspector opens - three more sites fed a WORKFLOW id into the revision-keyed molecule (guaranteed-null /revisions/query on their views), same defect as the DeployVariantButton fix: EvaluatorPlaygroundHeader (evaluator playground load), WorkflowRevisionDrawer MetadataSidebar (drawer open), and annotationSessionController testsetSyncEvaluatorsAtom (annotation queues). All now read workflowDetailQueryAtomFamily. evaluatorColumns was audited and left alone: the evaluators table pre-seeds molecule entities, so no fetch fires there and the substitution would have added N real requests
Increment-4 round of the playground persistence work: - vault secrets paint-fast with REDACTED persistence: /secrets/ returns plaintext key material (provider keys, AWS secrets, session tokens, service-account JSON), so a dedicated vaultSecretsPersister serializes a redacted projection - every secret-value field becomes a truthy "[redacted]" sentinel (truthy so hasKey badges paint correctly from disk), and refetchOnRestore: "always" guarantees one immediate refresh so a sentinel can never linger into the provider edit modal and get saved back over a real key. 8 tests incl. a no-raw-secret-in-payload sweep - session records paint-from-disk: chat history renders instantly on warm reload via a dedicated recordsPersister (7d maxAge, prefix agenta-rec, refetchOnRestore: "always" - fires even observer-less, closing the bare fetchQuery restore gap). The one-shot loadSessionMessages path gains an onRefreshed callback so the fresh transcript is re-delivered after the post-restore revalidate; adoption stays behind the existing busy/ strictly-ahead guards, so live streams and optimistic tails are never clobbered - sidebar all-apps list + agent-flags batch gated at the sidebar source (returns loading until idle): the query atoms stay untouched because archived-apps/overview/evaluation pages render primary content from them. Orgs list gated enabled: neededForBoot || idle where neededForBoot means the URL carries no workspace id - auth/post-login/workspace-selection flows fetch immediately, workspace-scoped routes defer to idle; org DETAIL and the fa56d9f dedup seeding untouched
…9KB parsed) The June bundle work (PR #4864) kept the monolithic AgentaApiClient (all 27 resource clients) out of _app via per-resource accessors. One line written since then regressed it: fetchSimpleApplication in workflow/api/api.ts imported getAgentaSdkClient from the @agenta/sdk root, whose barrel statically pulls the monolith - webpack concatenated 22 resource-client modules (~326KB parsed) back into _app. Measured: _app chunk 2672KB -> 2443KB parsed after re-pointing that call through a new getApplicationsClient() accessor. Prevention, since this is the second occurrence: - web/AGENTS.md Fern pattern now shows @agenta/sdk/resources accessors and names the root-barrel import as an anti-pattern (the old doc example was the template the regression copied) - packages eslint: no-restricted-imports error on the @agenta/sdk root (tests unaffected - package lint covers src/ only) - SessionInspector/api.ts (the only other root importer, async-chunk) migrated to getSessionsClient/getMountsClient accessors
…ayground chunk Two-track investigation of initial load (provider/auth chain + chunk waterfall and re-render amplifiers), with a ranked improvement plan. Headline findings: four serial null-render gates (SuperTokens init incl. an avoidable async chunk fetch; Layout chunk; protectedRouteReadyAtom which flips repeatedly and remounts the page subtree; the 10.19MB Playground chunk with zero warmup) and a render-blocking uncacheable /__env.js. Plan is analysis-only - no implementation in this commit.
…auth config) The boot analysis (docs/design/app-boot-optimizations/plan.md) found the initial load is a serial chain of null-render gates, and three lazily-split chunks were only DISCOVERED after earlier gates released - serializing their download+parse behind auth/session resolution instead of running in parallel with it: - Playground lazy graph (~3.2MB uncompressed, 63 files on the current build; an earlier stale manifest overstated this as 10MB): discovered only after the SuperTokens init gate, the Layout chunk, and the protected-route data gate. The import() thunk is now hoisted, shared by both dynamic() wrappers (webpack dedupes), and kicked at module eval - the graph downloads while the gates resolve. - Layout chunk (~336KB): same treatment from _app. - frontendConfig chunk (SuperTokens recipes): the code-split is deliberate (keeps prebuilt-UI recipe modules out of _app) and is preserved - but the chunk now warms during hydration, so AuthProvider's init await resolves from the in-flight request instead of adding a serial round-trip while the ENTIRE provider tree below it renders null. Verified no bundle regression: _app and route first-load byte-identical (modulo hash noise); all splits intact - the warmup changes when chunks download, not what is in them. Tier 1 items T1.3/T1.4 (protected-route latch, /__env.js inlining) and the Tier 2 re-render hygiene from the plan doc are intentionally not in this commit - land after this one is measured.
….2MB), mark T1.1-T1.2 built
…lain buttons A React DevTools profile of a playground reload showed 'ForwardRef(Anonymous)' as the single largest render line (594 renders / 222ms) - DevTools aggregates every unnamed forwardRef under one label, making the biggest cost center in the app unattributable. All seven anonymous forwardRefs now carry displayName (EnhancedButton x2, AddButton, RichChatInput, EditorInner, ColumnVisibilityHeader x2). The hunt also explained the Tooltip x173 / Trigger x231 churn in the same profile: EnhancedButton (both copies) unconditionally wrapped every button in an antd Tooltip even with empty tooltipProps, so every button render paid a Tooltip+Trigger render. The wrapper is now skipped when there is no tooltip title. The oss copy also gains the ref forwarding it declared but never applied.
…lifiers)
Targets the re-render amplifiers the boot analysis predicted and a React
profiler run confirmed (Context.Provider x632, Sidebar x10, PlaygroundHeader
x11, LoadableComponent x121 renders during one reload):
- stable identities: ThemeContext provider value + toggleAppTheme
(useMemo/useCallback), ThemeContextBridge token object, SWRConfig merged
config (incl. the default-parameter {} minted per render), Layout's inline
ConfigProvider theme literal
- widest subtree narrowed: AppWithVariants now subscribes a 3-field
selectAtom slice (pathname/routeLayer/asPath) plus a string-valued
baseAppURLAtom instead of the whole appStateSnapshotAtom + urlAtom (whose
org/recent-app churn re-rendered the entire content layout); the
snapshot's timestamp field is dropped entirely - grep showed zero
consumers, so every write no longer guarantees an identity change
- currentWorkflowContextAtom is equality-stable (previous-value reuse when
all 6 fields match) - PlaygroundRouter and ~15 other consumers stop
re-rendering on query-phase transitions; workflowKind changes still
propagate
- ProtectedRoute subscribes only protectedRouteReadyAtom; its five
side-effectful boot hooks (session/profile/project mounts) moved to a
memo'd null-rendering BootSubscriptions island - same effects, gate
re-renders only on the boolean flip; no latching (that is T1.3)
- Sidebar drops useRouter() for a string-valued appAsPathAtom;
PlaygroundHeader's churning-input inline atoms hoisted to module level
(selectedChildrenByParent, rootRevisionVersion, notAgent fallback)
Per-file oss tsc error counts identical to pre-change baseline.
…oot atom graph (T1.3) protectedRouteReadyAtom flipped false at six sites during boot auth resolution, and ProtectedRoute swapped null <-> children on it - so workspace/org switches and mid-boot re-syncs unmounted and remounted the entire page subtree (the profiler's mount-storm commits). Now: - ready = raw || (latch && sessionExists): the latch turns sticky on first true, absorbing boot-time churn; the live-session term makes it impossible for a dead session to keep a page visible. Sign-out paths clear both the raw flag and the latch. Five auth scenarios traced (cold logged-in, cold logged-out redirect - children never mount, sign-out, UNAUTHORISED mid-use, workspace switch) - pre-ready renders a BootShell (sidebar-rail ghost + empty content pane, collapse-state aware) instead of null, so gate release fills in rather than mount-storming; the auth page keeps a blank shell - the ~157ms first evaluation of the boot query-atom graph (query atom construction, persister wiring, observer creation - profiler-attributed to ProtectedRoute's first read) moves off the 322ms provider-mount commit: prewarmBootQueryGraph() runs at the start of AuthProvider's init effect, overlapping the frontendConfig chunk fetch. Safe pre-auth: the warmed queries are enabled-gated on a session atom that is provably false until SessionListener mounts
The agent config accordion subtree rendered its anonymous forwardRef children ~211 times during one playground load (AgentTemplateControl x6 at 77ms self, Memo(PlaygroundConfigSection) x15). Same taxonomy as the tier-2 sweep, applied to the config panel: - PlaygroundConfigSection molecule adapter atoms (data/serverData/ schemaQuery) wrapped equality-stable (deepEqual / field-wise) - upstream selectors mint fresh objects on every query fetch-state flip during boot - PlaygroundVariantConfig narrows its subscriptions (isPending flag and uri/flags fields via selectAtom instead of whole query/data objects); stable element/empty-object constants so memo children stop re-rendering - DrillInUIContext value stabilized: OSSdrillInUIProvider memoizes the components object (annotated via cast - the context's index-signature slots vs rich concrete components is a pre-existing type gap, and the two pre-existing whole-object tsc errors at HEAD are now resolved); useLLMProviderConfig memoizes footerContent/drawer/config; deployment policy object gets a stable identity - MoleculeDrillInContext: fields fallback no longer mints a fresh dep per render; TriggerManagementSection + useModelHarness subscription hygiene; AddTextLink et al follow the displayName convention Verification: entity-ui/playground-ui/playground builds + lint green, oss tsc zero errors on all touched files (2 better than baseline), entities 909/909. The accordion agent was cut off by a session limit before its verification pass; gates were re-run and two type issues fixed during integration
…ata, unblock org detail Third measured iteration on playground time-to-active (HAR + profiler pair from a warm reload): - the full revisions list STILL fired on warm reloads with a selection: bindRevisionsReady checked the selection at bind time, but the persisted last-selection is only restored inside tryApplyDefaults - which ran AFTER the list store.sub had mounted the query. The fetch won the race on every warm reload. Defaults are now applied first and the list subscription is created only when nothing was applied; the genuinely-selection-less cold load keeps the old retry mechanism - the current app's detail + latest-revision fetches fired at ~1450ms only because nothing subscribed them until the page tree mounted behind the gates and the chunk-parse window; their gates are fully URL-derivable at boot. prewarmBootQueryGraph gains a task queue (tasks run in the same synchronous burst as the profile prewarm once the real queryClient is bound) and PlaygroundRouter registers a current-workflow prewarm at module eval next to the chunk warmup. Both requests now leave with profile - selectedOrgQueryAtom was enabled-gated on the /profile response (the exact serialization 2ede5fa removed for projects); now gates on jwtReady like projectsQueryAtom. Org detail heads the level-3 cluster (plans/roles/ workspace resolution) - workflowDetailQueryAtomFamily persists via catalogPersister (30s staleTime kept). Invalidation-audited: no code invalidates that key, so disk restore + background revalidate adds no new staleness class. Kills the Playground -> ConfigureEvaluatorPage swap-flash for evaluator apps and feeds currentAppQueryAtom consumers instantly on warm reloads Also settled by this round's investigation: the 532-1061ms network dead zone is main-thread parse of the warmed chunk graph (the T1.1 tradeoff), not a data gate - the orgs-list idle gate captured nothing boot-critical. A sessionStorage ready-latch was evaluated and REJECTED (it would render protected content before SuperTokens confirms the session)
…loads Bottom-up gate audit (docs/design/app-boot-optimizations/gates.md) found the config panel's loading states outlived the IndexedDB-restorable data behind them. Five relaxations, each re-derived from the code: - selection before first render: state/url/playground.ts now runs the full syncPlaygroundStateFromUrl at module eval (guarded to playground routes), so the persisted selection / URL revisions / snapshot hash are applied BEFORE the first React commit instead of in playgroundSyncAtom.onMount one commit later. Kills the one-frame config/generation placeholder boxes; precedence (hash -> ?revisions= -> persisted -> defaults) is preserved by reusing the sync itself; later replays are idempotent via the existing dedupe guards. Bonus: the snapshot-over-localStorage priority now actually holds on hard reloads (the hash is processed before either onMount) - draft hydration fast-path: pending URL-snapshot hydrations now apply as soon as the SOURCE revision's query DATA is present (the !isPending term dropped at both apply sites) - the draft warm-reload skeleton now ends at IDB restore latency instead of full query settle. Presence checks stay on the molecule query's server baseline (not the merged selector) so a draft can never patch over an empty baseline - agent chrome decided by the synchronous persisted early-agent signal; the variantQueryPending disjunct is removed (it mislabeled prompt apps as agent chrome for the whole restore window) - AgentChatSkeleton only when pending AND no data - AgentTemplateControl chunk preloads on the early-agent signal (first effect) instead of requestIdleCallback Explicitly NOT relaxed after re-derivation: the PlaygroundConfigSection skeleton gate (dropping isPending would paint 'No configuration needed' during every restore window and permanently skeleton config-less workflows) and the draft local-server-data reseed (impossible before the source body; compare-mode clones already seed at hydration-apply). gates.md carries the full ladder with KEEP/RELAXED verdicts per gate.
…te, row memo)
Profiler-driven (agent playground boot). Three render-hygiene fixes on the session
rail/tab surfaces:
- Latch-mount SessionRail on first maximize. It sits in a `size={0}` + `inert`
splitter panel until the chat is maximized, but was rendered unconditionally —
mounting the whole session list (~1000 renders, ~10% of boot React work) plus
fetching its lazy chunk, all into a zero-width panel. Now mounted on first open
and kept mounted after, so toggling doesn't remount or lose scroll.
- Hover-gate the rename/delete/close clusters. Each button drags a Tooltip +
Trigger + icon subtree; a full history of rows paid all of that on boot behind
`opacity-0` for pixels nobody sees (~288 Button renders). Mount on hover/focus
instead; keyboard access preserved via the focusable row + focus-leave guard.
- Memoize SessionRailRow/SessionTag with id-taking callbacks and hoisted static
icons, so a streamed token or liveness poll no longer re-renders every row
(rows went from ~2 re-renders/mount to 0).
HAR-driven (agent playground boot). - revalidateSessionRecordsAtom now invalidates with `cancelRefetch: false`, mirroring revalidateSessionMountsAtom. A turn finishing (incl. the SDK auto-resuming the restored last turn on reload) fired this while the first records fetch was still in flight; the default cancelled it and started an identical request. Removed the cancelled sessions/records (+ mounts) pair. - workflowDetailQueryAtomFamily and workflowArtifactScopedQueryAtomFamily issue the same POST /workflows/query (same body, different cache key). Cross-prime each other's key so whichever runs first satisfies the other, removing the duplicate by-id workflows/query on boot.
…m-disk) Phase 1 of persisting playground list queries. The revision BODY was already disk-served (immutablePersister), but a warm reload still blocked on the latest-revision round-trip (1.7-3.2s on a slow backend) to resolve the fallback selector, the "latest" tag, and the dirty/commit affordance. - Attach catalogPersister to workflowLatestRevisionQueryAtomFamily: a warm reload paints the last-known latest from disk, then fires exactly one background revalidate (restored entry is older than the 30s staleTime). Live commits stay correct — invalidateAgentCommittedRevisionCache prefix-invalidates this key on the data-committed-revision stream signal. - The two setQueryData prime paths (revisions-by-workflow queryFn, and the commit handler) bypass the persister, so they now also persistQueryByKey to IDB — otherwise a session that only ever primed the latest (dedicated query disabled) would leave nothing on disk to restore. Mirrors primeWorkflowRevisionDetailCache. Class-B semantics: bounded staleness only on cold reload — a revision committed on another device is briefly absent until the single revalidation lands.
Before persisting the workflow variant/revision LIST queries, establish what the selection auto-select logic will see when a list paints from disk. - persist.test.ts: a catalog-restored list query reports isPending:false (so isPending-gated consumers never flash on the background revalidate) but isFetched:true — EVEN for an empty restored list. - autoSelectLatestChild.restore.test.ts: feeds those restore-shaped states into the real resolveAutoSelectLatestChild. Non-empty restore selects the first child; an EMPTY restore returns "complete" with no selection (the isFetched===false wait-guard is bypassed by restore) instead of waiting for the revalidate that brings the real revision. Gate result: persisting the list queries is NOT a blind persister-add — the auto-select path must distinguish restored-but-revalidating from settled first.
The `agenta:persist:disable` localStorage flag silently no-oped every IndexedDB read/write while GC kept reporting entries, so a flag left set from earlier A/B debugging disabled the whole paint-from-disk layer with no visible signal — every reload went cold and the persistence work looked like it did nothing. Persistence was already default-on (the flag only had to be absent); removing the check makes it unconditional and self-heals any browser with a stale flag still in localStorage. Drops isPersistDisabled + the two getItem/setItem guards + its test. The debug flag (agenta:persist:debug) stays.
What was tackled (persistence layer, cold-load request reduction, boot render gates, session-UI render cost, request dedup, latest-revision persistence), what was deliberately left alone and why (Phase 2 list persistence, the sidebar Menu, deep config-panel work, org persistence, the billing 502, the serial boot gate chain), and the learnings worth not relearning. Explicitly scoped OUT: the drive/file/mount surfaces, which ship separately in PR #5400 that this branch is stacked on.
Make the agent playground config panel's loading skeletons match the rendered panel so nothing shifts when the schema resolves: - ConfigAccordionSection: collapse the extra-slot's vertical footprint (-my-2) so an add-button no longer inflates section headers — every section row (config + Triggers) is now a uniform height. - AgentConfigSkeleton: mirror the real header row (gap-2, 16px icon, min-h-44) so a bordered row totals 45px and the last row 44px. - Both loading gates (hydration + schema) render the skeleton inside the same px-4 inset as the real agent_config field wrapper. - Route modern `agent-template` fields to the always-expanded body branch; the wrapper check only matched the legacy `agent_config` name, so they fell through to the collapsible else branch with a wider inset. - Tighten the region-header-to-content gap to px-4 pt-1 pb-3 (was py-3) across config, Triggers, and Files, plus their skeletons.
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis change documents and implements app-boot optimizations across IndexedDB query persistence, prewarming, idle gating, warm-reload hydration, workflow cache priming, session restoration, render-stability improvements, and per-resource Fern client usage. ChangesBoot and persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant AuthProvider
participant BootGraph
participant QueryPersister
participant IndexedDB
participant Playground
Browser->>AuthProvider: initialize client boot
AuthProvider->>BootGraph: prewarmBootQueryGraph()
BootGraph->>QueryPersister: subscribe boot query atoms
QueryPersister->>IndexedDB: restore persisted query data
Browser->>Playground: preload playground module
Playground->>BootGraph: prewarmCurrentWorkflowQueries()
BootGraph->>QueryPersister: subscribe workflow detail and latest revision
QueryPersister-->>Playground: restored or refreshed workflow data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/packages/agenta-shared/tests/unit/persist.test.ts (1)
1-523: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun Prettier to fix formatting.
CI reports Prettier
--checkfailures in this file across two jobs.pnpm --filter `@agenta/shared` exec prettier --write tests/unit/persist.test.tsSource: Pipeline failures
web/oss/src/components/Playground/Components/Menus/SelectVariant/index.tsx (1)
405-436: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFallback order places
selectedRevisionData?.namebefore the newselectedVariantSlug, violating the name→slug guideline and making the slug fallback effectively unreachable.The label chain is
selectedVariantName ?? selectedRevisionData?.name ?? selectedVariantSlug ?? selectPlaceholder. As per coding guidelines, "Resolve variant labels from the variant'sname, thenslug, usingworkflow_variant_id; never label a variant from revision fields," and "Never userevision.nameas a display label... revisions contribute onlyversionandmessage." SinceselectedRevisionData(the revision entity) is truthy once the revision loads, it will almost always short-circuit beforeselectedVariantSlugis reached, so this diff's stated goal — a slug fallback "when the variants list isn't cached yet" — rarely fires in practice.🐛 Proposed fix
const variantName = selectedVariantName ?? - selectedRevisionData?.name ?? selectedVariantSlug ?? + selectedRevisionData?.name ?? selectPlaceholderSource: Coding guidelines
🧹 Nitpick comments (1)
web/oss/src/components/Playground/Components/PlaygroundVariantConfig/index.tsx (1)
101-120: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMissing equality comparator on the
workflowParamsselectAtom may defeat the narrowing goal.
workflowUri's selector returns a primitive, so defaultObject.iscomparison is fine. ButworkflowParams's selector returns an object (d?.data?.parameters) with no custom equality function. IfworkflowMolecule.selectors.datamints a fresh wrapper object on every query-phase flip during boot — as explicitly documented elsewhere in this same PR (PlaygroundConfigSection.tsx'sstableAtomcomment: "upstream selectors mint fresh objects per query flip during boot") — this atom will still emit a new reference on every recompute even whenparameterscontent is unchanged, re-rendering this component on every boot-time resolution exactly as the comment above (lines 99-100) says this refactor is meant to avoid.
TriggerManagementSection.tsx'suseAgentTriggersandPlaygroundConfigSection.tsx'sstableAtomboth add an explicit field-by-field/deep equality comparator toselectAtomfor this exact reason — worth mirroring here forworkflowParams.♻️ Proposed fix
const workflowParams = useAtomValue( useMemo( () => selectAtom( workflowMolecule.selectors.data(variantId), (d) => d?.data?.parameters as Record<string, unknown> | undefined, + (a, b) => a === b || JSON.stringify(a) === JSON.stringify(b), ), [variantId], ), )Can you confirm whether
workflowMolecule.selectors.datapreserves the.data.parametersobject reference across query-phase transitions, or mints a fresh object each time?
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 04c54a1f-cb74-46b4-bea7-3a0df723c0e8
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (107)
docs/design/app-boot-optimizations/gates.mddocs/design/app-boot-optimizations/plan.mddocs/design/data-optimizations/README.mddocs/design/playground-query-persistence/plan.mdweb/AGENTS.mdweb/oss/src/ThemeContextBridge.tsxweb/oss/src/components/AgentChatSlice/AgentChatPanel.tsxweb/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/oss/src/components/AgentChatSlice/assets/loadSession.tsweb/oss/src/components/AgentChatSlice/components/Inspector/Inspector.tsxweb/oss/src/components/AgentChatSlice/components/SessionRail.tsxweb/oss/src/components/AgentChatSlice/components/SessionTagBar.tsxweb/oss/src/components/DrillInView/OSSdrillInUIProvider.tsxweb/oss/src/components/EnhancedUIs/Button/index.tsxweb/oss/src/components/Evaluators/components/ConfigureEvaluator/EvaluatorPlaygroundHeader.tsxweb/oss/src/components/InfiniteVirtualTable/components/ColumnVisibilityHeader.tsxweb/oss/src/components/Layout/Layout.tsxweb/oss/src/components/Layout/ThemeContextProvider.tsxweb/oss/src/components/Playground/Components/AgentCatalogPrefetcher.tsxweb/oss/src/components/Playground/Components/MainLayout/index.tsxweb/oss/src/components/Playground/Components/Menus/SelectVariant/index.tsxweb/oss/src/components/Playground/Components/Modals/DeployVariantModal/assets/DeployVariantButton/index.tsxweb/oss/src/components/Playground/Components/PlaygroundVariantConfig/index.tsxweb/oss/src/components/PlaygroundRouter/index.tsxweb/oss/src/components/ProtectedRoute/ProtectedRoute.tsxweb/oss/src/components/SessionInspector/api.tsweb/oss/src/components/Sidebar/Sidebar.tsxweb/oss/src/components/Sidebar/dynamic/source.tsweb/oss/src/components/pages/_app/index.tsxweb/oss/src/hooks/useLLMProviderConfig.tsxweb/oss/src/hooks/useSession.tsweb/oss/src/lib/api/SWRConfig.tsxweb/oss/src/lib/helpers/auth/AuthProvider.tsxweb/oss/src/state/access/atoms.tsweb/oss/src/state/app/atoms/templates.tsweb/oss/src/state/appState/atoms.tsweb/oss/src/state/appState/index.tsweb/oss/src/state/appState/types.tsweb/oss/src/state/boot/prewarmBootQueryGraph.tsweb/oss/src/state/org/selectors/org.tsweb/oss/src/state/profile/selectors/user.tsweb/oss/src/state/project/selectors/project.tsweb/oss/src/state/url/auth.tsweb/oss/src/state/url/playground.tsweb/oss/src/state/url/test.tsweb/oss/src/state/workflow/prewarmCurrentWorkflow.tsweb/oss/src/state/workflow/selectors/workflow.tsweb/packages/agenta-annotation/src/state/controllers/annotationSessionController.tsweb/packages/agenta-entities/package.jsonweb/packages/agenta-entities/src/environment/state/store.tsweb/packages/agenta-entities/src/gatewayTool/hooks/useToolActionDetail.tsweb/packages/agenta-entities/src/gatewayTool/hooks/useToolCatalogActions.tsweb/packages/agenta-entities/src/gatewayTool/hooks/useToolCatalogCategories.tsweb/packages/agenta-entities/src/gatewayTool/hooks/useToolCatalogIntegrations.tsweb/packages/agenta-entities/src/gatewayTool/hooks/useToolConnectionsQuery.tsweb/packages/agenta-entities/src/gatewayTool/hooks/useToolIntegrationDetail.tsweb/packages/agenta-entities/src/gatewayTrigger/hooks/useTriggerCatalogEvents.tsweb/packages/agenta-entities/src/gatewayTrigger/hooks/useTriggerCatalogIntegrations.tsweb/packages/agenta-entities/src/gatewayTrigger/hooks/useTriggerEvent.tsweb/packages/agenta-entities/src/gatewayTrigger/hooks/useTriggerSchedules.tsweb/packages/agenta-entities/src/gatewayTrigger/hooks/useTriggerSubscriptions.tsweb/packages/agenta-entities/src/secret/state/atoms.tsweb/packages/agenta-entities/src/secret/state/persistence.tsweb/packages/agenta-entities/src/session/index.tsweb/packages/agenta-entities/src/session/state/records.tsweb/packages/agenta-entities/src/shared/openapi/serviceSchemaAtoms.tsweb/packages/agenta-entities/src/trace/state/store.tsweb/packages/agenta-entities/src/workflow/api/api.tsweb/packages/agenta-entities/src/workflow/index.tsweb/packages/agenta-entities/src/workflow/state/appUtils.tsweb/packages/agenta-entities/src/workflow/state/evaluatorTemplateAtoms.tsweb/packages/agenta-entities/src/workflow/state/index.tsweb/packages/agenta-entities/src/workflow/state/inspectMeta.tsweb/packages/agenta-entities/src/workflow/state/persistedCatalog.tsweb/packages/agenta-entities/src/workflow/state/persistedInspect.tsweb/packages/agenta-entities/src/workflow/state/store.tsweb/packages/agenta-entities/tests/unit/secret-persist-redaction.test.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AddTextLink.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentOperationsSections.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/TriggerManagementSection.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AgentConfigSkeleton.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsxweb/packages/agenta-entity-ui/src/DrillInView/components/MoleculeDrillInContext.tsxweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection.tsxweb/packages/agenta-entity-ui/tests/unit/autoSelectLatestChild.restore.test.tsweb/packages/agenta-playground-ui/src/components/WorkflowRevisionDrawer/MetadataSidebar.tsxweb/packages/agenta-sdk/src/resources.tsweb/packages/agenta-shared/package.jsonweb/packages/agenta-shared/src/api/persist/debug.tsweb/packages/agenta-shared/src/api/persist/gc.tsweb/packages/agenta-shared/src/api/persist/idbStorage.tsweb/packages/agenta-shared/src/api/persist/index.tsweb/packages/agenta-shared/src/api/persist/persisters.tsweb/packages/agenta-shared/src/api/persist/version.tsweb/packages/agenta-shared/src/state/idleReady.tsweb/packages/agenta-shared/src/state/index.tsweb/packages/agenta-shared/tests/unit/persist.test.tsweb/packages/agenta-shared/tests/unit/persistNullGuard.test.tsweb/packages/agenta-ui/src/Editor/Editor.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/components/ColumnVisibilityHeader.tsxweb/packages/agenta-ui/src/RichChatInput/RichChatInput.tsxweb/packages/agenta-ui/src/components/presentational/EnhancedButton.tsxweb/packages/agenta-ui/src/components/presentational/buttons/AddButton.tsxweb/packages/agenta-ui/src/components/presentational/section/ConfigAccordionSection.tsxweb/packages/eslint.config.mjsweb/pnpm-workspace.yaml
💤 Files with no reviewable changes (2)
- web/packages/agenta-entities/src/workflow/state/persistedCatalog.ts
- web/packages/agenta-entities/src/workflow/state/persistedInspect.ts
| @@ -1,4 +1,4 @@ | |||
| import {useRef, useState} from "react" | |||
| import {memo, useCallback, useRef, useState} from "react" | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mouse-leave unmounts a focused hover-action button, dropping keyboard focus.
Both files switched the rename/delete/close button cluster from always-mounted (CSS-hidden) to conditionally mounted via a single hot boolean fed by both mouse and focus events. The blur handler (onBlurRow/onBlurChip) correctly keeps hot true when focus moves into the cluster (checks relatedTarget containment), but the mouse-leave handler (onLeave) clears hot unconditionally. If a keyboard user tabs into the row/chip (mounting the cluster) and then the mouse cursor happens to leave the row's bounds while a button inside it still has focus, that button is unmounted mid-focus, dropping focus to <body> and disrupting keyboard navigation. This is a new regression from the conditional-mount change (previously the buttons stayed in the DOM, just hidden).
web/oss/src/components/AgentChatSlice/components/SessionRail.tsx#L63-68: inonLeave, don't clearhotif the row currently contains focus (e.g. checkdocument.activeElementcontainment).web/oss/src/components/AgentChatSlice/components/SessionRail.tsx#L111-116: no change needed here beyond wiring the updatedonLeave; the mouseleave binding stays as-is.web/oss/src/components/AgentChatSlice/components/SessionTagBar.tsx#L122-126: apply the same focus-containment guard toonLeaveas inSessionRail.tsx.web/oss/src/components/AgentChatSlice/components/SessionTagBar.tsx#L200-205: no change needed here beyond wiring the updatedonLeave; the mouseleave binding stays as-is.
🛠️ Proposed fix (apply the same pattern in both files)
- const onLeave = useCallback(() => setHot(false), [])
+ const onLeave = useCallback((e: React.MouseEvent<HTMLDivElement>) => {
+ // Don't drop the cluster if it (or a descendant) currently holds keyboard focus.
+ if (!e.currentTarget.contains(document.activeElement)) setHot(false)
+ }, [])
Comment on lines
+63
to
+68
| const onEnter = useCallback(() => setHot(true), []) | ||
| const onLeave = useCallback(() => setHot(false), []) | ||
| const onBlurRow = useCallback((e: React.FocusEvent<HTMLDivElement>) => { | ||
| // Keep the cluster while focus moves INTO it (row → rename button). | ||
| if (!e.currentTarget.contains(e.relatedTarget as Node | null)) setHot(false) | ||
| }, []) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mouse-leave can unmount a focused rename/delete button.
See consolidated comment (anchored here) for the shared root cause with SessionTagBar.tsx.
Also applies to: 111-116
Comment on lines
+122
to
+126
| const onEnter = useCallback(() => setHot(true), []) | ||
| const onLeave = useCallback(() => setHot(false), []) | ||
| const onBlurChip = useCallback((e: React.FocusEvent<HTMLDivElement>) => { | ||
| if (!e.currentTarget.contains(e.relatedTarget as Node | null)) setHot(false) | ||
| }, []) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mouse-leave can unmount a focused rename/close button.
See consolidated comment (anchored on SessionRail.tsx) for the shared root cause and fix.
Also applies to: 200-205
Comment on lines
+1
to
+4
| import {catalogPersister} from "@agenta/shared/api/persist" | ||
| import {logAtom} from "@agenta/shared/state" | ||
| import type {User} from "@agenta/shared/types" | ||
| import type {QueryKey} from "@tanstack/react-query" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n 'clearPersistedQueryCache' web/oss/src web/packages/agenta-shared/src -g '*.ts' -g '*.tsx'Repository: Agenta-AI/agenta
Length of output: 586
🏁 Script executed:
sed -n '1,220p' web/oss/src/state/profile/selectors/user.tsRepository: Agenta-AI/agenta
Length of output: 2273
🏁 Script executed:
sed -n '1,180p' web/oss/src/hooks/useSession.tsRepository: Agenta-AI/agenta
Length of output: 3946
🏁 Script executed:
rg -n 'profileQueryAtom|profile' web/oss/src web/packages/agenta-shared/src -g '*.ts' -g '*.tsx'Repository: Agenta-AI/agenta
Length of output: 10598
🏁 Script executed:
sed -n '1,200p' web/oss/src/state/profile/index.tsRepository: Agenta-AI/agenta
Length of output: 834
🏁 Script executed:
sed -n '1,200p' web/packages/agenta-shared/src/api/persist/idbStorage.tsRepository: Agenta-AI/agenta
Length of output: 3065
🏁 Script executed:
rg -n 'sessionExistsAtom|doesSessionExist|signOut\(|clearPersistedQueryCache\(|resetProfileData\(' web/oss/src -g '*.ts' -g '*.tsx'Repository: Agenta-AI/agenta
Length of output: 8019
🏁 Script executed:
sed -n '280,340p' web/oss/src/state/url/auth.tsRepository: Agenta-AI/agenta
Length of output: 3097
🏁 Script executed:
sed -n '150,220p' web/oss/src/lib/api/assets/fetchClient.tsRepository: Agenta-AI/agenta
Length of output: 2134
🏁 Script executed:
sed -n '180,280p' web/oss/src/lib/api/assets/axiosConfig.tsRepository: Agenta-AI/agenta
Length of output: 3620
🏁 Script executed:
sed -n '50,100p' web/oss/src/pages/workspaces/accept.tsxRepository: Agenta-AI/agenta
Length of output: 2443
🏁 Script executed:
sed -n '460,520p' web/oss/src/pages/auth/[[...path]].tsxRepository: Agenta-AI/agenta
Length of output: 4032
Clear persisted profile data on every sign-out path
profileQueryAtom is persisted under the bare ["profile"] key, but only useSession.logout() clears the IndexedDB cache. The other forced signOut() paths can leave stale profile PII on disk, so a later login may hydrate the wrong user before refetching.
Comment on lines
+161
to
172
| let previousWorkflowContext: CurrentWorkflowContext | null = null | ||
|
|
||
| // Reuse the previous object when all fields are unchanged, so query-phase identity | ||
| // churn in the underlying detail query doesn't re-render subscribers. | ||
| export const currentWorkflowContextAtom = atom<CurrentWorkflowContext>((get) => { | ||
| const next = computeCurrentWorkflowContext(get) | ||
| if (previousWorkflowContext && workflowContextEquals(previousWorkflowContext, next)) { | ||
| return previousWorkflowContext | ||
| } | ||
| previousWorkflowContext = next | ||
| return next | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Module-level mutable previousWorkflowContext breaks atom/store isolation.
The reuse-previous-value optimization is implemented with a let previousWorkflowContext declared at module scope, shared across every Jotai store instance that imports this module (multiple createStore() instances, parallel test runs, or any future multi-store usage). This can leak a stale/foreign context between stores/tests instead of being scoped per-store.
The same "keep previous reference when equal" pattern is already implemented safely elsewhere in this PR via selectAtom(base, v => v, equals) (see PlaygroundConfigSection.tsx's stableAtom helper), which keeps the memoization inside the atom graph rather than in module-level mutable state.
♻️ Proposed fix using selectAtom
-let previousWorkflowContext: CurrentWorkflowContext | null = null
-
-// Reuse the previous object when all fields are unchanged, so query-phase identity
-// churn in the underlying detail query doesn't re-render subscribers.
-export const currentWorkflowContextAtom = atom<CurrentWorkflowContext>((get) => {
- const next = computeCurrentWorkflowContext(get)
- if (previousWorkflowContext && workflowContextEquals(previousWorkflowContext, next)) {
- return previousWorkflowContext
- }
- previousWorkflowContext = next
- return next
-})
+// Reuse the previous object when all fields are unchanged, so query-phase identity
+// churn in the underlying detail query doesn't re-render subscribers. selectAtom keeps
+// this memoization store-scoped instead of module-scoped.
+export const currentWorkflowContextAtom = selectAtom(
+ atom(computeCurrentWorkflowContext),
+ (v) => v,
+ workflowContextEquals,
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let previousWorkflowContext: CurrentWorkflowContext | null = null | |
| // Reuse the previous object when all fields are unchanged, so query-phase identity | |
| // churn in the underlying detail query doesn't re-render subscribers. | |
| export const currentWorkflowContextAtom = atom<CurrentWorkflowContext>((get) => { | |
| const next = computeCurrentWorkflowContext(get) | |
| if (previousWorkflowContext && workflowContextEquals(previousWorkflowContext, next)) { | |
| return previousWorkflowContext | |
| } | |
| previousWorkflowContext = next | |
| return next | |
| }) | |
| // Reuse the previous object when all fields are unchanged, so query-phase identity | |
| // churn in the underlying detail query doesn't re-render subscribers. selectAtom keeps | |
| // this memoization store-scoped instead of module-scoped. | |
| export const currentWorkflowContextAtom = selectAtom( | |
| atom(computeCurrentWorkflowContext), | |
| (v) => v, | |
| workflowContextEquals, | |
| ) |
Comment on lines
+47
to
+58
| const named = next as NamedSecretRow | ||
| if (named.content !== undefined) { | ||
| if (typeof named.content === "string") { | ||
| named.content = named.content ? VAULT_PERSIST_REDACTED : "" | ||
| } else { | ||
| // Keep keys (env-var-style names, comparable to routinely cached config); redact values. | ||
| named.content = Object.fromEntries( | ||
| Object.keys(named.content).map((key) => [key, VAULT_PERSIST_REDACTED]), | ||
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent TypeError when named.content is null.
If named.content is null, typeof null evaluates to "object". This causes Object.keys(null) to throw a TypeError, which will synchronously crash the persister's serialization hook.
🛡️ Proposed fix to explicitly exclude null
const named = next as NamedSecretRow
- if (named.content !== undefined) {
+ if (named.content !== undefined && named.content !== null) {
if (typeof named.content === "string") {
named.content = named.content ? VAULT_PERSIST_REDACTED : ""
} else {
// Keep keys (env-var-style names, comparable to routinely cached config); redact values.
named.content = Object.fromEntries(
Object.keys(named.content).map((key) => [key, VAULT_PERSIST_REDACTED]),
)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const named = next as NamedSecretRow | |
| if (named.content !== undefined) { | |
| if (typeof named.content === "string") { | |
| named.content = named.content ? VAULT_PERSIST_REDACTED : "" | |
| } else { | |
| // Keep keys (env-var-style names, comparable to routinely cached config); redact values. | |
| named.content = Object.fromEntries( | |
| Object.keys(named.content).map((key) => [key, VAULT_PERSIST_REDACTED]), | |
| ) | |
| } | |
| } | |
| const named = next as NamedSecretRow | |
| if (named.content !== undefined && named.content !== null) { | |
| if (typeof named.content === "string") { | |
| named.content = named.content ? VAULT_PERSIST_REDACTED : "" | |
| } else { | |
| // Keep keys (env-var-style names, comparable to routinely cached config); redact values. | |
| named.content = Object.fromEntries( | |
| Object.keys(named.content).map((key) => [key, VAULT_PERSIST_REDACTED]), | |
| ) | |
| } | |
| } |
Comment on lines
+63
to
+74
| export const redactPersistedVaultQuery = (persisted: PersistedQuery): PersistedQuery => { | ||
| const data = persisted.state.data | ||
| if (!Array.isArray(data)) return persisted | ||
|
|
||
| return { | ||
| ...persisted, | ||
| state: { | ||
| ...persisted.state, | ||
| data: (data as LlmProvider[]).map(redactVaultSecretRow), | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Ensure fail-safe redaction for unexpected payload shapes.
If vaultSecretsPersister is mistakenly applied to a query returning a single object, or if the API contract changes in the future (e.g., returning { secrets: [...] }), the !Array.isArray(data) check evaluates to false and silently returns the payload unredacted. By the fail-safe principle, unexpected shapes should be dropped from persistence to prevent inadvertently leaking plaintext secrets to IndexedDB.
🔒️ Proposed fix to enforce fail-safe redaction
export const redactPersistedVaultQuery = (persisted: PersistedQuery): PersistedQuery => {
const data = persisted.state.data
- if (!Array.isArray(data)) return persisted
+ if (!data) return persisted
+ if (!Array.isArray(data)) {
+ // Fail-safe: drop payloads that do not match the expected array shape
+ // to prevent accidental plaintext secret leakage.
+ return {
+ ...persisted,
+ state: {
+ ...persisted.state,
+ data: undefined,
+ },
+ }
+ }
return {
...persisted,
state: {
...persisted.state,
data: (data as LlmProvider[]).map(redactVaultSecretRow),
},
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const redactPersistedVaultQuery = (persisted: PersistedQuery): PersistedQuery => { | |
| const data = persisted.state.data | |
| if (!Array.isArray(data)) return persisted | |
| return { | |
| ...persisted, | |
| state: { | |
| ...persisted.state, | |
| data: (data as LlmProvider[]).map(redactVaultSecretRow), | |
| }, | |
| } | |
| } | |
| export const redactPersistedVaultQuery = (persisted: PersistedQuery): PersistedQuery => { | |
| const data = persisted.state.data | |
| if (!data) return persisted | |
| if (!Array.isArray(data)) { | |
| // Fail-safe: drop payloads that do not match the expected array shape | |
| // to prevent accidental plaintext secret leakage. | |
| return { | |
| ...persisted, | |
| state: { | |
| ...persisted.state, | |
| data: undefined, | |
| }, | |
| } | |
| } | |
| return { | |
| ...persisted, | |
| state: { | |
| ...persisted.state, | |
| data: (data as LlmProvider[]).map(redactVaultSecretRow), | |
| }, | |
| } | |
| } |
ashrafchowdury
approved these changes
Jul 21, 2026
Contributor
|
I have QAed it and looks good to me |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Reloading the app (the agent playground especially) was slow, and reloading it a second time was no faster than the first. Every reload refetched the same data and blocked first paint on the round-trip. This was most acute on the local EE dev backend, where requests take several seconds each (the numbers below come from there). Prod is much faster, so this is not about a slow prod backend. Even on a fast backend, though, first paint still waits on the round-trip, and that wait is what paint-from-disk removes.
This branch adds a per-query IndexedDB persistence layer so a warm reload paints from disk instead of blocking on the network. That layer shipped with a debug kill-switch flag (
localStorage["agenta:persist:disable"]) for A/B comparison. During development the flag got left enabled in the browser, which silently no-oped every disk read and write, so across many profiling runs the persistence looked like it did nothing and every warm reload behaved like a cold one. The failure was invisible because the disk reads and writes returned early before logging, while the garbage-collector kept reporting entries on disk. Removing the switch (and pinning@tanstack/query-coreto a single version so the persister and the app share one query client) made the layer actually run.On top of that, the boot path did more React work than it needed to (whole subtrees rendered into hidden or zero-width containers, list rows re-rendered on every streamed token) and fired redundant or self-cancelling network requests.
What this does
Makes warm reloads paint from disk, and cuts the render and request cost of both cold and warm boots. The work groups into five areas.
1. Persistence actually runs now (the unlock).
Removed the
agenta:persist:disablekill switch entirely so persistence is unconditional, and pinned@tanstack/query-coreto a single version so the persister and the app share one query client. With those fixed, the per-query IndexedDB layer this branch adds does its job: profile, projects, workflow detail/artifact, revision bodies, schemas, harness catalogs, session records, and vault secrets all restore from disk on reload, then revalidate in the background.2. Latest-revision paints from disk (new).
The revision body was already disk-served, but a warm reload still blocked on the latest-revision round-trip to resolve the version selector, the "latest" tag, and the dirty/commit affordance. That query is now persisted too (with the two
setQueryDataprime paths mirrored to disk, since those bypass the persister). Result, on a warm reload:Before:
POST /workflows/revisions/query(windowing limit 1) blocked first paint for 1.7 to 5 seconds on the local dev backend.After: that request is gone from the paint path. The selector paints from disk; a single low-priority revalidate runs behind it. On prod the blocking wait is smaller, but paint-from-disk removes it either way.
3. Session-UI render cost.
Three fixes on the session rail and tab bar, all profiler-confirmed:
SessionRaillived in asize={0}+inertsplitter panel until the chat is maximized, but was rendered unconditionally. It mounted the whole session list (about 1000 renders, 10 to 15 percent of all boot React work) plus its lazy chunk into a zero-width panel. It now latch-mounts on first open.opacity-0on every row, about 984 renders for buttons nobody sees. They now mount on hover/focus, with keyboard access preserved.SessionRailRow,SessionTag) are memoized with stable id-taking callbacks, so a streamed token no longer re-renders every row (they went from about 2 re-renders per mount to 0).4. Request dedup and cancellation.
revalidateSessionRecordsAtominvalidated with the defaultcancelRefetch: true, which killed an in-flight records fetch and started an identical one on every warm boot (the SDK auto-resumes the restored last turn, which fires the revalidation). It now passescancelRefetch: false, matching the mounts revalidation. Separately, two workflow atoms issued the samePOST /workflows/queryunder different cache keys; they now cross-prime each other so only one request fires.5. Boot render-gate hygiene.
Warming the serialized boot chunks, latching the protected-route gate, prewarming the boot atom graph, un-gating org detail from
/profile, config-accordion render hygiene, and naming anonymousforwardRefs. Detail lives in the design docs underdocs/design/app-boot-optimizations/anddocs/design/playground-query-persistence/.A full write-up of what was tackled, what was deliberately left alone, and the learnings is in
docs/design/data-optimizations/README.md.Scope
This branch does not touch the drive, file, or mount surfaces. That work shipped separately in #5400, which merged into main; this branch is rebased onto main and contains only the data and render work.
Tests / notes
[persist] HITdebug logs confirm every listed query restores from disk on reload, and the warm-reload HAR shows the blocking latest-revision request gone. The session-UI render wins are confirmed across five profiler captures.isFetched-under-restore behavior of the selection auto-select path.isFetched: trueeven when empty, which the auto-select logic isn't ready for, and the lists are not on the critical path once latest-revision is persisted. Documented as a follow-up.billing/subscriptionreturns 502 in local EE dev because billing is flagged on without Stripe configured. The frontend handles it (no retry on 5xx, deferred to idle, degrades to free); it is not a production issue.What to QA
workflows/revisions/querybefore first paint).