Open
Conversation
…or api & docs to enable https
Contributor
Reviewer's GuideMigrate local HTTPS and subdomain routing to the portless reverse proxy, simplifying mock services, UI, API, and docs startup to run plain HTTP behind a centralized HTTPS proxy on :1355 while removing mkcert-based certificate handling and the custom local HTTPS proxy. Sequence diagram for HTTPS request via portless to mock payment serversequenceDiagram
actor User
participant Browser
participant PortlessProxy as Portless_proxy:1355
participant MockPayment as Mock_payment_server_HTTP
User ->> Browser: Open https://mock-payment.sharethrift.localhost:1355
Browser ->> PortlessProxy: TLS handshake and HTTPS request
PortlessProxy ->> MockPayment: HTTP request to http://localhost:PORT
MockPayment -->> PortlessProxy: HTTP response
PortlessProxy -->> Browser: HTTPS response
Browser -->> User: Render payment mock UI/API response
Flow diagram for dev environment startup with portlessflowchart LR
A[pnpm run dev] --> B[proxy:stop\nportless proxy stop]
B --> C[proxy:start\nportless proxy start --https]
C --> D[turbo run azurite gen:watch start --parallel]
subgraph ServiceStartScripts
D --> UIStart[apps/ui-sharethrift\nportless sharethrift.localhost vite]
D --> APIStart[apps/api\nportless data-access.sharethrift.localhost node start-dev.mjs]
D --> DocsStart[apps/docs\nportless docs.sharethrift.localhost node start-dev.mjs]
D --> MockAuthStart[packages/cellix/mock-oauth2-server\nportless mock-auth.sharethrift.localhost tsx watch src/index.ts]
D --> MockPaymentStart[packages/cellix/mock-payment-server\nportless mock-payment.sharethrift.localhost node dist/src/index.js]
D --> MockMessagingStart[packages/sthrift/mock-messaging-server\nportless mock-messaging.sharethrift.localhost node -r dotenv/config dist/src/index.js]
end
subgraph PortInjection
UIStart --> UIEnv[portless injects PORT env\nUI binds to http://127.0.0.1:PORT]
APIStart --> APIEnv[portless injects PORT env\nstart-dev.mjs passes --port to func]
DocsStart --> DocsEnv[portless injects PORT env\nstart-dev.mjs passes --port to docusaurus]
MockAuthStart --> MockAuthEnv[portless injects PORT env\nexpress listens on http://localhost:PORT]
MockPaymentStart --> MockPaymentEnv[portless injects PORT env\nexpress listens on http://localhost:PORT]
MockMessagingStart --> MockMessagingEnv[mock server listens on http://localhost:PORT]
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
jason-t-hankins
requested review from
jasonmorais and
nnoce14
as code owners
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Several services now hard-code
https://*.sharethrift.localhost:1355(e.g., mock payment, mock OAuth2 allowedRedirectUris, Vite open URL); consider centralizing the base URL/port in a shared config or env variable so that future changes don’t require touching multiple files. - The
scripts/dev-cleanup.mjsscript relies onpkill -fwith string patterns, which is both OS-specific and somewhat brittle; it may be more robust to detect and terminate processes by PID/port using a cross‑platform approach or a library designed for this.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several services now hard-code `https://*.sharethrift.localhost:1355` (e.g., mock payment, mock OAuth2 allowedRedirectUris, Vite open URL); consider centralizing the base URL/port in a shared config or env variable so that future changes don’t require touching multiple files.
- The `scripts/dev-cleanup.mjs` script relies on `pkill -f` with string patterns, which is both OS-specific and somewhat brittle; it may be more robust to detect and terminate processes by PID/port using a cross‑platform approach or a library designed for this.
## Individual Comments
### Comment 1
<location path="scripts/dev-cleanup.mjs" line_range="29-30" />
<code_context>
+ 'start-dev.mjs',
+];
+
+for (const pattern of patterns) {
+ run(`pkill -f '${pattern}'`);
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The `pkill -f`-based cleanup is fragile and Unix-specific; consider a more targeted or cross-platform approach.
Using `pkill -f` with a broad pattern like `'start-dev.mjs'` risks killing unrelated processes (e.g., other Node scripts with similar names) and is not Windows-compatible. Given this runs automatically from the main `dev` command, a mistaken match would be surprising. Consider a safer approach such as tracking PIDs, matching more specific command lines (e.g., full paths), or making this cleanup opt-in via a flag/env var.
</issue_to_address>
### Comment 2
<location path="apps/docs/docs/decisions/0025-portless-local-https.md" line_range="58" />
<code_context>
+
+**Negative**
+
+- Developers neeed to run `mkcert -install` and `mkcert` on each machine
+- Certificate files require explicit exclusion from version control
+- `local-https-proxy.js` must be kept in sync with Azure Functions behaviour
</code_context>
<issue_to_address>
**issue (typo):** Fix typo in "neeed" → "need".
```suggestion
- Developers need to run `mkcert -install` and `mkcert` on each machine
```
</issue_to_address>
### Comment 3
<location path="apps/docs/docs/decisions/0025-portless-local-https.md" line_range="39-40" />
<code_context>
+- Single port (`1355`) for all services: `.env` and `local.settings.json` are simpler
+- Subdomain names in URLs (`data-access`, `mock-auth`, etc.) make it immediately obvious which service is being called
+- `local-https-proxy.js` is deleted, now one less script to maintain
+- Removes the need for compatibility issues with Azure Function's --cert flag
+
+**Negative**
</code_context>
<issue_to_address>
**suggestion (typo):** Clarify wording and possessive form for Azure Functions `--cert` flag.
The wording here is awkward and the possessive form should match "Azure Functions." Suggest: "Removes compatibility issues with Azure Functions' `--cert` flag."
```suggestion
- `local-https-proxy.js` is deleted, now one less script to maintain
- Removes compatibility issues with Azure Functions' `--cert` flag
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes the local development setup by adopting portless, a zero-config reverse proxy that provides automatic HTTPS with trusted certificates for subdomain-based local development. It replaces the previous mkcert-based certificate management and custom Node.js HTTPS proxy with a simpler, unified approach.
Changes:
- Removes mkcert certificate generation scripts and the custom
local-https-proxy.jsproxy, consolidating all HTTPS functionality into portless - Standardizes all local services to use a single port (1355) with named subdomains (e.g.,
https://sharethrift.localhost:1355) - Adds wrapper scripts (
start-dev.mjs) for services that don't respect thePORTenvironment variable - Introduces a cleanup script to prevent zombie processes and stale proxy state between dev sessions
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/setup-local-certs.js | Removes the mkcert-based certificate setup script (no longer needed with portless) |
| scripts/dev-cleanup.mjs | Adds cleanup script to stop portless proxy and kill stale processes before dev sessions |
| packages/sthrift/mock-messaging-server/src/index.ts | Removes HTTPS logic; server now runs plain HTTP behind portless proxy |
| packages/sthrift/mock-messaging-server/package.json | Wraps start command with portless to register subdomain |
| packages/sthrift/messaging-service-mock/src/index.test.ts | Updates test to use simplified server start signature (no useHttps param) |
| packages/cellix/mock-payment-server/src/index.ts | Removes certificate detection and HTTPS server logic; updates BASE_URLs to use port 1355 |
| packages/cellix/mock-payment-server/package.json | Wraps start command with portless subdomain registration |
| packages/cellix/mock-oauth2-server/src/index.ts | Removes certificate file loading and HTTPS server creation; uses environment variable for BASE_URL |
| packages/cellix/mock-oauth2-server/package.json | Wraps start and dev commands with portless |
| package.json | Updates dev script to run cleanup, start portless proxy with --https flag, and execute services in parallel |
| knip.json | Adds portless to ignoreBinaries list |
| apps/ui-sharethrift/vite.config.ts | Simplifies Vite config by removing certificate loading and host configuration |
| apps/ui-sharethrift/package.json | Wraps start command with portless |
| apps/ui-sharethrift/.env | Updates all URLs to use port 1355 with subdomain structure |
| apps/docs/start-dev.mjs | Adds wrapper to pass PORT to Docusaurus with explicit IPv4 binding |
| apps/docs/package.json | Updates start command to use portless and wrapper script |
| apps/docs/docusaurus.config.ts | Removes custom HTTPS webpack plugin (no longer needed) |
| apps/docs/docs/technical-overview/localhost-subdomain-setup.md | Removes outdated mkcert setup documentation |
| apps/docs/docs/decisions/0025-portless-local-https.md | Adds ADR documenting the decision to adopt portless and rationale |
| apps/api/start-dev.mjs | Adds wrapper to pass PORT to func start command |
| apps/api/package.json | Wraps start command with portless subdomain registration |
| .certs/.gitignore | Removes .certs directory gitignore (directory no longer needed) |
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several services now hardcode HTTPS base URLs (e.g.
FRONTEND_BASE_URL/PAYMENT_BASE_URLin the mock payment server andBASE_URLin the mock OAuth server); consider deriving these from environment variables with sensible defaults so non-standard hosts/ports or alternative dev setups don’t require code changes. - In the new
start-dev.mjswrappers for API and docs, you currently spawn child processes without wiring through their exit codes or signals; consider listening forexit/erroron the spawned process and propagating failures so the parent script correctly reflects dev server startup errors. - The root
devscript usesportless proxy stop ; portless proxy start --https, which will ignore a failure inproxy stop; if it’s important to avoid running with an unexpected proxy mode, consider making the second command conditional (&&) or explicitly tolerating/handling a missing proxy process with|| trueto make the intent clear.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several services now hardcode HTTPS base URLs (e.g. `FRONTEND_BASE_URL` / `PAYMENT_BASE_URL` in the mock payment server and `BASE_URL` in the mock OAuth server); consider deriving these from environment variables with sensible defaults so non-standard hosts/ports or alternative dev setups don’t require code changes.
- In the new `start-dev.mjs` wrappers for API and docs, you currently spawn child processes without wiring through their exit codes or signals; consider listening for `exit`/`error` on the spawned process and propagating failures so the parent script correctly reflects dev server startup errors.
- The root `dev` script uses `portless proxy stop ; portless proxy start --https`, which will ignore a failure in `proxy stop`; if it’s important to avoid running with an unexpected proxy mode, consider making the second command conditional (`&&`) or explicitly tolerating/handling a missing proxy process with `|| true` to make the intent clear.
## Individual Comments
### Comment 1
<location path="package.json" line_range="14" />
<code_context>
- "setup:certs": "node scripts/setup-local-certs.js",
- "proxy:start": "node build-pipeline/scripts/local-https-proxy.js",
- "dev": "pnpm run build && pnpm run setup:certs && turbo run //#proxy:start azurite gen:watch start --parallel",
+ "dev": "pnpm run build && portless proxy stop ; portless proxy start --https && turbo run azurite gen:watch start --parallel",
"start": "turbo run build && concurrently pnpm:start:* --kill-others-on-fail --workspace=@sthrift/api",
"format": "turbo run format",
</code_context>
<issue_to_address>
**issue:** Using `;` as a command separator can cause portability issues across shells/platforms.
This script chains `portless proxy stop` and `portless proxy start --https` with a bare `;`, which is POSIX-specific and will fail in Windows `cmd.exe` and some tooling. If you expect non-POSIX environments, consider a cross-platform approach: e.g., wrap this in a small Node script, use a runner like `npm-run-all`, or split into composable scripts (e.g. `proxy:stop`, `proxy:start:https`, `proxy:restart`) that you chain via `pnpm`/`turbo`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ess for api/docs to handle error
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- There are several hard-coded BASE_URL/redirect/CORS URL strings (e.g.
https://sharethrift.localhost:1355and mock subdomains) scattered across services; consider centralising these into shared config or env helpers to avoid drift if the port or domain pattern ever changes. - In
packages/sthrift/messaging-service-mock/src/index.test.tsthe updatedmockServer = await startServer(...)line appears to have lost its indentation relative to the surrounding code, which may cause a linting/style issue and is worth correcting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are several hard-coded BASE_URL/redirect/CORS URL strings (e.g. `https://sharethrift.localhost:1355` and mock subdomains) scattered across services; consider centralising these into shared config or env helpers to avoid drift if the port or domain pattern ever changes.
- In `packages/sthrift/messaging-service-mock/src/index.test.ts` the updated `mockServer = await startServer(...)` line appears to have lost its indentation relative to the surrounding code, which may cause a linting/style issue and is worth correcting.
## Individual Comments
### Comment 1
<location path="packages/cellix/mock-payment-server/src/index.ts" line_range="1344-1349" />
<code_context>
- return;
- }
+ // HTTP server — portless handles TLS/proxy at the subdomain level
+ const server = app.listen(portToTry, () => {
+ console.log(` Mock Payment Server listening on http://localhost:${portToTry}`);
+ console.log(` CORS origin: ${FRONTEND_BASE_URL}`);
+ console.log(` Microform origin: ${PAYMENT_BASE_URL}`);
+ });
</code_context>
<issue_to_address>
**suggestion:** Log messages still reference localhost/HTTP instead of the externally visible HTTPS/subdomain URL.
Since portless terminates TLS and routes via subdomains, the externally reachable URL is `PAYMENT_BASE_URL` (e.g. `https://mock-payment.sharethrift.localhost:1355`), but the log prints `http://localhost:${portToTry}`. That’s misleading when debugging. Please log `PAYMENT_BASE_URL` (and optionally the internal bind address separately) so logs show the URL developers should actually use in the browser.
```suggestion
// HTTP server — portless handles TLS/proxy at the subdomain level
const server = app.listen(portToTry, () => {
console.log(` Mock Payment Server externally reachable at: ${PAYMENT_BASE_URL}`);
console.log(` Internal bind (HTTP): http://localhost:${portToTry}`);
console.log(` CORS origin: ${FRONTEND_BASE_URL}`);
console.log(` Microform origin: ${PAYMENT_BASE_URL}`);
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
packages/sthrift/mock-messaging-server/src/index.ts,startServerstill binds to the hardcoded default port 10000 instead of usingprocess.env.PORT, which will cause a mismatch when the service is launched viaportless; consider reading the port fromprocess.env.PORT(with a sensible default) so the proxy and server stay aligned. - In
apps/ui-sharethrift/vite.config.ts, the dev server no longer specifies a port and relies on Vite’s defaults; sinceportlessinjectsPORT, it would be safer to wireserver.porttoprocess.env.PORTexplicitly to avoid any mismatch between the proxy’s target port and the Vite dev server.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `packages/sthrift/mock-messaging-server/src/index.ts`, `startServer` still binds to the hardcoded default port 10000 instead of using `process.env.PORT`, which will cause a mismatch when the service is launched via `portless`; consider reading the port from `process.env.PORT` (with a sensible default) so the proxy and server stay aligned.
- In `apps/ui-sharethrift/vite.config.ts`, the dev server no longer specifies a port and relies on Vite’s defaults; since `portless` injects `PORT`, it would be safer to wire `server.port` to `process.env.PORT` explicitly to avoid any mismatch between the proxy’s target port and the Vite dev server.
## Individual Comments
### Comment 1
<location path="apps/docs/docs/decisions/0025-portless-local-https.md" line_range="5-7" />
<code_context>
+sidebar_position: 25
+sidebar_label: 0025 Portless Local HTTPS
+description: "Decision record for replacing mkcert + manual HTTPS proxy with portless for local development."
+status:
+date: 2026-02-26
+deciders:
+---
+
</code_context>
<issue_to_address>
**suggestion:** Consider either populating or removing empty `status` and `deciders` frontmatter fields
These keys are present but empty (with trailing spaces), which some Docusaurus/YAML tooling might misinterpret. Consider either populating them (e.g. `status: accepted`, `deciders: []`) or removing them until you have real values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
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
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.
dynamic port management with the portless package allows for automatic port assignment through the reverse proxy, so each subdomain can always be found at https://subdomain:1355. Removes excess code related to certificate checks in every subdomain, as portless handles internally.
https://github.com/vercel-labs/portless/tree/main
Summary by Sourcery
Adopt portless-based HTTPS subdomain routing across local services and simplify dev server startup.
New Features:
Enhancements:
Build:
Tests:
Chores:
Summary by Sourcery
Adopt portless as the unified local HTTPS proxy for all dev services and remove bespoke certificate-based HTTPS handling.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores: