Skip to content

fix(client): keep the Session header seats, without taking the corner seat (#803) - #820

Merged
Birfy merged 1 commit into
ThinkFlowLab:mainfrom
bjf-frz:fix/corner-seat-collision
Oct 23, 2026
Merged

Birfy merged 1 commit into
ThinkFlowLab:mainfrom
bjf-frz:fix/corner-seat-collision

Conversation

@bjf-frz

@bjf-frz bjf-frz commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Fixes #803

Reproduce

On main (6f9b60c), open FlyBot. core/src/client/index.js:888 registers HeaderSeats into conversation.session.header.corner:

shadow('conversation.session.header.corner', { id: 'bot-header-seats', order: 100, inject: face }, HeaderSeats),

That seat is single (ui-conversation/src/client/apply.ts:407), and ui-sidebar-right already registers its right-sidebar button there with no priority, i.e. 0 (ui-sidebar-right/src/client/index.ts:238). SlotCore.register throws on a second registration in the same cell (ui-slots/src/index.ts:1216-1219):

single slot "conversation.session.header.corner" already has a registration at priority 0
  — register at a different priority to shadow it (lowest renders)

SlotRegistry.inject rethrows it out of the caller's effect (ui-renderer/src/client/registry.ts:266-270), so FlyBot's whole client plugin fails from apply and the pill does not mount either. id/order do not change the rank — they are list-kind fields; on a single seat only priority ranks. Registering at priority: -10 would push the shell's button out instead, which is the original bug with the roles swapped.

Ran against DSH's own SlotCore (not a reimplementation):

core.register({ name: 'conversation.session.header.corner' }, C)                        -> OK, priority 0
core.register({ name: 'conversation.session.header.corner',
                id: 'bot-header-seats', order: 100 }, C)                                -> THREW (message above)

What changed

The effects do not need a seat. installHeaderEffects (core/src/client/header-effects.js) is installed once from apply and follows the main-view Session through the sessions list and the roster.

  • core/src/client/header.js: HeaderSeats is gone. HeaderPill keeps its own UI; --bt-first-sep stays with it, because it needs a turn and so always has a header to be drawn in.
  • core/src/client/index.js: installs the effects; nothing registers at .corner. The pill stays in .utilities (unchanged, and still beside .actions, so the subagent catalog keeps rendering).
  • core/src/client/header-effects.js: new, carries no React. Each effect keeps its own dependency key, because the sessions list answers before the roster does and blank flips on the same Session when the first message arrives — a single "did the Session change" guard drops a friend's flags on the first pass; beginVisit/endVisit keyed on more than the Session would open a second visit for a chat that was only just opened.
  • core/test/header-effects.test.js: new, jsdom. Drives roster-arrives-late, blank flipping, the view moving to another Session, and one visit per Session.
  • core/test/header-slot.test.js: pins that nothing registers at .corner, with the reason, and that no page effect runs in the pill. Fails if the .corner line comes back.
  • docs/subsystems/core-client.md, changelog.d/803-…: say the seat is taken and why.

What I ran

At c2f3c39:

  • core npm test — 1292 tests, 1292 pass, 0 fail (DSH_HARNESS an installed harness).
  • node --test test/header-slot.test.js test/header-effects.test.js — 5/5. Re-adding the .corner registration turns header-slot red; removing the per-effect keys turns header-effects red.
  • node scripts/build-client.mjs --check — exit 0.
  • node scripts/ds-bot-patches.mjs check — all three green.
  • core lint — 43 warnings / 0 errors (the baseline count), arch-docs: OK (15 changed file(s)).
  • Registering FlyBot's line-up against DSH's SlotCore with .corner owned by the shell: no throw, corner keeps 1 occupant, actions keeps the subagent catalog.

Note

This is the same change as Birfy#28 (48624e0), with that PR's blocking review finding fixed and this collision fixed. Commented the collision evidence on #28 before it merged.

…inkFlowLab#803)

The follow-up that moved the pill's page effects out of the pill put them in
`conversation.session.header.corner`. That seat cannot take them: it is
`single`, and `ui-sidebar-right` already registers its right-sidebar button
there with no `priority` (0). `SlotCore.register` throws on a second
registration in the same cell —

  single slot "conversation.session.header.corner" already has a
  registration at priority 0 — register at a different priority to shadow it

— and `SlotRegistry.inject` rethrows it out of the caller's effect, so
FlyBot's whole client plugin fails from `apply` and the pill never mounts.
`id` and `order` do not help: they are list-kind fields, and on a single seat
the only ranking field is `priority`. Pushing it aside with `priority: -10`
would hide the shell's own button, which is the failure this issue is about.

The effects no longer need a seat at all. `installHeaderEffects`
(`core/src/client/header-effects.js`) is installed once from `apply` and
follows the main-view Session through the sessions list and the roster.

- `core/src/client/header.js`: drops `HeaderSeats`; `--bt-first-sep` stays in
  `HeaderPill`, since it needs a turn and so always has a header to be drawn in.
- `core/src/client/index.js`: installs the effects, registers nothing at
  `.corner`.
- `core/src/client/header-effects.js`, `core/test/header-effects.test.js`: new;
  each effect keeps its own dependency key, because the sessions list answers
  before the roster and `blank` flips on the same Session when the first
  message arrives, while `beginVisit` must stay keyed by the Session alone.
- `core/test/header-slot.test.js`: pins that nothing registers at `.corner`,
  with the reason, and that no page effect runs in the pill.
- `docs/subsystems/core-client.md`, changelog: say the seat is taken.

Fixes ThinkFlowLab#803
@Birfy Birfy added bug Something isn't working agent-ready Maintainer-approved spec: an agent may take it. Only maintainers set this; never in a template area:ui Web/desktop UI and copy labels Oct 23, 2026
@Birfy

Birfy commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Triaged on f20cfa37.

Repo side confirmed: core/src/client/index.js:888 still registers bot-header-seats into conversation.session.header.corner, and header.js still exports HeaderSeats. The load-bearing fact I cannot verify from this repository is the shell side — ui-sidebar-right taking that seat at priority 0 and SlotCore.register throwing on a second registration in a single cell. Those files live in the DSH harness, not here; the transcript in the report is the evidence for them.

So acceptance starts with the reproduction, and the implementation only happens if it reproduces:

  1. On the running host built from f20cfa37, open FlyBot and record what actually happens to the client plugin: the throw from apply (the single slot ... already has a registration at priority 0 message), or the missing pill and missing --bt-first-sep effects. Paste it in the PR. If it does not reproduce, stop and report with the evidence instead of refactoring.
  2. After the change: the pill mounts, the page effects run, the Session header seats that The Session header's child seats never render, so the subagent catalog is unreachable #803 asked for still render, and the shell's own corner button is still there (one occupant, not zero and not two).
  3. A test pins that nothing registers at conversation.session.header.corner, with the reason, and goes red if the registration comes back.
  4. The effects keep their own dependency keys: roster arriving after the sessions list, blank flipping on the same Session, a visit opening once per Session.
  5. core npm test, build-client --check, ds-bot-patches check and arch-docs are green.

Two notes for the pull request: use Fixes #820 (#803 is already closed; Refs #803 if you want the link) and keep #803's own acceptance intact — the seats must still render, the fix is only about not owning the shell's seat.

Since this report includes the full patch plan, a pull request from the reporter's fork is the shortest path; if that is not possible, a Bot implements it under this issue.

@Birfy Birfy removed agent-ready Maintainer-approved spec: an agent may take it. Only maintainers set this; never in a template bug Something isn't working labels Oct 23, 2026
@Birfy

Birfy commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Correction from Triage: #820 is a pull request, not an issue — I set an issue label on it at 07:36Z by mistake.

  • agent-ready removed: it is a claim label for an issue, not a review verdict. Nothing here is approved for an agent.
  • bug removed as well: type labels belong on issues; area:ui stays as the region label.
  • No verdict exists at c2f3c390; this PR needs an owner on our side, like every open PR. A Dev will shepherd it.

The evidence in my earlier comment still stands as review scope: on f20cfa37 the repo does register bot-header-seats into conversation.session.header.corner (core/src/client/index.js:888), and the collision itself is in the DSH shell (ui-sidebar-right at priority 0 on a single seat), which this repository cannot verify — your reproduction against DSH's own SlotCore is the load-bearing check.

Fixes #803 is correct: #803 was closed with #28's approach, and this replaces it.

@bjf-frz

bjf-frz commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Reproduction, as asked. Run against DSH's own SlotCore (packages/client/ui-slots/lib/index.js), with the shell's declarations and both plugins' registrations in host order — no reimplementation, no mocks.

import { SlotCore } from '.../packages/client/ui-slots/lib/index.js'
const core = new SlotCore()
// ui-conversation declares the seats (apply.ts:395-407)
core.register({ name: 'root', children: { 'conversation.session.header': { kind: 'single', scope: 'session' } } }, AppFrame)
core.register({ name: 'conversation.session.header', children: {
  'conversation.session.header.actions':   { kind: 'list',   scope: 'session' },
  'conversation.session.header.utilities': { kind: 'list',   scope: 'session' },
  'conversation.session.header.corner':    { kind: 'single', scope: 'session' },
} }, ConversationSessionHeader)
// ui-sidebar-right takes .corner (index.ts:238), ui-subagent takes .actions
core.register({ name: 'conversation.session.header.corner' }, ExpandButton)
core.register({ name: 'conversation.session.header.actions', id: 'subagent-catalog', order: -30 }, SubagentCatalogAction)
// FlyBot on f20cfa37: index.js:885 then :888
core.register({ name: 'conversation.session.header.utilities', id: 'bot-pill', order: 100 }, HeaderPill)
core.register({ name: 'conversation.session.header.corner', id: 'bot-header-seats', order: 100 }, HeaderSeats)

Output:

RESULT: THREW from the FlyBot registration
  single slot "conversation.session.header.corner" already has a registration at priority 0 — register at a different priority to shadow it (lowest renders)
corner occupants: ExpandButton

The throw does not stay inside the registration — it fails apply:

  • SlotCore.register throws synchronously at the same-priority check (ui-slots/src/index.ts:1216-1219).
  • slots.inject(key, callback) reconciles synchronously and rethrows a setup failure out of the caller (ui-renderer/src/client/registry.ts:262-267: try { reconcile() } catch (error) { stop(); throw error }), and reconcile runs the callback through ctx.effect.
  • FlyBot's line is shadow('conversation.session.header.corner', …), which is exactly ctx.slots.inject(name, () => ctx.slots.register(…)) (core/src/client/index.js:792).

So on a host built from f20cfa37 the client plugin fails to load; it is not a silent no-op and not a console.warn. The id and order fields do not move the rank — they are list-kind fields, and on a single seat priority is the only ranking field, defaulting to 0. priority: -10 would instead push ExpandButton out, which is the original #803 failure with the roles swapped.

One caveat I will state plainly rather than overclaim: I could not run a host build of f20cfa37 end to end — this repository has no DSH shell in it, and my running install predates #28. What is above is the shell's own registry code executing the exact registrations both plugins make, plus the call path that turns the throw into a failed apply. If you want the GUI-level artifact too, ui-sidebar-right's plugin log is where it will land, and the symptom will be the whole FlyBot client bundle failing rather than a missing pill.

Everything else in the review scope is already in the branch at c2f3c39:

  1. Nothing registers at conversation.session.header.corner; core/test/header-slot.test.js pins it with the reason and goes red if the line returns (verified both ways).
  2. The effects keep their own dependency keys — core/test/header-effects.test.js drives roster-arriving-late, blank flipping on the same Session, the view moving, and one visit per Session (verified red on the single-key form).
  3. core npm test 1292/1292, build-client --check exit 0, ds-bot-patches check three green, arch-docs: OK (15 changed file(s)), lint 43 warnings / 0 errors (baseline).
  4. The Session header's child seats never render, so the subagent catalog is unreachable #803's own acceptance is intact: the pill stays in .utilities, .actions keeps the subagent catalog, and .corner keeps exactly one occupant (the shell's button).
  5. Fixes #803 as corrected.

@Birfy

Birfy commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS at c2f3c39
Written-by: Reviewer2

Ran:

  • https://codeload.github.com/ThinkFlowLab/FlyBot/tar.gz/c2f3c390… extracted to a clean tree; FLYBOT_CORE=<tip>/core DSH_HARNESS=<installed harness>, then node --import ./test/harness-resolve.mjs --test test/header-slot.test.js test/header-effects.test.js test/looks.test.js in core → 11 tests, 11 pass, 0 fail.
  • node scripts/build-client.mjs --check → exit 0, so the committed core/client.js is what src/client builds.
  • Knife, the seat: shadow('conversation.session.header.corner', { id: 'bot-header-seats', order: 100, inject: face }, HeaderPill) put back into src/client/index.js → #803: the page effects are installed from apply, not parked in a slot red at ds-bot must not register into conversation.session.header.corner …; the other two cases still pass. Restored → 11/11.
  • Knife, the dependency keys: all six run(id, dep, setup) dependency strings in header-effects.js replaced with the bare sessionId → both cases of header-effects.test.js red, the first at the friend-chat flag follows the roster. Restored → 11/11.
  • The collision, against DSH's own SlotCore (installed @deepseek-ai/dsh-client-ui-slots@0.2.0-rc.2 lib/index.js, not a reimplementation): register({ name: 'conversation.session.header.corner' }) → OK at priority 0; the same seat again at priority 0 → threw single slot "conversation.session.header.corner" already has a registration at priority 0 — register at a different priority to shadow it (lowest renders).
  • Read the installed packages around it: dsh-client-ui-sidebar-right/lib/client.js does ctx.slots.inject("conversation.session.header.corner", () => ctx.slots.register({ name: …, locale: NS, store, inject: … })) with no priority; dsh-cordis-client-runner/lib/client.js declares that seat kind: "single"; dsh-client-ui-renderer/lib/client.js:1343 inject reconciles inside ctx.effect at declaration time and rethrows a callback setup failure synchronously (try { reconcile() } catch (error) { stop(); throw error }).
  • diff -u patches/ds-bot/files/{src/client/header-effects.js,test/header-effects.test.js,test/header-slot.test.js} against the tree copies → identical.
  • Re-read right before writing: GET /repos/ThinkFlowLab/FlyBot/pulls/820 → head c2f3c3902f69c140bdd29acbae8c91802d916318 (branch bjf-frz:fix/corner-seat-collision), base 6f9b60c7…, mergeable: true; three comments (Triage's triage and its correction, the author's reproduction) and no Verdict: and no review, so this tip carried no conclusion. Checks on this head: nine completed/success; enqueue-guard is red with do not enqueue #820: no Verifier report names the tip c2f3c3902f69 — see the first note.

What I checked:

  • The bug is real in the direction the PR reports: .corner is single, the sidebar's button owns it at priority 0, and id/order rank list seats only, so that line could not shadow the occupant — it threw, and the throw leaves the declaring plugin's apply, which is FlyBot's own client plugin. My probe is that throw on the shipped code.
  • The fix removes the claim instead of ranking around it. priority: -10 would take the seat and evict the shell's own button — the original bug with the roles swapped — and the diff contains no priority.
  • The effects genuinely no longer depend on the pill being painted: installHeaderEffects is installed once from apply inside ctx.effect(…) (index.js:766), finds the Session the way the rest of this client does (retainedBy.mainView > 0, as sidebar.js:653, overlays.js:23, brand.js:51 and several sites in index.js), and its teardown unsubscribes both sources and then runs every effect's own cleanup.
  • The per-effect keys are load-bearing, not decoration: knife B reproduces the failure the PR description names — with one "did the Session change" guard, the roster answers after the sessions list, so a friend's chat has no bot on the first pass and its flags never get set.
  • The visit ledger is keyed on the Session alone and run calls the previous off() before the new setup(), so a roster answer that only adds the Bot's name cannot close and reopen a visit; header-effects.test.js drives one-visit-per-Session on jsdom.
  • --bt-first-sep is the one thing that rightly stays in HeaderPill: it sizes a separator from the pill's own width and needs a turn, so it always has a header to be drawn in.
  • header-slot.test.js still pins the part of The Session header's child seats never render, so the subagent catalog is unreachable #803 that must not regress: no claim on conversation.session.header, the pill stays in .utilities with id: 'bot-pill', and none of the five page effects runs in the pill body.

Notes (non-blocking, not merge-gate items):

  • docs/subsystems/core-client.md:98 still says HeaderPill holds the composer label "in a layout effect" and that its cleanup runs "when the header unmounts or the session changes". Both are now false — holdComposerLabel is called from header-effects.js:65 under ctx.effect, and its cleanup runs on a dependency change or on plugin dispose — and that sentence sits two bullets from the new bullet that says the opposite. Worth a one-line follow-up; the code is right, the prose is stale.
  • composer-label and face-at-menu have no case that reds when they are removed: deleting either run(…) leaves header-effects.test.js green. They are only covered indirectly (test/composer-label.test.js, the face-room tests).
  • header-slot.test.js proves the absence of the .corner registration from source text, not by running apply. I closed that gap by hand with the SlotCore probe above; if the register call ever moves behind a helper, the text pin goes quiet.
  • enqueue-guard on this head is red only because no verdict named the tip: .github/workflows/enqueue-guard.yml re-judges the live head on issue_comment, so this comment flips that same run without a new commit. The nine code checks were green before it, and mergeable_state: unstable comes from that one check alone.
  • ds-bot-patches.mjs check needs git, which this machine does not have (spawnSync git ENOENT), so I compared the patch mirrors with diff instead; the package check on this head runs the real script and is green.
  • Base 6f9b60c7 is behind main (82afc513 when read). Nothing to rebase (mergeable: true), but the merge queue re-runs against current main.
  • Scope: this is a fork head from an outside contributor and it changes user-visible behaviour on load, so the owner should decide whether to take it here instead of waiting for the same change through Birfy/FlyBot#28 (already merged there). The change is a repair — on main the client plugin fails from apply — not a redesign.

@github-actions github-actions Bot added the pass label Oct 23, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 23, 2026
Merged via the queue into ThinkFlowLab:main with commit 7c1a5fc Oct 23, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ui Web/desktop UI and copy pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The Session header's child seats never render, so the subagent catalog is unreachable

2 participants