theme: overlap checksum fetch with file system setup - #8256
Conversation
Start mounting the local theme file system before fetching remote checksums during theme push, then wait for both operations together. This matches the existing pattern used by theme pull and lets the two independent operations overlap without changing behavior. Update push unit tests to mock the file system and checksum fetch.
There was a problem hiding this comment.
Pull request overview
This PR improves theme push performance by overlapping two independent operations: mounting/initializing the local theme file system and fetching remote theme checksums. It aligns theme push with the existing concurrency pattern already used by theme pull.
Changes:
- Start mounting the local theme file system before fetching remote checksums during
theme push. - Wait for both
fetchChecksums(...)andthemeFileSystem.ready()viaPromise.all(...)before starting the upload. - Update unit test setup to mock
mountThemeFileSystem()with a filesystem that supportsready()and to mockfetchChecksums().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/theme/src/cli/services/push.ts | Overlaps checksum fetch with local FS initialization by awaiting both together, matching theme pull’s pattern. |
| packages/theme/src/cli/services/push.test.ts | Updates mocks to cover the newly awaited fetchChecksums + themeFileSystem.ready() behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I have signed the CLA! |
EvilGenius13
left a comment
There was a problem hiding this comment.
Hi @Arjunmehta312, thank you for opening this PR!
Could you please confirm that you tested these changes locally? A screenshot would be an easy way to show everything is still working! Thanks 😄
| filters: options, | ||
| listing: options.listing, | ||
| }) | ||
| const [themeChecksums] = await Promise.all([fetchChecksums(theme.id, session), themeFileSystem.ready()]) |
There was a problem hiding this comment.
Is there any reason we couldn't just literally flip the order of the values and keep this as const themeChecksums = await fetchChecksums(theme.id, session)?
There was a problem hiding this comment.
Good question. I considered that approach as well. The reason I used Promise.all is that mountThemeFileSystem() starts its asynchronous setup immediately, and ready() exposes that promise. Waiting on fetchChecksums() first leaves the mount promise without a rejection handler until later. Using Promise.all attaches handlers to both operations immediately while still allowing them to run concurrently. It also matches the existing pattern used in theme pull.
If I'm mistaken about that behavior, I'm happy to change it.
|
Hi @EvilGenius13 |
Summary
Start mounting the local theme file system before fetching remote checksums during
theme push, then wait for both operations together.This matches the existing pattern used by
theme pulland allows the two independent operations to overlap while preserving the existing behavior.Testing