[PR #1525] [MERGED] fix(javascript): yield during per-file projection and graph construction #2940

New issue

Closed

opened 2026-10-17 21:12:04 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-17 21:12:04 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1525
Author: @kaoru0822-kitauji
Created: 10/10/2026
Status: ✅ Merged
Merged: 10/10/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/javascript-graph-construction-steps

📝 Commits (3)

9f68c82 fix(javascript): yield during per-file projection and graph construction
cf5deba chore(review): integrate current main for cooperative phase checks
b88d8f8 chore(review): include merged public receiver regressions

📊 Changes

11 files changed (+438 additions, -83 deletions)

View changed files

📝 src/application/javascript/JavaScriptApplicationCancellation.test.ts (+40 -0)
📝 src/application/javascript/JavaScriptArtifactAnalysis.ts (+38 -16)
📝 src/application/javascript/JavaScriptArtifactGraphBuilder.ts (+40 -14)
📝 src/application/javascript/JavaScriptArtifactGraphFindings.ts (+4 -3)
📝 src/application/javascript/JavaScriptArtifactReconstruction.ts (+2 -1)
📝 src/application/javascript/JavaScriptModuleRelationships.ts (+5 -3)
📝 src/application/javascript/JavaScriptSemanticGraphBuilder.ts (+46 -23)
📝 src/domain/javascript/javascriptApplicationAnalysisSteps.test.ts (+59 -0)
📝 src/domain/javascript/javascriptApplicationGraph.ts (+106 -23)
📝 src/domain/javascript/javascriptSemanticGraph.ts (+65 -0)
➕ src/domain/zodCollectionSteps.ts (+33 -0)

📄 Description

Refs #1462 (T2 responsiveness under #1065). This removes the long event-loop blocks after the per-file stage and splits the per-file stage itself. A residual remains and is described below.

Where the pauses were

Measured in-process with a 1 ms interval timer recording event-loop gaps, plus the issue's SDK stdio probe. On the issue's 1,200-function fixture (SHA-256 1c6cafca…7711) on main:

phase	longest block
per-file stage for `index.js` (parse 13, receivers 11, static 16, semantic 71, projection ~130 ms)	~250 ms
`buildImmutableJavaScriptArtifactGraphSteps`: all `add*` phases plus the factory's synchronous parse, content check and digest run before the first step	~270–290 ms
`finishImmutableSteps`: the semantic factory's synchronous whole-graph schema parse and normalization	~275 ms (≈1.5–3 s on Obsidian)

Change

Per file (analyzeAndProjectJavaScriptArtifactFiles): parse, receivers/static analysis, semantic analysis and each of the ~17 semantic projection passes are now steps. They are driven by the existing time-sliced completeJavaScriptAnalysisSteps (8 ms slices, abort checks). projectFileSteps is new on JavaScriptSemanticGraphProjection; the synchronous projectFile and analyzeJavaScriptArtifactFiles drain the same steps, so their results are unchanged.
Application graph input: buildImmutableJavaScriptArtifactGraphSteps is a generator. It yields between phases, per module link (addJavaScriptModuleRelationshipsSteps) and per file of findings (addJavaScriptStaticFindingsSteps). buildJavaScriptArtifactGraph drains it synchronously.
Sealing: new sealTransferredJavaScriptApplicationGraphSteps and sealTransferredJavaScriptSemanticGraphSteps are for inputs the caller transfers, meaning it neither retains nor mutates them. They validate the graph's own fields and container constraints (such as nodes.min(1)) synchronously. Elements are then validated (and cloned) in steps through the new src/domain/zodCollectionSteps.ts, followed by the content checks (checkGraphContentSteps, now stepwise) and the canonical digest. Only the analysis pipeline uses them, because it builds those inputs privately. It now passes a copy of fingerprints, which it clears afterwards.
Unchanged: the public createImmutable…GraphSteps factories keep their synchronous deep capture. That capture is pinned by captures nested input before yielding…, which still passes. A content-check failure in the transfer path re-runs the content schema, so the error is the exact ZodError the copying path reports.

Results are identical

reconstructJavaScriptArtifact on main and on this branch gives the same application graph ID, semantic graph ID and SHA-256 of the complete serialized result for:

the issue fixture;
Obsidian app.asar and the 23 MB obsidian.asar;
Tabby app.asar;
the #1495 box fixture.

Measured effect

1,200-function fixture, in-process: the longest event-loop block goes from about 290 ms to about 90 ms.

The issue's MCP probe (warmed SDK stdio client, abort, then ping), swept over abort times of 50–600 ms on an M-series Mac:

	worst ping after abort
main	286 ms (abort at 250 ms)
this branch	56 ms

The client wait ended within about 2 ms of each abort, as before.

Obsidian obsidian.asar (36 files), full in-process analysis:

	longest block	total
main	10.9 s (`app.js` analysis plus projection)	41.1 s
this branch	5.4 s	41.7 s

The post-file graph phases no longer appear among the longest blocks.

Residual (not fixed here)

The remaining multi-second blocks are single analyzer stages on very large single files: analyzeParsedJavaScriptSemantics or static analysis of Obsidian's 3.7 MB app.js (about 5.4 s) and lib/mermaid.min.js (about 4.4 s). Splitting those needs either steps inside the domain analyzers or an owned execution boundary such as a worker. Both are larger design changes, so I left #1462 open.

Verification

A new test in JavaScriptApplicationCancellation.test.ts (#1462) aborts three event-loop turns after the one large file starts, and checks that cancellation lands before build_javascript_application_graph. On main it fails because the phase list includes that phase.
New tests in javascriptApplicationAnalysisSteps.test.ts: the transfer factories seal graphs equal to the copying factories, and those graphs are validated and immutable. For a malformed semantic node and a stale application node ID, they report the same issues as the whole-graph parse.
npx vitest run src/domain src/application passes (169 files, 2,131 tests).
npm run test:focused passes on tests/composition/javascript-applications, tests/acceptance/applications/{javascript,application}*, tests/boundary/mcp/{application,analysisView}* and tests/boundary/cli/analysisView* (20 files, 111 tests).
npm run check:fast and npm run docs:check pass. One new non-blocking warning: max-lines on javascriptApplicationGraph.ts, where the ownership WeakSet and the new sealing steps must share a module. JavaScriptSemanticGraphBuilder.ts and javascriptSemanticGraph.ts already carry max-lines warnings on main.
Not run: Linux, Node 22/24, two-CPU constrained hosts, or the Notion ASAR from the issue.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/1525 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/10/2026 **Status:** ✅ Merged **Merged:** 10/10/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/javascript-graph-construction-steps` --- ### 📝 Commits (3) - [`9f68c82`](https://github.com/morluto/rea/commit/9f68c82d74b1911376ed259a371c81cf509e5e43) fix(javascript): yield during per-file projection and graph construction - [`cf5deba`](https://github.com/morluto/rea/commit/cf5deba8652de4dad8d96a50160576a2591b424c) chore(review): integrate current main for cooperative phase checks - [`b88d8f8`](https://github.com/morluto/rea/commit/b88d8f859b031763f708c8a018ddd50c0e04dfe6) chore(review): include merged public receiver regressions ### 📊 Changes **11 files changed** (+438 additions, -83 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/JavaScriptApplicationCancellation.test.ts` (+40 -0) 📝 `src/application/javascript/JavaScriptArtifactAnalysis.ts` (+38 -16) 📝 `src/application/javascript/JavaScriptArtifactGraphBuilder.ts` (+40 -14) 📝 `src/application/javascript/JavaScriptArtifactGraphFindings.ts` (+4 -3) 📝 `src/application/javascript/JavaScriptArtifactReconstruction.ts` (+2 -1) 📝 `src/application/javascript/JavaScriptModuleRelationships.ts` (+5 -3) 📝 `src/application/javascript/JavaScriptSemanticGraphBuilder.ts` (+46 -23) 📝 `src/domain/javascript/javascriptApplicationAnalysisSteps.test.ts` (+59 -0) 📝 `src/domain/javascript/javascriptApplicationGraph.ts` (+106 -23) 📝 `src/domain/javascript/javascriptSemanticGraph.ts` (+65 -0) ➕ `src/domain/zodCollectionSteps.ts` (+33 -0) </details> ### 📄 Description Refs #1462 (T2 responsiveness under #1065). This removes the long event-loop blocks after the per-file stage and splits the per-file stage itself. A residual remains and is described below. ## Where the pauses were Measured in-process with a 1 ms interval timer recording event-loop gaps, plus the issue's SDK stdio probe. On the issue's 1,200-function fixture (SHA-256 `1c6cafca…7711`) on `main`: | phase | longest block | |---|---| | per-file stage for `index.js` (parse 13, receivers 11, static 16, semantic 71, projection ~130 ms) | ~250 ms | | `buildImmutableJavaScriptArtifactGraphSteps`: all `add*` phases plus the factory's synchronous parse, content check and digest run before the first step | ~270–290 ms | | `finishImmutableSteps`: the semantic factory's synchronous whole-graph schema parse and normalization | ~275 ms (≈1.5–3 s on Obsidian) | ## Change - **Per file** (`analyzeAndProjectJavaScriptArtifactFiles`): parse, receivers/static analysis, semantic analysis and each of the ~17 semantic projection passes are now steps. They are driven by the existing time-sliced `completeJavaScriptAnalysisSteps` (8 ms slices, abort checks). `projectFileSteps` is new on `JavaScriptSemanticGraphProjection`; the synchronous `projectFile` and `analyzeJavaScriptArtifactFiles` drain the same steps, so their results are unchanged. - **Application graph input**: `buildImmutableJavaScriptArtifactGraphSteps` is a generator. It yields between phases, per module link (`addJavaScriptModuleRelationshipsSteps`) and per file of findings (`addJavaScriptStaticFindingsSteps`). `buildJavaScriptArtifactGraph` drains it synchronously. - **Sealing**: new `sealTransferredJavaScriptApplicationGraphSteps` and `sealTransferredJavaScriptSemanticGraphSteps` are for inputs the caller *transfers*, meaning it neither retains nor mutates them. They validate the graph's own fields and container constraints (such as `nodes.min(1)`) synchronously. Elements are then validated (and cloned) in steps through the new `src/domain/zodCollectionSteps.ts`, followed by the content checks (`checkGraphContentSteps`, now stepwise) and the canonical digest. Only the analysis pipeline uses them, because it builds those inputs privately. It now passes a copy of `fingerprints`, which it clears afterwards. - **Unchanged**: the public `createImmutable…GraphSteps` factories keep their synchronous deep capture. That capture is pinned by `captures nested input before yielding…`, which still passes. A content-check failure in the transfer path re-runs the content schema, so the error is the exact `ZodError` the copying path reports. ## Results are identical `reconstructJavaScriptArtifact` on `main` and on this branch gives the same application graph ID, semantic graph ID and SHA-256 of the complete serialized result for: - the issue fixture; - Obsidian `app.asar` and the 23 MB `obsidian.asar`; - Tabby `app.asar`; - the #1495 box fixture. ## Measured effect 1,200-function fixture, in-process: the longest event-loop block goes from about **290 ms to about 90 ms**. The issue's MCP probe (warmed SDK stdio client, abort, then ping), swept over abort times of 50–600 ms on an M-series Mac: | | worst ping after abort | |---|---| | main | 286 ms (abort at 250 ms) | | this branch | 56 ms | The client wait ended within about 2 ms of each abort, as before. Obsidian `obsidian.asar` (36 files), full in-process analysis: | | longest block | total | |---|---|---| | main | 10.9 s (`app.js` analysis plus projection) | 41.1 s | | this branch | 5.4 s | 41.7 s | The post-file graph phases no longer appear among the longest blocks. ## Residual (not fixed here) The remaining multi-second blocks are single analyzer stages on very large single files: `analyzeParsedJavaScriptSemantics` or static analysis of Obsidian's 3.7 MB `app.js` (about 5.4 s) and `lib/mermaid.min.js` (about 4.4 s). Splitting those needs either steps inside the domain analyzers or an owned execution boundary such as a worker. Both are larger design changes, so I left #1462 open. ## Verification - A new test in `JavaScriptApplicationCancellation.test.ts` (#1462) aborts three event-loop turns after the one large file starts, and checks that cancellation lands before `build_javascript_application_graph`. On `main` it fails because the phase list includes that phase. - New tests in `javascriptApplicationAnalysisSteps.test.ts`: the transfer factories seal graphs equal to the copying factories, and those graphs are validated and immutable. For a malformed semantic node and a stale application node ID, they report the same issues as the whole-graph parse. - `npx vitest run src/domain src/application` passes (169 files, 2,131 tests). - `npm run test:focused` passes on `tests/composition/javascript-applications`, `tests/acceptance/applications/{javascript,application}*`, `tests/boundary/mcp/{application,analysisView}*` and `tests/boundary/cli/analysisView*` (20 files, 111 tests). - `npm run check:fast` and `npm run docs:check` pass. One new non-blocking warning: `max-lines` on `javascriptApplicationGraph.ts`, where the ownership `WeakSet` and the new sealing steps must share a module. `JavaScriptSemanticGraphBuilder.ts` and `javascriptSemanticGraph.ts` already carry `max-lines` warnings on `main`. - Not run: Linux, Node 22/24, two-CPU constrained hosts, or the Notion ASAR from the issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-17 21:12:04 +00:00