feat(cli): add app domain commands#33
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements end-to-end custom domain management for preview apps. Changes span public specification documents, CLI type contracts, preview-provider APIs, command handlers with polling, presenters, and comprehensive test coverage across unit, controller, and CLI help tests. ChangesCustom Domain Management Feature
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 3
🤖 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 `@packages/cli/src/controllers/app.ts`:
- Line 822: The domain lookup failure message is hardcoded to use
domainCommandError("show", ...) which mislabels failures for other actions;
update all calls that currently pass the literal "show" (e.g., the call after
resolveDomainByHostname and the occurrences at lines referenced) to pass the
actual command name variable used by the surrounding handler (e.g., command,
cmdName, or actionName) so domainCommandError receives the real operation
(remove/retry/wait/list/etc.) instead of "show"; search for
domainCommandError("show", ...) and replace the first argument with the correct
operation variable in the surrounding function (and ensure that variable is in
scope or add a parameter if needed) so error messages accurately reflect the
attempted command.
In `@packages/cli/src/presenters/app.ts`:
- Around line 535-545: The current domainFixFields function always returns DNS
remediation text whenever domain.failureReason exists, which can mislead users
for non-DNS failures; update domainFixFields (the function named
domainFixFields) to first check that the failure reason/category indicates a
DNS-related failure (e.g., domain.failureReason.category === "dns" or
domain.failureReason.type === "dns" depending on the domain shape) and only then
construct the DNS instruction using domain.dnsRecords[0] (falling back to {
type: "CNAME", name: domain.hostname, value: "edge.prisma.app" }); for any other
failure category return an empty array. Ensure you reference
domain.failureReason and domain.dnsRecords in the conditional so non-DNS
failures don't get DNS remediation text.
In `@packages/cli/tests/app.test.ts`:
- Around line 68-85: The test currently calls executeCli for "app domain
--help", "app domain add --help", and "app domain wait --help" but misses the
other domain subcommands; update the tests to also invoke executeCli for "app
domain show --help", "app domain remove --help", and "app domain retry --help"
so help output for those commands is validated; add the same three executeCli
calls in the second help block (the one around the 165-176 region) to keep both
help assertion groups symmetric, referencing the executeCli helper and the argv
arrays to locate where to add the new calls.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 7a5f37da-8826-41eb-ad71-ac5f3bd4560a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
docs/product/command-principles.mddocs/product/command-spec.mddocs/product/error-conventions.mdpackages/cli/package.jsonpackages/cli/src/commands/app/index.tspackages/cli/src/controllers/app.tspackages/cli/src/lib/app/preview-provider.tspackages/cli/src/presenters/app.tspackages/cli/src/shell/command-meta.tspackages/cli/src/types/app.tspackages/cli/tests/app-controller.test.tspackages/cli/tests/app-provider.test.tspackages/cli/tests/app.test.ts
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 `@packages/cli/src/controllers/app.ts`:
- Around line 1662-1678: The code unconditionally emits a concrete CNAME
remediation when DNS errors occur; change the logic so that when
isDomainDnsError(error) is true you first inspect the error for a
provider-reported CNAME target (or any extractable target) and only call
domainDnsNotConfiguredError(hostname, error) / include a CNAME-specific
nextSteps if that target exists; otherwise return a generic CliError (using
CliError with code "NO_DEPLOYMENTS", domain "app", summary/why/fix/debug via
formatDebugDetails(error)) that omits the CNAME instruction and provides a
generic DNS remediation and non-actionable nextSteps; apply the same conditional
handling in the other similar block (the section around the other occurrence
noted in the review).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 7a45098d-c7a2-40bf-a28b-c61c4b52dd3c
📒 Files selected for processing (4)
docs/product/command-spec.mddocs/product/error-conventions.mdpackages/cli/src/controllers/app.tspackages/cli/tests/app-controller.test.ts
luanvdw
reviewed
May 26, 2026
Member
luanvdw
left a comment
luanvdw
left a comment
There was a problem hiding this comment.
Thanks @AmanVarshney01, the CLI shape is looking good!
Can we do an additional pass on DNS guidance? where possible, the CLI must always give a truthful next step after app domain add/show. IIUC right now it can omit records when the API omits dnsRecords, and it can print CNAME fixes for non-CNAME/non-DNS failures. Can we either rely on authoritative dnsRecords from the API, or make the fallback a tested provider contract?
I’d also fix the hardcoded "show" error label in shared hostname lookup. After that I think this achieves the J20 CLI objective without expanding scope. wdyt?
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/presenters/app.ts (1)
320-320:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign failure tone with failure category presence.
Line 320 still uses
failureReasononly for tone, but Line 535 now returnsfailureCategoryeven when reason is absent. That can render a real failure as dim.Proposed fix
- { key: "failure", value: formatDomainFailure(result.domain), tone: result.domain.failureReason ? "error" : "dim" }, + { + key: "failure", + value: formatDomainFailure(result.domain), + tone: (result.domain.failureCategory || result.domain.failureReason) ? "error" : "dim", + },Also applies to: 533-536
🤖 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 `@packages/cli/src/presenters/app.ts` at line 320, The tone for the "failure" field currently checks only result.domain.failureReason which misses cases where failureCategory is present; update the tone logic where the object uses formatDomainFailure(result.domain) (and the other occurrence around the block that previously checked failureReason at the same area) to use failureCategory OR failureReason (e.g., tone set when result.domain.failureCategory || result.domain.failureReason) so any real failure indicated by failureCategory renders as "error" instead of "dim".
🤖 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.
Outside diff comments:
In `@packages/cli/src/presenters/app.ts`:
- Line 320: The tone for the "failure" field currently checks only
result.domain.failureReason which misses cases where failureCategory is present;
update the tone logic where the object uses formatDomainFailure(result.domain)
(and the other occurrence around the block that previously checked failureReason
at the same area) to use failureCategory OR failureReason (e.g., tone set when
result.domain.failureCategory || result.domain.failureReason) so any real
failure indicated by failureCategory renders as "error" instead of "dim".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 62f3aab1-9f35-458e-b1c0-da4b6a2bb0fe
📒 Files selected for processing (6)
docs/product/command-spec.mdpackages/cli/src/controllers/app.tspackages/cli/src/lib/app/domain-guidance.tspackages/cli/src/presenters/app.tspackages/cli/tests/app-controller.test.tspackages/cli/tests/app-presenter.test.ts
AmanVarshney01
force-pushed
the
aman/app-domain-cli
branch
from
17618f5 to
10d7672
Compare
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 `@docs/product/command-spec.md`:
- Line 860: Update the documented exit codes so the timeout uses the
runtime-failure code instead of the usage/config code: change the sentence
"exits 0 on `active`, 1 on terminal `failed`, and 2 on timeout" so that timeout
returns 1 (e.g., "exits 0 on `active`, 1 on terminal `failed` and on timeout;
reserve 2 for usage/config errors"). Also scan nearby examples or tables in this
spec to ensure any other mentions of timeout exit code are updated to 1.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1cc6d0da-c302-4126-bd29-3430250bad36
📒 Files selected for processing (14)
docs/product/command-principles.mddocs/product/command-spec.mddocs/product/error-conventions.mdpackages/cli/src/commands/app/index.tspackages/cli/src/controllers/app.tspackages/cli/src/lib/app/domain-guidance.tspackages/cli/src/lib/app/preview-provider.tspackages/cli/src/presenters/app.tspackages/cli/src/shell/command-meta.tspackages/cli/src/types/app.tspackages/cli/tests/app-controller.test.tspackages/cli/tests/app-presenter.test.tspackages/cli/tests/app-provider.test.tspackages/cli/tests/app.test.ts
AmanVarshney01
force-pushed
the
aman/app-domain-cli
branch
from
10d7672 to
5277b70
Compare
luanvdw
previously approved these changes
May 27, 2026
Member
luanvdw
left a comment
luanvdw
left a comment
There was a problem hiding this comment.
@AmanVarshney01 thanks for the follow-up changes, this is in a great shape!
Would you take a look at the below feedback, and see if we can either include it in this PR, or tackle as follow up work? I'm approving the PR so long, as these are non-blocking changes.
- Can we handle the
domain already registered elsewhereas a more guided error path?
addDomaincurrently treats409as idempotent if the hostname is found on the selected app, which is good. But if the hostname is registered on a different app/project/workspace, it falls through to the generic “custom domain add failed” path and suggestsapp domain show <hostname>. Sinceshowresolves within the selected app, that can send the user/agent into a loop: show says not found, add says conflict, etc. I’d make this a specific recovery message: “This hostname is already registered outside the selected app. Select the owning app, remove it there, or contact support.” We don’t need list/transfer/reassign in this PR, just a better dead-end.
2 smaller nits:
renderAppDomainShowtones the failure row as error only whenfailureReasonexists, butformatDomainFailurecan show afailureCategorywithout a reason. IffailureCategory || failureReasonis present, the row should render as an error.- can we make
retryoutput a bit more informative?app domain retrycurrently prints hostname/status only. Since retry is part of the recovery path, it would be useful to include DNS records and/or the same failure/fix fields when present, similar toshow. Not blocking, but it would make the command feel less like “now run show to understand what happened.”
AmanVarshney01
force-pushed
the
aman/app-domain-cli
branch
from
fc650fe to
0f4772b
Compare
luanvdw
approved these changes
May 27, 2026
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
prisma-cli app domaincommands for add, show, remove, retry, and wait