Skip to content

✨ Add explicit project targeting for build commands#239

Draft
Robdel12 wants to merge 1 commit intomainfrom
rd/fix-project-selection
Draft

✨ Add explicit project targeting for build commands#239
Robdel12 wants to merge 1 commit intomainfrom
rd/fix-project-selection

Conversation

@Robdel12
Copy link
Copy Markdown
Contributor

Why

The CLI was still mixing user login state and project-targeted build creation, which made local interactive workflows feel broken after removing project:select. This PR makes project targeting explicit and repo-local while keeping CI token flows working.

Summary

  • add explicit target resolution for run, upload, preview, and finalize via --project-id or --org + --project
  • support repo-local target config in vizzly.config.js and send explicit target payloads to the API when using user auth
  • scope login state by apiUrl for cloud and self-hosted setups, and write global config atomically to avoid startup races while reading auth
  • add unit and integration coverage for targeting precedence, api-url-scoped auth, and the upload/finalize user-auth flow

Test Plan

  • node --test tests/commands/targeting.integration.test.js tests/utils/global-config.test.js tests/commands/logout.test.js tests/commands/whoami.test.js tests/commands/run.test.js tests/commands/upload.test.js tests/commands/finalize.test.js tests/commands/preview.test.js tests/utils/project-target.test.js
  • npm test (currently still fails in unrelated tests/utils/colors.test.js cases)

Make run, upload, preview, and finalize resolve an explicit target from flags, repo config, or project-token context.

Also scope login state by API URL and write global config atomically so startup tasks do not race auth reads.
@vizzly-testing
Copy link
Copy Markdown

vizzly-testing bot commented Mar 24, 2026

Vizzly - Visual Test Results

CLI Reporter - Approved

19 comparisons, no changes detected.

View build

CLI TUI - 2 changes need review
Status Count
Changed 2
Auto-approved 3
Changes needing review (2)

vizzly-help · 1202×1430 · 214.9% diff

vizzly-help

vizzly-run-help · 1202×722 · 444.9% diff

vizzly-run-help

Review changes


rd/fix-project-selection · f49e488e

@claude
Copy link
Copy Markdown

claude bot commented Mar 24, 2026

PR Review

Overall this is a solid, well-motivated PR. The architecture is clear, the resolveTargetFromSources precedence chain is easy to follow, the integration test with a real HTTP stub server is a great addition, and the migration path from authauthByApiUrl is handled gracefully. A few things worth looking at before merge:


🐛 Potential Issues

1. normalizeApiUrl(null) is silently safe but looks like a bug

// global-config.js
export function normalizeApiUrl(apiUrl = DEFAULT_AUTH_API_URL) {
  let parsedUrl = new URL(apiUrl || DEFAULT_AUTH_API_URL);

The default parameter only fires for undefined, not null. If any caller passes null, apiUrl || DEFAULT_AUTH_API_URL saves it — but the double-fallback looks accidental. Worth either documenting the intent or unifying to one approach.

2. finalize always requires a target even for project-token users

finalizeCommand calls resolveProjectTarget with requireTarget: true:

let resolvedTarget = await resolveProjectTarget({
  command: 'finalize',
  options,
  config,
  requireTarget: true,
});

When the API key is a project token, resolveProjectTarget returns { source: 'project-token', target: null } — so no error is thrown and config.target stays undefined. That path is fine. But double-check that this is the intended UX: a project-token user running finalize should never be prompted with the "needs a target project" error. The test "keeps project token finalize working without an explicit target" covers it ✓.

3. Concurrent migration race window (minor)

getAuthTokens calls loadMigratedGlobalConfig(true), which will write the config when it migrates authauthByApiUrl. Two processes launching simultaneously right after an upgrade could both read the old config and both try to write. The atomic temp-file rename helps for the write itself, but whichever write lands second would overwrite the first (both writes are identical in this case, so data is safe). Worth a comment if you want to be explicit.


🧹 Code Quality

4. cleanString is duplicated

Identical helper defined in both src/api/core.js and src/utils/project-target.js. Could live in a shared internal util or project-target.js could export it for core.js to import.

5. Two definitions of the default API URL

DEFAULT_AUTH_API_URL = 'https://app.vizzly.dev' in global-config.js and DEFAULT_API_URL in client.js are the same string. If the cloud URL ever changes, it needs updating in two places. One export from client.js (already imported in config-loader.js) would be enough.

6. tokenContext in resolveTargetFromSources appears unused

resolveProjectTarget calls resolveTargetFromSources without a tokenContext:

let resolvedTarget = resolveTargetFromSources({
  options,
  configTarget: config.target,
  // tokenContext not passed
});

The token-context lookup path exists in resolveTargetFromSources but is never reachable from resolveProjectTarget. The test even confirms this ("returns config target without calling token context"). That's fine if it's a placeholder for future token introspection, but worth a // TODO: comment so it doesn't look like a dead branch.

7. Dead options.apiUrl fallback after CLI option move

In loginCommand, logoutCommand, and whoamiCommand:

let apiUrl = globalOptions.apiUrl || options.apiUrl || getApiUrl();

Since --api-url was promoted to a global option this PR, options.apiUrl will always be undefined for these commands going forward. The fallback is harmless but misleading.

8. Inconsistent error display for ValidationError across commands

finalize test asserts:

c.args[0] === 'Failed to finalize parallel build' && c.args[1]?.message?.includes('...')

upload test asserts:

c.args[0].includes('This command needs a target project')

The error appears as the second argument in finalize and as the first in upload. Whichever is intentional, it's worth making consistent.


✅ What's Good

  • The resolveTargetFromSources precedence chain (flag:project-id > flag:slug > config > token-context > project-token passthrough) is clean and well-tested.
  • The Zod schema validation for target (rejecting partial organizationSlug/projectSlug pairs) is exactly the right place to catch config mistakes.
  • The atomic write via rename() is a meaningful correctness improvement.
  • The integration test standing up a real HTTP server and asserting on actual request payloads is excellent — much more confidence than mocking.
  • buildTargetPayload correctly omits target from the build payload for CI/project-token flows (verified by the "CI-style project token" integration test).

One more thing worth confirming: since run now requires a target when using user auth (no requireTarget: false override), this is a breaking change for logged-in users who don't have a target in vizzly.config.js. That's the explicit goal of this PR, but the migration story (error message directing users to add target to config) should be surfaced in docs or a migration note.

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