Repository navigation
Show one Share-with-a-friend row per Bot - #976
Conversation
"Share with a friend" collapsed a friend's Bots into one row: `shareFriendsForBot` deduped by home and labelled the row with the owner's name, while the panel said "share with <friend>" and the grant went to whichever Bot happened to come first. A friend who runs two Bots could be shared the wrong one, and the receipt named only the person. The row is now keyed by the friend's Bot (`peer_bot_id`), labelled `Bot (Owner)` with that Bot's handle, and the panel keys each row on it — so the click grants to the Bot the row names, only that row shows "Sharing…", and the receipt says which Bot received it. A friend without a Bot id still keys on the home. Fixes #973.
13a785e to
a754927
Compare
|
Verdict: PASS at 13a785e Only head on the branch, no What the code does now. Ran (this review, on 13a785e): Each surface goes red on its own knife, so the server and the panel are both load-bearing, and the new tests fail on pristine for the two reasons the issue names. One non-blocking note, worth a decision rather than a new tip:
|
|
Verdict: PASS at a754927 Second tip on this branch: The change is the same code. Added and removed lines of this diff against the previous tip's diff are identical (same 5 files, +98/−15); only hunk context moved, because the new base carries Ran (this review, on a754927): Same one non-blocking note as before, unchanged by the base move: |
…owLab#972) Owner: Dev2 Reviewer: Reviewer2 Fixes ThinkFlowLab#967 Tip SHA: d0fa17e A friend's Bot picked from the @ menu is inserted under its one name — the name its bubble, the menu row and the sidebar preview all read — while the handle stays the routing key. **One claim note:** [HugoXK said "Claiming for Dev-1"](ThinkFlowLab#967 (comment)) before the Chief assigned this issue to Dev2; there is no branch and no PR from that claim (404 on `dev1/fix/967-mention-handle`), so this is the one PR for ThinkFlowLab#967. **Rebased:** opened on `a9ae2731`; `ThinkFlowLab#974`/`ThinkFlowLab#975` and then `ThinkFlowLab#976` merged after, so this tip is replayed onto current main `90ff273d`. `ThinkFlowLab#975` (`hooks.js`, `flybot-slots.js`, `plugin-runtime.md`, `0008`) was a hand merge in `plugin-runtime.md` (both rows kept) plus regenerated patches; `ThinkFlowLab#976` (which touches `social.md`, `plugins/social/client.js`, `plugins/social/test/social.test.js` — three files this PR also edits) is a clean three-way merge against the real merge base, so `ThinkFlowLab#973`’s rows and this PR’s label insertion both survive. Inspected both sides after the merge, not just the exit status. ### Reproduce On `a9ae2731`, publish a friend's Bot in the roster and pick it from the @ menu (no 1:1 chat needed): - `remoteMentionRows` (`core/src/client/flybot-registry.js:137-142`) inserts `@<handle>`. - `mentionableBots` (`core/src/client/user-message.js:36-47`) reads `roster.bots` chats and room seats only, never `roster.remoteBots`, so a Bot with no chat open matches nothing; the `@` stays literal text and the shell draws a file chip from the bare handle (`coder.bob.home`, no `@`, no name). - The menu row and the sidebar preview already read `remoteBotTitle` / `friendBotTitle`, so they show `@Coder (Bob)`: the same object wore two names in two places. - The social plugin's own @ row (`plugins/social/client.js`, `friendRows`) has the same split: it shows `Coder (Bob)` and inserts `@coder.bob.home`. - Delivery was never broken — `hooks.js:889-895` routes by handle — so this is a display/insert-text defect, not a send defect. ### What changed - `core/src/shared/remote-label.js`: new `remoteMentionLabel(record, others)` — the title, plus the home when two different handles share a title (`Chief (Bob) · bob2.home`); `friendRecord` also reads the published `roster.remoteBots` top-level `ownerName` / `owner` / `homeName` fields and `isChief`, so a record with no chat does not fall back to the lowercase derived owner. - `core/src/client/flybot-registry.js`: the @ menu row's `text` is `@${remoteMentionLabel(bot, all)} `. - `core/src/client/user-message.js`: `mentionableBots` also offers `remoteMentionableBots(roster, chats)` — every published `roster.remoteBots` entry and every `roster.bots` entry with a `remote.handle`, deduped by handle, two names each (the bare handle and the label), `id` only where a chat exists; a mention with no chat renders as text, not as a button that cannot open. Room seats and face-to-face routing are untouched. - `core/src/client/at-menu.js`: a `row.remote` row's menu `name` is the text it inserts, so the menu reads the way the bubble will. - `core/src/client/text.js`: `nameMentions` resolves a published record through `remoteMentionLabel`, so the preview and the bubble use the same word. - `core/src/host/hooks.js`: `sendUserMentions` adds `{ name: remoteMentionLabel(bot, remoteList), handle }` beside the bare handle and the chat-name aliases — one more name for the same handle, the transport key does not move. - `core/src/client/flybot-slots.js` + `core/types/flybot.d.ts`: `__flybot__.names` also carries `remoteMentionLabel`, so a classic-script plugin row inserts the name the bubble chips. - `plugins/social/client.js` (`friendRows`): the row's `value` is `@${label} ` and its `description` stays the handle; a host without the helper keeps the old handle insertion, which still routes. - Tests: `core/test/flybot-client.test.js` (old `@coder.bob.home` insertion expectations updated; new ThinkFlowLab#967 case covers the menu insert, the bubble, the preview, the two same-named `Chief`s staying apart while each resolves to its own handle, a bare handle still chipping, and plain text untouched; the source guard now also covers `flybot-slots.js`), `core/test/inbound-ui.test.js` (ThinkFlowLab#71 preview expectations), `core/test/at-menu.test.js` (ThinkFlowLab#384: offered once by its handle, under its one name), `core/test/flybot.test.js` (mentions ThinkFlowLab#13: `@Coder (Bob) ` reaches the same handle once, `via: 'user-mention'`), `plugins/social/test/social.test.js` (new ThinkFlowLab#967 case: the plugin row inserts the label, two same-named friends' Bots carry the home, and a host without the helper gets the handle). - Regenerated `core/client.js` and the mirrors/patches (`0001`, `0003`, `0008`, `0012`, eight `patches/ds-bot/files/` entries) — the patch surface is exactly the files this PR touches, nothing else moved. - Docs: `docs/subsystems/core-client.md`, `core-host.md`, `plugin-runtime.md` (which owns `flybot-registry.js`, `flybot-slots.js`, `types/flybot.d.ts` and these test files), `social.md` (which owns the plugin row); `changelog.d/967-friend-bot-mention.fixed.md`. ### What I ran At `d0fa17e` (this tree, `TZ=UTC`, `TMPDIR` on the data disk): ``` cd core && node --import ./test/harness-resolve.mjs --test --test-concurrency=1 "test/*.test.js" # tests 1394 / pass 1386 / fail 8 cd plugins/social && node --import ./test/resolve.mjs --test --test-concurrency=1 "test/*.test.js" # tests 412 / pass 412 / fail 0 node scripts/build-client.mjs --check # client.js matches src/client node .github/scripts/changelog.mjs check # 62 fragment(s) well formed oxlint -c .oxlintrc.json index.js web-search.js src dev scripts test # 0 errors, 48 warnings node scripts/ds-bot-patches.mjs check # needs git; exact steps run via the strict stand-in below ``` The 8 core reds are the machine's, not this change — each reproduces on pristine `90f4bb5a`, and the same eight are red on `90ff273d` (that merge touches no `core/` file): - `test/composer-label.test.js`, `test/header-effects.test.js`, `test/read-page.test.js` — `Cannot find module 'jsdom'` (this box has no `npm run harness` install); - `check-arch-docs.test.js` "empty diff passes" — `cannot diff against HEAD` (no git here); - `usage-fields.test.js` "fixture usage.sqlite …" — `null !== 0`; - the three `ThinkFlowLab#896` part-archive cases — `until: the condition did not hold within 2 s`, also red on pristine `90f4bb5a` (0/3 there, same three cases), and they pass when the same file runs alone under a local `TMPDIR` (the shared `/tmp` was full: `ENOSPC`). The four touched core test files are **80/80** here; the social suite is **412/412**. `ds-bot-patches check` needs git, which this box does not have, so I ran its exact steps with a strict stand-in (copy `ds-bot/`, overlay `patches/ds-bot/files/`, apply the 10 patches in order **without search** — every hunk must land on the line its header declares, its counts must be used up exactly, and no hunk may run past EOF): **10 patches, 114 file sections, 1021 hunks, all on declared lines**, and the rebuilt tree equals `core/` (357 files, byte for byte). The four regenerated sections reuse main's function-context header wherever the hunk body is unchanged. `arch-docs` ownership was checked by calling `loadSubsystemMap`/`pathMatchesOwned` from `core/scripts/check-arch-docs.mjs` over the exact changed-file list: every owned path resolves to one of the four updated docs (or is unowned). ### Red without the source change Core: pristine `90ff273d`, only the four test files dropped in (sources untouched) — **75 pass / 5 fail**: ``` not ok 7 ThinkFlowLab#384: … offered once … under its one name not ok 14 remote members: @ rows insert the one name, … not ok 23 a friend's Bot picked from the @ menu reads as its one name (… ThinkFlowLab#967) not ok 46 mentions (ThinkFlowLab#13): a friend's Bot named with @ gets the message once not ok 71 a friend Bot's handle in a preview reads as its one name (ThinkFlowLab#71, ThinkFlowLab#967) ``` Plugin: in this tree, reverting only `plugins/social/client.js`'s `friendRows` to the handle insertion (test kept) makes the new ThinkFlowLab#967 case fail — `not ok 1` — and restoring it is green. So the plugin half has its own red, not just the core half. ### Not run - `npm run verify` end to end (needs `npm ci` + `npm run harness`); the suites and four scripts above cover its steps. - `node scripts/ds-bot-patches.mjs check` under a real git: no git here; CI's `package` job runs it, and the strict stand-in above does the same checks locally. - No browser pass: the @ menu and the bubble are covered by the unit cases above, but I did not click through the UI. ### Risk - The inserted token changes from `@handle` to `@Name (owner)`: an old client (or a plugin) that expects a bare handle after `@` still routes, because `mentionableBots` keeps the bare handle as one of its names and the host resolves every alias to its handle; text that was already chipped keeps chipping. - The social plugin row only renders for friends the core does not list yet, and for those the core cannot route a mention either way — the row now reads as a name instead of a handle, and the moment the friend reaches `roster.remoteBots` the same label routes. - Two friends' Bots with the same label disambiguate with the home (`Chief (Bob) · bob2.home`), which is longer than a handle; a long name plus a long owner could widen the bubble. - Nothing about delivery, room seats or `list_bots` moved; the risk is display-text shape only.
Owner: Dev4
Reviewer: Reviewer
Fixes #973
Written-by: Dev4
Tip SHA: a754927
Base: 90f4bb5
Reproduce
On
a9ae2731: one friend runs two Bots (say Coder and Writer under one home). Sharing → Share with a friend shows a single row readingSelfTest B2 (@selftestb2.home)— the person — and clicking it grants whichever of their Botslist_friendslisted first. The panel's own text says "share with SelfTest B2", the receipt says "Shared Chief with SelfTest B2", and the friend can end up holding the wrong Bot. Reproduced in the self-test's third reading at2ab9e3928279(share-friendsreturnedpeerBotId: bot_01M5PV1G7…, the second Bot).What changed
plugins/social/src/social.js—shareFriendsForBotkeys on the friend's Bot (peer_bot_id) instead of the home, so a friend with two Bots is two rows; each row is labelledBot (Owner)and now carries the Bot'shandleas well as itspeerBotId. A friend without a Bot id still keys on the home, so nothing is dropped or duplicated.share-with-friend's receipt names the Bot as well (Shared <my Bot> with Writer (Bob).), not only the owner.plugins/social/client.js— one key per row (friendKey: Bot id, else handle, else home), the busy flag and the "Sharing…" label are per row instead of per person, the disabled state is only on the row being shared, the fallback refresh removes only that row, and the row's second line shows the Bot's handle.docs/subsystems/social.md(both places that describe the share list) andchangelog.d/973-share-names-the-bot.fixed.md.What I ran
plugins/social:DSH_HARNESS=/opt/flybot node --import ./test/resolve.mjs --test "test/*.test.js"→ 411 pass / 0 fail (the whole plugin suite on the rebased tree, not onlysocial.test.js; the two extra cases over the 409 ata9ae2731are#968's, already in main).changelog.mjs check→ 62 fragments well formed; nocore/orpatches/file is touched by this PR, so the patch series cannot move.90f4bb5a(#974+#975). They toucheddocs/subsystems/social.md,plugins/social/client.js,plugins/social/src/social.jsandplugins/social/test/social.test.jstoo, so those four were three-way merged againsta9ae2731rather than replaced: the rebuilt files differ from90f4bb5aonly by this PR's hunks, and the suite above runs on that tree.Red without the source change
With only the new tests on pristine sources:
not ok 303(the share list still returns one row and the grant goes topb-coder) andnot ok 308(both rows carry the same React key) → 357 pass / 2 fail. Restored: 409/409. The panel case asserts the two rows have two distinct keys, one reason per row is what lets the click reach the right Bot.Not run
npm run verifyas a whole (the sharednode_moduleson this box makes the full core run abort with esbuildEAGAIN); CI'stests-okis the judge. This PR changes nocore/file, so the core suite is not affected by it.arch-docs: needsgit; both owned paths changed here (plugins/social/src/,plugins/social/client.js) have their subsystem doc updated in this PR.Risk
The one way to make this worse: listing a friend twice for the same Bot. The key is the Bot id, so a repeated
peer_bot_idcollapses exactly as before; a friend whose server sends no Bot id still collapses by home.grantedHomesis unchanged — if any grant exists for that home the whole friend is still omitted, which is the older behaviour and narrower than this PR (per-Bot grant filtering would need the grant's Bot id, whichlist_bot_grantsdoes not give us today); it cannot show a Bot the friend already has, only hide one they do not.