refactor(node-ui): extract ApproveWalletsModal batch orchestration into a reducer + runner (#1350)#1453
Merged
Merged
Conversation
…to a reducer + runner (#1350) Split the sequential approve-publishing-wallets write machine out of the 801-line ApproveWalletsModal into a pure, separately-testable seam: - `src/ui/pca/approveBatch.ts` (new): - `approveBatchReducer` — the display-state transitions (replaces the 9 batch useStates); `initialApproveBatchState`. - `selectCounts` / `selectApprovedWallets` — pure selectors. #9 lives here: `confirmed` (chain re-read) stays SEPARATE from `submitted` (unconfirmed). - `runApproveBatch(input, deps, dispatch, shouldStop)` — the sequential per-row loop + the ordered error taxonomy, pure of React. Device-prompt counters become runner-local closure vars. - Relocated byte-identical: RowStatus/Row/ROW_LABEL/ROW_TONE + the wallet error classifiers. - `ApproveWalletsModal.tsx`: 9 batch useStates + 2 device refs → one `useReducer`; `run()` is now a thin handler assembling `deps` + awaiting `runApproveBatch`. Input-selection state + the alreadyApprovedHere probe + the entire render are untouched (labels/tones/testids byte-for-byte). Behavior-preserving: all financial invariants preserved exactly — #9 registered≠submitted, owner-gate(403) whole-batch abort, cap abort, cooperative stop finalization, N5-conflict (conflict only on positive registered===false && adapterSupported!==false), stranded-only-after- successful-deregister, sponsor vs dead-sponsor, the two swallow-on-non- wallet-signed-deregister catches, and onApproved() after BATCH_DONE. Re-entrancy: `BATCH_START` (running=true + pending rows) is dispatched SYNCHRONOUSLY at the top of runApproveBatch, before the pre-loop planning awaits — matching the original's synchronous reset. That flip is the sole guard against a concurrent double-submit (the modal footer renders the Approve/Retry trigger only while `!running`). The wallet-batch signing lock is flipped post-planning via a separate `SET_WALLET_BATCH_SIGNING` action. Tests: 15 pure reducer/runner unit tests (`test/pca-approve-batch.test.ts`) pinning every invariant above incl. the synchronous-BATCH_START guard; the 24 existing DOM tests (pca-approve-modal) stay green as the behavior oracle. Fixes #1350 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3swmeQGT8WGttaZsp1NQJ
…ferential revert dep (#1350) R1 review: - (🔴 test) The happy-path ordering test asserted presence/counts, so a regression that fired onApproved before BATCH_DONE, or started both writes concurrently in address order, would still pass. Rewrote it to OBSERVE ordering via controlled register promises + a shared event log: the 2nd write must not START until the 1st RESOLVES, and BATCH_DONE is asserted strictly before onApproved. - (🟡 deps) `describeWalletPcaRevert` is defined in approveBatch.ts yet was routed back through `ApproveBatchDeps` (the modal imported our own export and passed it in). Removed it from the deps contract; the runner calls the module function directly. Behavior-identical (it is a pure fn of its args, and no test path reaches it — describePcaError short-circuits the `??`). Held (design re-architectures beyond this behavior-preserving extraction, tracked for a follow-up): modelling the flow as a discriminated-union / prepared per-row plan (the loose mode/selfCoverage/deregisterFrom flags + deregisterFrom precedence are byte-identical to origin's run() and set mutually-exclusively by the three call sites), and moving all canonical helpers behind default deps. Verified: pca-approve-batch (15) + pca-approve-modal (24) green; full PCA suite 465; tsc src/ui zero-delta (touched files clean); build:ui exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3swmeQGT8WGttaZsp1NQJ
…the runner (#1350) R2 review (🟡): the runner tests stubbed describePcaError to the desired code, so the `?? describeWalletPcaRevert(...)` fallback — the wallet-managed on-chain revert path — was never exercised. A regression that dropped the `??` or broke the revert-name switch would slip through. Added three tests that drive the fallback through runApproveBatch itself (registerAgent rejects with a viem ContractFunctionRevertedError, describePcaError returns null): - AgentCapReached → cap sweep (current + remaining) + break - AgentAlreadyRegistered → N5 ambiguity probe (registered:false + adapter ok → conflict) - NotAccountOwner → per-row error, NOT the whole-batch abort (that is HttpError-403 only) — pins the intentional daemon-403-vs-wallet-revert asymmetry 18 batch tests + 24 DOM tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3swmeQGT8WGttaZsp1NQJ
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
2 participants
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.
Summary
Extracts the sequential approve-publishing-wallets write machine out of the 801-line
ApproveWalletsModalinto a pure, separately-testable reducer + async runner — a structural refactor with no behavior change. This is the #1350 half of the ConvictionDetailView decomposition series (PR6/#1446 landed #1348).src/ui/pca/approveBatch.ts(new):approveBatchReducer+initialApproveBatchState— the display-state transitions, replacing the 9 batchuseStates the modal held.selectCounts/selectApprovedWallets— pure selectors. Gossip ingestion validation is incomplete (I-002) #9 lives here:confirmed(chain re-read) is kept separate fromsubmitted(unconfirmed), never folded.runApproveBatch(input, deps, dispatch, shouldStop)— the sequential per-row loop + the ordered error taxonomy, pure of React (device-prompt counters become runner-local closure vars;depsare injected).RowStatus/Row/ROW_LABEL/ROW_TONE+ the wallet error classifiers are relocated byte-identical.ApproveWalletsModal.tsx: 9 batchuseStates + 2 device refs → a singleuseReducer;run()is now a thin handler that assemblesdepsand awaitsrunApproveBatch. The input-selection state, thealreadyApprovedHereprobe effect, and the entire render (labels, tones, copy,data-testids) are untouched.Behavior-preserving — every financial invariant preserved exactly: #9 registered≠submitted · owner-gate (403 /
NotAccountOwner) whole-batch abort ·AgentCapReachedabort · cooperative stop finalization (unconditionalBATCH_DONE) · N5-conflict (dangerconflictonly on positiveregistered===false && adapterSupported!==false) · stranded-only-after-a-successful-deregister · sponsor vs dead-sponsor distinction · the two swallow-on-non-wallet-signed-deregister catches ·onApproved()fires afterBATCH_DONE, never in the reducer.Re-entrancy / double-submit guard (found by the internal adversarial gate, fixed here): the original set
running=truesynchronously on click, before the async pre-loop planning (resolveWalletBinding×N +signerKindForAccount→fetchPca). Because the footer renders the Approve/Retry trigger only while!running, that synchronous flip is the sole guard against a second concurrent run.BATCH_STARTis therefore dispatched synchronously at the top ofrunApproveBatch(before planning); the wallet-batch signing lock is flipped afterward via a separateSET_WALLET_BATCH_SIGNINGaction — byte-matching the original's synchronous-reset-then-post-planning-signing ordering.Related
Files changed
packages/node-ui/src/ui/pca/approveBatch.tsapproveBatchReducer) + selectors (selectCounts,selectApprovedWallets) + async runner (runApproveBatch) + relocatedROW_LABEL/ROW_TONE/error classifiers.packages/node-ui/src/ui/pages/conviction/ApproveWalletsModal.tsxuseStates + 2 device refs →useReducer;run()reduced to adeps-assembling handler aroundrunApproveBatch. −370 net lines; render/copy/testids untouched.packages/node-ui/test/pca-approve-batch.test.tsBATCH_STARTdouble-submit guard.Test plan
vitest run test/pca-approve-batch.test.ts— 15 pure reducer/runner tests green (transitions · abort policy · N5 · stranded · stop · Gossip ingestion validation is incomplete (I-002) #9 · sync-BATCH_START guard).vitest run test/pca-approve-modal.test.ts— the 24 existing DOM tests stay green as the behavior oracle (incl. "403 aborts the rest, no stuck approving…").tsc -p src/ui/tsconfig.json— zero errors in the touched files (the 32 pre-existingsrc/uierrors are unrelated project-view debt; this lane is not wired to CI).build:ui(production rollup) — exit 0.origin/main): taxonomy + Gossip ingestion validation is incomplete (I-002) #9 accounting CLEAN; the double-submit finding above was caught, fixed, and re-verified CLEAN.kosava-node-ui-e2e).