Dev command engine URL now configurable via the webapp #1948
Conversation
|
WalkthroughThis change introduces a new environment variable, Changes
Sequence Diagram(s)sequenceDiagram
participant WebApp
participant API
participant CLI
participant DevSupervisor
WebApp->>API: Request dev config
API-->>WebApp: Respond with { engineUrl }
CLI->>API: Fetch dev config
API-->>CLI: Respond with { engineUrl }
CLI->>DevSupervisor: Pass engineUrl
DevSupervisor->>CLI: Set engineURL via setEngineURL
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
nicktrn
changed the title
nicktrn
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: 0
🧹 Nitpick comments (1)
packages/cli-v3/src/apiClient.ts (1)
667-669: Well-implemented setEngineURL methodThe
setEngineURLmethod correctly sanitizes the URL by removing trailing slashes, consistent with howapiURLis handled in the constructor.Consider adding URL validation to ensure the provided engine URL is properly formatted:
private setEngineURL(engineURL: string) { + try { + // Validate URL format + new URL(engineURL); this.engineURL = engineURL.replace(/\/$/, ""); + } catch (error) { + logger.warn(`Invalid engine URL provided: ${engineURL}. Using default.`); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/routes/engine.v1.dev.config.ts(1 hunks)packages/cli-v3/src/apiClient.ts(11 hunks)packages/cli-v3/src/dev/devSupervisor.ts(1 hunks)packages/core/src/v3/schemas/api.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/webapp/app/routes/engine.v1.dev.config.ts (1)
apps/webapp/app/env.server.ts (1)
env(719-719)
packages/cli-v3/src/apiClient.ts (3)
packages/core/src/v3/apiClient/core.ts (1)
wrapZodFetch(713-753)packages/core/src/v3/schemas/api.ts (2)
DevConfigResponseBody(430-436)DevConfigResponseBody(437-437)packages/cli-v3/src/dev/devSupervisor.ts (1)
eventSource(340-367)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
apps/webapp/app/env.server.ts (1)
627-629: LGTM: Good addition of configurable dev engine URL.The new environment variable is properly documented and uses a sensible default fallback chain (APP_ORIGIN or localhost).
packages/core/src/v3/schemas/api.ts (1)
435-435: LGTM: Schema update matches the new API contract.The
engineUrlstring property correctly extends theDevConfigResponseBodyschema to support the new configuration option.apps/webapp/app/routes/engine.v1.dev.config.ts (1)
24-24: LGTM: Correctly exposes the environment variable through the API.The change properly includes the
engineUrlin the JSON response, making it available to clients.packages/cli-v3/src/dev/devSupervisor.ts (1)
87-87: LGTM: Properly integrates the configurable engine URL.The change correctly sets the engine URL on the client using the retrieved configuration, completing the end-to-end implementation of the feature.
packages/cli-v3/src/apiClient.ts (4)
48-49: Good addition of a separate engineURL propertyAdding a distinct property for the engine URL allows for better configurability and separation of concerns, enabling different endpoints for API and engine operations if needed.
56-56: Appropriate initialization of engineURLInitializing
engineURLto matchapiURLby default provides backward compatibility while enabling the new configuration option.
424-424: Good approach exposing the setEngineURL methodExposing the
setEngineURLmethod through thedevgetter follows the established pattern in this class and provides a clean interface for external configuration.
493-493: Consistent updates to use engineURL for all dev endpoint URLsAll development-related API endpoints have been updated to use the configurable
engineURLproperty instead ofapiURL, ensuring consistency throughout the codebase.Also applies to: 510-510, 564-564, 582-582, 598-598, 616-616, 635-635, 655-655
Can be set via a new env var and is returned by the dev config response.
Summary by CodeRabbit