Skip to content

fix(ui): keep the diff engine off the startup path - #784

Open
benvinegar wants to merge 1 commit into
mainfrom
claude/pr-759-review-4gzat1
Open

fix(ui): keep the diff engine off the startup path#784
benvinegar wants to merge 1 commit into
mainfrom
claude/pr-759-review-4gzat1

Conversation

@benvinegar

Copy link
Copy Markdown
Member

Follow-up to #759.

What happened

src/main.tsx imported disposeHighlightWorker from the ui/diff/worker barrel. That barrel re-exports the compact-payload and HAST modules, which import @pierre/diffs as a runtime value, so the diff engine landed back in the entrypoint's eager import graph. That reverses the guarantee in .changeset/defer-startup-graph.md: hunk --version, --help, daemon serve, the markup commands, and hunk session * all paid for it again.

Measured on this branch's parent:

import ui/diff/worker (barrel):            101.1 ms
import worker/highlightWorkerClient only:    1.9 ms

Eagerly reachable local modules from src/main.tsx went 68 → 62, and @pierre/diffs leaves the eager set.

The disposal was also in the wrong place

runInteractiveApp returns once the app is mounted — it does not await app exit — so the entrypoint's finally { disposeHighlightWorker() } fired immediately after root.render(...), long before any large diff requested a worker. It disposed nothing. Moving the call into the app's own shutdown() puts it beside hostClient.stop(), where there is actually a worker to release.

Changes

  • src/main.tsx: drop the eager worker import and the entrypoint-side disposal.
  • src/ui/runInteractiveApp.tsx: dispose the syntax worker in shutdown().
  • test/cli/startup-graph.test.ts: walk the entrypoint's eager imports via Bun.Transpiler.scanImports, skipping dynamic-import (the deferral mechanism itself) and type-only imports (already erased). Asserts no @pierre/* or @opentui/* package is eagerly reachable, and reports the offending chain on failure.

The guard was verified to fail on the regression it covers:

@pierre/diffs via src/main.tsx -> src/ui/diff/worker/index.ts
  -> src/ui/diff/worker/highlightCompact.ts
  -> src/ui/diff/worker/highlightHast.ts -> @pierre/diffs

A static walk was chosen over a timing assertion so the failure names the import that caused it instead of varying with machine load.

Validation

  • bun run typecheck, bun run lint, bun run format
  • bun test — 3028 pass, 4 fail, all reproduced on unmodified main: change-block line pairing (confirmed failing at 5ebe975), one PTY flake, and two website/ specs missing @axe-core/playwright in this environment.
  • bun run test:integration — 116 pass, 1 fail; that same failure occurs on unmodified main.
  • bun run test:tty-smoke — 0/9 in this sandbox, identical on unmodified main (no usable TTY here), so it is unverified rather than passing.

Generated by Claude Code

The syntax worker's disposal was imported into the executable entrypoint through the diff worker
barrel, which re-exports the compact payload and HAST modules. Those load @pierre/diffs eagerly, so
`hunk --version`, `--help`, `daemon serve`, and `hunk session *` paid for the diff engine again.

That disposal was also in the wrong place: the entrypoint resolves once the review app is mounted,
not once it exits, so it fired before the first large diff ever asked for a worker. Move it into the
app's own shutdown path, where it can release a worker that actually exists.

Add a startup-graph test that walks the entrypoint's eager imports, so the next accidental import
fails with the offending chain instead of quietly costing every headless command.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2ZhrEHs971XcBuAMP6RwR
@vercel

vercel Bot commented Aug 16, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hunk-web Ignored Ignored Aug 16, 2026 9:34pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR removes the syntax-worker barrel from the CLI entrypoint's eager import graph and moves worker disposal into the interactive application's shutdown lifecycle.

  • Keeps headless CLI commands from eagerly loading Pierre or OpenTUI dependencies.
  • Releases the lazily created highlighting worker during actual application shutdown.
  • Adds a static import-graph regression test and a patch changeset.

Confidence Score: 4/5

The PR appears safe to merge, with only the non-blocking need to colocate the new regression test with the source behavior it protects.

The worker cleanup relocation follows the application's real shutdown lifecycle, and the entrypoint no longer eagerly reaches the diff engine; the remaining issue concerns repository test organization rather than runtime correctness.

Files Needing Attention: test/cli/startup-graph.test.ts

Important Files Changed

Filename Overview
src/main.tsx Removes the eager worker-barrel import and the ineffective disposal call that ran immediately after mounting.
src/ui/runInteractiveApp.tsx Moves idempotent worker disposal into the application-owned shutdown closure alongside other runtime cleanup.
test/cli/startup-graph.test.ts Adds startup-graph and disposal-location regression checks, but places the test outside the repository's required colocated-test structure.
.changeset/defer-syntax-worker-startup.md Records the headless-startup and syntax-worker lifecycle fixes as a patch release.

Sequence Diagram

sequenceDiagram
  participant CLI as src/main.tsx
  participant App as runInteractiveApp
  participant UI as AppHost
  participant Worker as Highlight worker
  CLI->>CLI: Resolve startup plan
  alt Headless command
    CLI-->>CLI: Complete without loading interactive graph
  else Interactive app
    CLI->>App: Dynamic import and mount
    App->>UI: Render application
    UI->>Worker: Create lazily for eligible diff
    UI->>App: Request shutdown
    App->>Worker: disposeHighlightWorker()
    App->>UI: Unmount and destroy renderer
  end
Loading
Prompt To Fix All With AI
### Issue 1
test/cli/startup-graph.test.ts:1
**Startup test is not colocated**

This regression test lives under `test/cli/` instead of beside the source it protects, contrary to the repository's test-colocation requirement and making it easier to overlook during future startup lifecycle changes.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix(ui): keep the diff engine off the st..." | Re-trigger Greptile

@@ -0,0 +1,113 @@
import { describe, expect, test } from "bun:test";

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.

P2 Startup test is not colocated

This regression test lives under test/cli/ instead of beside the source it protects, contrary to the repository's test-colocation requirement and making it easier to overlook during future startup lifecycle changes.

Context Used: testing.mdc Cursor rule (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: test/cli/startup-graph.test.ts
Line: 1

Comment:
**Startup test is not colocated**

This regression test lives under `test/cli/` instead of beside the source it protects, contrary to the repository's test-colocation requirement and making it easier to overlook during future startup lifecycle changes.

**Context Used:** testing.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/testing.mdc))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Leaving this where it is — test/cli/ is the location this repo's own convention calls for.

CLAUDE.md states both rules:

- Colocate unit tests with the code they cover (`src/core/foo.ts` + `src/core/foo.test.ts`, ...).
- Use repo-level `test/` directories by intent:
  - `test/cli/` for black-box CLI contract coverage.

This is black-box coverage of a CLI contract: it treats src/main.tsx as an opaque entrypoint and asserts a property of the whole command surface — that --version, --help, daemon serve, and session * answer without loading the diff engine. It isn't a unit test of any one module, so there is no single source file to sit beside. It lands next to test/cli/entrypoint.test.ts, which covers the same entrypoint the same way.

Worth flagging that the cited testing.mdc is from modem-dev/modem, a different repository — hunk has no .cursor/ directory, and its own CLAUDE.md is the governing convention here.

On the underlying concern — that this is easy to overlook during future startup lifecycle changes — the test fails loudly with the offending import chain rather than a generic assertion, e.g.:

@pierre/diffs via src/main.tsx -> src/ui/diff/worker/index.ts
  -> src/ui/diff/worker/highlightCompact.ts
  -> src/ui/diff/worker/highlightHast.ts -> @pierre/diffs

Generated by Claude Code

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.

You're right, and I apologize for the noise. The placement is correct per hunk's own CLAUDE.md, and I should not have cited testing.mdc from modem-dev/modem — that rule doesn't govern this repository.

The failure message with the full import chain is a good touch; it makes the guard self-documenting when it trips.

Shall I remember a rule: "In modem-dev/hunk reviews, do not cite .cursor/ rules from modem-dev/modem — hunk's CLAUDE.md is the governing convention, and test/cli/ is the correct location for black-box CLI contract tests"?

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