Skip to content

fix(cli): include external srcset images in project archives - #6335

Merged
miguel-heygen merged 2 commits into
heygen-com:mainfrom
user-github-me:fix/cli-publish-external-srcset
Oct 25, 2026
Merged

miguel-heygen merged 2 commits into
heygen-com:mainfrom
user-github-me:fix/cli-publish-external-srcset

Conversation

@user-github-me

Copy link
Copy Markdown
Contributor

Publishing a project with <img srcset="../shared/retina.png 2x"> leaves out the image and retains its outside-project path. Include local srcset candidates in the archive and rewrite their URLs, including nested HTML and <picture><source> elements.

Reuse the bundler's srcset walker with the existing publishing resolver, deduplication and disk staging. Preserve descriptors, inline data and remote URLs, and retain current percent-encoded filename handling. Source files stay unchanged; HTML with no localized candidate keeps its original bytes.

This is the separate follow-up requested in the review of #5309.

Validation:

  • Six archive/staging cases pass, including commas and an encoded filename containing spaces, Unicode, # and %. Five fail on unchanged main 42b84632; the unchanged-HTML control already passes.
  • All 122 publishing-related CLI tests and 203 bundler tests pass. A Node check using the built core export confirms the missing images are included with correct root/nested references and unchanged source HTML.
  • Full workspace build, core/runtime and CLI typechecks, lint, formatting, test reachability, normal commit hooks and both comment checks pass. The sharing guide passes Mint validation and broken-link checks.

@mintlify

mintlify Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit e5b1017.

Approve preview

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for picking this up as the follow-up from #5309. The change is small and well placed.

What works well:

  • Moving the bundler's existing srcset walker into packages/core/src/compiler/srcset.ts keeps one spec-correct parser instead of adding a second. Its behaviour is unchanged. I checked it against trailing commas, data: candidates, commas inside parenthesised descriptors and a.png,b.png 2x (one URL, per the spec), and all come out right.
  • rewriteHtmlSrcsets (publishProject.ts:545) uses the same resolver, dedup map and relative-path logic as src/href/poster. A file referenced by both src and srcset is archived once. Because targets go through encodeUrlPath, a name containing a comma or a space cannot break the srcset syntax.
  • The tests cover the cases that matter: nested entries, <picture><source>, encoded names, the on-disk staging path, and a control showing that untouched HTML keeps its original bytes.

CI: the required Test check is red only because the Test (runtime) shard hit its 10-minute job timeout while the CLI suite was still running. Every test that ran passed, including the six new ones and the existing publishProject suite. Recent runs finish that shard in about 5 minutes, so this looks like a slow runner. A re-run should clear it.

Optional follow-up, not for this PR: packages/cli/src/cloud/offload.ts:894 still splits srcset on , and skips any srcset that contains data:. Now that the spec walker is exported, cloud-render offload could use it too.

Optional nit: CSS_URL_RE and isPathInside are also exported from the ./compiler subpath (compiler/index.ts:114). If you want rewriteSrcsetUrls there too for consistency, it is a one-line addition. Nothing needs it today.

Happy to approve once the runtime shard is green.

Verdict: COMMENT
Reasoning: The code is correct and reuses the existing parser, but the required Test check is red from a runtime-shard timeout that needs a re-run before approval.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The runtime shard that timed out has re-run and every check is now green at e5b1017e, so I'm approving. The change is as I reviewed it at c3bd3ced; the only newer commit merges main.

The two optional follow-ups from my last review (cloud-render offload still splitting srcset on commas, and exporting rewriteSrcsetUrls from the ./compiler subpath) can wait for a later PR.

Verdict: APPROVE
Reasoning: The code is correct and reuses the existing srcset parser, and all checks pass at this head.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 25, 2026
@jrusso1020

Copy link
Copy Markdown
Collaborator

Approved and queued. One follow-up: external assets (src, href, srcset, url()) can resolve to any file outside the project, which then gets uploaded by publish. Could you send a small follow-up that limits those to media file types? A project referencing something like ../../.aws/credentials shouldn't end up in the archive.

Merged via the queue into heygen-com:main with commit 707c150 Oct 25, 2026
84 checks passed
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.

3 participants