Skip to content

refactor(arch): close DI, import-boundary and lifecycle gaps#2813

Merged
charlesvien merged 3 commits into
mainfrom
refactor/arch-di-boundary-fixes
Jun 24, 2026
Merged

refactor(arch): close DI, import-boundary and lifecycle gaps#2813
charlesvien merged 3 commits into
mainfrom
refactor/arch-di-boundary-fixes

Conversation

@charlesvien

@charlesvien charlesvien commented Jun 21, 2026

Copy link
Copy Markdown
Member

Problem

A review of the architecture post package split surfaced a set of gaps where apps/code and the portable packages had drifted from this repo's own stated rules: portable core/ui, strongly-typed DI, a thin host, one-line tRPC forwards. None of these are structural redesigns. They are the codebase not yet matching its own architecture doc.

Changes

DI token safety and import boundaries

  • Remove the forbidden Object.freeze token bags MAIN_TOKENS, RENDERER_TOKENS (apps/code) and TOKENS (workspace-server); use the standalone Symbol.for(...) consts at every call site so TypedContainer<BindingMap> actually type-checks get/bind.
  • biome.jsonc: drop the !@posthog/core negations that allowed import cycles back into api-client, workspace-client and platform; block core, workspace-client and host-router from workspace-server.
  • workspace-server/src/trpc.ts: replace the package-level container.get() service-locator with a createAppRouter(services) factory resolved in serve.ts (the composition seam). The container import is removed entirely so any missed closure is a compile error.
  • GIT_DIFF_SOURCE binding uses the injector ctx.get() instead of the module-level container.

Lifecycle and composition root

  • Move container.unbindAll() out of AppLifecycleService into the index.ts composition root via a beforeExit hook on gracefulExit, so the service no longer reaches for the global container.

State placement

  • Move the session domain store into @posthog/core as a zustand/vanilla store; @posthog/ui keeps a thin React wrapper. The immer proxy-lifetime guards in the dequeue paths are preserved verbatim.
  • Add a 5 minute TTL to SessionService.previewConfigOptionsCache so stale cloud model lists do not persist for the renderer lifetime.

Reliability and correctness

  • WorkspaceServerService: supervised restart with exponential backoff and a max-attempts cap, a statusChanged event stream, and a renderer failure banner with retry.
  • packages/agent: thread LLM gateway config explicitly (GatewayEnv) into the Claude subprocess env instead of mutating the shared global process.env, which clobbered config across concurrent sessions.
  • clearApplicationStorage now clears the encrypted SecureStore alongside localStorage (previously encrypted settings survived a factory reset).
  • AgentService.getOrCreateSession overloads remove the unreachable null return on the new-session path.

Also removes a redundant immer dependency added to core/package.json (immer already resolves via zustand's peer, and declaring it would break --frozen-lockfile).

How did you test this?

All run locally in the branch worktree:

  • pnpm typecheck: 22/22 packages pass
  • pnpm lint: clean
  • pnpm test: apps/code 151, @posthog/core 1617, @posthog/ui 787, @posthog/agent 778 pass. @posthog/workspace-server 511 pass / 5 fail, the 5 being a pre-existing better-sqlite3 native-ABI mismatch (NODE_MODULE_VERSION 145 vs 147), unrelated to these changes.
  • pnpm install --frozen-lockfile: consistent
  • node scripts/check-host-boundaries.mjs: no new violations

Rewrote app-lifecycle/service.test.ts for the container-teardown move and added a beforeExit hook test. The session store move is covered by the existing sessionStore (10), sessionServiceHost (119) and recovery integration (7) suites.

Not yet covered by an automated test: the WorkspaceServerService restart supervisor (the service had no prior test file). Worth a follow-up.

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

@charlesvien charlesvien changed the title tighten architecture boundaries and DI safety refactor(arch): close DI, import-boundary and lifecycle gaps Jun 21, 2026
@charlesvien charlesvien marked this pull request as ready for review June 21, 2026 00:07

charlesvien commented Jun 21, 2026

Copy link
Copy Markdown
Member Author

@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit a43877f.

@greptile-apps

greptile-apps Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/code/src/renderer/components/Providers.tsx:79
**"Retry" button does nothing in the `failed` state**

`invalidateConnection()` only refetches the `workspaceServer.getConnection` query. When status is `"failed"` the workspace server process has exhausted all its auto-retry attempts and `wsServer.getConnection()` still returns `null`, so the refetch just returns `null` again and the banner persists. To actually restart the server, something must call `wsServer.start()` (with `restartAttempts` reset, as `stop()` does), which requires a new tRPC mutation or procedure — `invalidateConnection` does not trigger that path.

### Issue 2 of 2
packages/ui/src/utils/clearStorage.ts:19-30
**Always reloads even when server-side clear fails**

Previously, a failure in `clearAllData` showed an alert and did **not** reload — the user kept the app in its original state. With `Promise.allSettled`, the page always reloads regardless of whether either operation succeeded. If `folders.clearAllData` fails, the server-side folder registrations survive while `localStorage` is wiped, potentially leaving the app in an inconsistent state on reload (e.g., no locally-known folders but the server still has them). Errors are only logged to the console, so the user gets no indication that the clear was partial.

Reviews (1): Last reviewed commit: "tighten architecture boundaries and DI s..." | Re-trigger Greptile

Comment thread apps/code/src/renderer/components/Providers.tsx Outdated
Comment thread packages/ui/src/utils/clearStorage.ts
@charlesvien charlesvien force-pushed the refactor/arch-di-boundary-fixes branch from a8670ec to 9943d59 Compare June 21, 2026 00:30
@charlesvien charlesvien added the Stamphog This will request an autostamp by stamphog on small changes label Jun 21, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates denied this PR: it matches the auth deny-list, exceeds size limits (2830 lines, 33 files), and is classified as a never-auto-approve tier due to its multi-area refactor scope. Additionally, there are unresolved bot comments raising substantive concerns about broken retry behavior and a behavior regression in clearStorage error handling.

@stamphog stamphog Bot removed the Stamphog This will request an autostamp by stamphog on small changes label Jun 21, 2026
@charlesvien charlesvien added the Stamphog This will request an autostamp by stamphog on small changes label Jun 21, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates denied this PR: it touches auth code, exceeds size limits at 2830 lines across 33 files, and is classified as a never-auto-approve multi-area refactor. There are also unresolved substantive bot comments about a broken retry button and a behavior regression in clearStorage error handling that need to be addressed.

@stamphog stamphog Bot removed the Stamphog This will request an autostamp by stamphog on small changes label Jun 21, 2026
@charlesvien charlesvien force-pushed the refactor/arch-di-boundary-fixes branch 3 times, most recently from 9e7ce55 to 800c58e Compare June 21, 2026 08:53
@charlesvien charlesvien added the Stamphog This will request an autostamp by stamphog on small changes label Jun 21, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates denied this PR: it touches auth code, exceeds size limits (3014 lines, 34 files), and is classified as a never-auto-approve multi-area refactor. Two substantive bot-raised concerns remain: the "Retry" button doing nothing in the failed state, and a behavior regression in clearStorage that always reloads even when server-side clearing fails.

@stamphog stamphog Bot removed the Stamphog This will request an autostamp by stamphog on small changes label Jun 21, 2026
@charlesvien charlesvien force-pushed the refactor/arch-di-boundary-fixes branch from 800c58e to 65aae56 Compare June 22, 2026 05:56
@charlesvien charlesvien added the Stamphog This will request an autostamp by stamphog on small changes label Jun 22, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates denied this PR: it touches auth code, exceeds size limits (3014 lines, 34 files), and is classified as a never-auto-approve multi-area refactor. Two substantive bot-raised concerns also remain unaddressed: the "Retry" button doing nothing in the failed state, and a behavior regression in clearStorage that always reloads even when server-side clearing fails.

@stamphog stamphog Bot removed the Stamphog This will request an autostamp by stamphog on small changes label Jun 22, 2026

@tatoalo tatoalo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here and in gateway-models.ts we are still reading process.env. ANTHROPIC_BASE_URL so we would break the model picker, we would need to thread it explicitly. Approving not to block but this needs to be addressed

@charlesvien charlesvien force-pushed the refactor/arch-di-boundary-fixes branch 4 times, most recently from 9760f97 to f7c7d9d Compare June 23, 2026 22:06
@charlesvien charlesvien force-pushed the refactor/arch-di-boundary-fixes branch from f7c7d9d to c501f7d Compare June 24, 2026 00:41
@charlesvien charlesvien merged commit 3eac81b into main Jun 24, 2026
24 checks passed

Copy link
Copy Markdown
Member Author

Merge activity

@charlesvien charlesvien deleted the refactor/arch-di-boundary-fixes branch June 24, 2026 00:58
charlesvien added a commit that referenced this pull request Jun 24, 2026
)

## Problem

Two reliability and security gaps from the package split surfaced, deliberately split out from #2813 because they touch runtime-sensitive Electron config and warrant their own review.

1. If `boot()`, a module-scope import or an Inversify resolution throws during renderer startup, the user gets a blank window with no error and no recovery short of force-quitting.
2. The main window runs with `nodeIntegration: true`, which applies in production, not just dev.

## Changes

- Add a `BootErrorBoundary` (`apps/code/src/renderer/components/BootErrorBoundary.tsx`) around the root React tree, plus a `try/catch` around the synchronous bootstrap in `main.tsx` and a `.catch` on the async `boot(container)`. Any of those failing now renders a minimal, theme-independent error screen with a Reload button instead of a blank window, and logs the failure.
- Set `webPreferences.nodeIntegration: false` on the main window. `contextIsolation: true` is already set (so the renderer main world never had Node anyway) and the renderer is Vite-bundled with zero `node:*`/`require` usage in the main world, so this is defense in depth with no functional change.

Scope note: the dev-only `webSecurity: false` override is left in place. It does not affect production (production already runs with `webSecurity: true`) and flipping it risks breaking dev resource loading. The remaining dev/prod parity gap is deferred.

## How did you test this?

All run locally in the branch worktree:
- `pnpm typecheck`: 22/22 packages pass
- `pnpm lint`: clean
- `pnpm test`: `apps/code` 151 pass
- `node scripts/check-host-boundaries.mjs`: no new violations

Not yet done: a manual smoke test of the running app (dev and packaged). The `nodeIntegration` flip is low risk given `contextIsolation: true`, but it changes runtime Electron config, so a human should confirm the window still loads and agents/terminals work before merge. This PR is left as a draft for that reason.

## Automatic notifications

- [ ] Publish to changelog?
- [ ] Alert Sales and Marketing teams?
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.

2 participants