Skip to content

feat(vite-plugin-cloudflare): add tunnel shortcut hint#13922

Merged
edmundhung merged 2 commits into
mainfrom
edmundhung/vite-tunnel-shortcuts
May 15, 2026
Merged

feat(vite-plugin-cloudflare): add tunnel shortcut hint#13922
edmundhung merged 2 commits into
mainfrom
edmundhung/vite-tunnel-shortcuts

Conversation

@edmundhung
Copy link
Copy Markdown
Member

@edmundhung edmundhung commented May 14, 2026

Fixes n/a.

This diff looks huge mainly as I refactored the shortcuts plugin to patch bindCLIShortcuts and register custom shortcuts all together instead of doing it individually, so that the order of shortcuts and hints logged are more predictable.

You can review this per commit.


A picture of a cute animal (not mandatory, but encouraged)


Open in Devin Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 14, 2026

🦋 Changeset detected

Latest commit: 23a9b7d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudflare/vite-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk May 14, 2026
@workers-devprod workers-devprod requested review from a team and petebacondarwin and removed request for a team May 14, 2026 20:09
@workers-devprod
Copy link
Copy Markdown
Contributor

workers-devprod commented May 14, 2026

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

✅ All changesets look good

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 14, 2026

Now let me check the "display tunnel shortcut hint" test more closely. The isTunnelOpen mock returns false first, then true. I need to understand which calls hit which mock return values:

The test at line 401 mocks isTunnelOpen with .mockReturnValueOnce(false).mockReturnValueOnce(true). Then:

  1. addShortcuts(mockServer, mockContext) - this doesn't call isTunnelOpen
  2. mockServer.bindCLIShortcuts() - no print: true, so no call to isTunnelOpen
  3. mockServer.bindCLIShortcuts({ print: true }) - this calls isTunnelOpen() → returns false → "start tunnel"
  4. mockServer.bindCLIShortcuts({ print: true }) - this calls isTunnelOpen() → returns true → "close tunnel"

That logic checks out.

Now let me look at a potential issue: when isTunnelOpen() is called in the hint printing code and tunnelManager hasn't been initialized yet (i.e., tunnelManager is undefined), isTunnelOpen() returns false, which is correct — it would show "start tunnel".

Let me check if TunnelManager.isOpen() has a side effect issue. Looking at lines 87-102: isOpen() can call this.dispose() as a side effect when the tunnel has expired. This is called from the shortcut hint printing path (just to display text). That means merely printing the shortcut hints could dispose the tunnel if it happens to be expired. This seems like it could be surprising behavior, but it's pre-existing in isOpen() and not introduced by this PR.

Let me also verify the test for "prints shortcut hints in registration order" at line 123. It mocks isTunnelOpen to return false, sets up the context, calls addShortcuts, and then checks the printed output. The test expects all three hints (b, e, t) to be printed. This is correct.

One thing I notice: the "prints shortcut hints in registration order" test uses vi.spyOn(tunnelPlugin, "isTunnelOpen").mockReturnValue(false) but the "display tunnel shortcut hint" test also needs this mock. However, looking at the test flow, beforeEach creates a fresh mockServer each time, so the bindCLIShortcuts mock is reset. But the isTunnelOpen spy from one test could leak into another if not cleaned up. Looking at beforeAll — there's no vi.restoreAllMocks() in beforeEach. However, vitest's shared config has restoreMocks: true, which should handle this.

Let me verify one more thing — in test "prints shortcut hints in registration order" (line 123), the mock vi.spyOn(tunnelPlugin, "isTunnelOpen").mockReturnValue(false) is set before addShortcuts is called. But isTunnelOpen is only invoked when bindCLIShortcuts({ print: true }) is called, which happens later. This is fine.

Now let me think about the actual behavioral change and potential issues:

  1. Previously, bindCLIShortcuts was called 3 separate times (once per add*Shortcut function), each with its own subset of shortcuts. Only the last call's shortcuts would actually be registered since Vite's bindCLIShortcuts replaces the previous shortcuts.

Wait, let me re-examine the old code more carefully. Each add*Shortcut function was wrapping server.bindCLIShortcuts and then calling it. So the chain was:

  • addBindingsShortcut wraps bindCLIShortcuts → calls bindCLIShortcuts({customShortcuts: [binding]})
  • addExplorerShortcut wraps (the already-wrapped) bindCLIShortcuts → calls it with {customShortcuts: [explorer]}
  • addTunnelShortcut wraps (the double-wrapped) → calls it with {customShortcuts: [tunnel, extend]}

The new code consolidates all of this into a single function with a single wrapper and a single call with all shortcuts. This is a much cleaner approach.

Let me check if there's an issue with the printBindingsShortcut action closure. Previously, workerConfigs was captured at function definition time. Now (line 55), ctx.allWorkerConfigs is read inside the action callback, which means it reads the current value when the shortcut is pressed. This is actually an improvement — it means if worker configs change (e.g., after a server restart), the shortcut will show the latest bindings.

The hint for bindings is also now conditional on ctx.allWorkerConfigs.length > 0 (line 127), but the b shortcut is always registered in the customShortcuts array regardless. Previously, addBindingsShortcut would return early if workerConfigs.length === 0, meaning the shortcut wouldn't be registered at all. Now the shortcut is always registered but the hint won't show. This is a minor behavioral change — pressing b with zero worker configs will now attempt to run the action (which will just be a no-op loop). This is harmless.

Overall, this PR is a clean refactor that consolidates three separate shortcut registration functions into one, adds the tunnel hint, and adds proper tests. The code is correct and the tests are comprehensive.

LGTM

github run

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 14, 2026

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@13922

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@13922

miniflare

npm i https://pkg.pr.new/miniflare@13922

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@13922

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@13922

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@13922

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13922

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@13922

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@13922

wrangler

npm i https://pkg.pr.new/wrangler@13922

commit: 23a9b7d

@edmundhung edmundhung force-pushed the edmundhung/vite-tunnel-shortcuts branch from 36bbf62 to 94c717a Compare May 14, 2026 20:24
@edmundhung edmundhung force-pushed the edmundhung/vite-tunnel-shortcuts branch from 94c717a to 23a9b7d Compare May 14, 2026 20:36
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Nice refactoring!

Copy link
Copy Markdown
Contributor

@workers-devprod workers-devprod left a comment

Choose a reason for hiding this comment

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

Codeowners reviews satisfied

@github-project-automation github-project-automation Bot moved this from Untriaged to Approved in workers-sdk May 15, 2026
@edmundhung edmundhung merged commit 23800f8 into main May 15, 2026
55 of 56 checks passed
@edmundhung edmundhung deleted the edmundhung/vite-tunnel-shortcuts branch May 15, 2026 09:44
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants