[DX-961] Fix auth keys commands --app flag to resolve app names to IDs#173
[DX-961] Fix auth keys commands --app flag to resolve app names to IDs#173umair-ably wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request refactors app ID handling across authentication key commands by replacing manual app-id retrieval and validation logic with calls to a common Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR updates the auth:keys:* CLI commands to allow --app to accept either an app ID or an app name, centralizing app selection via requireAppId() and updating unit tests/docs accordingly.
Changes:
- Switch key commands to use
requireAppId()for app selection (enabling app name → ID resolution via Control API). - Add a reusable
mockAppResolution()test helper and update unit tests to mock the new resolution flow. - Update README/help text to document
--appas “ID or name”.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/commands/auth/keys/create.ts |
Uses requireAppId() so --app can be a name or ID. |
src/commands/auth/keys/current.ts |
Uses requireAppId() so --app can be a name or ID. |
src/commands/auth/keys/get.ts |
Resolves --app via name→ID (but currently after auth-info display). |
src/commands/auth/keys/list.ts |
Uses requireAppId() for name→ID resolution (but currently after auth-info display). |
src/commands/auth/keys/revoke.ts |
Uses requireAppId() for consistent app selection / name support. |
src/commands/auth/keys/switch.ts |
Uses requireAppId() for consistent app selection / name support. |
src/commands/auth/keys/update.ts |
Uses requireAppId() for consistent app selection / name support. |
test/helpers/control-api-test-helpers.ts |
Adds mockAppResolution() helper for app-resolution-related nock mocks. |
test/unit/commands/auth/keys/*.test.ts |
Updates tests to mock app resolution flow and adjusts expectations. |
README.md |
Updates flag docs to “app ID or name”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
umair-ably
force-pushed
the
DX-961-app-flag
branch
from
0a7d471 to
61435d6
Compare
![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
🧹 Nitpick comments (3)
test/unit/commands/auth/keys/list.test.ts (1)
86-122: Cover the non-matching app-name branch too.These additions protect the happy path (
--app MyApp) and the empty-account path (No apps found), butresolveAppIdFromNameOrId()also has a separate user-facing failure when apps exist and none match the supplied name. Adding a--app MissingAppcase here would keep that branch covered as well.Also applies to: 171-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/auth/keys/list.test.ts` around lines 86 - 122, Add a test that covers the "apps exist but none match the supplied name" branch by calling the same command flow used in the existing test but passing a non-existent app name (e.g., ["auth:keys:list", "--app", "MissingApp"]). Reuse the same mocked /v1/me and /v1/accounts/:accountId/apps responses (returning appsReply with MyApp) and assert that the CLI prints the user-facing failure message produced by resolveAppIdFromNameOrId (the "no matching app" / "No apps matching" style error), instead of proceeding to /v1/apps/:appId/keys; ensure you do not mock the keys endpoint for this case and verify stderr/stdout contains the expected error text.test/unit/commands/auth/keys/current.test.ts (1)
1-7: Prefer the shared unit-test cleanup instead of adding a file-localafterEach.This hook duplicates lifecycle work that the suite already centralizes in
test/unit/setup.ts, and it makes this file an exception to the standard unit-test pattern.Based on learnings, "Per-file afterEach cleanup hooks are not needed for unit tests because the global setup resets all mock state before every test."🧹 Suggested cleanup
-import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect } from "vitest"; @@ -import { - mockAppResolution, - controlApiCleanup, -} from "../../../../helpers/control-api-test-helpers.js"; +import { mockAppResolution } from "../../../../helpers/control-api-test-helpers.js"; @@ - afterEach(() => { - controlApiCleanup(); - }); -Also applies to: 15-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/auth/keys/current.test.ts` around lines 1 - 7, Remove the file-local afterEach cleanup hook and its import from this test (the afterEach import from "vitest" and the per-file call that invokes controlApiCleanup), relying on the shared global cleanup in test/unit/setup.ts; find and delete the afterEach(...) usage and any direct calls to controlApiCleanup inside this file, and remove the now-unused afterEach import so the file follows the standard global unit-test lifecycle.test/unit/commands/auth/keys/update.test.ts (1)
90-118: Exercise--appwith an app name here, not the resolved ID.This still passes
appIdinto--app, so it only proves the command can re-resolve an ID. The regression this PR is fixing is resolving a name and then patching/v1/apps/{resolvedId}/keys/...; if the implementation accidentally usedflags.appdownstream, this test would still stay green.🧪 Minimal test tweak
- it("should update key with --app flag", async () => { + it("should update key when --app is an app name", async () => { const appId = getMockConfigManager().getCurrentAppId()!; mockAppResolution(appId); @@ const { stdout } = await runCommand( - ["auth:keys:update", mockKeyId, "--app", appId, "--name=UpdatedName"], + ["auth:keys:update", mockKeyId, "--app", "Test App", "--name=UpdatedName"], import.meta.url, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/auth/keys/update.test.ts` around lines 90 - 118, The test currently passes the resolved app ID into the CLI via the --app flag, so change it to pass an app name instead: create a const appName (a human-readable name) and keep appId as the resolved ID; call mockAppResolution(appName, appId) or mockAppResolution(appId) adjusted to map the name to the ID, leave mockKeysList and the nock patch targeting appId, and invoke runCommand with ["auth:keys:update", mockKeyId, "--app", appName, "--name=UpdatedName"]; update assertions if they reference the input flag value. Reference symbols: getMockConfigManager(), mockAppResolution(...), mockKeysList(...), runCommand(...), mockKeyId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/auth/keys/update.ts`:
- Around line 43-52: The code calls requireAppId(flags) before validating flags
and outside the existing try/catch, which can cause remote app resolution errors
to surface instead of local validation failures; move the call to
requireAppId(flags) so it runs after local validation of --name/--capabilities
and inside the existing try/catch/error wrapper used by keyUpdate (i.e., defer
app resolution after parseKeyIdentifier(args.keyName) and after validation, then
call requireAppId(flags) within the try block and use this.fail(...) on errors),
and apply the same change for the similar block referenced around the other
occurrence (lines ~54-61) to ensure all fallible lookups are wrapped.
---
Nitpick comments:
In `@test/unit/commands/auth/keys/current.test.ts`:
- Around line 1-7: Remove the file-local afterEach cleanup hook and its import
from this test (the afterEach import from "vitest" and the per-file call that
invokes controlApiCleanup), relying on the shared global cleanup in
test/unit/setup.ts; find and delete the afterEach(...) usage and any direct
calls to controlApiCleanup inside this file, and remove the now-unused afterEach
import so the file follows the standard global unit-test lifecycle.
In `@test/unit/commands/auth/keys/list.test.ts`:
- Around line 86-122: Add a test that covers the "apps exist but none match the
supplied name" branch by calling the same command flow used in the existing test
but passing a non-existent app name (e.g., ["auth:keys:list", "--app",
"MissingApp"]). Reuse the same mocked /v1/me and /v1/accounts/:accountId/apps
responses (returning appsReply with MyApp) and assert that the CLI prints the
user-facing failure message produced by resolveAppIdFromNameOrId (the "no
matching app" / "No apps matching" style error), instead of proceeding to
/v1/apps/:appId/keys; ensure you do not mock the keys endpoint for this case and
verify stderr/stdout contains the expected error text.
In `@test/unit/commands/auth/keys/update.test.ts`:
- Around line 90-118: The test currently passes the resolved app ID into the CLI
via the --app flag, so change it to pass an app name instead: create a const
appName (a human-readable name) and keep appId as the resolved ID; call
mockAppResolution(appName, appId) or mockAppResolution(appId) adjusted to map
the name to the ID, leave mockKeysList and the nock patch targeting appId, and
invoke runCommand with ["auth:keys:update", mockKeyId, "--app", appName,
"--name=UpdatedName"]; update assertions if they reference the input flag value.
Reference symbols: getMockConfigManager(), mockAppResolution(...),
mockKeysList(...), runCommand(...), mockKeyId.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ad8e1b59-cd3b-455f-809a-37f7956f74ee
📒 Files selected for processing (16)
README.mdsrc/commands/auth/keys/create.tssrc/commands/auth/keys/current.tssrc/commands/auth/keys/get.tssrc/commands/auth/keys/list.tssrc/commands/auth/keys/revoke.tssrc/commands/auth/keys/switch.tssrc/commands/auth/keys/update.tstest/helpers/control-api-test-helpers.tstest/unit/commands/auth/keys/create.test.tstest/unit/commands/auth/keys/current.test.tstest/unit/commands/auth/keys/get.test.tstest/unit/commands/auth/keys/list.test.tstest/unit/commands/auth/keys/revoke.test.tstest/unit/commands/auth/keys/switch.test.tstest/unit/commands/auth/keys/update.test.ts
umair-ably
force-pushed
the
DX-961-app-flag
branch
from
61435d6 to
812b3a2
Compare
umair-ably
force-pushed
the
DX-961-app-flag
branch
from
812b3a2 to
c1b28dd
Compare
|
@coderabbitai review |
Summary by CodeRabbit
Release Notes
New Features
--appflag in key management commands now accepts app names in addition to app IDs for easier reference.Improvements