refactor(web): rename packages/documents to packages/web, fix the broken Pages deploy - #918
Merged
Merged
Conversation
packages/documents (the private, unpublished web UI) and packages/documents.js (a different, published package -- the conversion engine) were one character apart and easy to confuse. Renaming the web UI's own directory and npm package name to "web" removes that collision. Updates every reference to the old path and package name across the workspace: package.json (name/repository.directory/homepage), the README badge, ci.yml's deploy-site job, the root README's package table and turbo examples, knip.config.ts's workspace-scoped entry, turbo.json's five package-scoped task keys, SECURITY.md, document-compute.js's README, and the two issue templates' package dropdowns. pnpm-lock.yaml is regenerated via a real pnpm install rather than hand-edited; alongside the renamed importer key, that install also picked up picomatch and fdir refreshing to their current allowed versions -- an ordinary side effect of a full install, not something worth fighting to exclude from a freshly-regenerated lockfile. Also seeds a web@2.3.1 tag on the same commit documents@2.3.1 already tags: the release orchestrator finds a package's last release by its newest matching tag, and without this the very next release would see no prior web@ tag at all, compute a first release, and write 1.0.0 into packages/web/package.json -- a real version downgrade from today's 2.3.2 and a restarted changelog, even though nothing about the package's own release history actually changed.
vite.config.ts hardcoded the Pages base path as "/documents/", the directory's old name. Every asset the build emitted therefore linked to a path GitHub Pages was no longer serving, and the deployed site at exadev.github.io/documents.js loaded a blank page: the root HTML returned 200, but every JS/CSS/manifest URL inside it 404'd. Deriving the path segment from GITHUB_REPOSITORY (owner/repo, set automatically by every Actions run), falling back to the name parsed from the git remote for a context where that variable is unset, removes the class of bug entirely -- a repo rename can no longer leave a stale literal behind for a future build to silently ship. parseGitHubRepoUrl is renamed to parseGitHubRepo and now returns both the repo name and the full URL, since __APP_REPO_URL__ already needed the URL and the base path now needs the bare name from the same parse.
deploy-site's own success only proves actions/deploy-pages accepted the upload -- it says nothing about whether the page GitHub Pages now serves actually works. The base-path bug this branch fixes shipped past every existing check for exactly that reason: the root HTML returned 200 while every asset it referenced 404'd. The new job fetches the live deployed HTML, extracts every script, stylesheet, icon, and manifest URL it references, and fetches each one individually. Named distinctly from the pre-existing test-smoke job (that one exercises each package's own built dist/ as a local artifact; this one exercises the live Pages deployment over the network) so a failure in either is unambiguous about which broke.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
1 participant
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.
Summary
packages/documents (the private, unpublished web UI) and packages/documents.js (the published conversion-engine package) were one character apart and easy to confuse. This renames the web UI's directory and npm package name to web, and fixes a real production bug it surfaced along the way: exadev.github.io/documents.js/ has been serving a blank page, because vite.config.ts hardcoded the Pages base path as /documents/ -- the directory's old name -- so every JS/CSS/manifest asset the build emitted 404'd.
Three commits:
Verified the fix end-to-end: building locally with
CI=1 GITHUB_REPOSITORY=ExaDev/documents.jsnow emits/documents.js/...asset paths in dist/index.html, matching what Pages actually serves this repo at.One deliberate non-change: the PWA manifest's app name/short_name still say "documents" -- that's the product name shown to end users installing the PWA, not the package name, and renaming the package doesn't imply renaming the product.
Test plan
pnpm exec eslint . --cache --max-warnings 0-- whole workspace, cleanpnpm exec turbo run _typecheck _build _test --filter=web-- cleanpnpm exec turbo run _knip _typecheck --filter=//-- cleangit grep -n "packages/documents\b"(excludingpackages/documents.js) -- zero remaining referencesCI=1 GITHUB_REPOSITORY=ExaDev/documents.js-- confirmed every emitted asset path is/documents.js/...ci.ymlverify-deployed-sitejob against a real deploy once merged to main)