Skip to content

refactor(desktop): enforce a metadata-free renderer startup boundary - #2176

Merged
Astro-Han merged 2 commits into
apache:mainfrom
Colafornia:perf/model-metadata-firstscreen
Aug 6, 2026
Merged

refactor(desktop): enforce a metadata-free renderer startup boundary#2176
Astro-Han merged 2 commits into
apache:mainfrom
Colafornia:perf/model-metadata-firstscreen

Conversation

@Colafornia

@Colafornia Colafornia commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep full model metadata behind the main-process and lazy Settings boundaries
  • reuse onboarding:getSnapshot to project only first-screen model choices, thinking levels, provider labels, and session send outcomes
  • remove provider registry, model catalog, model thinking, and generated metadata from the renderer startup graph without adding another IPC channel
  • align the session health notice with the main-process send projection and self-heal legacy connection locks at the storage summary boundary

Closes #2063
Relates to #2084

 sequenceDiagram
      participant R as Renderer Main Thread
      participant IPC as IPC Bridge
      participant M as Main Process
      participant MM as model-metadata 520KB

      Note over R,MM: BEFORE
      R->>MM: parse + eval 520 KB (5 import paths)
      Note right of R: BLOCKED — not needed for first screen
      R->>R: build choices / thinking / menu / display / hero
      R->>IPC: hasSecret per connection
      IPC-->>R: secret presence
      R->>R: re-derive projection
      Note over R: first screen ready

      Note over R,MM: AFTER
      M->>MM: parse 520 KB (already loaded)
      M->>M: buildChatModelChoices + projectSessionSendOutcome
      M->>IPC: onboarding:getSnapshot (existing channel)
      IPC-->>R: chatModelChoices + sessionSendOutcomes
      R->>R: render first screen (snapshot + local + constant)
      Note over R: no metadata on main thread
      Note over M: SettingsModal lazy-loads on user click
Loading

Measured result

Metric Baseline Optimized Change
Static startup JS 1,951,976 B 1,339,905 B -31.4%
Startup metadata-name matches 266 0 -100%
Fresh-V8 startup-module read + parse median 25.35 ms 18.59 ms -26.7%
Cold-start median to mounted AppFrame 1,045.6 ms 1,049.0 ms +0.3%
Chromium ScriptDuration median 433.0 ms 432.5 ms -0.1%

The artifact and isolated parse cost improved materially. End-to-end cold start and Chromium script duration were unchanged within run-to-run noise, so this PR does not claim a user-visible wall-clock startup improvement.

Architecture boundary

The main process remains the metadata authority. The renderer startup path consumes a lightweight projection; full catalog data remains available only to main-process code and lazy-loaded Settings paths. Explicit core subpaths make that boundary independent of barrel reachability, with sideEffects: false and tree-shaking as a second defense.

FIRST_RUN_PROVIDER_TYPES is intentionally a metadata-free product constant. A contract test keeps it aligned with the first four recommended providers at test time rather than reintroducing a runtime registry dependency.

Review follow-up

  • enforce the real Vite startup chunk closure and metadata markers in a build-backed contract test
  • pin the complete renderer snapshot projection and every physical session outcome
  • keep first-run providers aligned with the recommended provider order
  • lock sessions in the message append transaction and migrate legacy unlocked user sessions once in schema v22; read paths remain pure
  • document conservative credential-read failures and event-triggered snapshot timing; remove the dead connection revision
  • preserve the existing Codex empty-inventory fallback, which remains covered by its legacy compatibility test

Verification

  • npm --workspace @maka/desktop run typecheck — passed
  • npm --workspace @maka/desktop test — 1,751 passed
  • npm --workspace @maka/storage test — passed
  • targeted UI contract tests — 12 passed
  • npm --workspace @maka/desktop run build:renderer — passed, including third-party notice verification
  • git diff --check — passed

@Colafornia
Colafornia marked this pull request as draft August 4, 2026 15:57
@Colafornia
Colafornia force-pushed the perf/model-metadata-firstscreen branch 3 times, most recently from d197de3 to 2fcb7e0 Compare August 4, 2026 17:00
@Colafornia
Colafornia marked this pull request as ready for review August 4, 2026 17:08
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for this — I verified the headline claims by rebuilding both trees: the startup JS drops ~32% (1,974KB → 1,339KB on my fresh build), the metadata-name markers go from 266 matches to 0 across all 27 startup chunks, EmptyState shrinks 812KB → 135KB, and the boundary is real (all five metadata import paths are cut; what remains sits behind the lazy SettingsModal). The moved computations check out line-by-line against the pre-PR versions — same filters, ordering, labels, and OAuth redaction — and the honest framing ("no user-visible wall-clock improvement claimed") is appreciated. Three things to handle before merge, none questioning the design:

Merge blocker — the branch conflicts with main on general-settings-page.tsx (main's #2216 reformatted it). Resolution is mechanical (re-apply the buildChatModelChoices swap on main's version), just needs a rebase.

P2 — the claimed contract test for the startup boundary doesn't exist. The PR body says "A contract test keeps it aligned with the first four recommended providers at test time", but provider-firstscreen-contract.test.ts only asserts the providerDisplay fallback for an unknown type — it doesn't scan the startup graph or the built chunks. Your own docs/model-metadata-firstscreen-optimization.md lists "startup chunks contain zero metadata markers" as an acceptance criterion, and nothing enforces it: a later refactor re-importing the metadata into any startup-reachable module ships green, silently returning the 520KB to the first screen — the exact regression this PR exists to prevent. The repo already has the pattern (dependency-boundary.test.ts import-closure assertions, or a chunk-marker grep in CI) — worth adding, since the perf claim is otherwise unverifiable.

P2 — the snapshot payload has no runtime contract pin. The renderer consumes chatModelChoices (8 fields) and sessionSendOutcomes[sessionId] with only two spot checks in onboarding-service.test.ts; thinkingLevels dropping off the wire, outcomes keyed by the wrong id, or a session missing from the map all pass CI while the UI silently loses thinking chips or the health notice. A deepEqual of one real snapshot's full shape plus a "every session has an outcome" assertion would pin it.

P3 (optional): the FIRST_RUN_PROVIDER_TYPES constant duplicates RECOMMENDED_PROVIDER_TYPES.slice(0,4) with no correspondence test (same "claimed contract" gap); the list() self-heal is a write in a read path whose cost scales with unhealed sessions (assistant-only previews never heal and pay a full message read forever); the health notice now lags one snapshot pull (deliberate, worth a comment); the codex zero-entry retry and the hasSecret error semantics are deliberate behavior changes worth a sentence each in the body; connectionsRevision is now dead state with a stale comment.

The design and the refactor itself are sound — happy to approve once rebased and the boundary test lands (or is explicitly deferred).

@Colafornia
Colafornia force-pushed the perf/model-metadata-firstscreen branch 2 times, most recently from cd30b8c to 59be1c6 Compare August 6, 2026 02:48
Keep full model metadata behind the main-process and lazy-settings boundaries, and project only first-screen data through onboarding snapshots.

Refs apache#2063

Relates to apache#2084
Lock sessions when user messages are appended and migrate legacy unlocked sessions once, keeping read paths pure.
@Colafornia
Colafornia force-pushed the perf/model-metadata-firstscreen branch from 59be1c6 to 30c68c9 Compare August 6, 2026 06:18
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for the follow-up — both P2s are closed with verified enforcement, and I confirmed by mutation rather than inference:

  • Boundary contract test: the new provider-firstscreen-contract.test.ts runs a real Vite build and BFS-walks the startup chunk graph — I injected import { lookupModelMetadata } with a real call into a startup-reachable module and the test fails, listing the exact modules (model-metadata.generated.js, model-metadata.js). A static re-import can no longer ship green; the FIRST_RUN_PROVIDER_TYPES correspondence is pinned too (order, not just membership).
  • Snapshot contract: the onboarding test deep-equals the complete chatModelChoices[0] (all 8 fields incl. providerLabel, connectionName, isDefault, thinkingLevels) and both sessions' sessionSendOutcomes, plus the Object.keys(sessionSendOutcomes) ≡ session ids invariant.
  • The storage change is safe: the migration is pure SQL (v22, gated on connectionLocked = 0 AND EXISTS(user message)), atomic + idempotent in the version-bumped transaction; the append-time lock lives inside the one and only INSERT INTO session_messages site, so no bypass path exists; read paths are now genuinely pure (the self-heal is gone entirely); new sessions are born unlocked and lock on first user append; rename/archive/hasSecret are unaffected; runtime-host converges to the same state a few ms earlier. The renamed storage test fails on the pre-commit head (pins the eager lock) and the migration test is real (downgrades to v21, reopens, asserts the WHERE semantics). The rebase is clean — 10/11 jointly-touched files are byte-identical to a 3-way merge and the general-settings-page.tsx resolution keeps both sides (main's feat(desktop): complete Runtime Host opt-in parity #2216 reformatting + the buildChatModelChoices swap, zero remaining callers of the old function).

Five optional notes, none blocking:

  • Docs acceptance criterion 2 (model-catalog-choices.ts / chat-model-selection.ts statically absent) is only enforced transitively today (those modules import a forbidden one); adding both names to FORBIDDEN_STARTUP_MODULES would pin it directly.
  • A dynamic import() executed at mount from a startup-reachable module would ship green — inherent to the static-BFS mechanism; a doc sentence noting the boundary is static-only would keep expectations honest.
  • apps/desktop/e2e/session-health-notice.spec.ts:9 still describes the old read-path self-heal; the behavior is identical but the comment describes removed code.
  • No negative test for the eager lock (assistant-only append must not lock) — the old test had the same blind spot, so nothing was weakened, but a regression to "lock on any append" would currently pass.
  • Interaction with fix(storage): import legacy JSONL session transcripts into SQLite (#2260) #2263 (still open): its importer restores connectionLocked after appending messages, so an imported session with connectionLocked: false + user messages would stay unlocked (the migration only covers pre-upgrade rows) — worth re-checking the restore order there before that PR lands.

Merging now — the boundary finally has teeth.

@Astro-Han
Astro-Han merged commit c1ee5c0 into apache:main Aug 6, 2026
12 checks passed
@Colafornia
Colafornia deleted the perf/model-metadata-firstscreen branch August 13, 2026 06:51
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.

perf(desktop): remove models.dev metadata from the renderer startup path

2 participants