fix(app): tell the user localhost means the container, not their machine (#1346) - #1347
Conversation
The most common thing a user does after `neoboard demo` is connect their own
database. On a Docker install that database is on the HOST, so they type
neo4j://localhost:7688 — and inside the app container localhost is the
container. Neo4j says "Could not perform discovery. No routing servers
available", which classified as `network`, whose hint told them to verify the
host, the port, that the database is running, and their firewall.
All four are already correct. The message, the docs and the form never
mention Docker, so there was no thread to pull.
Now classified as container_loopback when the error is a network failure AND
the URI host is loopback AND the app is containerised. The hint names
host.docker.internal and the service-name alternative.
All three conditions matter. In LOCAL mode the app runs on the host, where
localhost is exactly right — telling that user to use a Docker hostname sends
them somewhere that does not exist for them. And it narrows `network` only,
never overriding auth or bad_uri: a loopback auth failure is still an auth
failure.
The host is parsed, not substring-matched. "myhost-localhost.example.com"
contains "localhost" and is not loopback. An unparseable URI returns false
rather than throwing — this runs on an error path, where a throw would
replace a bad message with a 500.
Second half, and the reason the hint would otherwise be a new bug:
host.docker.internal resolves automatically on Docker Desktop but NOT on
Linux without an explicit host-gateway mapping, and no compose file had one.
A hint naming a hostname that does not resolve is worse than no hint. Added
to all three compose files that run the app, with an assertion so its absence
cannot go unnoticed — proven by deleting one and watching the check name it.
The URI reaches the classifier and goes no further: it can carry a password,
so a test pins that neither it nor the credentials appear in the result.
Verified against the running stack before writing any of this:
from inside neoboard-app, localhost:7688 -> ECONNREFUSED
host.docker.internal:7688 -> OPEN, 6053 nodes
Verified: 3203 app tests, typecheck, lint.
Closes #1346
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughConnection tests now classify containerized loopback failures separately and provide Docker-specific guidance. The CLI adds an opt-in ChangesContainer loopback classification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectionTestRoute
participant isContainerised
participant connectionTestErrorResult
participant classifyConnectionError
ConnectionTestRoute->>isContainerised: detect runtime
ConnectionTestRoute->>connectionTestErrorResult: pass error, URI, and container status
connectionTestErrorResult->>classifyConnectionError: classify driver message with context
classifyConnectionError-->>connectionTestErrorResult: return container_loopback or existing code
connectionTestErrorResult-->>ConnectionTestRoute: return sanitized result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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
🤖 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 `@scripts/__tests__/docs-accuracy.test.mjs`:
- Around line 233-237: Update the missing-file validation around the withApp
filter to parse each Compose file as YAML and inspect the app service’s
extra_hosts configuration. Require that the app service contains the
host.docker.internal:host-gateway mapping, rather than accepting matches in
comments or unrelated sections; preserve the existing missing-file behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1741e9c1-af16-444b-8d4e-2a6b08cb8831
⛔ Files ignored due to path filters (3)
docker/docker-compose.full.ymlis excluded by!docker/**docker/docker-compose.prod-full.ymlis excluded by!docker/**docker/docker-compose.prod.ymlis excluded by!docker/**
📒 Files selected for processing (9)
app/src/app/api/connections/[id]/test/route.tsapp/src/app/api/connections/test-inline/route.tsapp/src/lib/connector/__tests__/connection-error-classifier.test.tsapp/src/lib/connector/__tests__/connection-test-result.test.tsapp/src/lib/connector/__tests__/is-containerised.test.tsapp/src/lib/connector/connection-error-classifier.tsapp/src/lib/connector/connection-test-result.tsapp/src/lib/connector/is-containerised.tsscripts/__tests__/docs-accuracy.test.mjs
…e localhost Two corrections to the first cut of this fix. 1. Host access is now a CLI flag, not a hardcoded compose line. `neoboard start --full --expose-host` layers docker-compose.expose-host.yml, which is the only file carrying the host-gateway mapping. Off by default: most installs never need it — a database in the same compose network is reached by its service name, a remote one by its hostname — and it routes from the container out to the host's network, which is not something to enable for everyone. The overlay is layered AFTER the base file; compose merges left to right, so an overlay listed first would be silently overridden and do nothing. Tested. A second test asserts the overlay is the ONLY carrier of the mapping — if a base file also had it, the flag would appear to work while doing nothing, and deleting the overlay would break nothing visible until a Linux user hit it. 2. The hint said "not your machine", which is wrong on a deployed instance. The connection is opened by the NeoBoard SERVER, not the browser. A user on their own PC connecting to a NeoBoard deployed on a server means the SERVER's localhost — and host.docker.internal is the server's host too, not theirs. The old copy read as though their laptop were reachable. It is not, and there is no flag that makes it so. The hint now distinguishes three cases: the database on the app's own host (--expose-host + host.docker.internal), in the same Docker network (service name), and on the user's own computer with NeoBoard deployed elsewhere — not reachable at all, expose it at a routable address first. That third case had no answer before and would have sent a remote user chasing a hostname forever. Verified against the running stack with the flag active: neo4j://host.docker.internal:7688 OK bolt://host.docker.internal:7688 OK neo4j://localhost:7688 container_loopback (hint fires) neo4j://neo4j:7687 OK (service name still works) And without the flag: compose resolves 0 host-gateway entries, 1 with it. Verified: 3203 app tests, 338 CLI tests, typecheck, lint. Refs #1346 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cli/src/__tests__/commands/start.test.ts`:
- Around line 277-284: Move the start command tests from
cli/src/__tests__/commands/start.test.ts to
cli/src/commands/__tests__/start.test.ts, preserving their coverage and updating
imports as needed. Move the Docker tests from
cli/src/__tests__/lib/docker.test.ts to cli/src/lib/__tests__/docker.test.ts,
likewise preserving behavior and correcting relative imports.
In `@cli/src/commands/start.ts`:
- Line 46: Validate the exposeHost option in the start command so --expose-host
is rejected unless Docker full-stack mode (--full) is enabled. Cover both
invalid combinations—local mode with --expose-host and --expose-host without
--full—while preserving valid full-stack behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fdd5ae75-3f10-48a1-8909-827ff049de01
⛔ Files ignored due to path filters (1)
docker/docker-compose.expose-host.ymlis excluded by!docker/**
📒 Files selected for processing (8)
app/src/lib/connector/__tests__/connection-error-classifier.test.tsapp/src/lib/connector/connection-error-classifier.tscli/src/__tests__/commands/start.test.tscli/src/__tests__/lib/docker.test.tscli/src/commands/start.tscli/src/index.tscli/src/lib/docker.tsscripts/__tests__/docs-accuracy.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/lib/connector/tests/connection-error-classifier.test.ts
- app/src/lib/connector/connection-error-classifier.ts
| it("passes --expose-host through to compose (#1346)", () => { | ||
| // Off by default above; on only when asked for. | ||
| return runStart({ full: true, exposeHost: true }).then(() => { | ||
| expect(mockComposeUp).toHaveBeenCalledWith({ | ||
| full: true, | ||
| exposeHost: true, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Co-locate these tests with their sources.
cli/src/__tests__/commands/start.test.ts#L277-L284: move tocli/src/commands/__tests__/start.test.ts.cli/src/__tests__/lib/docker.test.ts#L99-L134: move tocli/src/lib/__tests__/docker.test.ts.
As per coding guidelines, tests live in an __tests__/ directory next to the file under test, within the same package.
📍 Affects 2 files
cli/src/__tests__/commands/start.test.ts#L277-L284(this comment)cli/src/__tests__/lib/docker.test.ts#L99-L134
🤖 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 `@cli/src/__tests__/commands/start.test.ts` around lines 277 - 284, Move the
start command tests from cli/src/__tests__/commands/start.test.ts to
cli/src/commands/__tests__/start.test.ts, preserving their coverage and updating
imports as needed. Move the Docker tests from
cli/src/__tests__/lib/docker.test.ts to cli/src/lib/__tests__/docker.test.ts,
likewise preserving behavior and correcting relative imports.
Source: Coding guidelines
… properly Two CodeRabbit findings, both valid. The Major one is a real bug I shipped. --expose-host overlays extra_hosts onto the `neoboard` service, which only the FULL compose defines. Without --full the overlay lands on a service that does not exist and compose refuses the entire project: service "neoboard" has neither an image nor a build context specified: invalid compose project An error about the wrong thing entirely — nothing in it says "you forgot --full". Verified against real `docker compose config` before and after. Local mode is rejected too, with a different reason: the app runs on the host there, so localhost already reaches the databases and the flag has nothing to do. The Minor one: the compose assertion was a raw substring check, which would also pass from a comment, an unrelated service, or malformed YAML — and a mapping on the WRONG service is precisely the failure it guards against. Now resolves the project with `docker compose config --format json` and asserts extra_hosts on the app service specifically. Declined, with reason: "co-locate these tests with their sources". CLAUDE.md does say tests live next to the file under test, but the cli/ package centralises every test under cli/src/__tests__/ — all 30 files. Moving one would make that package inconsistent rather than compliant. The convention gap is real and worth settling repo-wide; doing it inside this PR would bury a networking fix under a test migration. Verified: 341 CLI tests, 42 script tests, lint. And the real binary: `neoboard start --expose-host` now prints the fix instead of a compose error. Refs #1346 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All three addressed. Major — An error about the wrong thing entirely — nothing in it says "you forgot Minor — assert on the app service, not anywhere in the file. Correct: the raw substring would also pass from a comment, an unrelated service, or malformed YAML, and a mapping on the wrong service is exactly the failure that check exists to catch. Now resolves the project with Minor — co-locate tests with sources: declining, with reason. CLAUDE.md does say tests live next to the file under test, but the 341 CLI tests, 42 script tests, lint clean. |
CI failed on the check I had just made more rigorous. It passed `--env-file docker/.env`, which is gitignored and CLI-generated: present on my machine, absent in CI. couldn't find env file: /home/runner/work/neoboard/neoboard/docker/.env This is the same trap as #1221, in the test written to avoid traps. There I found that a `${VAR:?}` guard appeared not to work because docker/.env silently supplied the value; here the same file silently made a check pass. A test that reads a generated file is testing the machine it runs on. Now supplies the compose file's three required vars as placeholders in the child process env, so the check is self-contained. Verified the way it should have been the first time: moved docker/.env aside, ran it, watched it pass. Refs #1346 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third failure on this one check, and the most avoidable. Compose normalises extra_hosts differently across versions: CI emits host.docker.internal=host-gateway where this machine emits `:`. A literal comparison against my own compose's output is a check that asserts my Docker version. Now normalises the separator and accepts the map shape too, so it compares what the entry MEANS. Verified against all three forms — array with `:`, array with `=`, host->target object — plus a negative case, rather than against whatever this machine happens to print. Worth noting what the three failures actually were, because none was noise: the substring check would have passed from a comment; the --env-file dependency made it pass only where a gitignored file existed; and the literal comparison pinned one compose version. Each iteration asserted something narrower than the thing I meant. Refs #1346 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
2 participants
What
The most common thing a user does after
neoboard demois connect their own database. On a Docker install that database is on the host, so they typeneo4j://localhost:7688— and inside the app containerlocalhostis the container.The driver says
Could not perform discovery. No routing servers available. That classified asnetwork, whose hint told them to "verify the host and port, confirm the database is running, and check that no firewall is blocking the connection."All four are already correct. The host is right, the port is right, the database is running and reachable, no firewall involved. Nothing in the message, the docs, or the form mentions Docker. There is no thread to pull.
How
A new
container_loopbackcode, when the error is a network failure and the URI host is loopback and the app is containerised (/.dockerenv).All three conditions matter:
localhostis exactly right — telling that user to use a Docker hostname sends them somewhere that does not exist for them. That regression test is the important one.network, never overridingauth_failedorbad_uri. A loopback auth failure is still an auth failure.myhost-localhost.example.comcontains"localhost"and is not loopback. An unparseable URI returns false rather than throwing — this runs on an error path, where a throw replaces a bad message with a 500.The half that would otherwise have made this a new bug
host.docker.internalresolves automatically on Docker Desktop but not on Linux without an explicithost-gatewaymapping — and no compose file had one:A hint naming a hostname that does not resolve is worse than no hint. Added to all three compose files that run the app, with an assertion so its absence can't go unnoticed — proven by deleting it from one file and watching the check name that file.
Security
The URI reaches the classifier and goes no further; it can carry a password. A test pins that neither the credentials nor the host appear in the returned result.
Verified against the running stack, before writing any of this
3203 app tests, typecheck, lint clean.
Closes #1346
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--expose-hostflag to the start command to maphost.docker.internaland support connecting to services on the same machine from within containers.Bug Fixes
localhost/loopback addresses.Tests