-
Notifications
You must be signed in to change notification settings - Fork 266
fix(ui): keep the diff engine off the startup path #784
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
benvinegar
wants to merge
1
commit into
main
Choose a base branch
from
claude/pr-759-review-4gzat1
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+128
−8
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "hunkdiff": patch | ||
| --- | ||
|
|
||
| Keep `hunk --version`, `--help`, `daemon serve`, and `hunk session *` off the diff-engine startup | ||
| path again, and release the syntax worker when the review app exits instead of at startup. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { Transpiler } from "bun"; | ||
| import { existsSync, readFileSync, statSync } from "node:fs"; | ||
| import { dirname, join, relative, resolve } from "node:path"; | ||
|
|
||
| /** | ||
| * Guards the startup cost of commands that never build a changeset. | ||
| * | ||
| * `hunk --version`, `--help`, `daemon serve`, the markup commands, and `hunk session *` answer | ||
| * without a diff engine, renderer, or VCS backend. Those subsystems are reached through dynamic | ||
| * `import()` from the interactive plan, so only eager `import` statements can pull them into the | ||
| * entrypoint graph. Walking the static graph catches the regression a timing assertion would | ||
| * measure inconsistently across machines. | ||
| */ | ||
|
|
||
| const REPO_ROOT = resolve(import.meta.dir, "../.."); | ||
| const ENTRYPOINT = join(REPO_ROOT, "src/main.tsx"); | ||
|
|
||
| /** | ||
| * Package prefixes the entrypoint must not load before a command selects an interactive plan. | ||
| * | ||
| * These are the heavy graphs named in the startup-deferral contract: the Pierre diff engine and | ||
| * its renderer, and OpenTUI's embedded native library. | ||
| */ | ||
| const DEFERRED_PACKAGE_PREFIXES = ["@pierre/", "@opentui/"]; | ||
|
|
||
| const MODULE_EXTENSIONS = [".ts", ".tsx", ".js", ".jsx"]; | ||
|
|
||
| /** Resolve one relative specifier to a source file the walker can read. */ | ||
| function resolveLocalModule(fromFile: string, specifier: string) { | ||
| const base = resolve(dirname(fromFile), specifier); | ||
| const candidates = [ | ||
| base, | ||
| ...MODULE_EXTENSIONS.map((extension) => `${base}${extension}`), | ||
| ...MODULE_EXTENSIONS.map((extension) => join(base, `index${extension}`)), | ||
| ]; | ||
|
|
||
| return ( | ||
| candidates.find((candidate) => existsSync(candidate) && statSync(candidate).isFile()) ?? null | ||
| ); | ||
| } | ||
|
|
||
| /** Strip the entrypoint shebang so the transpiler can scan it as a module. */ | ||
| function readModuleSource(file: string) { | ||
| const source = readFileSync(file, "utf8"); | ||
| return source.startsWith("#!") ? source.slice(source.indexOf("\n") + 1) : source; | ||
| } | ||
|
|
||
| /** | ||
| * Walk every eagerly imported module reachable from the entrypoint. | ||
| * | ||
| * `scanImports` reports the imports that survive transpilation, so type-only imports are already | ||
| * excluded, and `dynamic-import` entries are skipped because those are exactly the deferral | ||
| * mechanism under test. Returns each external package with the chain that first reached it. | ||
| */ | ||
| function traceEagerExternals(entrypoint: string) { | ||
| const transpiler = new Transpiler({ loader: "tsx" }); | ||
| const visited = new Set<string>(); | ||
| const externals = new Map<string, string[]>(); | ||
|
|
||
| const walk = (file: string, chain: string[]) => { | ||
| if (visited.has(file) || file.endsWith(".json")) { | ||
| return; | ||
| } | ||
| visited.add(file); | ||
|
|
||
| for (const imported of transpiler.scanImports(readModuleSource(file))) { | ||
| if (imported.kind !== "import-statement") { | ||
| continue; | ||
| } | ||
|
|
||
| if (imported.path.startsWith(".")) { | ||
| const next = resolveLocalModule(file, imported.path); | ||
| if (next) { | ||
| walk(next, [...chain, relative(REPO_ROOT, next)]); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (!externals.has(imported.path)) { | ||
| externals.set(imported.path, [...chain, imported.path]); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| walk(entrypoint, [relative(REPO_ROOT, entrypoint)]); | ||
| return externals; | ||
| } | ||
|
|
||
| describe("CLI startup graph", () => { | ||
| test("entrypoint does not eagerly import the diff engine or renderer", () => { | ||
| const externals = traceEagerExternals(ENTRYPOINT); | ||
| const eagerlyDeferred = [...externals.entries()] | ||
| .filter(([packageName]) => | ||
| DEFERRED_PACKAGE_PREFIXES.some((prefix) => packageName.startsWith(prefix)), | ||
| ) | ||
| .map(([packageName, chain]) => `${packageName} via ${chain.join(" -> ")}`); | ||
|
|
||
| expect(eagerlyDeferred).toEqual([]); | ||
| }); | ||
|
|
||
| test("worker disposal stays with the interactive app rather than the entrypoint", () => { | ||
| // The entrypoint resolves once the app is mounted, so disposing from there would terminate the | ||
| // worker before the first large diff requested it. | ||
| const interactiveAppSource = readFileSync( | ||
| join(REPO_ROOT, "src/ui/runInteractiveApp.tsx"), | ||
| "utf8", | ||
| ); | ||
|
|
||
| expect(readModuleSource(ENTRYPOINT).includes("disposeHighlightWorker")).toBe(false); | ||
| expect(interactiveAppSource.includes("disposeHighlightWorker()")).toBe(true); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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!
There was a problem hiding this comment.
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.mdstates both rules:This is black-box coverage of a CLI contract: it treats
src/main.tsxas an opaque entrypoint and asserts a property of the whole command surface — that--version,--help,daemon serve, andsession *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 totest/cli/entrypoint.test.ts, which covers the same entrypoint the same way.Worth flagging that the cited
testing.mdcis frommodem-dev/modem, a different repository —hunkhas no.cursor/directory, and its ownCLAUDE.mdis 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.:
Generated by Claude Code
There was a problem hiding this comment.
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 ownCLAUDE.md, and I should not have citedtesting.mdcfrommodem-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"?