Skip to content

fix(cli): Derive debug port from server port#1460

Merged
Tobbe merged 2 commits intomainfrom
tobbe-fix-conflicting-debug-ports
Mar 25, 2026
Merged

fix(cli): Derive debug port from server port#1460
Tobbe merged 2 commits intomainfrom
tobbe-fix-conflicting-debug-ports

Conversation

@Tobbe
Copy link
Copy Markdown
Member

@Tobbe Tobbe commented Mar 25, 2026

If you try to simultaneously run two Cedar apps the second one won't start because the 8910 and 8911 ports are already taken.
So you go and update your cedar.toml file to run your second app on 8912 and 8913.
Now everything starts up just fine 🎉

The problem though is that they both launch with --debug-port 18911. I noticed this when running ps aux | grep rw-

First app, api side running on :8911
image

Second app, api side running on :8913
image

Notice how they both have the same debug port

This PR fixes this by adding '1' to the start of whatever port the dev server i running on.

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 25, 2026

Deploy Preview for cedarjs canceled.

Name Link
🔨 Latest commit 6c6305a
🔍 Latest deploy log https://app.netlify.com/projects/cedarjs/deploys/69c3c692f75a03000882d85e

@github-actions github-actions Bot added this to the next-release-patch milestone Mar 25, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 25, 2026

Greptile Summary

This PR fixes a real UX problem: when two Cedar apps run simultaneously on different ports, they previously both received --debug-port 18911, preventing the second app's debugger from binding. The fix derives the debug port dynamically from the API port (by prepending "1" as a string, e.g. 8911 → 18911, 8913 → 18913) instead of using a hard-coded default of 18911.

Key changes:

  • getApiDebugFlag now accepts apiAvailablePort and derives the debug port from it in all code paths (explicit flag with no value, TOML undefined)
  • DEFAULT_CONFIG.api.debugPort is changed from 18911 to undefined so the dynamic derivation applies by default
  • The test suite is refactored to use a live getConfig() call backed by a mocked cedar.toml string (mockCedarToml), making it easier to test TOML-driven behaviour

Two issues to address before merging:

  • The string concatenation approach ('1' + apiAvailablePort) produces TCP port numbers greater than 65535 for any API port ≥ 10000 (e.g. 10000 → "110000"). While unusual in practice, this would silently produce an invalid debug port. Arithmetic (e.g. 10000 + (apiAvailablePort % 10000)) would be safer.
  • In the test afterEach, vi.mocked(getConfig).mockRestore() / vi.mocked(getPaths).mockRestore() are called on plain vi.fn() mocks (not spies), where mockRestore() is a no-op. Tests that override these mocks with mockReturnValue may leak state into subsequent tests; mockReset() is the correct API here.

Confidence Score: 4/5

  • Safe to merge for the common case (dev ports 1000–9999); one targeted fix for the out-of-range port arithmetic is recommended before merge.
  • The core idea is correct and well-tested for typical dev ports. The string concatenation edge case only triggers for API ports ≥ 10000, which is uncommon in practice. The test cleanup issue is low-risk. A small arithmetic fix resolves both concerns cleanly.
  • packages/cli/src/commands/dev/apiDebugFlag.ts — both derivation sites use string concatenation rather than arithmetic, producing invalid port numbers for API ports ≥ 10000.

Important Files Changed

Filename Overview
packages/cli/src/commands/dev/apiDebugFlag.ts Core logic change: derives debug port by prepending "1" to the API port string. The string concatenation approach ('1' + port) produces out-of-range TCP port numbers (> 65535) for API ports ≥ 10000.
packages/cli/src/commands/dev/devHandler.ts Minimal change: passes apiAvailablePort as a second argument to getApiDebugFlag. Correctly threads the available port through to the debug port derivation.
packages/project-config/src/config.ts Changes DEFAULT_CONFIG.api.debugPort from 18911 to undefined. Correct — debug port is now derived dynamically rather than being a hardcoded default.
packages/cli/src/commands/dev/tests/dev.test.ts Test refactor: replaces static DEFAULT_CONFIG stubs with a live getConfig() call backed by a mockCedarToml variable. Adds a new test for port derivation. mockRestore() in afterEach is a no-op for vi.fn() mocks, so per-test mockReturnValue overrides may leak between tests.
packages/cli/src/commands/dev.ts Documentation-only change: updates the description of the --apiDebugPort flag to reflect the new dynamic derivation behaviour.
packages/project-config/src/tests/config.test.ts Snapshot update: removes the debugPort: 18911 expectation to match the new undefined default. Straightforward.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[devHandler resolves apiAvailablePort] --> B[getApiDebugFlag called with apiDebugPort and apiAvailablePort]
    B --> C{apiDebugPort passed via CLI?}
    C -- Yes --> D["--debug-port apiDebugPort"]
    C -- No --> E{--apiDebugPort flag with no value?}
    E -- Yes --> F["--debug-port '1'+apiAvailablePort e.g. 8911 to 18911"]
    E -- No --> G[Read config api.debugPort from cedar.toml]
    G --> H{debugPort value}
    H -- number --> I["--debug-port debugPort"]
    H -- false --> J["empty string - disabled"]
    H -- undefined --> K["--debug-port '1'+apiAvailablePort default derivation"]
    D --> L[Passed to nodemon exec command]
    F --> L
    I --> L
    J --> L
    K --> L
Loading

Reviews (1): Last reviewed commit: "fix(cli): Derive debug port from server ..." | Re-trigger Greptile

Comment thread packages/cli/src/commands/dev/apiDebugFlag.ts
Comment thread packages/cli/src/commands/dev/apiDebugFlag.ts
Comment thread packages/cli/src/commands/dev/__tests__/dev.test.ts
@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented Mar 25, 2026

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit 6c6305a

Command Status Duration Result
nx run-many -t test --minWorkers=1 --maxWorkers=4 ✅ Succeeded 3m 34s View ↗
nx run-many -t build:pack --exclude create-ceda... ✅ Succeeded 2s View ↗
nx run-many -t test:types ✅ Succeeded 10s View ↗
nx run-many -t build ✅ Succeeded 5s View ↗

☁️ Nx Cloud last updated this comment at 2026-03-25 11:56:15 UTC

@Tobbe Tobbe enabled auto-merge (squash) March 25, 2026 11:30
@Tobbe Tobbe merged commit 017b000 into main Mar 25, 2026
43 checks passed
@Tobbe Tobbe deleted the tobbe-fix-conflicting-debug-ports branch March 25, 2026 11:55
@github-actions
Copy link
Copy Markdown

The changes in this PR are now available on npm.

Try them out by running yarn cedar upgrade -t 4.0.0-canary.13673

@Tobbe Tobbe modified the milestones: next-release-patch, v3.1.0 Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant