refactor(desktop): fork the legacy store import off the first window's path by Te-River · Pull Request #54317 · anomalyco/opencode

Te-River · 2026-10-10T13:32:36Z

Issue for this PR

Type of change

Bug fix
New feature
Refactor / code improvement
Documentation

What does this PR do?

The first window is shown as soon as Electron is ready (windows/early.ts), and the main bundle and the Effect layers are built on the same thread afterwards. The legacy store import reads and decodes the whole userData directory inside that build, holding the layers — and the IPC port — back.

The import is a one-time migration of files the renderer no longer writes, and the early snapshot already handles the pre-import state (storage/window-snapshot.ts checks for legacy files; window-snapshot.test.ts covers it). This forks it, so the layers and the port are handed out first.

How did you verify your code works?

Measured with the repo's dev startup benchmark (bun run bench:devex), reading the main-process marks (lifecycle/marks.ts).

The window→layers block is 280ms (median of 4 runs). Split: main bundle evaluation 152ms, storage 72ms, logging 34ms, onboarding 4ms. Inside storage: sqlite open + migrate 29ms, legacy import 21ms.

Forking the import takes the storage interval from 72ms to 42-47ms (4 baseline runs vs 2 fixed runs, reproducible in the marks).

The benchmark's headline metric (visibleWindowToHome) does not resolve this: it is dominated by the dev-mode renderer (9.0-14.1s) and the isolated dev service (5.9-14.1s), and its windowVisible milestone is the adoption log after the layers, so the 280ms block sits outside it. Paired deltas were +101ms / −1459ms. The marks are the evidence here.

Two assumptions from the issue were measured and did not hold: theme resolution is 1.3ms (not "tens of milliseconds"), and onboarding is 4ms.

Machine (for comparability): AMD Ryzen 5 5600U (6C/12T), 13.8 GB RAM, AMD Radeon integrated graphics (driver 30.0.13044.0) plus a virtual display adapter, 1920×1080 @60Hz, Windows 11 Pro for Workstations build 26300, balanced power plan.

bun test src/main/storage (packages/desktop): 29 passed, 0 failed.

Screenshots / recordings

n/a

Checklist

I have tested my changes locally
I have not included unrelated changes in this PR

github-actions · 2026-10-10T13:32:46Z

Hey! Your PR title perf(desktop): fork the legacy store import off the first window's path doesn't follow conventional commit format.

Please update it to start with one of:

feat: or feat(scope): new feature
fix: or fix(scope): bug fix
docs: or docs(scope): documentation changes
chore: or chore(scope): maintenance tasks
refactor: or refactor(scope): code refactoring
test: or test(scope): adding or updating tests

Where scope is the package name (e.g., app, desktop, opencode).

See CONTRIBUTING.md for details.

github-actions · 2026-10-10T13:33:04Z

The following comment was made by an LLM, it may be inaccurate:

opencode-agent

Thanks for measuring the marks so carefully. As written, though, this change can lose migrated settings for users who still have legacy .dat files, so I don't think it can go in as is. I couldn't run the desktop app here (headless Linux), so this comes from reading the code. The storage tests (29 of 29) and bun run typecheck in packages/desktop pass.

The early snapshot's legacy check only protects the window until the layer is up. Once importLegacyStores is forked, the layer finishes straight away, installs setStorageSnapshotProvider and opens the IPC port, and nothing waits for the import to commit. So the snapshot and StorageItems can serve empty namespaces, which the renderer then keeps for its lifetime. Worse, any write the renderer flushes before the import's transaction creates a row that onConflictDoNothing keeps, and the import then deletes the legacy file, so the user's old value is lost. Details are inline.

Public API: unchanged.

opencode-agent · 2026-10-10T13:34:19Z

          : Effect.logInfo("imported legacy store files", { imported: result.imported, files: result.removed }),
      ),
      Effect.catch((error) => Effect.logWarning("failed to import legacy store files", { error })),
+      Effect.forkScoped,


Forking here lets the rest of this layer run before the import commits. setStorageSnapshotProvider is installed a few lines below, and from then on the snapshot handler no longer goes through readWindowSnapshot's "legacy files present → empty storage" guard. The IPC port, and with it StorageItems/StorageUpdate, is also handed out after the layers. So on the first launch with legacy .dat files:

StorageItems / snapshots can return empty namespaces, and the renderer loads each namespace only once, so it keeps that empty state.

Any StorageUpdate that flushes before the import's transaction creates rows that the import's onConflictDoNothing keeps. importLegacyStores then deletes the .dat file, so the user's old value is lost for good.

If the import has to leave the layer's critical path, the storage handlers and the snapshot provider need to wait for it, e.g. by awaiting a Deferred before answering StorageItems/StorageUpdate and before swapping the snapshot provider. Another option is a cheap synchronous check, so the import only blocks when legacy files actually exist. When there are none, which is the common case, the cost is just the readDirectory.

Te-River · 2026-10-10T14:40:11Z

Fixed — you were right, and the race was exactly as you described.

The import stays forked; a Deferred now gates the reads. The import completes it through Effect.ensuring, so a failure or an interrupt still releases it rather than deadlocking the handlers. The snapshot provider and StorageItems / StorageUpdate / StorageClear await it before answering. The layers and the port are still handed out first, so the fork's benefit is intact: the logging→storage mark is 48ms against the 72ms baseline, and the import commits ~334ms after the layers are ready.

StorageClear is in the set because it writes the same table; Drafts* is not, since the import does not touch drafts.sqlite.

Regression test: packages/desktop/src/main/storage/index.test.ts seeds a legacy opencode.global.dat, builds the layer, and reads through the real WindowSnapshotChannel handler. It asserts the stored value comes back. Before the change it fails with items: {} — verified by stashing the three source files and re-running.

It needs an Electron stub, so it adds packages/desktop/test/preload.ts and a bunfig.toml to register it, following packages/core/test/preload.ts.

bun test src/main/storage: 30 passed. The full packages/desktop suite is green apart from the pre-existing windows-installer.test.ts (pwsh missing here).

opencode-agent

The new commit fixes the race from my earlier comment: the snapshot provider and StorageItems/StorageUpdate/StorageClear now wait for the import, and the Deferred is completed in ensuring, so a failed import doesn't block them forever. I couldn't run the desktop app here (headless Linux). The full packages/desktop suite passes, typecheck is clean, and the new test fails against the previous ungated commit and passes with the gate.

Public API: unchanged.

One path is still not gated: the extension host gets storage.state directly (inline).

packages/desktop/src/main/storage/index.ts:68: storage.state is still handed out ungated here, and the extension host uses it directly (extension/index.ts passes it to createHost, and extension/storage.ts reads and writes through it). gui-extensions/src/browser/restore.ts migrates from { state: ["opencode.browser.dat", key] }, which is one of the legacy files this import copies. If the host loads before the import commits, read() returns null and caches it in entry.json for the rest of the process. A later write() then overwrites the imported value, so the comment above ("every read and write of the state table waits on ready") doesn't hold yet. Making the host's loading step wait for storage.ready (e.g. adding it to the Promise.all in host()) would close this.

opencode-agent

The new push only rebases the branch; both commits are unchanged. The race from my first review is fixed by the gating commit (the snapshot provider and the storage IPC handlers wait on ready). The point from my last review about the extension host getting storage.state without waiting on ready is still open. I checked this on the rebased head: the packages/desktop tests pass and tsc -b is clean. I couldn't run the desktop app here (headless Linux).

Public API: unchanged.

opencode-agent

The new commit fixes the point from my last review: host() now waits for storage.ready, so the host's stores can't read state and cache an empty value before the import commits. I couldn't run the desktop app here (headless Linux). The full packages/desktop suite passes, tsc -b is clean, and the new test fails without the fix and passes with it.

Public API: unchanged.

The one thing left is the new test: it is large and depends on timing for a one-line change (inline).

opencode-agent · 2026-10-23T12:22:07Z

+          )
+          const loading = Context.get(context, Extensions.Service).host()
+          const loadedBeforeReady = yield* Effect.promise(async () => {
+            await new Promise((resolve) => setTimeout(resolve, 25))


The fix is one line, but this test is 90 lines and replaces six modules with mock.module stubs (./host, ./assets, ../windows, ../windows/protocol, ../service/desktop-cli, ../lifecycle). In bun test those stubs apply to the whole run, not just this file, so other test files can end up using them depending on the order files run. It also uses a 25ms setTimeout to check that the host hasn't loaded yet, which proves nothing if loading is just slower than 25ms. I'd drop this test so the change stays minimal. If you want to keep it, cut it down to the ordering check without the sleep, e.g. check that loading is still pending after a microtask flush, then resolve ready, then check that createHost ran.

Te-River · 2026-10-23T12:22:13Z

Closed in cc2eb5c60a: host() now awaits storage.ready as a third entry in its Promise.all, so createHost cannot run before the legacy import commits. The import itself is still forked off the layer, so the first-window path is unchanged — the snapshot provider and StorageItems/StorageUpdate keep their own yield* storage.ready.

The regression test holds a Deferred for ready, asserts that createHost is not called while it is pending, and asserts that the host then receives the same state reference the import wrote. It fails on the previous commit with Expected: 0 / Received: 1.

packages/desktop extension + storage suites: 72 pass.

Te-River · 2026-10-24T02:02:37Z

Taken as suggested: the test keeps the ordering check but drops the sleep. After host() is called, one microtask flush (Promise.race([loading.then(...), Promise.resolve()])) has to still report pending, then ready resolves and createHost runs with the same state reference the import wrote. No timer can make a slow load look like a correct order. 90 lines down to 70.

On the mocks: all six are genuinely on index.ts's import graph — ./host, ./assets, ../windows, ../windows/protocol, ../service/desktop-cli and ../lifecycle are each pulled in directly, and the Electron ones would fail to load in bun test without the stubs, so I could not drop any of them. Whole-suite runs show no cross-file effect: packages/desktop is 132 pass with the one pre-existing pwsh environment failure, and the focused suites are 72 pass across three consecutive runs.

tsc -b reports nothing in packages/desktop.

opencode-agent

The only change since my last review is the rewritten extension/index.test.ts. The sleep is gone, but the new check can't fail: with the storage.ready wait removed from extension/index.ts (the version from bed5f50ba), the test still passes. Details are inline. The full packages/desktop suite passes and tsc -b is clean. I couldn't run the desktop app here (headless Linux).

Public API: unchanged.

opencode-agent · 2026-10-24T02:06:11Z

+        // One microtask flush settles the import and CLI promises while `ready` still gates the host, so a
+        // load here ignored the gate. No sleep, so a slow load cannot fake a pass.
+        const beforeReady = yield* Effect.promise(() =>
+          Promise.race([loading.then(() => "loaded" as const), Promise.resolve("pending" as const)]),


Promise.resolve("pending") is already resolved, so it always wins this Promise.race. Loading the host needs at least a dynamic import() and a runPromise, which can't finish in one microtask anyway, so beforeReady is "pending" whether or not the host waits for ready. I put back the extension/index.ts from bed5f50ba (no storage.ready wait), and this test still passes. A check that can fail would be to first let the CLI and import promises settle, e.g. by awaiting import("./host") and desktopCli.resolve in the test, and only then check that createHost hasn't run while ready is still unresolved. Otherwise, as I said before, dropping this test keeps the one-line fix minimal and also gets rid of the six global mock.module stubs.

opencode-agent

This push only rebases the branch; all four commits are unchanged, so my earlier comments on extension/index.test.ts still apply. On the rebased head the packages/desktop suite passes and tsc -b is clean. I couldn't run the desktop app here (headless Linux).

Public API: unchanged.

Te-River · 2026-10-24T03:19:41Z

You were right on both counts, and the second one mattered more than the first.

The Promise.race assertion was always true: Promise.resolve("pending") is already fulfilled, so its reaction is queued before the race even starts, while loading.then(...) is a fresh promise that needs another turn. It read "pending" whether or not the host waited. Removing the gate reproduced exactly what you saw.

The test now encodes the contract instead of guessing at timing: the state handed to the service is a getter that throws unless the ready effect has actually been evaluated. The flag is set by the production code evaluating storage.ready (via Effect.tap on the substitute), never by the test, so ordering alone decides the outcome.

With runPromise(storage.ready) removed from the Promise.all, the test fails at the real read point — index.ts building { state: storage.state } — with storage state read before the legacy import committed. With it restored, packages/desktop extension + storage is 72 pass across three consecutive runs, and the full suite is 132 pass with the one pre-existing pwsh environment failure.

No sleep, no timer, no test-only parameter in production code. 69 lines.

opencode-agent

The rewritten test now catches the bug. Reading state throws until the ready effect has actually run, so a host that skips the wait fails no matter how the timing works out. I restored the ungated extension/index.ts and the test fails; with the fix it passes. The full packages/desktop suite passes and tsc -b is clean. I couldn't run the desktop app here (headless Linux).

Public API: unchanged.

opencode-agent

This push only rebases the branch; all four commits are unchanged. On the rebased head the packages/desktop suite passes and tsc -b is clean. I couldn't run the desktop app here (headless Linux).

Public API: unchanged.

opencode-agent

This push only rebases the branch; all four commits are unchanged. On the rebased head the packages/desktop suite passes and tsc -b is clean. I couldn't run the desktop app here (headless Linux).

Public API: unchanged.

The first window is shown as soon as Electron is ready, and the layers are built on the same thread afterwards. The legacy store import reads and decodes the whole userData directory inside that build, holding the layers (and the IPC port) back. The import is a one-time migration of files the renderer no longer writes, and the early snapshot already handles the pre-import state (window-snapshot.ts checks for legacy files, covered by window-snapshot.test.ts). Fork it so the layers and the port are handed out first. Measured on the dev startup benchmark: the storage interval drops from 72ms to 42-47ms (median of 4 baseline runs vs 2 fixed runs).

Forking the import let the layer finish before it committed: a snapshot or StorageItems could serve an empty namespace the renderer keeps for its lifetime, and a write before the import's transaction would be kept by onConflictDoNothing while the import deleted the legacy file. Add a Deferred the import completes (through ensuring, so a failure or interrupt still releases it). The snapshot provider and the state handlers await it; the layers and the port are still handed out first. Regression test: with a legacy .dat present, the snapshot handler must wait for the import. It fails before this change (items: {} instead of the stored value).

…eads state

opencode-agent

This push rebases onto the extension/ → plugin/ rename and squashes the last two commits; the fix is unchanged. The plugin host test now uses four module stubs instead of six and still catches the bug: with the storage.ready wait removed it fails, and with the fix it passes. The full packages/desktop suite passes and tsc -b is clean. I couldn't run the desktop app here (headless Linux).

Public API: unchanged.

github-actions · 2026-11-10T22:13:48Z

Automated PR Cleanup

Thank you for contributing to opencode.

Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions.

This PR was closed because it matched the following cleanup criteria:

The PR was created more than 1 month ago
The PR had fewer than 2 positive reactions
Positive reactions are counted as thumbs-up, heart, celebration, or rocket reactions on the PR

PRs created within the last month are not affected by this cleanup.

If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate.

Thanks again for taking the time to contribute.

Te-River requested review from Brendonovich and Hona as code owners October 10, 2026 13:32

github-actions Bot added the needs:title label Oct 10, 2026

opencode-agent Bot reviewed Oct 10, 2026

View reviewed changes

Te-River mentioned this pull request Oct 10, 2026

desktop: the window is shown before the main bundle and layers finish on the same thread #54296

Open

Te-River changed the title ~~perf(desktop): fork the legacy store import off the first window's path~~ refactor(desktop): fork the legacy store import off the first window's path Oct 10, 2026

github-actions Bot removed the needs:title label Oct 10, 2026

thdxr changed the base branch from v2 to dev October 10, 2026 19:41

Te-River force-pushed the desktop-startup-blocking branch 6 times, most recently from 95ef0af to 0db0902 Compare October 17, 2026 13:10

Te-River force-pushed the desktop-startup-blocking branch 3 times, most recently from ab0f886 to fee6124 Compare October 22, 2026 00:57

opencode-agent Bot reviewed Oct 22, 2026

View reviewed changes

Te-River force-pushed the desktop-startup-blocking branch from fee6124 to bed5f50 Compare October 23, 2026 11:12

opencode-agent Bot reviewed Oct 23, 2026

View reviewed changes

opencode-agent Bot reviewed Oct 24, 2026

View reviewed changes

Te-River force-pushed the desktop-startup-blocking branch from 973be88 to eb6243a Compare October 24, 2026 03:00

opencode-agent Bot reviewed Oct 24, 2026

View reviewed changes

Te-River force-pushed the desktop-startup-blocking branch from eb6243a to 3dc66fb Compare October 24, 2026 03:19

opencode-agent Bot reviewed Oct 24, 2026

View reviewed changes

Te-River force-pushed the desktop-startup-blocking branch from 3dc66fb to 5f76a6d Compare October 24, 2026 11:52

opencode-agent Bot reviewed Oct 24, 2026

View reviewed changes

Te-River force-pushed the desktop-startup-blocking branch from 5f76a6d to 2a72d73 Compare October 25, 2026 11:17

opencode-agent Bot reviewed Oct 25, 2026

View reviewed changes

Te-River added 3 commits October 27, 2026 20:03

fix(desktop): wait for the storage import before the extension host r…

165c44a

…eads state

Te-River force-pushed the desktop-startup-blocking branch from 2a72d73 to 165c44a Compare October 27, 2026 12:10

opencode-agent Bot reviewed Oct 27, 2026

View reviewed changes

opencodejr Bot mentioned this pull request Oct 28, 2026

Slow startup / long loading time every time the OpenCode desktop app is launched (30+ seconds before the UI becomes usable) #41477

Closed

github-actions Bot closed this Nov 10, 2026

github-actions Bot added the automated-pr-cleanup label Nov 10, 2026

Conversation

Te-River commented Oct 10, 2026

Issue for this PR

Type of change

What does this PR do?

How did you verify your code works?

Screenshots / recordings

Checklist

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot Oct 10, 2026

Choose a reason for hiding this comment

Uh oh!

Te-River commented Oct 10, 2026

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

Te-River commented Oct 23, 2026

Uh oh!

Te-River commented Oct 24, 2026

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Te-River commented Oct 24, 2026

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 10, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant