refactor(app-shell): one isAiStudioEnabled() accessor for features.aiStudio, replacing two inline spellings - #5645
Merged
Conversation
… two inline spellings `features.marketplace` had a documented accessor carrying the fail-open doctrine; `features.aiStudio` was read inline at two call sites in two different spellings — `ChatDock` un-chained, `HomePage` optional-chained — so neither reader could cite the doctrine and the un-chained one is a TypeError, not a fail-open, against a snapshot whose `features` is absent. That is the shape PR #5575 measured crashing 29 tests. - `isAiStudioEnabled()` sibling of `isMarketplaceEnabled()`, same docblock treatment. - Both call sites moved onto it; no inline `features.aiStudio` read remains. - The four Home suites' module mocks taught the new export (an explicit factory replaces the whole module, so an unlisted export is `undefined` at the call site). - New coverage: the accessor's fail-open doctrine, and the dock's default body under the partial snapshot the un-chained read could not survive. Part of #5577
…all the snapshot
Two corrections found by ablating the probe rather than trusting it:
- `vi.mock('../runtime-config')` resolved to `src/layout/runtime-config`, which
does not exist. Vitest no-ops an unresolvable factory mock instead of erroring,
so the partial snapshot was never installed and the file was a phantom: it
passed identically against the un-chained pre-fix read. Corrected to
`'../../runtime-config'` — measured red on that read afterwards.
- The two crash-closure cases stay green for ANY optional-chained inline read, so
they pin the crash and say nothing about where the doctrine lives. Added a case
that pins the accessor call itself, scoped to exactly that and measured to fail
on a chained inline re-spelling.
Part of #5577
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sales
marked this pull request as ready for review
August 21, 2026 23:50
This was referenced Aug 21, 2026
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
1 participant
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.
Fixes #5577
features.marketplacehas a documented accessor whose docblock is where the fail-opendoctrine is written down.
features.aiStudiohad none — it was read inline at two callsites, in two different spellings, and neither reader could cite the doctrine.
packages/app-shell/src/layout/ChatDock.tsx:263—getRuntimeConfig().features.aiStudio !== false(un-chained)packages/app-shell/src/console/home/HomePage.tsx:401—getRuntimeConfig().features?.aiStudio !== false(chained)Both verified on
origin/main@f1c27f037before any edit; both line numbers as filed.What changed
isAiStudioEnabled()inruntime-config.ts, directly afterisMarketplaceEnabled(),carrying the same docblock treatment: what the flag means, the "never infer this from
the shape of a failure" warning, and the fail-open paragraph.
features.aiStudioread remains at any callsite (
grepforgetRuntimeConfig().featuresin either file returns 0).the whole module, so an export it does not list is
undefinedat the call site.The measurement the card left open: can
ChatDockactually receive a partial snapshot?No — not on any path that exists today. This PR is drift-prevention, not a live bug
fix. Stated plainly because the answer changes what this PR is, and it was measured
rather than assumed in either direction.
Production: structurally impossible. The accessor and both call sites read the
module's own singleton. Every writer of it constructs
featuresas an object — theinitial
{ ...defaults },applyUpdate's{ ...current.features, ...(patch.features ?? {}) },and
resetRuntimeConfigForTesting()— and the module exports no setter, so no hostcan install a partial snapshot through its API. Driven rather than read off the source:
a new case boots
initRuntimeConfig()against a server body carrying nofeatureskeyat all and asserts the snapshot still has one. There is also no bundler alias pointing
runtime-configanywhere else (checked across everyvite.config.*/vitest.config.*).Test harness: no suite reaches it.
ChatDockConversation— the only holder of thatread — is module-private and mounts only when
ChatDockPanel/ChatDockMobileSheetget no
childrenoverride. Every construction in the repo was enumerated:StudioAiCopilotpasseschildrenat both call sites; all 20 renders inChatDock.test.tsxpass
children(its own header says so);ConsoleLayoutis the single consumer that doesnot — and it gates the dock behind
useAiSurfaceEnabled()anddock.expanded. Of thesix suites in the repo that stand
runtime-configin with a factory, none mountsConsoleLayout; of the suites that mountConsoleLayout, none mocksruntime-config.But the crash shape is real, and it is now measured rather than inferred. Reverting
this call site to the pre-fix un-chained read, with everything else on this branch intact:
So the asymmetry the card describes was one house-idiom mock away from the failure that
crashed 29 tests across four suites in PR #5575 before it was corrected there. This PR
closes it at the source, by leaving no inline read to get wrong.
Published-surface reachability (clause ②)
isAiStudioEnabled()is NOT reachable from@object-ui/app-shell'sexportsmap —which is the opposite of what "a sibling next to an existing published accessor" suggests,
so it is worth stating explicitly rather than assuming.
The
exportsmap has exactly one code entry,"."→./dist/index.js.src/index.tsre-exports twelve symbols from
./runtime-config.js— andisMarketplaceEnabledis notamong them. The new accessor follows its sibling's posture exactly and is likewise not
added there, so the package's published surface is unchanged. Adding it would have been an
unrequested widening of the published API with no consumer pulling on it.
Measured, not just read:
dist/index.jsanddist/index.d.tsare byte-identical(same sha256) before and after — see the table below.
Changeset
.changeset/ai-studio-accessor-5577.md,patch(nevermajor— fixed group). Theauthority's own verdict line:
Both dist legs measured at the real
dist/path,tsconfig.tsbuildinfocleared betweenbuilds (this package is
composite: trueand keeps it atpackages/app-shell/tsconfig.tsbuildinfo,i.e. outside
dist/, so wipingdist/alone would not have forced a re-emit).Compared by sha256, not byte count:
dist/pathruntime-config.js702c3da611469d2c7d421f86372a3feeruntime-config.d.tsb63345d08faa95647b91fb750487d782layout/ChatDock.js32be92e2c26a14900b06cac3b2f83b07console/home/HomePage.js221950d741935d82ca74007cbdaeab5aindex.js56216a9f70bdce5f56216a9f70bdce5findex.d.ts15cceec868260eee15cceec868260eeeBoth legs moved at
runtime-config, and the entry point did not — the new export existsin the built artifacts but not on the published entry surface.
Tests
All runs from the repo root (package-cwd
vitestis refused, objectui#3378), ate2d01613a, the final commit.Superset, derived not sampled.
packages/app-shellhas 488 test files and its fullsuite exceeds the container's foreground cap. The behavioural surface here is exactly two
expressions, so the suites that can change verdict are those that mount
HomePage's gateor
ChatDock's default body. The run covers every Home-surface suite, every suite thatmounts
ConsoleLayoutor importsChatDock, both remainingruntime-configstand-ins(
PreviewBadge,provisionEnvironment— included to show they are unaffected, notassumed to be), the marketplace suites that build the same
featuresfixture, thesurfaceAgentconsumer of the flag, and theapps/console+i18n+permissionssuites that route through Home. 44 files, 384 tests, all passing.
pnpm --filter '@object-ui/app-shell' type-check— clean, 0 errors, after building thedependency closure first (
--filter '@object-ui/app-shell^...' build, 29 projects). Thescript name is echoed in the log (
> @object-ui/app-shell@17.6.0 type-check), so this isnot a zero-match
--filterreporting green having run nothing. Note this package'stype-checkistsc --noEmit && tsc -p tsconfig.test.json, so the new test file istype-checked too.
Lint.
eslint .overpackages/app-shell— 0 errors across the 920 fileseslint selected from its own config (count read from
--format json, not estimated).All 9 changed files appear in that output with
errorCount: 0. The repo-wideturbo run lintover the other 46 projects is CI's run: no file outsidepackages/app-shellchanged (the changeset gate independently reports "9 source file(s) of 1 released
package(s)"), and this config is
tseslint.configs.recommendedwith noproject/projectService— linting is per-file and not type-aware, so nothing in this diff can movean untouched file's verdict.
Gates, exit codes captured before any pipe:
check-changeset-presence0,check-changeset-no-major0,check-control-bytes0,check-package-self-import0,check-phantom-dependencies0,check-i18n-call-site-keys0,check-node-esm-load --specifiers-only0.The two known-broken gauges reproduce their documented failures and name zero files from
this diff:
check-eager-closure-budgetexits 2 ("No eager-closure report atapps/console/dist/eager-closure.json… This is a broken gauge, not a passing budget"),check-doc-snippet-typesexits 1 (unbuiltpackages/cli,plugin-markdown,plugin-timeline). Noted, not touched.Reverse verification — five legs, each scoped to one claim and each measured
Every mutation asserted its anchor's pristine count first, then confirmed on disk that the
injected text was present and the removed text absent, since
str.replacereturnshappily on zero hits. Restores ran from a
trap … EXIT INT TERM, so a foreground-capSIGTERM mid-mutation could not leave a mutated tree for a later measurement. No
dist/preflight was owed: the root
vitest.config.mtsaliases workspace packages tosrc/, andevery file under mutation is imported by relative path within
packages/app-shell/src, sono built artifact is in the loop for these runs.
ChatDock→ pre-fix un-chained inline readTypeError: … (reading 'aiStudio')ChatDock→ chained inline read!== false→=== true!== false→=== trueruntime-config.test.tsLeg B is why the probe has three cases and not two. The two crash-closure cases stay green
for any optional-chained inline read, so on their own they pin the crash and say nothing
about where the doctrine lives — which is the actual subject of this card. The third case
pins the accessor call itself and fails on a chained re-spelling; leg B is the measurement
that it can.
Leg D is a negative result worth recording rather than hiding. Mutating the accessor's
own
!== falseto=== trueleaves all 16 cases green, so no test here distinguishesthem. That is not a gap in the tests — it is a property of the module:
initRuntimeConfigalready normalises
aiStudioto a real boolean on every write (body.features.aiStudio !== false),and every other writer seeds it from
defaults, socurrent.features.aiStudiois neverundefinedand the two spellings cannot disagree through the module's public API. Leg Emutates that normalisation instead and goes red, which is what confirms the unit tests can
fail at all. The accessor keeps
!== falsefor parity with its sibling and because itbecomes load-bearing the moment either the parser or the snapshot's provenance changes —
but this PR does not claim a test proves it today.
A correction found by ablating rather than trusting
The first version of the new probe wrote
vi.mock('../runtime-config', …). Fromsrc/layout/__tests__/that resolves tosrc/layout/runtime-config, which does not exist,and vitest no-ops an unresolvable factory mock instead of erroring — so the partial
snapshot was never installed and the file passed identically against the un-chained pre-fix
read. Leg A caught it: a probe that cannot fail had been sitting green. Corrected to
'../../runtime-config'and re-measured; the leg-A row above is from the corrected probe.Generated by Claude Code