Repository navigation
ci: cut a PR push from ~15 min of CI to ~2.5 - #8101
Merged
Merged
Conversation
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (122)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No installed development build can load this pull request as an update: its native fingerprint is |
- Drop the desktop Build job. In ~2,000 runs it never failed on its own except two out-of-memory exits; it spent ~10 min packaging installers. - Delete the slowest tests (real-socket and CLI timeouts, the generated adversarial/bug-hunt suites) and run host-service tests with --parallel. - TypeScript 7 for typecheck. The three desktop files that use the compiler API import @typescript/typescript6. - Bun 1.4.2, with a regenerated lockfile. Fixes it needed: zod pins to 4.6.5, metro patched at every resolved version (Expo pins its own), removeDirectoryTree also recovers on ENOTEMPTY, and tests restore the native AbortController/AbortSignal. - CLI: PRs build linux-x64 only, and only when a CLI dependency changed; macOS installs skip lifecycle scripts and share the bun cache. - Marketing: remove generateStaticParams. Every route is dynamic (the nav reads the session), so the build tried and failed to prerender 3,171 pages; the route table is unchanged. - Vercel deploys: no second bun install before `vercel build`, and no Sentry sourcemap upload on previews. - ci.yml takes a runner input; ci-blacksmith.yml runs the same jobs on Blacksmith runners (not required) so we can compare speed and cost.
saddlepaddle
force-pushed
the
saddlepaddle/ci-speedups
branch
from
October 3, 2026 03:43
5cf22c7 to
3ed4e31
Compare
Lingui finds sources with node:fs globSync and passes `exclude` through. A bare `**/*.test.*` never matches the absolute paths that globSync returns for absolute include patterns. Bun 1.3 matched it anyway; Bun 1.4.2 and Node do not, so extraction picked up a <Trans id="greeting" /> from a marketing test and check:i18n failed for every locale. Anchoring the patterns at the repo root works on all three runtimes. Also moves the Lingui packages to 6.8.0 so @lingui/conf has one version (swc-plugin stays at 6.6.0: it is built against Next's SWC). ci-blacksmith.yml grants pull-requests: read, which the nested cli-changes job in ci.yml asks for.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Test files shared one module registry, so a mock.module in one file replaced that module for every file after it. Which tests that broke depended on file order, and Blacksmith's runners order files differently from GitHub's: the CLI's notify tests, trpc's user tests and the API's Linear webhook tests each failed there for this reason. --isolate gives each file a fresh global object and module registry. The whole suite goes from 92 s to 101 s locally.
With each test file isolated, the three trpc files that import @superset/db/client without mocking it construct the real client, which throws without DATABASE_URL. They passed locally because the repo's gitignored .env supplies one; CI has none.
…estore Trying both on previews to measure them. Each deploy job spent 23-31 s restoring a 1.2 GB cache for the whole monorepo and 11-12 s on a global npm install of the CLI before its build started.
Boot twice spent ~3 of its 4 minutes rebuilding the image from nothing; the apt layer alone is 101 s and never depends on our code. A local build now reads and writes a layer cache in GitHub's container registry when SANDBOX_IMAGE_CACHE_REF is set. PRs from forks cannot write packages, so they build without it.
Turbopack writes a sourcemap for every server chunk: 472 files and about 100 MB of marketing's 149 MB server output. Sentry is their only reader. Preview builds have no Sentry token, so the maps were built, uploaded to Vercel with the deployment and never used. Production keeps them.
…token" This reverts commit 141ba62.
The blog and changelog OG image routes read from public/ by a path built at request time, so Next's tracer copied the whole folder, about 47 MB, into the server functions on top of serving it as static files. Those routes only read team avatars, changelog covers, the logo and the font. Excluding video, GIFs and the folders they never touch takes the copy from 49.5 MB to 12.2 MB.
Every marketing route rendered per request because the layout's CTAButtons reads the session through headers(). With cacheComponents on and a Suspense boundary around the buttons, pages are prerendered and only the buttons stream in at request time. - blog, changelog, compare, legal, themes and team prerender one sample per locale; other slugs are generated on first visit and cached. Unknown slugs still return 404. - leaderboard, stats, starchart, fight, the-production-run and user/[handle] stay per request. Their `revalidate` exports never took effect and cacheComponents rejects them, so they are gone; profile and fighter lookups are cached for five minutes instead. - The sitemap caches its handle list, so it is static again when the API is reachable at build and per request when it is not. The build never depends on the API. - CTAButtons reads headers() outside its try block, which swallowed the signal Next uses to stop a prerender. An unknown top-level URL is treated as a profile handle. It now returns 200 with the not-found page and a noindex tag instead of 404, because the shell is sent before the profile lookup runs.
Prerendering every page in all 17 locales took the Marketing preview job from about 2 minutes to 4m35s: 740 pages on 3 workers (64 s) and a 305 MB upload. Only the default locale is prerendered now, 100 pages; other locales are generated on first visit and cached, the same way unlisted blog slugs already are. A locale that is not prerendered goes through Next's static render on first visit, where the profile page's `new Date()` is rejected, so the page now declares itself request-time with connection().
Test is the slowest required check at about 2m25s on 4 vCPU. The turbo test step is 79 s of that and its longest task, host-service, already runs with --parallel, so it scales with cores.
They need Node's test runner and real PTYs, so they cannot join the bun test process, but nothing made them wait for it. They are now a turbo task in the same invocation as the bun suites instead of a 40 s step after them.
The ghcr registry cache saved about 30 s of a 4 minute job: every layer was a hit, but pulling them back and loading the 1.7 GB image took 96 s. Blacksmith's builder keeps the layers on a disk attached to the runner, so there is no cache to pull or push. This replaces the registry cache and its code in image.ts.
The root test script caps turbo at two tasks, which suited 2-core runners. On 8 vCPU the terminal node-tests took one of the two slots for 40 s.
setup-docker-builder v2 requires cache-key; without it the action fell back to an uncached local builder.
The CLI build was gated on a hand-kept list of the paths it depends on, and previews skipped desktop-only PRs. Both run in under three minutes now, so they run always: a stale path list or a skipped deploy can let a broken build merge.
The rules come from what this branch had to remove or fix: tests that slept on real timeouts, adversarial suites, mock.module leaks that only failed in another file order, and a flaky suite nobody could explain.
- loadProfile cached a rate-limited answer for five minutes, so one refused read showed "unavailable" to everyone on that instance. It now caches that answer for seconds and a real profile for five minutes. - fetchPublicHandles returned [] on any error and the sitemap cached it. It now throws, the cached read fails uncached, and the sitemap falls back to no profile URLs for that request only. - Two comments still described ISR pages that are request-time now.
Each preview push saved five caches keyed by its merge SHA, about 3.9 GB, visible only to that PR. One PR took the repo past its 10 GB Actions cache limit, and a new PR always built cold. Production deploys on main now write the cache and previews only restore it, so every PR starts from main's latest build and saves nothing. Docs has no production deploy in this workflow, so its cache step is gone.
Experiment: a cold web deploy spends 29 s in bun install and 66 s in the Next compile on GitHub's 4-core runner.
Deploy Web went from 2m46s to 1m41s with a cold Next cache: bun install inside vercel build 29 s -> 3 s with the dependency cache restored, and the compile 66 s -> 34 s on the faster cores. The other four deploys get the same runner and cache.
Previews moved to Blacksmith runners, whose actions/cache is a separate store from GitHub's. Production has to write the Next build cache to the same store for previews to read it, and it gets the same faster install and compile.
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| runs-on: blacksmith-4vcpu-ubuntu-2404 | ||
| environment: preview | ||
| needs: deploy-database | ||
| needs: create-database |
Contributor
There was a problem hiding this comment.
Knowledge Base Used: Make Production Migration Failures Visible
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/deploy-preview.yml
Line: 97
Comment:
**Previews bypass migration verification** When a PR adds a migration, the preview API can be published as soon as the Neon branch is created, before the migration runs or its result is verified. If the migration fails, the preview links remain live against the old schema, and database-backed requests can fail. Marking the database as failed in the PR comment does not stop those deployments.
**Knowledge Base Used:** [Make Production Migration Failures Visible](https://app.greptile.com/personal-org-6810/-/custom-context/knowledge-base/superset-sh/superset/-/reverts/incident-mitigation_7672-20260919-migration-reported-success-1bcf64a.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| @@ -149,7 +165,6 @@ jobs: | |||
| GH_CLIENT_ID: ${{ secrets.GH_CLIENT_ID }} | |||
| GH_CLIENT_SECRET: ${{ secrets.GH_CLIENT_SECRET }} | |||
| NEXT_PUBLIC_SENTRY_DSN_API: ${{ secrets.NEXT_PUBLIC_SENTRY_DSN_API }} | |||
Contributor
There was a problem hiding this comment.
SENTRY_AUTH_TOKEN, but the Next.js Sentry configuration reads that variable to authenticate source-map uploads. Preview errors will lose source-mapped stack traces, making failures harder to diagnose. The token is also removed from the other preview app builds.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/deploy-preview.yml
Line: 167
Comment:
**Preview source maps lose authentication** The preview build no longer receives `SENTRY_AUTH_TOKEN`, but the Next.js Sentry configuration reads that variable to authenticate source-map uploads. Preview errors will lose source-mapped stack traces, making failures harder to diagnose. The token is also removed from the other preview app builds.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| expect(result.elapsedMs).toBeGreaterThanOrEqual(25_000); | ||
| }, 45_000); | ||
|
|
||
| test("real sockets recover from a rejected upgrade and a slow retry", async () => { |
Contributor
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/tunnel/tunnel-client.integration.test.ts
Line: 107
Comment:
**Long handshake coverage removed** Removing the long-handshake and stalled-upgrade socket tests leaves only a 4.5-second delayed socket case. That case cannot catch a regression that cancels a valid 25-second handshake too early or forwards a request after an upgrade stalls. Short tests with configurable time budgets would retain coverage without waiting for real timeouts.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This was referenced Oct 4, 2026
This was referenced Oct 19, 2026
This branch was previously deployed
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.
Why
A PR push took about 15 minutes of CI (median of 40 runs): Build 14.4 min, Test 8.6 min, Typecheck 6.4 min, and preview deploys about 7 min. It now takes about 2.5 minutes, with the required checks done in about 1.5.
Timings on the last run
What changes
CI
mainruleset.mainbuild all four targets.Toolchain
@typescript/typescript6.bun test --isolate.Tests
adversarialandbug-hunt-2suites, andgit-watcher-lazy-registration.integration.test.ts, which died with exit 137 in 30 of 32 flaky runs for a reason we did not find.mock.module; CLI tests drive the real notify function.AGENTS.mdhas a Tests section with the rules this came from.Deploys
create-databasejob and no longer wait for migrations.Marketing
cacheComponentsis on. Pages are prerendered for the default locale and the session buttons stream in; other locales and unlisted slugs render on first visit and are cached.leaderboard,stats,starchart,fight,the-production-runand profile pages stay per request.public/video, GIFs and unused folders are excluded from the server functions.Behaviour changes to know about
noindextag, where they returned 404. They are treated as profile handles, and the page shell is sent before the lookup. Unknown blog, changelog, compare, legal, team and theme URLs still return 404.removeDirectoryTreealso recovers onENOTEMPTY;LocalGitCredentialProviderignoresEPIPEon git's stdin.Verified
prebuild(electron-vite compile, native modules, runtime validation) passes locally on macOS arm64.Not verified
--ignore-scripts; they first run on push tomain.llms.txt,robots.txt,schemamap.xml,.well-known) against production.expo exportwith the metro patches.After merge
mainruleset.Summary by CodeRabbit