[PR #3792] [MERGED] fix(javascript): keep empty URL path segments significant #5902

New issue

Closed

opened 2026-10-31 01:10:19 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:10:19 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/3307-url-empty-segments

📝 Commits (2)

0b972b8 fix(javascript): keep empty URL path segments significant
b0d79fd fix(javascript): resolve HTML base and reference as one URL path

📊 Changes

4 files changed (+176 additions, -29 deletions)

View changed files

📝 docs/javascript-artifact-reconstruction.md (+4 -0)
📝 src/application/javascript/JavaScriptArtifactPathResolution.test.ts (+46 -5)
📝 src/application/javascript/JavaScriptArtifactPathResolution.ts (+105 -24)
📝 src/application/javascript/JavaScriptHtmlBaseResolution.test.ts (+21 -0)

📄 Description

Fixes #3307

Root cause

JavaScriptArtifactPathResolution joined ESM relative imports and HTML script references (including the document base) with posix.join. POSIX normalization collapses // before it applies .., so ./assets//../app.js became app.js. Under URL semantics, which is how both ESM and HTML resolve, .. removes the preceding empty segment, giving assets/app.js. When both files existed, the graph linked the importer and the renderer to the wrong file and still reported resolved with complete coverage. URL references had the opposite problem: they refused every //, even when dot segments cancel it to an exact path.

Change

There is now one owner for URL-path candidates, urlRelativeCandidate(directory, reference):

URL dot-segment rules apply over canonical segments: . is dropped, and .. pops the previous segment even when it is empty.
A remaining empty segment, such as ./assets//app.js, stays unavailable, with the existing "Repeated URL path separators…" limitation. A trailing empty segment is directory form and is handled by the existing exact-file check.
A reference that climbs above the artifact root still produces a .. candidate, so confineCandidate rejects it as before.

ESM imports (module-specifier + import), HTML references (with and without <base href>, including the base itself) and URL references all use it. CommonJS require and runtime entrypoints keep filesystem path semantics. The docs (javascript-artifact-reconstruction.md) state the rule.

Validation

The issue's public CLI repro (index.html, main.mjs, main.cjs; both app.js and assets/app.js exist):

source	main	this PR
`index.html` module script	`app.js` (wrong)	`assets/app.js`
`main.mjs` ESM import	`app.js` (wrong)	`assets/app.js`
`main.cjs` require	`app.js`	`app.js` (unchanged, filesystem semantics)

Each target matches new URL(…) and Node's CommonJS resolver respectively. Output grows from 70,675 to 70,696 B, from the longer resolved paths.

Real applications, CLI on an older main build vs this PR:

Discord app.asar: normalized output identical (108,108,885 B both; 15.2 s vs 14.3 s).
Notion app.asar (--integrity-policy record-and-continue): equivalent once host-budget numbers are erased and hash ordering is ignored (125.8 MB both; 41.3 s vs 38.3 s).

These bundles contain no // references, so nothing changes for them.

Tests:

keeps empty URL path segments significant replaces does not collapse empty URL path segments. Each case is checked against new URL first, then across url-reference, html-reference, ESM import and CommonJS require. ./nested//worker.js stays unresolved for the URL contexts. ./nested//../worker.js resolves to dist/nested/worker.js for the URL contexts and to dist/worker.js for require.
That test fails on main for both cases; I checked by restoring main's resolver.
158 JavaScript/domain test files pass (2,966 tests), along with npm run check:fast and npm run docs:check.

Behavior change to note: URL references such as new URL("./nested//../worker.js", import.meta.url) previously stayed unresolved. They now resolve to the exact URL target, matching the HTML and ESM contexts and the URL parser.

🤖 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/3792 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/15/2026 **Status:** ✅ Merged **Merged:** 10/15/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/3307-url-empty-segments` --- ### 📝 Commits (2) - [`0b972b8`](https://github.com/morluto/rea/commit/0b972b8dbcbc8af3171d3d3aeefdbf3f6d9ba30f) fix(javascript): keep empty URL path segments significant - [`b0d79fd`](https://github.com/morluto/rea/commit/b0d79fdc21d0554a528be66fb9ca5b35ea16923b) fix(javascript): resolve HTML base and reference as one URL path ### 📊 Changes **4 files changed** (+176 additions, -29 deletions) <details> <summary>View changed files</summary> 📝 `docs/javascript-artifact-reconstruction.md` (+4 -0) 📝 `src/application/javascript/JavaScriptArtifactPathResolution.test.ts` (+46 -5) 📝 `src/application/javascript/JavaScriptArtifactPathResolution.ts` (+105 -24) 📝 `src/application/javascript/JavaScriptHtmlBaseResolution.test.ts` (+21 -0) </details> ### 📄 Description Fixes #3307 ## Root cause `JavaScriptArtifactPathResolution` joined ESM relative imports and HTML script references (including the document base) with `posix.join`. POSIX normalization collapses `//` before it applies `..`, so `./assets//../app.js` became `app.js`. Under URL semantics, which is how both ESM and HTML resolve, `..` removes the preceding *empty* segment, giving `assets/app.js`. When both files existed, the graph linked the importer and the renderer to the wrong file and still reported `resolved` with complete coverage. URL references had the opposite problem: they refused every `//`, even when dot segments cancel it to an exact path. ## Change There is now one owner for URL-path candidates, `urlRelativeCandidate(directory, reference)`: - URL dot-segment rules apply over canonical segments: `.` is dropped, and `..` pops the previous segment even when it is empty. - A remaining empty segment, such as `./assets//app.js`, stays `unavailable`, with the existing "Repeated URL path separators…" limitation. A trailing empty segment is directory form and is handled by the existing exact-file check. - A reference that climbs above the artifact root still produces a `..` candidate, so `confineCandidate` rejects it as before. ESM imports (`module-specifier` + `import`), HTML references (with and without `<base href>`, including the base itself) and URL references all use it. CommonJS `require` and runtime entrypoints keep filesystem path semantics. The docs (`javascript-artifact-reconstruction.md`) state the rule. ## Validation **The issue's public CLI repro** (`index.html`, `main.mjs`, `main.cjs`; both `app.js` and `assets/app.js` exist): | source | main | this PR | |---|---|---| | `index.html` module script | `app.js` (wrong) | `assets/app.js` | | `main.mjs` ESM import | `app.js` (wrong) | `assets/app.js` | | `main.cjs` require | `app.js` | `app.js` (unchanged, filesystem semantics) | Each target matches `new URL(…)` and Node's CommonJS resolver respectively. Output grows from 70,675 to 70,696 B, from the longer resolved paths. **Real applications, CLI on an older `main` build vs this PR:** - Discord `app.asar`: normalized output identical (108,108,885 B both; 15.2 s vs 14.3 s). - Notion `app.asar` (`--integrity-policy record-and-continue`): equivalent once host-budget numbers are erased and hash ordering is ignored (125.8 MB both; 41.3 s vs 38.3 s). These bundles contain no `//` references, so nothing changes for them. **Tests:** - `keeps empty URL path segments significant` replaces `does not collapse empty URL path segments`. Each case is checked against `new URL` first, then across `url-reference`, `html-reference`, ESM `import` and CommonJS `require`. `./nested//worker.js` stays unresolved for the URL contexts. `./nested//../worker.js` resolves to `dist/nested/worker.js` for the URL contexts and to `dist/worker.js` for `require`. - That test **fails on `main`** for both cases; I checked by restoring main's resolver. - 158 JavaScript/domain test files pass (2,966 tests), along with `npm run check:fast` and `npm run docs:check`. **Behavior change to note:** URL references such as `new URL("./nested//../worker.js", import.meta.url)` previously stayed unresolved. They now resolve to the exact URL target, matching the HTML and ESM contexts and the URL parser. 🤖 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-31 01:10:19 +00:00