Repository navigation
[No QA] Migrate the PR-testing (now AdHoc) Cloudflare Worker to App - #104801
Conversation
Deploy each PR Worker and its matching assets together from one App SHA. Remove S3 web uploads and keep trusted deployment tooling on main. Preserve queued push builds while canceling superseded manual builds.
Use named PR Previews under the existing pr-testing-expensify Worker. Publish branch code and assets without per-PR Workers or zone routes. Keep artifact validation and code/assets identity checks. Remove unrelated generated action bundle changes from this migration.
roryabraham
changed the title
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. |
|
@madmax330 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| default: 'false' | ||
|
|
||
| concurrency: | ||
| group: adhoc-${{ github.event_name }}-${{ format('app-{0}-mobile-{1}', inputs.APP_PR_NUMBER || inputs.APP_REF, inputs.MOBILE_EXPENSIFY_PR || inputs.MOBILE_EXPENSIFY_REF) }} |
There was a problem hiding this comment.
don't we need platform in this key as well?
There was a problem hiding this comment.
We could add it but I don't think we need to. I was thinking we only manage concurrency on a per-PR basis, rather than per-platform-per-PR. Basically, if you start a web-only AdHoc build on a PR, then before it finishes start an all-platforms AdHoc build, it's expected that the latter would interrupt the former.
| "workspaces": [ | ||
| "server/victory-chart-renderer" | ||
| "server/victory-chart-renderer", | ||
| "web/cloudflare-worker" |
There was a problem hiding this comment.
are you sure about putting this here?
this is going to install all the dev dependencies in every CI run too, right?
There was a problem hiding this comment.
I think so - the wrangler dependency is used in scripts which is under the parent project. I think workspaces is the right tool for the job.
That said, you make a good point about unnecessarily bloating npm ci in unrelated CI jobs. So in d5b56e9, I updated our setupNode composite action to accept a workspaces input. Only the workspaces explicitly passed in are installed, and it defaults to an empty array.
9ddb94d
|
A preview of your ExpensifyHelp changes have been deployed to https://bb9eef52.helpdot.pages.dev ⚡️ |
roryabraham
changed the title
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
@roryabraham looks like this was merged without a test passing. These checks had not passed on 42c0b2b when it was merged:
Please add a note explaining why this was done and remove the |
|
didn't look like tests were failing? |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
Post-merge verification of the automatic push-to-main build for merge commit Web result: passed. The automatic web build and Preview publication both succeeded. The full CI run also finished green; the interactive verification below covers web only. Tested https://104801.adhoc.expensify.com:
The only browser console error was the previously observed |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.5.37-0 🚀
|
|
No help site update needed — this PR changes developer ad-hoc build hosting, not customer-facing features or workflows, and makes no existing help article sentence incorrect. |
Explanation of Change
Publish new ad-hoc web builds as named Previews under the dedicated
new-expensify-adhocWorker athttps://<PR number>.adhoc.expensify.com, with Worker code and web assets built from the same commit.This uses the shared app Worker runtime and existing deployment token. Builds report success only after verifying that the Preview serves the requested code and assets. There are no separate Workers or routes to manage for each PR.
Note: Cloudflare evicts the least recently deployed Preview when the limit of 500 is reached.
Rollout
The new
adhoc.expensify.comdomain is configured for Previews. The companion cleanup PR removes deprecated source and deployment configuration from that repository; it can be merged without deleting the live deployment. Keep the existingpr-testing-expensifyWorker,*.pr-testing.expensify.com/*route and S3 assets so historical build links continue working. Retiring those live resources is a separate later cleanup.The Web API CORS change and Nginx/Salt allowlist change are deployed on staging. The automatic post-merge build and web verification passed: the Preview serves the merge commit, and credentialed API/fingerprinting requests succeed without browser CORS errors. Authenticated and SAML sign-in remain unverified.
Merged main and refreshed the local install. All 66 Worker tests, 48 tooling tests, full root/Worker typechecks, branch lint/spelling, formatting and workflow validation pass. The live hosting experiment passed, including isolated builds, cancellation/rebuild, failed-build preservation, closed-PR retention and preservation of an existing S3-backed build. Live bundles were uncompressed; Brotli was not exercised.
Fixed Issues
For https://github.com/Expensify/Expensify/issues/688126
PROPOSAL: N/A
Tests
Steps 1–4 record the earlier
pr-testing-expensifyexperiment; its temporary route removal is not needed for the new setup. Steps 5–10 record the rerun onnew-expensify-adhoc/adhoc.expensify.com, with the legacy route left untouched.X-Preview-Experiment: A-v1response header inweb/cloudflare-worker/src/index.tsand anA-v1meta tag inweb/index.html. In PR B, added the same markers withB-v1. These let us check the deployed Worker code and HTML assets independently. B stayed unchanged as the control throughout the experiment.testBuild.ymlbuilds for both PRs usinggh workflow run testBuild.yml --repo Expensify/App --ref rory/migrate-pr-testing-worker -f APP_PULL_REQUEST_URL=https://github.com/Expensify/App/pull/105691 -f REVIEWED_CODE=true -f WEB=true -f IOS=false -f ANDROID=false, substituting B's URL for its build. Both published using the existing deployment token. Their final HTTP checks initially failed because the legacy wildcard route still sent requests to the S3 proxy.*.pr-testing.expensify.com/*route. Rangh run rerun 41903167252 --repo Expensify/App --failedandgh run rerun 41903166963 --repo Expensify/App --failed. Confirmed A and B both passed on attempt 2./on both hosts (105691.pr-testing.expensify.comand105692.pr-testing.expensify.com) and checked for HTTP 200, the exact expected Worker response header (A-v1orB-v1) and the matching HTML meta-tag value.A-adhoc-v1/B-adhoc-v1. Repeated the web-only dispatch command from step 2. Both A and B published successfully to the correctednew-expensify-adhocWorker with the updated token. Requested each numericadhoc.expensify.comhost and verified its exact header/meta marker and matching SHA/run/attempt JSON at/__expensify_previewand/preview-build.json./twice and verified different CSP nonces, each matching the HTML script nonce; checked HTMLno-cacheand absence ofETag/Last-Modified. Requested/r/12345as a navigation and verified the marked app shell; requested a nonexistent bundle and/merged-source-map.js.mapand verified 404s. Checked an HTML-listed bundle's immutable caching and identical bytes forbrandidentityrequests, and checked that/service-worker.jswas not immutable. The bundle responses had no content encoding: ad-hoc builds explicitly skip precompressed Brotli, so this did not test live Brotli compression.A-adhoc-v2, pushed it, and dispatched twice after the first entered the build workflow. Confirmed the superseded run was canceled and the replacement succeeded. Repeated the HTTP checks: A served its new marker and SHA/run/attempt; B retained its original marker and identity.Intentional E2E build failureand dispatched another build. Confirmed that exact error in the failed build and that publication was skipped. Repeated the HTTP checks: A still served its last successful v2 build and B was unchanged. Restored the normal hook in a new pushed commit.106137.pr-testing.expensify.com: its HTML returned 200 and its bundle SHA-256 matched the saved pre-experiment value. No legacy route or Worker was changed./home, their fixture meta tags and the phone/email sign-in forms. Investigated browser CORS errors by requesting staging's/api/Ping?accountID=unknownwith matchingOriginandRefererheaders: the new ad-hoc origin received no CORS headers, while the old PR-testing and staging app origins received the matchingAccess-Control-Allow-Originand credentials headers. These API and fingerprinting CORS issues were subsequently fixed on staging and retested successfully; authenticated/SAML sign-in was not performed.My agent tested a zillion other things, but this was really the core validation ^.
Offline tests
None.
QA Steps
None.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari