runtime: defer scheme resolution to globalThis.fetch in Image/Audio/Video - #404
Merged
TooTallNate merged 4 commits intoJul 8, 2026
Merged
Conversation
…ideo Allow embedders that install a session-time globalThis.fetch wrapper (to extend the scheme registry beyond http/https/blob/data/file/sdmc/ romfs) to have <img>/<audio>/<video> src loads honor their extended schemes. The engine's Image/Audio/Video src setters currently import the package-local ./fetch/fetch and capture it at module-init time. That is before any embedder installs its globalThis.fetch. As a result, any embedder-registered scheme silently rejects at scheme lookup even though globalThis.fetch could handle it. Move to a call-time globalThis.fetch lookup (a local function that returns globalThis.fetch(input, init) on each call). No behavior change for embedders that don't override globalThis.fetch.
Contributor
|
@natureglass is attempting to deploy a commit to the TooTallNate's Team Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: 02fc808 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
TooTallNate
added a commit
that referenced
this pull request
Jul 6, 2026
## Problem The **Runtime Type Diff** check fails on every fork-originated PR (seen on #404, #405) with: ``` RequestError [HttpError]: Resource not accessible by integration POST /repos/TooTallNate/nx.js/issues/<n>/comments → 403 ``` The type build + diff itself succeeds — only the sticky-comment posting fails. On the `pull_request` event, fork PRs get a **read-only** `GITHUB_TOKEN`; the workflow-level `permissions: pull-requests: write` cannot elevate past that cap. ## Fix Standard two-stage `workflow_run` pattern: - **`type-diff.yml`** (`pull_request`, `contents: read` only): builds base + PR types, diffs, writes the diff to the job summary, uploads it as the `type-diff` artifact. No API writes, so it can never 403. - **`type-diff-comment.yml`** (new, `workflow_run` on "Type Diff" completion, `pull-requests: write` + `actions: read`): downloads the artifact from the triggering run and posts/updates the sticky comment. Works for fork PRs because `workflow_run` workflows execute from the default branch with a base-repo token. ## Security notes The artifact is produced by a workflow that built untrusted fork code, so the comment workflow treats it as attacker-controlled: - PR number is resolved from the `workflow_run` payload / head SHA lookup — never from artifact contents (which could otherwise target an arbitrary PR). - The diff text is embedded using a code fence longer than any backtick run in the content, so it can't escape the fence and inject markdown into a bot-authored comment. - The comment workflow never checks out or executes code from the triggering ref. ## Notes - `workflow_run.pull_requests` is empty for fork PRs, so the PR is resolved via `listPullRequestsAssociatedWithCommit(head_sha)` with an exact `head.sha` match. - The `pull_request` job also writes the diff to `$GITHUB_STEP_SUMMARY`, so the result is visible on the run page even before/without the comment. - No changeset: workflow-only change, no published package affected. - `type-diff-comment.yml` only takes effect once merged to `main` (`workflow_run` workflows run from the default branch). After merge, re-running the check on #404 will use the updated `pull_request` workflow via the refreshed merge ref.
TooTallNate
added a commit
that referenced
this pull request
Jul 6, 2026
Follow-up to #409. The first live fork-PR run (on #404, after syncing it with main) failed in the comment workflow with: ``` ##[error]Unable to resolve PR for head SHA f638034 ``` Root cause: `listPullRequestsAssociatedWithCommit` returns an **empty list when the base repo is queried with a commit that only exists in a fork** — verified directly: - `GET /repos/TooTallNate/nx.js/commits/f638034.../pulls` → `[]` - `GET /repos/natureglass/nx.js_extended/commits/f638034.../pulls` → PR #404 So the fallback failed for exactly the fork PRs this workflow exists to serve (same-repo PRs never reach the fallback, since `workflow_run.pull_requests` is populated for them). ## Fix Query the `workflow_run.head_repository` (the fork) for the commit→PR association, and filter results to PRs that: - target this repo (`base.repo.full_name` match), and - have the run's exact head SHA (`head.sha` or `merge_commit_sha`) The association data comes from GitHub's API and is authoritative — a fork cannot fabricate a PR association pointing at an arbitrary PR, so the comment-targeting security property from #409 is preserved. After merging, re-running the "Type Diff" check on #404 will trigger a fresh comment run using this fixed workflow.
Contributor
📝 Runtime Type Changes✅ No changes to the public TypeScript API surface. |
Drops the copy-pasted late-binding fetch() wrapper from image.ts, audio.ts, and video.ts in favor of calling globalThis.fetch at the callsites. Same call-time resolution semantics (embedder-installed fetch wrappers still win), less duplication. Note a bare `fetch` identifier is not an option: esbuild would rename the bundled fetch declaration to fetch2, breaking def(fetch)'s global registration (which check-def-names.mjs guards against).
Merged
TooTallNate
pushed a commit
that referenced
this pull request
Jul 8, 2026
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ `main` is currently in **pre mode** so this branch has prereleases rather than normal releases. If you want to exit prereleases, run `changeset pre exit` on `main`.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ # Releases ## @nx.js/runtime@1.0.0-beta.6 ### Patch Changes - fix: cross-context canvas font size leakage — re-pin the shared FT_Face char_size at the start of `fillText()`, `strokeText()`, and `measureText()` ([#406](#406)) - feat: `Image`, `Audio`, and `Video` now resolve `globalThis.fetch` at call time, so embedder-installed `fetch` wrappers (e.g. custom URL schemes) are honored for `src` loads ([#404](#404)) - fix: `Video.play()` no longer rejects with `InvalidStateError` when called before `loadedmetadata` — playback is queued and the returned promise resolves once it actually begins, matching `HTMLMediaElement.play()`. A pending `play()` is rejected with `AbortError` by `pause()` or a superseding load, and with `NotSupportedError` on load failure ([#412](#412)) - fix: install WebGL2 `GL_CONSTANTS` with a single bulk `Object.defineProperties()` call per target instead of ~740 sequential `Object.defineProperty()` calls at module scope ([#405](#405)) ## create-nxjs-app@1.0.0-beta.6 ## @nx.js/nro@1.0.0-beta.6 ## @nx.js/nsp@1.0.0-beta.6 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Image.src,Audio.src, andVideo.srcsetters today doimport { fetch } from './fetch/fetch'at module init and capture that fetch closure../fetch/fetch's scheme registry is fixed athttp/https/blob/data/file/sdmc/romfs.Embedders that install a session-time
globalThis.fetchwrapper to extend the scheme registry (e.g. a customapp://scheme handler, or a resource-loader that resolves paths against an in-memory package) will find that<img src="app://foo.png">still rejects at scheme lookup — theImage.srcsetter is calling the pre-wrapper./fetch/fetch, not the embedder'sglobalThis.fetch.Move the setter's fetch reference to a call-time
globalThis.fetchlookup. Each of the three modules gets a local:which replaces the previous
import { fetch } from './fetch/fetch'. The call sites in the setters are unchanged (fetch(url).then(...)).Behavior
globalThis.fetch— the engine's global fetch is./fetch/fetch's exported fetch by default, so the delegation chain terminates at the same implementation.globalThis.fetchnow have their wrapper honored onImage.src/Audio.src/Video.srcassignments.Gotcha
The lookup MUST be call-time, not import-time. An
import { fetch } from './fetch/fetch'line captures the export at module init — before any embedder installs its wrapper. Replacing it withconst fetch = globalThis.fetch;at module top-level would freeze the pre-wrapper fetch and reintroduce the bug in code that looks like it should work. The function-body form guarantees the lookup happens per-call.Diff summary
Each file: replaces one
import { fetch } from './fetch/fetch';line with a ~12-line local function definition + explanatory comment.