Fix/dns argument handling#116
Conversation
|
@codex review |
🦋 Changeset detectedLatest commit: 528a73b The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Greptile SummaryThis PR fixes argument-handling across all DNS commands: it stops interactive prompts from firing in non-TTY/JSON output contexts by centralising the TTY check in a new
Confidence Score: 4/5Safe to merge; the two findings are confined to the new interactive update editor and affect only UX, not correctness of the non-interactive path. The core fixes — centralising the interactivity check, rejecting extra positional values, and threading output into resolvers — are clean and consistent. The new interactive field editor in update.ts has a gap for PULLZONE and SCRIPT record types: it surfaces a Value field that doesn't map to the right API field for those types, and it has no path to change the PullZoneId or ScriptId. Additionally, the CAA Tag picker doesn't pre-select the existing value. Neither of these blocks users of the non-interactive flag-based path. packages/cli/src/commands/dns/record/update.ts — the promptFieldChanges function needs type-aware field filtering for PULLZONE/SCRIPT records. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Command invoked] --> B{is ref given?}
B -- Yes --> C[resolveZone via API]
C --> D[Return zone]
B -- No --> E{ignoreManifest?}
E -- No --> F{manifest.id set?}
F -- Yes --> G[Load linked zone]
G --> D
F -- No --> H{isInteractive output?}
E -- Yes --> H
H -- No --> I[throw UserError: zone required]
H -- Yes --> J[Fetch all zones + show picker]
J --> K{offerLink?}
K -- Yes --> L[maybeLinkZone]
L --> D
K -- No --> D
subgraph update.ts
M[dns records update] --> N{hasFieldFlags?}
N -- Yes --> O[Apply flag overrides to API call]
N -- No --> P{isInteractive?}
P -- No --> Q[throw UserError: no changes]
P -- Yes --> R[promptFieldChanges]
R --> O
end
subgraph add.ts
S[dns records add] --> T{args.type given?}
T -- No --> U{isInteractive?}
U -- No --> V[throw UserError: type required]
U -- Yes --> W[Full interactive wizard]
T -- Yes --> X[buildRecord]
X -- Success --> Y[writeAndReport]
X -- UserError + no values + interactive --> Z[promptRecord]
Z --> Y
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Command invoked] --> B{is ref given?}
B -- Yes --> C[resolveZone via API]
C --> D[Return zone]
B -- No --> E{ignoreManifest?}
E -- No --> F{manifest.id set?}
F -- Yes --> G[Load linked zone]
G --> D
F -- No --> H{isInteractive output?}
E -- Yes --> H
H -- No --> I[throw UserError: zone required]
H -- Yes --> J[Fetch all zones + show picker]
J --> K{offerLink?}
K -- Yes --> L[maybeLinkZone]
L --> D
K -- No --> D
subgraph update.ts
M[dns records update] --> N{hasFieldFlags?}
N -- Yes --> O[Apply flag overrides to API call]
N -- No --> P{isInteractive?}
P -- No --> Q[throw UserError: no changes]
P -- Yes --> R[promptFieldChanges]
R --> O
end
subgraph add.ts
S[dns records add] --> T{args.type given?}
T -- No --> U{isInteractive?}
U -- No --> V[throw UserError: type required]
U -- Yes --> W[Full interactive wizard]
T -- Yes --> X[buildRecord]
X -- Success --> Y[writeAndReport]
X -- UserError + no values + interactive --> Z[promptRecord]
Z --> Y
end
Reviews (1): Last reviewed commit: "style: reformat dns.test.ts after placeh..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async function promptFieldChanges( | ||
| existing: DnsRecordModel, | ||
| ): Promise<Partial<UpdateDnsRecordModel>> { | ||
| const fields: { title: string; value: PromptableField | "Disabled" }[] = [ | ||
| { title: `Value (${existing.Value ?? ""})`, value: "Value" }, | ||
| { title: `Name (${recordName(existing.Name)})`, value: "Name" }, | ||
| { title: `TTL (${existing.Ttl ?? "default"})`, value: "Ttl" }, | ||
| ]; | ||
| if (existing.Type === RECORD_TYPES.MX || existing.Type === RECORD_TYPES.SRV) | ||
| fields.push({ | ||
| title: `Priority (${existing.Priority ?? 0})`, | ||
| value: "Priority", | ||
| }); | ||
| if (existing.Type === RECORD_TYPES.SRV) { | ||
| fields.push({ title: `Weight (${existing.Weight ?? 0})`, value: "Weight" }); | ||
| fields.push({ title: `Port (${existing.Port ?? 0})`, value: "Port" }); | ||
| } | ||
| if (existing.Type === RECORD_TYPES.CAA) { | ||
| fields.push({ title: `Flags (${existing.Flags ?? 0})`, value: "Flags" }); | ||
| fields.push({ title: `Tag (${existing.Tag ?? ""})`, value: "Tag" }); | ||
| } | ||
| fields.push({ | ||
| title: `Comment (${existing.Comment || "none"})`, | ||
| value: "Comment", | ||
| }); | ||
| fields.push({ | ||
| title: existing.Disabled ? "Enable the record" : "Disable the record", | ||
| value: "Disabled", | ||
| }); | ||
|
|
||
| const { picked } = await prompts({ | ||
| type: "multiselect", | ||
| name: "picked", | ||
| message: "Fields to change:", | ||
| choices: fields, | ||
| hint: "Space to toggle, Enter to confirm", | ||
| instructions: false, | ||
| }); | ||
| if (!picked || picked.length === 0) { | ||
| throw new UserError("No changes requested."); | ||
| } | ||
|
|
||
| const changes: Partial<UpdateDnsRecordModel> = {}; | ||
| for (const field of picked as (PromptableField | "Disabled")[]) { | ||
| if (field === "Disabled") { | ||
| changes.Disabled = !existing.Disabled; | ||
| continue; | ||
| } | ||
| const spec = FIELD_PROMPTS[field]; | ||
| const { value } = await prompts({ | ||
| type: spec.kind === "tag" ? "select" : spec.kind, | ||
| name: "value", | ||
| message: spec.message, | ||
| ...(spec.kind === "tag" | ||
| ? { choices: CAA_TAGS.map((t) => ({ title: t, value: t })) } | ||
| : { | ||
| initial: | ||
| (field === "Name" | ||
| ? recordName(existing.Name) | ||
| : existing[field]) ?? undefined, | ||
| }), | ||
| }); | ||
| if (value === undefined) throw new UserError(`${field} is required.`); | ||
| changes[field] = field === "Name" && value === "@" ? "" : value; | ||
| } | ||
| return changes; |
There was a problem hiding this comment.
promptFieldChanges always includes Value (and never PullZoneId or ScriptId) regardless of record type. For a PULLZONE or SCRIPT type record the meaningful editable field is PullZoneId/ScriptId, not Value. If a user selects the "Value" option in the picker and enters text, the Object.assign at line 232 sets body.Value on a record that was never supposed to carry that field, while the PullZoneId/ScriptId is absent from the seeded body (the accelerated-zone guard won't match these types). The result sent to the API has no link ID and a spurious Value, which is at minimum confusing and potentially corrupts the record depending on API behaviour.
| ...(spec.kind === "tag" | ||
| ? { choices: CAA_TAGS.map((t) => ({ title: t, value: t })) } | ||
| : { |
There was a problem hiding this comment.
Tag field for CAA records is displayed with the current value in the multiselect list title (Tag (issue)), but the subsequent select prompt doesn't set an initial to pre-select it. The user has to remember the value they just saw and pick it again manually. Passing the existing tag as the initial selection index would make the UX consistent with the numeric fields.
| ...(spec.kind === "tag" | |
| ? { choices: CAA_TAGS.map((t) => ({ title: t, value: t })) } | |
| : { | |
| ...(spec.kind === "tag" | |
| ? { | |
| choices: CAA_TAGS.map((t) => ({ title: t, value: t })), | |
| initial: CAA_TAGS.indexOf( | |
| (existing as { Tag?: string }).Tag as (typeof CAA_TAGS)[number], | |
| ), | |
| } | |
| : { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
3 participants
No description provided.