feat(preview): adapt browser HTTP over runner tunnels#993
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesPreview URL and network tunnel flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GetProxyTarget
participant guestPortTransport
participant Runner
Client->>GetProxyTarget: Request preview host and path
GetProxyTarget->>GetProxyTarget: Decode box ID and select target port
GetProxyTarget->>guestPortTransport: Select guest-port transport
guestPortTransport->>Runner: Establish HTTP CONNECT tunnel
Runner-->>guestPortTransport: Return guest connection
guestPortTransport-->>Client: Relay the guest request
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
G4614
force-pushed
the
codex/box-endpoint-url-fd
branch
from
July 22, 2026 10:51
3b944f6 to
063d398
Compare
G4614
marked this pull request as ready for review
July 22, 2026 10:53
📦 BoxLite review — couldn't completepowered by BoxLite |
G4614
enabled auto-merge
July 22, 2026 10:53
G4614
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/runner/pkg/api/controllers/proxy.go (1)
58-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding handler-level tests for
ProxyRequest/BoxliteNetworkProxy.The existing tests exercise the private helper functions (
toolboxEscapedPath,parseGuestPortProxyPath, etc.) individually, but there's nohttptest-driven test exercising the gin handlers themselves end-to-end (terminal dispatch, guest-port dispatch, 400 on bad port, 410 on legacy path). Given this dispatch logic was flagged as medium complexity, a couple of route-level tests would give more confidence in the wiring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/runner/pkg/api/controllers/proxy.go` around lines 58 - 105, The handler-level dispatch paths in ProxyRequest and BoxliteNetworkProxy lack end-to-end coverage. Add httptest-driven Gin route tests covering terminal dispatch, guest-port dispatch, invalid-port 400 responses, and legacy-path 410 responses, reusing existing test helpers and keeping private helper tests unchanged.apps/proxy/pkg/proxy/get_box_target.go (1)
232-232: 🚀 Performance & Scalability | 🔵 TrivialCache TTL for box-public status dropped from 1 hour to 3 seconds.
This trades a large increase in
IsBoxPubliccall volume (every unique box re-checked every 3s under load) for fresher public/private status. Likely intentional given the "defaults new boxes to public" change elsewhere in this stack, but worth confirming the API side (IsBoxPublic) can absorb the added QPS at scale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/proxy/pkg/proxy/get_box_target.go` at line 232, The box-public cache TTL in the cache update around boxPublicCache.Set was reduced to 3 seconds, potentially increasing IsBoxPublic API load. Confirm the shorter TTL is intentional and that IsBoxPublic can handle the added QPS; otherwise restore the established 1-hour TTL or use an approved TTL balancing freshness and capacity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/proxy/pkg/proxy/get_box_target_test.go`:
- Around line 1-2: Update the SPDX header in
apps/proxy/pkg/proxy/get_box_target_test.go lines 1-2 to use Apache-2.0 instead
of AGPL-3.0. Also update apps/common-go/pkg/proxy/proxy_test.go lines 1-2 to use
Apache-2.0; both sites require the same header correction.
In `@apps/proxy/pkg/proxy/get_box_target.go`:
- Around line 380-417: Move the shouldPoll ticker startup in updateLastActivity
before the fallible cache Has and UpdateLastActivity calls, while preserving the
existing polling interval, done-channel termination, and recursive
updateLastActivity invocation. Ensure the polling goroutine is scheduled
unconditionally for shouldPoll connections so initial cache or API errors do not
prevent later heartbeats.
In `@apps/runner/pkg/boxlite/guest_port_tunnel.go`:
- Around line 83-103: Update NewGuestPortTransport to configure an explicit
timeout for guest-port connections, including ResponseHeaderTimeout and, where
needed, a dial deadline for DialGuestPort, using the existing timeout
configuration or a sensible bounded duration. Ensure handleGuestPortProxy
requests cannot remain blocked indefinitely while preserving the current network
validation and error logging.
---
Nitpick comments:
In `@apps/proxy/pkg/proxy/get_box_target.go`:
- Line 232: The box-public cache TTL in the cache update around
boxPublicCache.Set was reduced to 3 seconds, potentially increasing IsBoxPublic
API load. Confirm the shorter TTL is intentional and that IsBoxPublic can handle
the added QPS; otherwise restore the established 1-hour TTL or use an approved
TTL balancing freshness and capacity.
In `@apps/runner/pkg/api/controllers/proxy.go`:
- Around line 58-105: The handler-level dispatch paths in ProxyRequest and
BoxliteNetworkProxy lack end-to-end coverage. Add httptest-driven Gin route
tests covering terminal dispatch, guest-port dispatch, invalid-port 400
responses, and legacy-path 410 responses, reusing existing test helpers and
keeping private helper tests unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac1ce7e3-6bcc-4c37-82dd-a94c3dac2c3d
📒 Files selected for processing (17)
apps/api-client-go/api/openapi.yamlapps/api/src/box/dto/create-box.dto.tsapps/api/src/box/dto/port-preview-url.dto.tsapps/api/src/box/services/box.service.spec.tsapps/api/src/box/services/box.service.tsapps/common-go/pkg/proxy/proxy.goapps/common-go/pkg/proxy/proxy_test.goapps/proxy/pkg/proxy/get_box_target.goapps/proxy/pkg/proxy/get_box_target_test.goapps/proxy/pkg/proxy/proxy.goapps/runner/pkg/api/controllers/proxy.goapps/runner/pkg/api/controllers/proxy_integration_test.goapps/runner/pkg/api/controllers/proxy_test.goapps/runner/pkg/api/server.goapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/guest_port_tunnel.goapps/runner/pkg/boxlite/guest_port_tunnel_test.go
G4614
force-pushed
the
codex/box-endpoint-url-fd
branch
from
July 22, 2026 11:27
063d398 to
b3070cb
Compare
G4614
changed the title
G4614
force-pushed
the
codex/box-endpoint-url-fd
branch
from
July 22, 2026 11:29
b3070cb to
6fcfd86
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/proxy/pkg/proxy/get_box_target.go (1)
403-440: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTransient error at connection start permanently disables activity polling.
updateLastActivityreturns early on any cacheHas/UpdateLastActivityerror before reaching theif shouldPollticker block. Since it is invoked once per connection withshouldPoll=true, a single transient Redis/API blip means the heartbeat ticker is never scheduled for the rest of that (potentially long-lived) connection. Schedule the polling goroutine before the fallible calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/proxy/pkg/proxy/get_box_target.go` around lines 403 - 440, Move the shouldPoll ticker goroutine setup in updateLastActivity before the fallible cache Has and BoxAPI.UpdateLastActivity calls, so transient errors cannot prevent polling from being scheduled. Preserve the existing ticker interval, recursive updateLastActivity invocation, done-channel shutdown, and one-time setup behavior.
🧹 Nitpick comments (1)
apps/proxy/pkg/proxy/get_box_target.go (1)
255-255: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPublic-status cache TTL dropped 1h → 3s greatly increases
IsBoxPubliccall volume.For frequently accessed public previews this is a ~1200x increase in cache misses hitting
PreviewAPI.IsBoxPublicon the hot request path. If the goal is faster propagation of visibility changes, consider a middle-ground TTL (e.g. tens of seconds) or event-driven invalidation to bound API load.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/proxy/pkg/proxy/get_box_target.go` at line 255, Update the TTL used by boxPublicCache.Set in the public-status caching flow to a middle-ground duration of at least tens of seconds, rather than 3 seconds, while preserving the existing cache behavior and key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/common-go/pkg/proxy/proxy.go`:
- Around line 151-159: Update the header cleanup block before
req.SetXForwarded() to delete the inbound X-Forwarded-For header, ensuring
SetXForwarded() builds the upstream value without preserving client-supplied
entries.
In `@apps/proxy/pkg/proxy/proxy.go`:
- Around line 43-49: Make the activity-poll stop-hook registration race-safe
around stopActivityPoll and the cleanup at line 118: install an idempotent
cancellation controller in the Gin context before starting the detached poll,
then have GetProxyTarget register/use that controller rather than publishing the
stop function afterward. Ensure cleanup can cancel the poll whether it runs
before or after poll initialization, while preserving single execution of
cancellation.
- Around line 195-197: Update the HTTP server configuration in the proxy startup
flow around the Addr, Handler, and ReadHeaderTimeout fields to add
configuration-backed ReadTimeout and IdleTimeout values. Ensure the selected
ReadTimeout explicitly accommodates legitimate upload durations while still
bounding slow request bodies, and preserve the existing handler and
header-timeout behavior.
In `@apps/proxy/pkg/proxy/tunnel.go`:
- Around line 65-77: Update the tunnel relay in the handler around Hijack and
ProxyBidirectionalStream to wrap clientConn with
common_proxy.NewBufferedConn(clientConn, buffered.Reader) after writing and
flushing the CONNECT response, then pass that wrapped connection to
ProxyBidirectionalStream so buffered client bytes are preserved.
---
Duplicate comments:
In `@apps/proxy/pkg/proxy/get_box_target.go`:
- Around line 403-440: Move the shouldPoll ticker goroutine setup in
updateLastActivity before the fallible cache Has and BoxAPI.UpdateLastActivity
calls, so transient errors cannot prevent polling from being scheduled. Preserve
the existing ticker interval, recursive updateLastActivity invocation,
done-channel shutdown, and one-time setup behavior.
---
Nitpick comments:
In `@apps/proxy/pkg/proxy/get_box_target.go`:
- Line 255: Update the TTL used by boxPublicCache.Set in the public-status
caching flow to a middle-ground duration of at least tens of seconds, rather
than 3 seconds, while preserving the existing cache behavior and key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a5002e2c-6175-46fb-896d-7472f67c5585
📒 Files selected for processing (21)
apps/api-client-go/api/openapi.yamlapps/api/src/box/dto/create-box.dto.tsapps/api/src/box/dto/port-preview-url.dto.tsapps/api/src/box/services/box.service.spec.tsapps/api/src/box/services/box.service.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.spec.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/api/src/boxlite-rest/boxlite-rest-routing.spec.tsapps/common-go/pkg/proxy/proxy.goapps/common-go/pkg/proxy/proxy_test.goapps/common-go/pkg/proxy/tunnel_test.goapps/infra/sst.config.tsapps/proxy/pkg/proxy/get_box_target.goapps/proxy/pkg/proxy/get_box_target_test.goapps/proxy/pkg/proxy/proxy.goapps/proxy/pkg/proxy/tunnel.goapps/proxy/pkg/proxy/tunnel_test.goapps/runner/pkg/api/controllers/network_tunnel.goapps/runner/pkg/api/controllers/network_tunnel_test.goapps/runner/pkg/api/server.goapps/runner/pkg/boxlite/guest_port_tunnel.go
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/api-client-go/api/openapi.yaml
- apps/common-go/pkg/proxy/proxy_test.go
- apps/api/src/box/dto/create-box.dto.ts
- apps/api/src/box/dto/port-preview-url.dto.ts
- apps/api/src/box/services/box.service.ts
G4614
force-pushed
the
codex/box-endpoint-url-fd
branch
from
July 23, 2026 03:57
0e2d8b2 to
6cf185f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/proxy/pkg/proxy/get_box_target.go`:
- Around line 112-119: Update the invalid target-port branch in GetProxyTarget
to wrap the ParseUint validation failure with
ctx.Error(common_errors.NewBadRequestError(...)) before returning, matching the
existing validation-error handling and ensuring malformed public-box Host
headers produce a 400 response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0901e7bb-51f1-41eb-8ad3-25f03b47904a
📒 Files selected for processing (9)
apps/api-client-go/api/openapi.yamlapps/api/src/box/dto/port-preview-url.dto.tsapps/api/src/box/services/box.service.spec.tsapps/api/src/box/services/box.service.tsapps/common-go/pkg/proxy/proxy.goapps/common-go/pkg/proxy/proxy_test.goapps/proxy/pkg/proxy/get_box_target.goapps/proxy/pkg/proxy/get_box_target_test.goapps/proxy/pkg/proxy/proxy.go
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/api/src/box/dto/port-preview-url.dto.ts
- apps/common-go/pkg/proxy/proxy_test.go
- apps/api-client-go/api/openapi.yaml
- apps/proxy/pkg/proxy/get_box_target_test.go
- apps/common-go/pkg/proxy/proxy.go
- apps/api/src/box/services/box.service.ts
DorianZheng
disabled auto-merge
July 23, 2026 06:47
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
2 participants
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.
Route browser HTTP previews through the edge proxy over the runner CONNECT tunnel merged in PR #995.
Test plan:
cd apps/common-go && go test -race ./pkg/proxycd apps/proxy && go test -race ./pkg/proxycd apps/api && yarn jest --passWithNoTests=true --runInBand src/box/services/box.service.spec.ts --testNamePattern='^BoxService preview URLs'cd apps/api && yarn prettier --check src/box/dto/port-preview-url.dto.ts src/box/services/box.service.ts src/box/services/box.service.spec.tsSummary by CodeRabbit
false); legacy preview formats remain supported.