Skip to content

refactor account handling#12690

Open
vicb wants to merge 2 commits intomainfrom
vicb/refactor-account-functions
Open

refactor account handling#12690
vicb wants to merge 2 commits intomainfrom
vicb/refactor-account-functions

Conversation

@vicb
Copy link
Contributor

@vicb vicb commented Feb 26, 2026

Minor refactoring of the account handling before landing active account.

Coded together with OCOpus

Objective

Improve naming, separation of concerns, and consistency for the account resolution functions.

  1. Rename getAccountChoicesfetchAllAccounts, rename file choose-account.tsfetch-accounts.ts, and extract the CLOUDFLARE_ACCOUNT_ID env var handling out to the caller
  2. Extract getActiveAccountId() — a synchronous, side-effect-free function for quiet account resolution
  3. Rename getAccountIdgetOrSelectAccountId, refactored to use getActiveAccountId then fetchAllAccounts
  4. Fix secrets-store/commands.ts — replace getAccountId with requireAuth (missing login check)
  5. Clean up pages/projects.ts — remove redundant getCloudflareAccountIdFromEnv() prefix
  6. Clean up containers/deploy.ts — remove redundant config.account_id || guard
  7. Add comprehensive JSDoc for all modified functions
  8. Update existing tests and add new tests

  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: internal code

A picture of a cute animal (not mandatory, but encouraged)


Open with Devin

@vicb vicb requested review from a team as code owners February 26, 2026 19:03
@changeset-bot
Copy link

changeset-bot bot commented Feb 26, 2026

⚠️ No Changeset found

Latest commit: 3a164f8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

devin-ai-integration[bot]

This comment was marked as resolved.


const accounts = await fetchAllAccounts(config);
if (accounts.length === 1) {
saveAccountToCache({ id: accounts[0].id, name: accounts[0].name });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the env var value was saved to cache before.

I assume that is only because the code was convoluted. Adding comment on the previous line:

  // vicb: here we have  !getCloudflareAccountIdFromEnv because it has
  // higher priority but was handled in getAccountChoices
	if (cachedAccount && !getCloudflareAccountIdFromEnv()) {
		return cachedAccount.id;
	}

  // vicb: this is now fetchAllAccounts that only fetches
  // env var handling has been extracted out
	const accounts = await getAccountChoices(config);

Copy link
Contributor Author

@vicb vicb Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That turned out to be an happy bug - the only reason why tests in packages/wrangler/src/__tests__/containers/push.test.ts were passing is because the value was cache 🥳

Added a fixup commit properly calling mockAccountId() to fix that

@pkg-pr-new
Copy link

pkg-pr-new bot commented Feb 26, 2026

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@12690

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@12690

miniflare

npm i https://pkg.pr.new/miniflare@12690

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@12690

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@12690

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@12690

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@12690

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@12690

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@12690

wrangler

npm i https://pkg.pr.new/wrangler@12690

commit: 3a164f8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Untriaged

Development

Successfully merging this pull request may close these issues.

1 participant