chore: drop dfx fallbacks and dfx_test_key; unify dev-server preflight across all vite configs - #1480
Merged
Merged
Conversation
AGENTS.md:49 says to always use icp-cli and never dfx, but six frontends still carried dfx-era plumbing. Two of them (cert-var, flying_ninja) had a full `// Try dfx` branch in vite.config.js — `dfx ping` + `dfx canister id backend`, proxying to the dfx replica port 4943 — reached only when the icp-cli branch throws. Removed, along with the "or: dfx start --background && dfx deploy" half of the no-network error and the "or 'dfx deploy'" half of actor.js's missing-canister-ID error. They now match rust/qrcode, which was already icp-cli-only. All six also baked a dfx-only canister-ID fallback into the bundle: `loadEnv(mode, "..", ["CANISTER_"])` feeding a `process.env.CANISTER_ID_BACKEND` define, which actor.js consulted when the ic_env cookie had no canister ID. Those vars come from the `.env` dfx generates on deploy — a file AGENTS.md:91 forbids committing — so the fallback could never fire. Canister IDs now come from the cookie only, which is what icp-cli populates in both `icp deploy` and `vite dev`. Incidentally removed: `host: devConfig.host` in both who_am_i configs, always undefined since getDevServerConfig never returns a host (it was the dfx branch that set one). Kept deliberately: REPLICA_PORT (live — filevault and both who_am_i need it to reach II on the network port rather than the dev-server port), .github/workflows/ninja_pr_checks.yml (ICP Ninja genuinely runs dfx, @dfinity/ninja-devs-owned), and basic_ethereum's "dfx_test_key" (the literal management-canister key name on local replicas, not a dfx dependency). Verified: all 12 files parse; `npm run build` succeeds for rust/qrcode (define removed entirely) and motoko/who_am_i (define retained for REPLICA_PORT), with no unresolved process.env references in either bundle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Supersedes this branch's earlier claim that basic_ethereum's "dfx_test_key" had to stay. It did not: the local network (PocketIC) provisions test_key_1 and key_1 to mimic mainnet, so dfx_test_key is not needed to develop locally, and the repo already uses test_key_1 everywhere it actually names a key. All three remaining mentions were comments, not usage: - rust/basic_ethereum/backend/lib.rs — the InitArg doc comment offered "dfx_test_key" (local dfx) as the local-development choice. Now names only test_key_1 (works on the local network and mainnet) and key_1 (mainnet production), matching threshold-ecdsa and x509. - rust/threshold-schnorr and rust/x509 integration tests — comments noting that with_test_threshold_keys_subnet() also provides dfx_test_key. True but beside the point; both tests use test_key_1. `grep` for dfx_test_key now returns nothing. `cargo check --target wasm32-unknown-unknown` passes for basic_ethereum. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR removes remaining dfx-era frontend plumbing and stale key-name guidance, aligning the examples with the repo rule to use icp-cli only.
Changes:
- Removed
dfxfallback logic andCANISTER_ID_*build-time env injection from affected Vite frontends; canister IDs now come solely from theic_envcookie. - Simplified frontend actor initialization to stop consulting
process.env.CANISTER_ID_BACKEND. - Updated Rust comments/docs to stop mentioning
dfx_test_key.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/x509/backend/tests/integration_tests.rs | Updates PocketIC key-subnet comment to remove dfx_test_key mention. |
| rust/threshold-schnorr/backend/tests/integration_tests.rs | Updates test doc comment to reference only test_key_1. |
| rust/basic_ethereum/backend/lib.rs | Updates InitArg doc comment to remove dfx_test_key guidance. |
| rust/who_am_i/src/frontend/vite.config.js | Removes loadEnv/CANISTER_ID_* injection; keeps REPLICA_PORT define and dev proxy config. |
| rust/who_am_i/src/frontend/src/actor.js | Drops process.env.CANISTER_ID_BACKEND fallback; relies on ic_env cookie only. |
| rust/qrcode/frontend/vite.config.js | Removes loadEnv and the baked CANISTER_ID_* define. |
| rust/qrcode/frontend/src/actor.js | Drops process.env.CANISTER_ID_BACKEND fallback; relies on ic_env cookie only. |
| rust/flying_ninja/frontend/vite.config.js | Removes dfx fallback branch and CANISTER_ID_* define; keeps icp-cli-only path. |
| rust/flying_ninja/frontend/src/actor.js | Drops process.env.CANISTER_ID_BACKEND fallback and removes dfx from the error message. |
| motoko/who_am_i/src/frontend/vite.config.js | Removes loadEnv/CANISTER_ID_* injection; keeps REPLICA_PORT define and dev proxy config. |
| motoko/who_am_i/src/frontend/src/actor.js | Drops process.env.CANISTER_ID_BACKEND fallback; relies on ic_env cookie only. |
| motoko/filevault/frontend/vite.config.js | Removes loadEnv/CANISTER_ID_* injection; keeps REPLICA_PORT define. |
| motoko/filevault/frontend/src/actor.js | Drops process.env.CANISTER_ID_BACKEND fallback; relies on ic_env cookie only. |
| motoko/cert-var/frontend/vite.config.js | Removes dfx fallback branch and CANISTER_ID_* define; keeps icp-cli-only path. |
| motoko/cert-var/frontend/src/actor.js | Drops process.env.CANISTER_ID_BACKEND fallback and removes dfx from the error message. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot flagged this on the two configs this branch already touched, and
it is right — the failure is in fact broader than the review said.
`getDevServerConfig()` wraps both `icp canister status backend -e local
-i` and `icp network status --json` in one try/catch, then reports "No
local network running". But `icp canister status` is the first call and
it fails on a *canister-ID lookup*, before it ever reaches the network:
$ icp canister status frontend -e local -i # network DOWN
Error: failed to lookup canister ID for canister 'frontend' ...
$ icp canister status frontend -e local -i # network UP, not deployed
Error: failed to lookup canister ID for canister 'frontend' ...
Identical either way, so a running-but-undeployed project is told its
network is down. The suggested remedy already covered both cases; only
the diagnosis was wrong.
Reworded in all 15 configs that carry it, not just the two flagged —
motoko/{cert-var,daily_planner,filevault,flying_ninja,random_maze,
superheroes,who_am_i,vetkeys/basic_vetkd,
vetkeys/password_manager_with_metadata} and the rust twins plus
rust/qrcode.
Also tidied the double blank line left where the dfx branch was removed
in cert-var and flying_ninja.
Note for a follow-up: the hello_world / llm_chatbot / evm_block_explorer
/ image-classification / face-recognition configs already have the
better structure — `icp network status` outside the try, and a separate
try/catch around the canister lookup with its own "deploy the backend"
message. Converging the other 15 on that shape would remove the
ambiguity rather than reword it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-on from the error-message fix: rewording the message papered over
a structural problem. Two distinct failure causes shared one try/catch,
so no wording could be accurate.
hello_world's shape was closer but not correct either. It splits the
canister lookup into its own try/catch with a precise message, but
leaves `icp network status` unguarded — a down network there surfaces a
raw "Command failed: icp network status ..." plus a vite stack instead
of a remedy. It also reads the proxy target from `networkStatus.api_url`
rather than hardcoding a port, which the other 15 did not.
All 23 dev-server configs now do the same two-step preflight, each step
guarded separately and each failing with its own message and exit 1:
No local network running. Start it and deploy first:
icp network start -d && icp deploy
Canister 'backend' is not deployed on the local network. Deploy it first:
icp deploy backend
Both `execSync` calls take `stdio: "pipe"` so the raw CLI error is
suppressed and only the actionable message is shown.
The proxy target is now `networkStatus.api_url` everywhere, replacing
the hardcoded `http://127.0.0.1:8000` in 15 configs. That was not merely
cosmetic: on a project configured for another port (or `port: 0` for
parallel worktrees) the dev server was silently proxying to whatever
happened to be on 8000 — possibly a different project's network.
`replicaPort` in filevault and both who_am_i is likewise derived from
`api_url` instead of the "8000" literal, so the Internet Identity URL
they build is right on any port.
Verified end-to-end on rust/qrcode against a network on port 8125:
1. no network → network message, no stack, no raw CLI error
2. network up, no deploy → deploy message (the case Copilot flagged)
3. deployed → dev server starts; GET /api/v2/status through
the proxy returns byte-identical output to a
direct call on 8125, confirming it follows
api_url rather than 8000
All 23 files parse; every one has both guards and none hardcodes a port.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ical
The previous commit unified error handling but left two shapes behind:
15 configs hardcoded `-e local` and the canister name inline, while the
other 8 read `ICP_ENVIRONMENT` and used a `CANISTER_NAME` constant. Only
the latter could target anything but the local network.
All 23 now share the same two module-level constants and a byte-identical
`getDevServerConfig()` (verified: one md5 across all 23, modulo
indentation and the optional replicaPort line):
const CANISTER_NAME = "backend";
const ENVIRONMENT = process.env.ICP_ENVIRONMENT || "local";
Every `icp` invocation and every message now interpolates them, so the
remedy printed is the one that actually applies:
Canister "backend" is not deployed in environment "local". Deploy it first:
icp deploy backend -e local
The 8 inline configs were refactored to call the helper rather than
open-code the preflight, so their `defineConfig` bodies collapse to the
build early-return plus `server: getDevServerConfig()`. Cookie key order
is now uniform too (`ic_root_key` first).
No config hardcodes `-e local`, `127.0.0.1:8000`, or a canister name any
more.
Verified on rust/qrcode (helper group) against a network on port 8126:
1. no network -> environment "local" message
2. ICP_ENVIRONMENT=staging -> environment "staging" message
3. network up, not deployed -> deploy message with `-e local`
4. deployed -> dev server starts; GET /api/v2/status
through the proxy is byte-identical to
a direct call on 8126
And on rust/hello_world (converted inline group): `npm run build` succeeds
without invoking icp, and the dev server prints the network guard.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot spotted that the network guard's suggested command dropped the
environment while the canister guard's kept it, so with ICP_ENVIRONMENT
set the printed remedy pointed at the default environment rather than the
one the dev server had just failed to reach. It was the only remaining
place: across all 23 configs only four icp commands are emitted, and the
other three (`network status`, `canister status`, `deploy <name>`) already
carried `-e ${ENVIRONMENT}`.
icp network start -d -e ${ENVIRONMENT} && icp deploy -e ${ENVIRONMENT}
Checking that also turned up a second case worth separating. `icp network
status -e <env>` fails for two different reasons, and treating both as
"network not running" is only right for one of them:
$ icp network status -e nope --json
Error: project does not contain an environment named 'nope'
A typo'd or undefined ICP_ENVIRONMENT was reported as a stopped network,
with a remedy that could not work — `icp network start -e nope` fails the
same way. The guard now branches on that stderr and says so instead:
Unknown environment "nope". Check ICP_ENVIRONMENT against the
environments in icp.yaml.
For a genuinely non-local environment the assumption holds and needs no
special case: `icp network status -e ic --json` succeeds without any
`environments:` block, returning the mainnet api_url, so the guard passes
and the dev server proxies to mainnet.
Helper remains byte-identical across all 23 (one md5). Verified on
rust/qrcode against a network on port 8127: no network, unknown
environment, network-up-not-deployed, and the deployed happy path with a
byte-identical proxied /api/v2/status.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
marc0olo
marked this pull request as ready for review
August 27, 2026 12:38
lwshang
approved these changes
Aug 27, 2026
eichhorl
approved these changes
Aug 27, 2026
mbjorkqvist
approved these changes
Aug 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
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.
AGENTS.md:49 says to always use icp-cli and never dfx, but the repo still carried dfx-era plumbing in six frontends and three key-name comments. This removes all of it.
1. The dfx fallback branch (
cert-var,flying_ninja)Both
vite.config.jsfiles had a full// Try dfxbranch —dfx ping+dfx canister id backend, proxying/apito the dfx replica port4943— reached only when the icp-cli branch throws. Removed, along with:or:\n dfx start --background && dfx deployhalf of the no-network erroror 'dfx deploy'half ofactor.js's missing-canister-ID errorThey now match
rust/qrcode, which was already icp-cli-only.2. The dead
CANISTER_ID_*env path (all six frontends)Every one of the six baked a dfx-only canister-ID fallback into the bundle:
loadEnv(mode, "..", ["CANISTER_"])feeding aprocess.env.CANISTER_ID_BACKENDdefine, whichactor.jsconsulted when theic_envcookie carried no canister ID.Those vars come from the
.envthat dfx writes on deploy — a file AGENTS.md:91 forbids committing — so the fallback could never fire in this repo. Canister IDs now come from the cookie alone, which is what icp-cli populates in bothicp deployandvite dev.Six examples, not the three first spotted:
motoko/cert-var,motoko/filevault,motoko/who_am_i,rust/flying_ninja,rust/qrcode,rust/who_am_i.3.
dfx_test_keyis no longer advertisedThe local network (PocketIC) provisions
test_key_1andkey_1to mimic mainnet, sodfx_test_keybuys nothing locally — and the repo already usedtest_key_1everywhere it actually names a key (threshold-ecdsa,threshold-schnorr,x509,basic_bitcoin, all the vetkeys examples). The three remaining mentions were comments, not usage:rust/basic_ethereum/backend/lib.rs— theInitArgdoc comment offered"dfx_test_key"(local dfx) as the local-development choice. Now names onlytest_key_1(test key, works on the local network and ICP mainnet) andkey_1(mainnet production), matching howthreshold-ecdsaandx509already phrase it.rust/threshold-schnorrandrust/x509integration tests — comments noting thatwith_test_threshold_keys_subnet()also providesdfx_test_key. True, but beside the point: both tests usetest_key_1.grepfordfx_test_key(and "local dfx") across the repo now returns nothing.4. Incidental
host: devConfig.hostin bothwho_am_iconfigs — alwaysundefined, sincegetDevServerConfig()never returns ahost. It was the dfx branch that set one.5. One dev-server preflight shape across all 23 configs (from review)
Copilot flagged the
getDevServerConfig()throw on the two configs this branch already touched, and it was right — the failure is broader than the review said.icp canister status backend -e local -iis the first call in the try block and it fails on a canister-ID lookup, before it ever reaches the network:Identical either way (verified locally against a running network), so a running-but-undeployed project got told its network was down. The suggested remedy already covered both cases; only the diagnosis was wrong.
Rewording turned out to paper over a structural problem — two distinct causes sharing one try/catch means no wording can be accurate — so all 23 dev-server configs were converged on one preflight shape instead.
hello_world's shape was closer but not correct either: it splits the canister lookup into its own try/catch with a precise message, but leavesicp network statusunguarded, so a down network surfaces a rawCommand failed: icp network status ...plus a vite stack instead of a remedy. It also reads the proxy target fromnetworkStatus.api_urlrather than hardcoding a port, which the other 15 did not.Every config now does the same two-step preflight, each step guarded separately, each failing with its own message and
exit 1:Both
execSynccalls takestdio: "pipe", so the raw CLI error is suppressed and only the actionable message shows.The proxy target is now
networkStatus.api_urleverywhere, replacing hardcodedhttp://127.0.0.1:8000in 15 configs. Not cosmetic: on a project configured for another port — orgateway.port: 0for parallel worktrees — the dev server was silently proxying to whatever sat on 8000, possibly another project's network.replicaPortinfilevaultand bothwho_am_iis likewise derived fromapi_urlinstead of the"8000"literal, so the Internet Identity URL they build is correct on any port.Verified end-to-end on
rust/qrcodeagainst a network on port 8125:GET /api/v2/statusthrough the proxy returns byte-identical output to a direct call on 8125, confirming it followsapi_urland not 8000All 23 files parse; each has both guards and none hardcodes a port.
6. Environment-agnostic and identical
Unifying the error handling still left two shapes: 15 configs hardcoded
-e localand the canister name inline, while the other 8 readICP_ENVIRONMENTand used aCANISTER_NAMEconstant. Only the latter could target anything but the local network.All 23 now share the same two module-level constants and a byte-identical
getDevServerConfig()— verified as one md5 across all 23, modulo indentation and the optionalreplicaPortline:Every
icpinvocation and every message interpolates them, so the printed remedy is the one that actually applies:The 8 inline configs were refactored to call the helper instead of open-coding the preflight, so their
defineConfigbodies collapse to the build early-return plusserver: getDevServerConfig(). Cookie key order is uniform too (ic_root_keyfirst). No config hardcodes-e local,127.0.0.1:8000, or a canister name any more.A follow-up review caught the one command that still dropped the environment — the network guard's
icp network start -d && icp deploy, while the other three (network status,canister status,deploy <name>) already carried-e ${ENVIRONMENT}. Now:That check surfaced a second case worth separating:
icp network status -e <env>fails for two different reasons, and only one is "network not running".A typo'd or undefined
ICP_ENVIRONMENTwas reported as a stopped network, with a remedy that cannot work (icp network start -e nopefails identically). The guard now branches on that stderr and saysUnknown environment "nope". Check ICP_ENVIRONMENT against the environments in icp.yaml.instead. For a genuinely non-local environment the assumption holds with no special case:icp network status -e ic --jsonsucceeds even without anenvironments:block, returning the mainnetapi_url, so the guard passes and the dev server proxies to mainnet.Re-verified on
rust/qrcode(helper group) against a network on port 8127 — no network, unknown environment, network-up-not-deployed, and the deployed happy path with a byte-identical proxied/api/v2/status— and onrust/hello_world(converted inline group), wherenpm run buildsucceeds without invokingicpand the dev server prints the network guard.Kept deliberately
REPLICA_PORT— live, not dead.filevaultand bothwho_am_ineed it to reach II on the network port rather than the dev-server port. Thedefineblock stays in those three for it..github/workflows/ninja_pr_checks.yml— pins dfx 0.31.0 on purpose; ICP Ninja genuinely runs dfx, and it's@dfinity/ninja-devs-owned.Verification
32 files. All changed JS files parse.
npm run buildsucceeds forrust/qrcode(define removed entirely) andmotoko/who_am_i(define retained forREPLICA_PORT); neither bundle has an unresolvedprocess.envreference from our code.cargo check --target wasm32-unknown-unknownpasses forbasic_ethereum.