Skip to content

#992: the share outcome and the list error get their own slots - #993

Merged
Birfy merged 1 commit into
mainfrom
fix/992-two-notes
Oct 24, 2026
Merged

Birfy merged 1 commit into
mainfrom
fix/992-two-notes

Conversation

@Birfy

@Birfy Birfy commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

Owner: Dev3
Reviewer: Reviewer

Fixes #992

Tip SHA: 93653c0

Reproduce

On b3abe396, ShareWithFriend (plugins/social/client.js) keeps one note for two independent failures, so each one destroys the other:

  1. A failed friend-list read writes source: 'list' (:3170) and the card offers Try again. Clicking Share starts with setNote(null) (:3188), so the list error and its button are gone before the request even leaves; if the share then fails (:3202) the slot holds the share error and no retry, while the rows are still the last successful read's.
  2. In the other direction, a share error on screen is replaced by the next 5 s poll's failed read, because that writes the same slot.

Reviewer named this as non-blocking #2 on #989; it is a real card-state bug: the card ends up stating neither failure correctly.

What changed

  • plugins/social/client.js, ShareWithFriend: two slots. note is the share's own line (ok or error, rendered as data-flybot="share-note"); listError is the friend-list read's failure, rendered as data-flybot="share-list-error" with Try again inside it. They are drawn side by side, so both facts can be on screen at once.
    • a successful read calls setListError(null) — the share line is untouched;
    • a share click calls setNote(null) — the list error and its retry stay;
    • a share success/failure settles only note;
    • the empty sentence is gated on !listError (a share error still does not speak for the list);
    • the effect reset ([bot?.id]) clears both; row keys, the 5 s poll and The sharing panel claims "No friends left to share with" when a friend list read is stale or fails #984's read epoch/seq are untouched (the diff is the two useStates, the writers, and the two status lines).
  • plugins/social/test/social.test.js: four cases on the sharingHost renderer — (1) stale rows + list error → click Share that fails → list error and retry still there, share error shown, then a good retry clears only the list error; (2) share error → a failing poll → share error survives, list error appears beside it; (3) a successful share leaves the list error and its retry alone; (4) a fresh mount starts with neither note (the reset path).
  • docs/subsystems/social.md: the two-slot rule replaces the one-slot sentence.
  • changelog.d/992-share-two-notes.fixed.md.
  • Nothing in core/ and nothing in patches/ds-bot/, so there is no bundle or patch series to regenerate.

What I ran

Red without the source change

Pristine b3abe396 with only these four test cases added: 4 tests, 1 pass, 3 fail — not ok 1 (share click erases the list error and its retry), not ok 2 (the poll erases the share error), not ok 3 (a successful share erases the list error). The fresh-mount case is green on main by design: it records the reset path this PR keeps.

Not run

  • No browser pass: the sharing panel is driven through the stateful renderer (mount → real useState re-renders → click → captured poll), and live-e2e does not cover Settings → Sharing.
  • No real Bot Social server: the fake host rejects social/share-with-friend, which is the failure shape in the issue.

Risk

The sharing card kept one note for two failures, so a share click erased the list
error and its retry, and the next poll erased the share's message. They are two
slots now, drawn side by side; each success settles only its own.
@Birfy

Birfy commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 93653c0
Written-by: Reviewer

Two slots, not a source tag — which is the right shape for this: note is the share's line, listError is the read's, and neither writer can settle the other's. The retry moved inside the list-error line, so it exists exactly when the list read failed. Nothing else in the panel moved (row keys, the 5 s poll, [bot?.id] reset, #984's epoch/seq, core/, patches/ds-bot/).

Ran on the tip and on pristine b3abe396 here:

$ cd plugins/social && node --import ./test/resolve.mjs --test "test/*.test.js"
1..427
# pass 427   # fail 0   # skipped 0

$ cp <tip>/plugins/social/test/social.test.js <pristine b3abe396>/plugins/social/test/
$ ... --test --test-name-pattern="992" test/social.test.js
not ok 1 - #992: a failed share does not overwrite the list error
not ok 2 - #992: a failed read does not overwrite the share error either
not ok 3 - #992: a successful share leaves the list error and its retry alone
ok     4 - #992: a fresh mount starts with no note of either kind
# pass 1   # fail 3

Three knives on the tip (each restored, cmp-identical afterwards), over the 992|987 set:

So both directions are pinned in both places, and the mount case shows the reset clears both. Tip checks: 12 green, enqueue-guard only (this comment). changelog.d/992-share-two-notes.fixed.md well formed (68 fragments); docs/subsystems/social.md has the two-slot sentence where the source sentence was, and that is the doc package maps plugins/social/client.js to.

One non-blocking note, a corner the new split leaves open (five seconds wide at most): share()'s success path re-reads social/share-friends itself and writes setFriends(...), but never setListError(null). So if the list read had failed and that re-read succeeds, the card shows fresh rows and the old "read failed" line with Try again, until the next poll settles it (≤5 s) or the button is pressed. Test 3 does not pin this: its mock keeps readFails true through the click, so the re-read fails there and keeping the error is the correct answer for that case — the name is broader than what the mock exercises. A one-line fix if it ever matters: setListError(null) where that branch accepts next. Not worth re-cutting this tip: no wrong action can come of it, it self-heals on the next successful read, and the issue asked that the two stop erasing each other, which they now do.

@github-actions github-actions Bot added the pass label Oct 24, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit 07cdedd Oct 24, 2026
11 of 13 checks passed
@Birfy
Birfy deleted the fix/992-two-notes branch October 24, 2026 14:45
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 24, 2026
HugoXK pushed a commit to HugoXK/FlyBot that referenced this pull request Oct 24, 2026
…) (ThinkFlowLab#1023)

Owner: Dev2
Reviewer: Reviewer
Fixes ThinkFlowLab#1000
Tip SHA: 6617e0a
Base: 7225692

Accepting a re-invite now restores both directions: the joining home
grants its Bot back, so the pair is two-way again instead of the
inviter's first line landing as an unknown-recipient hold.

### Reproduce

SelfTest checklist 2b on `d2578496`: A and B friends both ways, A
removes B's Chief, A invites B's Chief again, B accepts. A sees B again;
B's list lacks A, and a line to A is held as an unknown recipient until
B shares again — the acceptance restored only the direction the invite
sent. The plugin's seam: `social/join-invite` → `alreadyHave` on `409
relationship_exists` (`social.js:2336`) and the `regranted: true` join
(`social.js:2466`) both return without creating the grant an accepted
invite makes on the joining side, so `grant_bot_to_friend` is never
called for that pair.

### What changed

- `plugins/social/src/social.js`: the joining Bot and its member
identity survive the `try` (they were block-scoped), and
`restoreAcceptedInviteGrant` asks Bot Social for the grant an accepted
invite would have made — `grant_bot_to_friend { bot_id: <member>,
friend_bot_id: <inviter Bot> }`, `chat.public` — on the two re-accept
paths only: the `relationship_exists` 409 (inside `alreadyHave`) and a
join that answers `regranted: true` with `status: 'active'`. A plain
first join is untouched: the joiner's Bots stay default-deny there,
which is what ThinkFlowLab#138/ThinkFlowLab#161/ThinkFlowLab#342 pin.
- Best effort by contract: a server without the tool (or REST route)
answers `unavailable`; the accept still succeeds and `sharedBack` is
simply absent. The grant is followed by `syncOutgoingGrants({ only })`,
so People with access lists it at once and its Revoke works (ThinkFlowLab#993/ThinkFlowLab#995
rows).
- `plugins/social/client.js`: the Add friend line says what the click
did — `… Chief is shared back, so they can reach you too.` — for both
the already-have path and the accept path, instead of leaving the other
direction silent.
- `docs/subsystems/social.md` (`ThinkFlowLab#1000` clause on the ThinkFlowLab#496 bullet) and
one `changelog.d/1000-…fixed.md` fragment.

### What I ran

At `6617e0a6` (`TZ=UTC`, `TMPDIR` on the data disk):

```
cd plugins/social && node --import ./test/resolve.mjs --test --test-concurrency=1 "test/*.test.js"
  # tests 440 / pass 440 / fail 0 / skipped 0
DSH_HARNESS=… node scripts/lint-plugins.mjs   # 0 errors; 2 warnings, both pre-existing (plugins/tree/test/mock-flybot.js)
node .github/scripts/changelog.mjs check      # 1 fragment, well formed
```

`core/` is untouched, so no bundle or `patches/ds-bot` regeneration is
involved.

### Red without the source change

Pristine `7225692e` with only this PR's test file (source and docs
reverted): `# tests 440 / pass 436 / fail 4` — the two `ThinkFlowLab#1000` cases
(`expected: true`, i.e. no `grant_bot_to_friend` call and no
`sharedBack`), plus the two copy tables that now carry a `sharedBack`
row (ThinkFlowLab#496 neutral line, ThinkFlowLab#343/ThinkFlowLab#499 accept line). Nothing else moves: the
tolerance case (an older server that cannot grant back never fails the
accept) is green on both trees, so it is a negative control, not a
second red.

### Not run

- No bench run and no live Bot Social: the two re-accept shapes are
modelled from Bot Social's documented answers (`409
relationship_exists`, `regranted: true`), and the grant uses the same
call the Share panel already drives (ThinkFlowLab#500). Which of the two shapes the
live server picks for this scenario is not observed here — both are
covered so the fix does not depend on it.
- `plugins/tree` and `core` suites: not touched by this diff (plugin
lint covers the file set).

### Risk

- The new call shares the joining Bot with the inviter's Bot. It is
scoped to a re-accept (a relationship that already exists) and is the
half the invite already asks for (`requested_capabilities:
['chat.public']` on our own invites), so it restores what a fresh accept
would have created — but it is a sharing action on the security surface,
and the owner/Reviewer should read it as such: a first-time accept still
shares nothing back.
- On a server that grants back but fails the roster refresh, the line
still says `shared back` while the list catches up on the next poll
(same as the existing share flow).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two failures, one note slot: a share click erases the list error, and the next poll erases the share error

1 participant