Skip to content

feat: Changing pre-filled value for interact tab#950

Open
C0mberry wants to merge 3 commits intosolana-foundation:masterfrom
hoodieshq:feat-prefilled-value-update
Open

feat: Changing pre-filled value for interact tab#950
C0mberry wants to merge 3 commits intosolana-foundation:masterfrom
hoodieshq:feat-prefilled-value-update

Conversation

@C0mberry
Copy link
Copy Markdown
Contributor

@C0mberry C0mberry commented Apr 16, 2026

Description

  • updating wallet id value for fields in interact tab

Type of change

  • New feature

Screenshots

Screen.Recording.2026-04-16.at.14.24.17.mov

Testing

  1. open program
  2. go to interact tab
  3. connect 2 wallet account
  4. change them and see how values are changing

Related Issues

HOO-263

Checklist

  • My code follows the project's style guidelines
  • All tests pass locally and in CI
  • I have run build:info script to update build information
  • CI/CD checks pass
  • I have included screenshots for protocol screens (if applicable)

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 16, 2026

@C0mberry is attempting to deploy a commit to the Solana Foundation Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 16, 2026

Greptile Summary

This PR refactors wallet address pre-filling in the IDL interact tab so that switching wallets updates signer fields that were previously auto-filled, while leaving fields the user edited manually untouched. The createWalletPrefillDependency function is extracted from the generic useFormPrefill subscription loop and driven directly by a useEffect in InteractInstruction, with a lastPrefillAddressRef tracking the last auto-filled address.

The implementation logic is correct, but the new hasPreviousWallet branch — the key behavioral change — is never reached in the updated test suite because all test cases supply makeRef() (an undefined ref). A dedicated test for the wallet-switching scenario would round out coverage.

Confidence Score: 5/5

Safe to merge — the implementation is correct and the only finding is missing test coverage for the new wallet-switching path.

All findings are P2 (missing test for new branch). The logic itself is sound: hasPreviousWallet correctly identifies auto-filled fields, updates them on wallet switch, and leaves user-edited fields untouched. No correctness or data-integrity issues were found.

wallet-prefill-provider.spec.ts — add a test that exercises the hasPreviousWallet branch (wallet A → wallet B scenario).

Important Files Changed

Filename Overview
app/features/idl/interactive-idl/model/form-prefill/providers/wallet-prefill-provider.ts Removes ExternalDependency interface conformance and publicKey param; adds lastPrefillAddressRef so wallet-switch overwrites previously auto-filled signer fields — logic is correct but the new hasPreviousWallet branch has no test coverage.
app/features/idl/interactive-idl/ui/InteractInstruction.tsx Moves wallet prefill from useFormPrefill's subscription loop into a dedicated useEffect that fires on publicKey changes; adds lastPrefillAddressRef owned here; cleanly removes the dependency from externalDependencies.
app/features/idl/interactive-idl/model/form-prefill/providers/tests/wallet-prefill-provider.spec.ts All tests use makeRef() (undefined), so hasPreviousWallet is always false — the wallet-switching path introduced in this PR is never exercised.
bench/BUILD.md Routine bundle-size table update (1.10 MB → 1.08 MB for nftoken-collection-nfts route); unrelated to feature change.

Sequence Diagram

sequenceDiagram
    participant W as Wallet Adapter
    participant C as InteractInstruction
    participant R as lastPrefillAddressRef
    participant P as createWalletPrefillDependency
    participant F as React Hook Form

    Note over C: Component mounts, ref = undefined

    W->>C: publicKey changes (Wallet A)
    C->>P: createWalletPrefillDependency(instruction, fieldNames, ref)
    P->>F: getValues(signerPath) → ""
    Note over P: isEmpty = true → fill
    P->>F: setValue(signerPath, "AddrA")
    P->>R: ref.current = "AddrA"

    W->>C: publicKey changes (Wallet B)
    C->>P: createWalletPrefillDependency(instruction, fieldNames, ref)
    P->>F: getValues(signerPath) → "AddrA"
    Note over P: hasPreviousWallet = ("AddrA" === ref.current) → true → fill
    P->>F: setValue(signerPath, "AddrB")
    P->>R: ref.current = "AddrB"

    Note over C: User manually edits signer field to "CustomAddr"

    W->>C: publicKey changes (Wallet C)
    C->>P: createWalletPrefillDependency(instruction, fieldNames, ref)
    P->>F: getValues(signerPath) → "CustomAddr"
    Note over P: isEmpty=false, hasPreviousWallet=false → skip
    P->>R: ref.current = "AddrC"
Loading

Reviews (1): Last reviewed commit: "chore: build info" | Re-trigger Greptile

const pdaPrefillDependency = createPdaPrefillDependency(idl, instruction, fieldNames);
useFormPrefill({
config: {
externalDependencies: [walletPrefillDependency, knownAccountsPrefillDependency, pdaPrefillDependency],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue: There are some concerns with architecture and a simpler fix available.

The current approach lifts wallet prefill out of the useFormPrefill framework into a an ad-hoc useEffect + useRef in InteractInstruction.tsx, while pda and knownAccounts providers still live inside the framework. A few problems with this:

  1. Asymmetric pattern. Three sibling providers, two follow the ExternalDependency contract, one doesn't. A future maintainer adding a fourth provider has to guess which pattern to copy. The "stale prefill" problem isn't unique to wallet - it could apply to any provider, so the right fix is either fully inside the framework or fully outside, not split.

  2. Implementation detail leaks into the call site. lastPrefillAddressRef is created in the UI component and threaded into the provider as a parameter. The provider's internal bookkeeping should not appear in the component that renders accounts.

  3. Misleading eslint-disable. The comment claims fieldNames is stable, but useInstructionForm returns a fresh fieldNames object on every render (see use-instruction-form.ts:48). The closure captures the latest value, so it's harmless in practice - but the comment will mislead the next reader.

  4. Reinvents what react-hook-form already tracks. We're maintaining external state to answer "did the user type this value, or did we prefill it?" RHF already answers that via isDirty. Every prefill in this provider is called with shouldDirty: false, so:

    • prefilled fields → isDirty: false
    • user-typed fields → isDirty: true (RHF sets this automatically on onChange)

    The "match previous wallet address" check is a workaround for not using the signal RHF already gives us.

Suggested fix:

  1. Revert InteractInstruction.tsx to the symmetric form — drop the useRef, useEffect, and the eslint-disable. Wallet rejoins the externalDependencies array next to the other two:

    const walletPrefillDependency = createWalletPrefillDependency(instruction, publicKey, fieldNames);
    const knownAccountsPrefillDependency = createKnownAccountsPrefillDependency(instruction, fieldNames);
    const pdaPrefillDependency = createPdaPrefillDependency(idl, instruction, fieldNames);
    useFormPrefill({
        config: { externalDependencies: [walletPrefillDependency, knownAccountsPrefillDependency, pdaPrefillDependency] },
        form,
    });
  2. Restore id and getValue on createWalletPrefillDependency so it conforms to ExternalDependency<PublicKey>. Drop the lastPrefillAddressRef parameter — no external state needed.

  3. Replace the isEmpty || hasPreviousWallet check with a single dirty check inside onValueChange:

    for (const path of signerPaths) {
        if (form.getFieldState(path).isDirty) continue;
        form.setValue(path, walletAddress as unknown as FormValue, {
            shouldDirty: false,
            shouldValidate: false,
        });
    }

    Note: getFieldState reads internal RHF state directly, no formState subscription required.

  4. Keep shouldDirty: false on every setValue (already the case) — that's the load-bearing invariant that makes step 3 work.

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.

2 participants