diff --git a/.beans/ATFS-o1ro--atfs-init-interactive-mode-writes-the-devatfsserve.md b/.beans/ATFS-o1ro--atfs-init-interactive-mode-writes-the-devatfsserve.md index 0b0c5eb..dc10b2a 100644 --- a/.beans/ATFS-o1ro--atfs-init-interactive-mode-writes-the-devatfsserve.md +++ b/.beans/ATFS-o1ro--atfs-init-interactive-mode-writes-the-devatfsserve.md @@ -5,7 +5,7 @@ status: completed type: feature priority: normal created_at: 2026-08-24T06:57:14Z -updated_at: 2026-08-24T07:38:40Z +updated_at: 2026-08-24T08:12:49Z --- Make atfs init interactive when no handle is given: prompt for the atproto handle, print the env vars as today, then offer to write the dev.atfs.server record directly (enter account password/app password to log in and write it; press enter to just print the record instead; a wrong password falls back to printing rather than erroring). @@ -147,3 +147,54 @@ open exactly as designed; a corrected domain landing correctly in the final serviceDid; and the final printed record's shape in every case. golang.org/x/term is kept only for isRealTerminal's own check — the password-masking use is gone, replaced by huh's own. + + +## Follow-up 2 (same session): a real huh.MultiSelect, and the owner is now editable + +Two problems with the huh.NewText() "one per line" design from the previous +follow-up, per feedback: it genuinely didn't work as a multi-entry +mechanism (Text's default keymap submits/advances the field on plain +Enter — Next is bound to both tab and enter — so a second line needed +alt+enter/ctrl+j, undiscoverable and already flagged as a rough edge last +time), and the owner was unconditionally prepended to accounts in code +with no way to see or remove them. + +Replaced with a two-step, actually-native design: + +- "Other accounts" is back to a single-line huh.NewInput(), parsed as a + comma-separated list (resolveAccountList) — this alone fixes the + original bug, since a single-line field's Enter submitting immediately + was never a problem to begin with; the bug was specifically Text's + newline-vs-submit ambiguity. +- A new huh.NewMultiSelect[string]() confirmation step follows: options + are the owner (always first, labeled with whatever they were entered + as) plus everyone just resolved from the comma list, ALL pre-checked + (Option.Selected(true)) — accounts.minItems=1 is enforced via the + field's own Validate. Unchecking any box, owner included, removes it + from what gets written — exactly the "pre-fill first, let them remove + it" behavior asked for, and it generalizes to every entry, not just the + owner. + +Wiring detail worth recording: MultiSelect's real-time pre-selection +reconciliation (selectOptions(), called again on every OptionsFunc +recompute) matches its Options against whatever the bound Value slice +CURRENTLY holds, not what it held at construction — so seeding "owner +pre-checked" only works if the accounts variable already names the +owner's DID by the time this group's options first evaluate. Since ident +(the resolved owner) may still be nil at options.NewMultiSelect(...) +construction time (handle typed interactively, not yet resolved), the +seeding was moved into the other-accounts field's own Validate — which +by execution order always runs after ident is set, whichever path set it +(pre-resolved before the form for an argument-given owner; the hidden +handle group's own Validate otherwise). + +Verified via real pty (real network resolution of two distinct real +handles): the confirm screen renders both entries checked with the owner +first and the cursor there by default; toggling the owner off with space +and submitting leaves exactly the other account in the written/printed +record, confirmed by inspecting the final JSON. One cosmetic-only wrinkle +noted, not fixed: while the other-accounts network resolution is still in +flight, the confirm screen can flash a stale "at least one account is +required" validation message for a moment before the real options load — +never affects the final result, not worth the complexity to chase given +it wasn't asked for. diff --git a/README.md b/README.md index ba61514..9314246 100644 --- a/README.md +++ b/README.md @@ -203,7 +203,7 @@ ATFS_OWNER_DID=did:plc:... ATFS_IDENTITY_KEY=CAESQ... ``` -Run at a real terminal (not piped, not scripted), it goes further, prompting through a small [`charmbracelet/huh`](https://github.com/charmbracelet/huh) form: for a handle if none was given, then — after printing the env vars — for the domain the server will be reachable at and any other accounts allowed to upload, before offering to write the resulting `dev.atfs.server` record straight to your account, given your password or an app password. Press enter instead of a password to print the record rather than write it; a wrong password does the same rather than failing outright. Roughly (the real thing is a bordered, styled form, not plain lines — this is just the conversation it asks): +Run at a real terminal (not piped, not scripted), it goes further, prompting through a small [`charmbracelet/huh`](https://github.com/charmbracelet/huh) form: for a handle if none was given, then — after printing the env vars — for the domain the server will be reachable at and any other accounts to allow, with a checklist to confirm the final list (you're on it by default; uncheck yourself if you don't want to be), before offering to write the resulting `dev.atfs.server` record straight to your account, given your password or an app password. Press enter instead of a password to print the record rather than write it; a wrong password does the same rather than failing outright. Roughly (the real thing is a bordered, styled form, not plain lines — this is just the conversation it asks): ```sh $ atfs init @@ -219,8 +219,13 @@ Leave blank if you don't know yet > myatfs.example.com eg. myatfs.example.com Which other atproto accounts should be able to upload to this server? -One handle or DID per line — leave blank if there are none -> eg. friend.bsky.social +A comma-separated list of handles or DIDs — leave blank if there are none +> eg. friend.bsky.social, did:plc:... + +Confirm which accounts can upload to this server +Everyone just entered starts checked — including you — uncheck any you don't want +> [x] jp.example.com + [x] friend.bsky.social Your server requires a record in your atproto account. To create it, please enter your account password (or an app password). > Just press enter to print the record instead diff --git a/cmd/atfs/init.go b/cmd/atfs/init.go index fff6285..43869d7 100644 --- a/cmd/atfs/init.go +++ b/cmd/atfs/init.go @@ -61,12 +61,26 @@ func runInit(ctx context.Context, dir identity.Directory, owner string, stdin io } var ( - ident *identity.Identity - domain string - extraAccounts []string - password string + ident *identity.Identity + domain string + otherRaw string + accountOpts []acctOption // owner + resolved others, rebuilt each time otherRaw changes + accounts []string // MultiSelect's own bound value — which of accountOpts stayed checked + password string ) + // Resolved up front rather than waiting for the (possibly hidden) + // handle group's own Validate: the accounts-confirmation field below + // needs ident.DID to seed and pre-check the owner's own row, and it's + // built before form.Run() ever runs anything. + if owner != "" { + resolved, err := resolveOwner(ctx, dir, owner) + if err != nil { + return err + } + ident = resolved + } + if interactive { handleField := huh.NewInput(). Title("Your server needs an owning account, and a peer ID. To generate them, please enter your atproto handle:"). @@ -91,16 +105,45 @@ func runInit(ctx context.Context, dir identity.Directory, owner string, stdin io Value(&domain). Validate(validatePromptDomain) - accountsField := huh.NewText(). + otherAccountsField := huh.NewInput(). Title("Which other atproto accounts should be able to upload to this server?"). - Description("One handle or DID per line — leave blank if there are none"). - Placeholder("eg. friend.bsky.social"). + Description("A comma-separated list of handles or DIDs — leave blank if there are none"). + Placeholder("eg. friend.bsky.social, did:plc:..."). + Value(&otherRaw). Validate(func(s string) error { - resolved, err := resolveAccountLines(ctx, dir, s) + others, err := resolveAccountList(ctx, dir, s) if err != nil { return err } - extraAccounts = resolved + // The owner always leads the list, under the label they + // originally entered — and never twice, if they also typed + // themselves in here. + accountOpts = []acctOption{{label: owner, did: ident.DID.String()}} + for _, o := range others { + if o.did != ident.DID.String() { + accountOpts = append(accountOpts, o) + } + } + accounts = acctDIDs(accountOpts) // seeds the confirm step below with everyone pre-checked + return nil + }) + + confirmAccountsField := huh.NewMultiSelect[string](). + Title("Confirm which accounts can upload to this server"). + Description("Everyone just entered starts checked — including you — uncheck any you don't want"). + Value(&accounts). + OptionsFunc(func() []huh.Option[string] { + opts := make([]huh.Option[string], len(accountOpts)) + for i, a := range accountOpts { + opts[i] = huh.NewOption(a.label, a.did).Selected(true) + } + return opts + }, &accountOpts). + Filterable(false). + Validate(func(sel []string) error { + if len(sel) == 0 { + return errors.New("at least one account is required") + } return nil }) @@ -113,7 +156,8 @@ func runInit(ctx context.Context, dir identity.Directory, owner string, stdin io form := huh.NewForm( huh.NewGroup(handleField).WithHide(owner != ""), huh.NewGroup(domainField), - huh.NewGroup(accountsField), + huh.NewGroup(otherAccountsField), + huh.NewGroup(confirmAccountsField), huh.NewGroup(passwordField), ).WithShowHelp(false).WithInput(stdin).WithOutput(stdout) @@ -126,14 +170,13 @@ func runInit(ctx context.Context, dir identity.Directory, owner string, stdin io } if ident == nil { - // Either non-interactive, or owner was already given (the handle - // group above was hidden, so its Validate — the only other place - // ident gets set — never ran). - resolved, err := resolveOwner(ctx, dir, owner) - if err != nil { - return err - } - ident = resolved + // Non-interactive with no owner is already rejected above, so the + // only way here is the handle group having resolved it — this is + // just belt-and-braces against a future change to that flow. + return errors.New("no owner resolved — this shouldn't happen") + } + if len(accounts) == 0 && interactive { + accounts = []string{ident.DID.String()} } // Same generation + encoding internal/ipfs.firstRunIdentity uses for a @@ -167,7 +210,6 @@ func runInit(ctx context.Context, dir identity.Directory, owner string, stdin io return nil } - accounts := dedupeStrings(append([]string{ident.DID.String()}, extraAccounts...)) serviceDid := "" if domain != "" { serviceDid = "did:web:" + domain @@ -208,40 +250,49 @@ func validatePromptDomain(s string) error { return nil } -// resolveAccountLines resolves the extra-accounts field's whole block in -// one pass — one handle or DID per non-empty line — so a typo anywhere is +// acctOption pairs what someone typed — a handle or DID — with the DID it +// resolved to. The confirm-accounts MultiSelect needs both: the DID is the +// value that actually ends up in the record, but showing a checklist of +// bare DIDs would be unreadable, so the label is whatever the person +// actually typed (their handle, most of the time). +type acctOption struct { + label string + did string +} + +func acctDIDs(opts []acctOption) []string { + dids := make([]string, len(opts)) + for i, o := range opts { + dids[i] = o.did + } + return dids +} + +// resolveAccountList resolves the other-accounts field's whole value in one +// pass — a comma-separated list of handles or DIDs — so a typo anywhere is // reported (and the field kept open to fix it) without silently dropping -// the entries around it. -func resolveAccountLines(ctx context.Context, dir identity.Directory, block string) ([]string, error) { - var dids []string - for _, line := range strings.Split(block, "\n") { - line = strings.TrimSpace(line) - if line == "" { +// the entries around it. A repeated entry is kept once, under its first +// label. +func resolveAccountList(ctx context.Context, dir identity.Directory, raw string) ([]acctOption, error) { + var opts []acctOption + seen := make(map[string]bool) + for _, entry := range strings.Split(raw, ",") { + entry = strings.TrimSpace(entry) + if entry == "" { continue } - ident, err := resolveOwner(ctx, dir, line) + ident, err := resolveOwner(ctx, dir, entry) if err != nil { - return nil, fmt.Errorf("%q: %w", line, err) + return nil, fmt.Errorf("%q: %w", entry, err) } - dids = append(dids, ident.DID.String()) - } - return dids, nil -} - -// dedupeStrings preserves first-seen order — accounts is small and this -// only ever runs once per invocation, so there's no need for anything -// fancier than an O(n) scan. -func dedupeStrings(in []string) []string { - seen := make(map[string]bool, len(in)) - out := make([]string, 0, len(in)) - for _, s := range in { - if seen[s] { + did := ident.DID.String() + if seen[did] { continue } - seen[s] = true - out = append(out, s) + seen[did] = true + opts = append(opts, acctOption{label: entry, did: did}) } - return out + return opts, nil } const serverRecordNSID = "dev.atfs.server" diff --git a/cmd/atfs/init_test.go b/cmd/atfs/init_test.go index 19c9be6..a4fbf9a 100644 --- a/cmd/atfs/init_test.go +++ b/cmd/atfs/init_test.go @@ -154,51 +154,54 @@ func TestValidatePromptDomain(t *testing.T) { } } -func TestResolveAccountLines_ResolvesEachNonEmptyLine(t *testing.T) { +func TestResolveAccountList_ResolvesEachCommaSeparatedEntry(t *testing.T) { dir := fakeIdentityDir("https://pds.example.com") - dids, err := resolveAccountLines(context.Background(), dir, "\n"+testUserHandle.String()+"\n\n"+testOtherHandle.String()+"\n") + opts, err := resolveAccountList(context.Background(), dir, " ,"+testUserHandle.String()+", ,"+testOtherHandle.String()+",") if err != nil { - t.Fatalf("resolveAccountLines: %v", err) + t.Fatalf("resolveAccountList: %v", err) } - want := []string{testUserDID.String(), testOtherDID.String()} - if len(dids) != len(want) || dids[0] != want[0] || dids[1] != want[1] { - t.Errorf("resolveAccountLines = %v, want %v", dids, want) + if len(opts) != 2 { + t.Fatalf("resolveAccountList = %v, want 2 entries", opts) + } + if opts[0].did != testUserDID.String() || opts[0].label != testUserHandle.String() { + t.Errorf("opts[0] = %+v", opts[0]) + } + if opts[1].did != testOtherDID.String() || opts[1].label != testOtherHandle.String() { + t.Errorf("opts[1] = %+v", opts[1]) } } -func TestResolveAccountLines_BlankBlockIsFine(t *testing.T) { +func TestResolveAccountList_BlankIsFine(t *testing.T) { dir := fakeIdentityDir("https://pds.example.com") - dids, err := resolveAccountLines(context.Background(), dir, "\n \n") + opts, err := resolveAccountList(context.Background(), dir, " , ,") if err != nil { - t.Fatalf("resolveAccountLines: %v", err) + t.Fatalf("resolveAccountList: %v", err) } - if len(dids) != 0 { - t.Errorf("resolveAccountLines = %v, want none", dids) + if len(opts) != 0 { + t.Errorf("resolveAccountList = %v, want none", opts) } } -func TestResolveAccountLines_OneBadLineFailsTheWholeBlock(t *testing.T) { +func TestResolveAccountList_OneBadEntryFailsTheWholeList(t *testing.T) { dir := fakeIdentityDir("https://pds.example.com") - _, err := resolveAccountLines(context.Background(), dir, testUserHandle.String()+"\nnobody.invalid\n") + _, err := resolveAccountList(context.Background(), dir, testUserHandle.String()+", nobody.invalid") if err == nil { - t.Fatal("resolveAccountLines: want an error naming the bad entry") + t.Fatal("resolveAccountList: want an error naming the bad entry") } if !strings.Contains(err.Error(), "nobody.invalid") { - t.Errorf("error = %v, want it to name the offending line", err) + t.Errorf("error = %v, want it to name the offending entry", err) } } -func TestDedupeStrings(t *testing.T) { - got := dedupeStrings([]string{"a", "b", "a", "c", "b"}) - want := []string{"a", "b", "c"} - if len(got) != len(want) { - t.Fatalf("dedupeStrings = %v, want %v", got, want) +func TestResolveAccountList_DuplicateEntryKeptOnce(t *testing.T) { + dir := fakeIdentityDir("https://pds.example.com") + opts, err := resolveAccountList(context.Background(), dir, testUserHandle.String()+","+testUserDID.String()) + if err != nil { + t.Fatalf("resolveAccountList: %v", err) } - for i := range want { - if got[i] != want[i] { - t.Errorf("dedupeStrings = %v, want %v", got, want) - } + if len(opts) != 1 || opts[0].label != testUserHandle.String() { + t.Errorf("resolveAccountList = %v, want the handle form kept once", opts) } }