Skip to content

refactor: codebase review cleanup — dead code, duplication, complexity, unified patterns - #740

Merged
stefanhoelzl merged 2 commits into
mainfrom
review
Oct 2, 2026
Merged

stefanhoelzl merged 2 commits into
mainfrom
review

Conversation

@stefanhoelzl

Copy link
Copy Markdown
Owner

Fixes from a review of the codebase for unused code, overly complex code, duplication and patterns worth unifying. Squashed from the review branch and merged with main's concurrent-hooks / single-writer-fold changes.

Bugs

  • POSIX CWD-release cleanup shares one runner (cwd-release-kill.ts) with Windows: errors and surviving processes are reported to the deletion instead of swallowed by a bare catch {}
  • Timeouts always clear their timer (raceTimeout / withTimeout in src/utils/timeout.ts): dialog sessions, ProcessRunner.wait, OpenCode connect, the dialog mock
  • .cmd shims quote every argument (runAgentBinary, OpenCode launcher; shared cmd-quote.ts)
  • appctrl expand-sidebar retries like the e2e fixture (one shared expandSidebarOn)
  • DialogView walls off its form with an ErrorBoundary like PanelView; Form's orphan-focus timer is cleared on unmount
  • delete pipeline reports the kill-blockers step when the removal fails
  • CLAUDE.md rule violations: node:fs stat in api-server, string paths, path log-context keys, direct process.platform; ElectronBuildInfo's sync git call documented as an exception

Interface changes

  • ApiResult (with category) declared once in shared/api-protocol.ts
  • one workspace-identity shape (workspaceIdentityPayloadSchema) in workspace event payloads; agent:status-updated flattened
  • workspace:open decides fresh-vs-reopen once: fresh + workspaceName on every hook context and on workspace:created (plugin stdin keeps reopened)
  • streaming yield frames validated against per-hook-point frames schemas
  • UiState row keys are workspace refs; the agent runtime is keyed by WorkspaceRef
  • modules declare handlers with defineHooks / defineEvents (src/intents/declarations.ts): ~170 event as / ctx as / intent as casts removed, mistyped handlers fail to compile
  • every operation resolves workspaces through resolveWorkspaceIdentity

Simplification and deduplication

  • presentation module split into startup-surface / running-hooks / navigation / view-model (2,622 → ~1,630 lines); creation FormState
  • claude handleHook status rules extracted into a pure deriveStatus reducer with focused tests
  • delete pipeline state object, idempotency (singleton = per-key with a constant key; main's wait kept), open-project, app-start retry, git-worktree identify, ide-server lifecycle, api-server socket handling
  • plugin module binds hook points from OPEN_WORKSPACE_HOOKS / DELETE_WORKSPACE_HOOKS; hooks and automations share one script runner
  • sidekick extension split into connection / agent-terminal / ui-handlers / codehydra-api / workspace-commands
  • shared helpers: tagKey/encodeTag, prependPath, errorCode/toError, streamDownloadProgress, escapeHtml, FieldShell, .ch-icon-button, status cache with (ref, previous, next) transitions, activeWorkspaceRef
  • dead code removed: global dialog CSS, unused mock factories, test-only guards, storeText, unused deps ignore and @testing-library/user-event
  • test setup deduplicated (~3.4k lines; no test removed)
  • docs: PATTERNS.md error-surfacing rule, accurate shutdown-handler rule, stale references fixed

Left alone on purpose: extensions/markdown-review-editor/, migration code.

…y, unified patterns

Fixes from a review of the codebase for unused code, overly complex code,
duplication and patterns worth unifying.

Bugs
- POSIX CWD-release cleanup shares one runner with Windows: errors and
  surviving processes are reported to the deletion instead of swallowed
- Timeouts always clear their timer (raceTimeout/withTimeout): sessions,
  process wait, OpenCode connect, the dialog mock
- .cmd shims quote every argument (runAgentBinary, OpenCode launcher)
- appctrl expand-sidebar retries like the e2e fixture
- DialogView walls off its form with an ErrorBoundary; Form's orphan-focus
  timer is cleared on unmount
- delete pipeline reports the kill-blockers step when the removal fails
- rule violations: node:fs stat, string paths, path log-context keys,
  direct process.platform; ElectronBuildInfo's sync git call documented

Interface changes
- ApiResult (with category) declared once in shared/
- one workspace-identity shape in workspace event payloads;
  agent:status-updated flattened
- workspace:open decides fresh-vs-reopen once (`fresh`, `workspaceName` on
  every hook context and on workspace:created)
- yield frames validated against per-hook-point `frames` schemas
- UiState row keys are workspace refs; the agent runtime is keyed by ref
- modules declare handlers with defineHooks/defineEvents
  (src/intents/declarations.ts) — the event/ctx/intent casts are gone

Simplification and deduplication
- presentation module split (startup-surface, running-hooks, navigation,
  view-model); creation FormState; claude hook status as a pure reducer
- delete pipeline state, idempotency (singleton = per-key, `wait` kept),
  open-project, app-start retry, dispatcher, git-worktree identify,
  ide-server lifecycle, api-server socket handling
- plugin module binds hook points from the hook maps; one script runner
- sidekick extension split by concern
- shared helpers: tagKey/encodeTag, prependPath, errorCode/toError,
  streamDownloadProgress, escapeHtml, FieldShell, .ch-icon-button,
  workspace status cache with transitions, activeWorkspaceRef
- dead code removed (global dialog CSS, mock factories, test-only guards,
  unused deps `ignore` and `@testing-library/user-event`)
- test setup deduplicated (~3.4k lines)
- docs: error-surfacing rule, shutdown handler rule, stale references

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@stefanhoelzl stefanhoelzl added the internal no user facing changes label Oct 2, 2026
@stefanhoelzl
stefanhoelzl enabled auto-merge (rebase) October 2, 2026 19:42
buildServeEnv now takes the injected platform (linux in the mock deps) and the
OpenCode server's cwd is the native path, so these expectations failed only on
a Windows host. serve-env.test.ts covers the Windows env.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@stefanhoelzl
stefanhoelzl merged commit 96fc8cd into main Oct 2, 2026
16 checks passed
@stefanhoelzl
stefanhoelzl deleted the review branch October 2, 2026 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal no user facing changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant