Change the format of the proxy url - #7
Merged
Conversation
Align proxy requests with backend routing by putting workspace/project/environment in the proxy URL path. Remove config and is-personal from the wire payload to match the proxy request schema. Made-with: Cursor
Proxy integration
Bring back usePersonal support for proxy requests and interceptor rules by restoring is-personal on the proxy wire payload. Set the default proxy base URL back to the root host path instead of /v1/proxy and align tests accordingly. Made-with: Cursor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe PR flattens proxy scope (workspace, project, environment-id, is-personal) from a nested Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/proxy.ts (1)
87-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the new top-level scope fields in the posted wire body.
sendProxyWire()currently dropsworkspace,project, and"environment-id"when rebuildingwireBody, so the JSON payload no longer matchesProxyWireBody. Routing them into the URL is additive here, not a replacement for the contract this PR just introduced.Suggested fix
- const wireBody: Record<string, unknown> = { + const wireBody: ProxyWireBody = { url: body.url, method: body.method, + workspace: body.workspace, + project: body.project, + "environment-id": body["environment-id"], "is-personal": body["is-personal"], };🤖 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 `@src/proxy.ts` around lines 87 - 95, sendProxyWire is currently omitting the top-level workspace, project and "environment-id" fields when constructing wireBody; restore those fields so the posted JSON matches ProxyWireBody by adding workspace: body.workspace, project: body.project and "environment-id": body["environment-id"] to the wireBody object (in addition to still calling buildProxyRequestUrl(ctx.proxyUrl, body.workspace, body.project, body["environment-id"]) to create proxyRequestUrl). Update the construction of wireBody (the variable named wireBody) so it includes those three fields whenever present on body, while keeping the existing conditional headers/body handling.
🧹 Nitpick comments (2)
tests/proxy.test.ts (1)
419-431: ⚡ Quick winAssert
content-lengthis stripped too.This test injects both hop-by-hop headers, but it only verifies
transfer-encoding. Please also assert thatcontent-lengthis removed so the test fully covers the behavior described here.Suggested assertion
const res = await client.proxy.fetch("https://upstream/x"); + expect(res.headers.get("content-length")).toBeNull(); expect(res.headers.get("transfer-encoding")).toBeNull(); expect(res.headers.get("x-custom")).toBe("keep");🤖 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 `@tests/proxy.test.ts` around lines 419 - 431, In the "strips hop-by-hop headers (content-length, transfer-encoding) from the synthesized Response" test, add an assertion that the synthesized Response also has the "content-length" header removed: after calling client.proxy.fetch("https://upstream/x") and the existing transfer-encoding assertion, assert that res.headers.get("content-length") is null (reference: Enkryptify, proxy.fetch, res.headers).tests/interceptor.test.ts (1)
378-399: ⚡ Quick winAssert the full flattened scope contract here.
These tests currently prove URL routing and
is-personal, but they do not verify thatworkspace,project, andenvironment-idare still present at the top level of the wire body. A regression that keeps the routed URL correct while re-nesting or dropping those fields would still pass.Suggested assertions
const wire = await findProxyCall(fetchMock); expect(wire?.["is-personal"]).toBe(false); + expect(wire?.workspace).toBe("ws-x"); + expect(wire?.project).toBe("prj-y"); + expect(wire?.["environment-id"]).toBe("env-z"); + expect(wire?.config).toBeUndefined(); const proxyCall = await findCallByUrlPrefix(fetchMock, "https://proxy.test.com/"); expect(proxyCall?.url).toBe("https://proxy.test.com/ws-x/prj-y/env-z");const wire = await findProxyCall(fetchMock); expect(wire?.["is-personal"]).toBe(false); + expect(wire?.workspace).toBe("override-ws"); + expect(wire?.project).toBe("override-prj"); + expect(wire?.["environment-id"]).toBe("override-env"); + expect(wire?.config).toBeUndefined(); const proxyCall = await findCallByUrlPrefix(fetchMock, "https://proxy.test.com/"); expect(proxyCall?.url).toBe("https://proxy.test.com/override-ws/override-prj/override-env");Also applies to: 402-428
🤖 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 `@tests/interceptor.test.ts` around lines 378 - 399, Update the test that sets up Enkryptify (activeClient via makeConfig with workspace/project/environment and interceptor rules) to also assert the full flattened scope contract exists on the proxied wire body: after getting wire from findProxyCall(fetchMock) verify wire.workspace === "ws-x", wire.project === "prj-y" and wire["environment-id"] === "env-z" (and keep the existing is-personal check); apply the same additional assertions in the companion test that mirrors this one (the test using the same makeConfig values and fetchMock flow) so we ensure workspace, project and environment-id remain top-level fields and aren’t re-nested or dropped.
🤖 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 `@src/proxy.ts`:
- Around line 333-343: The hop-by-hop header set HOP_BY_HOP_RESPONSE_HEADERS
incorrectly lists "trailers" instead of the RFC-defined "trailer", so replace
the "trailers" entry with "trailer" in the HOP_BY_HOP_RESPONSE_HEADERS
ReadonlySet to ensure proxied Trailer headers are stripped from synthesized
responses; update the set initialization where HOP_BY_HOP_RESPONSE_HEADERS is
declared.
---
Outside diff comments:
In `@src/proxy.ts`:
- Around line 87-95: sendProxyWire is currently omitting the top-level
workspace, project and "environment-id" fields when constructing wireBody;
restore those fields so the posted JSON matches ProxyWireBody by adding
workspace: body.workspace, project: body.project and "environment-id":
body["environment-id"] to the wireBody object (in addition to still calling
buildProxyRequestUrl(ctx.proxyUrl, body.workspace, body.project,
body["environment-id"]) to create proxyRequestUrl). Update the construction of
wireBody (the variable named wireBody) so it includes those three fields
whenever present on body, while keeping the existing conditional headers/body
handling.
---
Nitpick comments:
In `@tests/interceptor.test.ts`:
- Around line 378-399: Update the test that sets up Enkryptify (activeClient via
makeConfig with workspace/project/environment and interceptor rules) to also
assert the full flattened scope contract exists on the proxied wire body: after
getting wire from findProxyCall(fetchMock) verify wire.workspace === "ws-x",
wire.project === "prj-y" and wire["environment-id"] === "env-z" (and keep the
existing is-personal check); apply the same additional assertions in the
companion test that mirrors this one (the test using the same makeConfig values
and fetchMock flow) so we ensure workspace, project and environment-id remain
top-level fields and aren’t re-nested or dropped.
In `@tests/proxy.test.ts`:
- Around line 419-431: In the "strips hop-by-hop headers (content-length,
transfer-encoding) from the synthesized Response" test, add an assertion that
the synthesized Response also has the "content-length" header removed: after
calling client.proxy.fetch("https://upstream/x") and the existing
transfer-encoding assertion, assert that res.headers.get("content-length") is
null (reference: Enkryptify, proxy.fetch, res.headers).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7171ac6a-4bed-4585-a003-56254c500b25
📒 Files selected for processing (3)
src/proxy.tstests/interceptor.test.tstests/proxy.test.ts
Consumers installing the SDK via a git URL (git+https://…#main) have no published dist/, so pnpm must build it during the prepare lifecycle. Append tsup to prepare; keep husky for local dev but guard it with `|| true` so it never blocks the build when there is no .git (the git-dep tarball case).
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.
Summary by CodeRabbit
Documentation
Refactor
Bug Fixes
Tests
Chores