Repository navigation
Conversation
90d09c7 to
37f9e13
Compare
…s in fixture pages Fixes the five Browser-journey shards failing on PR #1624, both caused by 21a245e. `AutoJoinSetting` and `ChannelIdDetail` are siblings in the settings `<dl>` and both used `key={channel.id}`, so React logged "two children with the same key" and the fixture console gate failed `channel-header-menu.spec:19` and `navigation-groups.spec:622`/`:838`. The row keeps a per-channel key (its pending/error state must not survive a channel switch) under a distinct `auto-join:` prefix. The channels bundle now injects `adminAccess`, so the hand-built plugin contexts in `tests/fixtures/mentions.tsx` and `conversation.tsx` never activated the plugin (`channel-completion.spec:7`/`:184`, `conversation.spec:13`). Both now provide `{ membership: undefined }`, as `service.test.tsx` already does; the panel hides the switch without a roster. Test: the auto-join visibility matrix asserts no duplicate-key warning (fails without the key change in the four cases that mount the row). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
Verdict: two recommended blockers, both in the 9021 fallback in default-channels.ts. The relay contract half holds up: the two-tag 9002 shape matches block/buzz#8594 across the TS validator, the browser broker and the Rust lifecycle route, the readback matches the bare 39000 tag, and the fallback can't reach private or archived channels. First-channel selection also held up when we tried to make it latch onto a channel whose metadata was still pending.
Validation gaps
- The fallback tests keep
list()empty across invocations, so nothing covers a second run after a partial success. - The switch is only tested as a component with a mocked lifecycle and membership. No page journey covers the
membershipwiring inindex.tsx/ChannelsPage.tsx, or9002→ republished39000→ switch shows on. - As the PR says, there's no native build or live-relay run yet.
Exit criteria: the two blockers are addressed, or Wes/the author explicitly accepts the re-add behavior; plus one test that runs the fallback twice with the first join's membership retained. The other inline comments are optional.
Reviewed at 2d3a7e7c4 against block/buzz#8594 at 502d0160. Hosted CI was green; we ran no suites locally.
| channelTypeTag(channel) !== "stream" | ||
| ) | ||
| throw new Error( | ||
| "Only active public channels can auto-join new members.", |
There was a problem hiding this comment.
Low (optional, product): an archived or private starter channel can't be un-flagged from the UI. block/buzz#8594 keeps the stored auto_join when a flagged channel is archived or made private, and rejects only Some(true) there, so auto_join=false is accepted. Here flaggable() rejects both directions on a non-active channel, and the switch is hidden for archived and private channels (ChannelSettingsPanel.tsx:199-202). An admin who archives a starter #general to retire it, and later unarchives it, finds new members being auto-joined again. The switch does show on after the restore, but nothing warns at archive time.
Fix: let auto-join-off skip the active-public check, and either offer an off-only switch on archived/private channels or mention it in the archive/private confirm copy. Document it in docs/channels.md either way.
|
From a UX perspective, this looks good to me! I was going to work on the starter channels settings as I was going through those, so it's super helpful to get that built in here. I also think this can work with the changes I'm proposing to onboarding with a default agent (instead of Fizz and co). In that flow, there's a generic agent that you customize as part of the onboarding experience, and then it gives you options for next steps. Initially, we had "Ask about this community" but it could actually be more specific to the starter channels and then get you into those more actively. |
Add `ChannelSummary.autoJoin?: true`, set from the bare relay-authored `["auto_join"]` tag on kind 39000 in `DiscoveryState.get()`, the single parser behind live, session, `searchPublic`, and restored snapshots. The tag is effective-state (present only while the relay auto-adds), so the flag clears when a newer metadata version omits it. `setList` compares `autoJoin` so a toggle-only metadata change replaces the summary. The browser fixture gains an `autoJoinChannels` option beside `lifecycleVisibility`, and `buzz-mcp` channel listings/details show an `auto-join` flag next to `private`/`archived`. Contract and parsing only (A1); first-channel selection, the 9021 fallback, and the admin switch follow separately. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… 9021
With no saved selection, `ChannelsPage` now prefers the first `autoJoin`
non-Solo channel ahead of its existing first-non-Solo fallback, so a new
member lands in the community's default channel instead of "Select a
channel to read it."
The relay normally joins new members into its `auto_join` channels during
the invite claim. `features/channels/default-channels.ts` is the backstop
for open or old relays and failed adds: once the live roster is verified
and empty, `ensureDefaultChannels` reads the public directory through a new
store `defaultChannels()` reader (the same shared pager as `searchPublic`)
and sends one 9021 per missing channel through `lifecycle.run("join")`,
sequentially, tolerating relay rejections. A `default-channels` view-state
marker in the `origin:viewer` scope makes it one-shot: the fallback runs
unless the marker is `done`, and writes `done` once every candidate settled
with success or a definitive `PublishRejected`, or once the verified roster
turns out non-empty, so leaving every channel later never re-adds them.
Unavailable channel actions, aborts and unconfirmed joins leave the marker
for the next session. `useDefaultChannels` is wired in `ChannelsPage`'s
workspace, keyed by `scope:generation`; no FOUNDATION edits.
`markDefaultChannelsPending` is exported for an admission to write
`pending`, but no caller is wired here: `CommunityDialog` is unchanged, so
the new dialog test's `joined` case fails at this commit until "fix(channels):
run the default-channel fallback only after a fresh joined claim" adds the
write.
Docs: `channels.md` local-first launch covers the selection preference and
the backstop; `communities.md` Try it mentions what a new member sees.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
`ChannelLifecycleCommand` gains `auto-join-on` and `auto-join-off`, commands rather than actions so the confirmation menu and dialog are untouched. `lifecycleTemplate` maps both to the narrow kind 9002 `[["h", id], ["auto_join", "true"|"false"]]`, and the two host validators move in lockstep: `validateLifecycleTemplate` (also imported by the dev broker) and the Rust `channel_writes.rs lifecycle()` accept the second tag as `archived` or `auto_join` with `true|false`, nothing else. `lifecycle.run` pre-reads signed 39000 and requires an active public stream channel before signing; channel roles cannot show community ownership, so the relay's owner/admin rule is left to the relay and its refusal is surfaced. Readback is the bare effective-state `auto_join` tag on the republished 39000, so success is that tag's presence (on) or absence (off), not the written value; a missing tag after an accepted write stays `ChannelLifecycleUnconfirmed`. `ChannelSettingsPanel` renders a `SwitchPreferenceRow` between Visibility and Members, shown only when the panel is editable, the channel is a public stream, and `managesCommunity(role)` holds over the shared `adminAccess.membership` roster, which the row `ensure()`s while mounted. The channels bundle injects `adminAccess` and threads `membership` through `ChannelsPage`. No optimistic state: the switch disables while `run` is in flight and then follows `channel.autoJoin`. `PublishRejected` shows the relay's reason or "Only community owners and admins can change this."; other failures show the lifecycle owner's refresh guidance. Docs: `channels.md` lifecycle and details sections. Tests: lifecycle template/readback/refusal cases, Rust and broker validator matrices, and panel visibility, pending, rejection and unconfirmed cases. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ep the computed selection
Addresses the review of 54560ac23..dbaaa0e9c.
`ensureDefaultChannels` treated an absent marker and `pending` alike, so
the `joined`-only write in `CommunityDialog` had no effect and the docs
promised an `already_member` guarantee the code did not keep. Keep the
"absent runs" semantics (open relays never claim) and delete
`markDefaultChannelsPending`, the `admitted` ref and its test; `done` is
the only marker value. `channels.md` and the module comment now state the
real guarantee: at most once per community, viewer and device, suppressed
thereafter by `done`; a non-empty verified roster also marks it done; a
member who left every default channel may be re-added once on a device
that never marked done.
`defaultChannels()` returns `{ ids, partial }` from the shared directory
pager, and the fallback leaves the marker unset when the page was capped,
so flagged channels past the bound are not silently given up on.
`ChannelsPage` keeps the computed choice sticky: a `chosen` ref tracks the
last resolved destination, consulted after the saved selection and before
the autoJoin/first-non-Solo preference, so a later flag or relay add never
swaps the channel being read; the preference re-evaluates only when
nothing is chosen or the chosen channel left the list. The late-44100
case still selects the arriving default channel.
Tests: partial read and busy-owner `Error` (not `PublishRejected`) leave
the marker unset; a reader on the first non-Solo channel stays when a
flagged channel arrives (fails without the ref).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…d draft Addresses the review of 883bcaef0. `ChannelsPage` cleared `chosen.current` whenever `requestedDestination` was undefined, which includes the `emptyDestination` case: an open Solo draft or an explicit empty route. A reader on a computed channel (no `selected-channel` saved, since that effect fires only for conversation navigation) who opened and closed New chat re-evaluated the autoJoin/first-non-Solo preference on close, so a flag that arrived meanwhile moved them. Only overwrite the ref when a destination resolved; a stale id for a channel that left the list is harmless because the `find` falls through anyway. The `default-channels.ts` module comment already states the accurate guarantee (a capped page leaves the marker unset, but a non-empty roster on a later mount spends the shot), so it is unchanged. Test: `workspace()` in `ChannelsPage.tabs.test.tsx` gains a `visit()` rerender; a reader on #Alpha who visits the draft route while a flagged guard, landing on #Beta). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…w members" Copy change only: the `SwitchPreferenceRow` label in `ChannelSettingsPanel`, the two role queries in its tests, and the two `channels.md` mentions. The description sentence under the row still reads correctly and is unchanged. Code identifiers (`auto-join-on`/`auto-join-off`, `autoJoin`, the `auto_join` tag) keep their names. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…s in fixture pages Fixes the five Browser-journey shards failing on PR #1624, both caused by 21a245e. `AutoJoinSetting` and `ChannelIdDetail` are siblings in the settings `<dl>` and both used `key={channel.id}`, so React logged "two children with the same key" and the fixture console gate failed `channel-header-menu.spec:19` and `navigation-groups.spec:622`/`:838`. The row keeps a per-channel key (its pending/error state must not survive a channel switch) under a distinct `auto-join:` prefix. The channels bundle now injects `adminAccess`, so the hand-built plugin contexts in `tests/fixtures/mentions.tsx` and `conversation.tsx` never activated the plugin (`channel-completion.spec:7`/`:184`, `conversation.spec:13`). Both now provide `{ membership: undefined }`, as `service.test.tsx` already does; the panel hides the switch without a roster. Test: the auto-join visibility matrix asserts no duplicate-key warning (fails without the key change in the four cases that mount the row). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ined claim Addresses the two blockers on `src/features/channels/default-channels.ts` in the PR #1624 review (threads on lines 33 and 34). Line 33, a missing marker re-adds existing members who left: absence of the `default-channels` marker now means "do nothing". `admitCommunity` reports `joined: true` only when its claim returned `joined` (never `already_member`), and `CommunityDialog` writes `pending` for that origin and viewer right after admission, so a later Back/Continue reclaim that answers `already_member` keeps it. Upgrades, new devices, cleared storage, recovered claims and code-less admissions write nothing, matching the relay contract in block/buzz#8594 ("once, at that moment ... a later kind:9022 leave stands"). No other client moment proves a new membership: a code-less admission is either an open-community newcomer or a returning member who never published a profile, so open communities get no client fallback. Line 34, one successful join strands the remaining defaults: the "non-empty verified roster -> done" early exit is gone. While `pending`, candidates are the flagged public channels the viewer has not joined (the store already excludes rostered ones), joined sequentially as before; `done` is written only when the directory read was complete and every candidate settled. Otherwise the marker stays `pending` and the next run joins only what is still missing. Optional sub-point from the same thread: a `PublishNotSent` (how the transport maps NIP-01 `rate-limited:`) or a `PublishRejected` whose text starts with `rate-limited:` no longer counts as settled. Tests: absent and `done` markers read and send nothing with an empty roster; two runs where #general's membership is retained and #welcome is still outstanding (the second joins only #welcome, then marks done); rate limits and unsent writes stay pending; `admitCommunity` reports `joined` only for a `joined` claim; the dialog writes `pending` for `joined` and nothing for `already_member`. The five fallback cases and the dialog `joined` case fail on the previous code. Module comment, `channels.md` and `communities.md` state the new guarantee. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Addresses the PR #1624 review thread on `src/features/relay/channel-lifecycle-protocol.ts:50` (on -> off -> on within one second loses the last toggle). `created_at` has one-second resolution and lifecycle tags are fixed, so a third toggle inside a second re-signed the first event byte for byte. The relay deduplicates by id before applying metadata, so the flag stayed off and the run ended `ChannelLifecycleUnconfirmed`. The lifecycle owner now keeps the last `created_at` it issued per channel and signs the next command for that channel at least one second later. That covers every lifecycle command (join/leave and archive/unarchive repeat the same way); `lifecycleTemplate` stays a pure template shared with buzz-mcp. Test: on, off, on under a frozen `Date.now` against a publish mock that deduplicates by id publishes three distinct ids at consecutive seconds and ends with the flag on (fails before: the third run is unconfirmed). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…the row dt/dd semantics Addresses the two low PR #1624 review threads on `src/bundled/channels/ChannelSettingsPanel.tsx`. Line 294 (copy): a confirmation timeout showed the raw `ChannelLifecycleUnconfirmed` message, two sentences that both say "Refresh channels before trying again" in a panel with no refresh action. It now maps to one sentence, as `ChannelJoinNotice` does: "Saved, but not confirmed yet. Try again to check." Setting the same value again is idempotent, so retrying is the check. Refusals and other failures keep their copy. Line 303 (a11y): the row and its `role="alert"` were direct `<dl>` children beside `div > dt/dd` rows (axe `definition-list`). The row is now a `div` whose `dt` holds the visible label (a `<label>` for the switch) and whose `dd` holds the description, the shared `Switch` and the error, keeping the `auto-join:` key. `SwitchPreferenceRow` renders its own title and cannot be split across `dt`/`dd`, so the row uses `Switch` in its documented aria-label mode for rows that own a visible label; the `dd` is a two-column grid that keeps the switch at the trailing edge with the error spanning below. Tests: the unconfirmed case throws the lifecycle owner's real wrapped error and every reported copy is matched exactly; the visibility matrix and the error cases assert each `dl` child is a `div` of `dt` then `dd` (fails before for the row and for the alert); clicking the visible term runs the command. Rendered check (throwaway Playwright pass over the browser fixture with the viewer temporarily served as community owner, not committed): Chromium light/dark at 480 and 1280 px, at 100% and 150% root text, plus WebKit dark 480/150% and light 1280/100%. Space on the focused switch and a click on the label both ran the command; the unconfirmed copy rendered under the switch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…auto-join switch Addresses the review's validation gap on PR #1624: no page journey covered the `membership` wiring in ChannelsPage, or 9002 -> republished 39000 -> switch shows on. The page test opens Channel details as an owner, checks the roster is ensured, toggles the switch through a lifecycle writer whose relay republishes the metadata with `auto_join`, and sees the switch turn on with no error. It fails if ChannelsPage drops the `membership` prop or if discovery stops reading the republished flag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Addresses the two warnings on `src/features/channels/default-channels.ts` in
review b1bf9fe0 (lines 56-71 and 56-58).
While the marker stayed `pending`, every run recomputed its candidates as
"flagged and not joined", so a later run re-added a channel the claim had
added and the member then left (a 9022 leave must stand). The first run after
`pending` now snapshots the candidates into the marker as `{ owed: [...] }`.
Later runs send 9021 only for owed ids the directory still lists as
candidates, drop each id as it settles (joined, no longer listed, or
definitively refused), and write `done` when nothing is owed. Channels the
relay already added are never owed. Each settled id is written immediately, so
an abort mid-run cannot leave a joined channel owed. Unavailable channel
actions leave the marker as is; rate limits, `PublishNotSent`, unconfirmed
joins and a busy lifecycle owner keep the id owed.
A capped directory page is now final: the snapshot owes whatever the read
returned, so a community past `PUBLIC_CHANNEL_DIRECTORY` reaches `done`
instead of rereading up to 40 pages on every Messages mount. The fallback was
the only reader of `defaultChannels()`'s `partial`, so the store and contract
return the ids alone again.
Tests: the review's case (the claim added #general, #welcome is owed and
unconfirmed, the member leaves #general, and the next run rejoins only
#welcome); an owed id that is already joined drops without a send; a capped
page settles and does not reread; lifecycle-unavailable keeps both `pending`
and an owed set; an abort keeps only the unsettled id owed. Ten of these fail
on the previous module. `channels.md` and the module comment describe the
owed set.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…cribe the switch by its error
Addresses the warning on `src/bundled/channels/ChannelSettingsPanel.tsx`
lines 295-301 and the a11y suggestion on lines 323-325 in review b1bf9fe0.
`PublishNotSent` extends `PublishRejected` and carries no `reason`. A
rate-limited or never-sent toggle therefore told an owner or admin "Only
community owners and admins can change this." It is now checked first and
reads "Not sent. Try again.", the same distinction `default-channels.ts`
draws in `refused`.
The `role="alert"` error now has an id, and the switch adds it to
`aria-describedby` while the error is shown, so tabbing back to the switch
still announces why the last toggle failed. While the switch is disabled
(pending or no channel actions), the row's `dt` label uses
`--text-unavailable`, as `PreferenceRow`'s disabled title does.
Tests: a `PublishNotSent("rate-limited: ...")` row in the error matrix, and
every row asserts the switch's accessible description includes the error and
drops it after a successful retry. All four rows fail on the previous panel.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
2d3a7e7 to
4ee6c82
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 One new P2 is inline; the prior fallback blockers and the timestamp, error-copy, and definition-list fixes are addressed in source.
Star Lord’s automated follow-up via Wes’s account, with independent Rocket, Mantis, and Groot coverage. Reviewed head 4ee6c825a46b517da1e692d42957065f3c9c72a4 against base 11dd17bb7775a2932ca2466235354105edf2041b. No code, tests, or apps executed; CI, native behavior, and live-relay acceptance remain unverified. The two demo videos were inspected through sampled frames only.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One additional leave-preservation defect remains on the unconfirmed publication path.
…t may have reached the relay Addresses the two PR #1624 review threads on `src/features/channels/default-channels.ts`. Line 77 (persist before joining): `writeView`'s `false` return was ignored. With a readable `pending` marker and a failing write, the run still sent every 9021 and left `pending`, so a later run could owe a channel it had joined and the member had since left. The first save now gates the loop: no 9021 is sent until the owed set is stored, and the marker stays as it was for the next run. A failed save after a settled join hit the same rejoin, and stopping alone did not fix it: storage still owed the channel that had just joined. That save failing now stops the run and forgets the marker (`clearView`, a removal that needs no quota). The remaining retries are given up rather than risking a rejoin. Line 87 (settle joins that may have taken effect): a 9021 accepted but unconfirmed on readback stayed owed, so a leave before the next run was undone. In the lifecycle owner, every error after `publicationStarted` other than `PublishRejected` becomes `ChannelLifecycleUnconfirmed`, and that includes the caller's abort, so a post-publication cancellation shows up the same way. `settles` (formerly `refused`) now counts `ChannelLifecycleUnconfirmed` as settled and is checked before the `signal.aborted` return. Failures known to happen before publication stay owed: unavailable actions, a busy owner, `PublishNotSent`, rate limits and a raw pre-publication abort. Tests: the harness's `run` gets a `publish` callback that records membership even when `run` then throws. New cases: an accepted join whose readback timed out, and one cancelled after publishing, both settle, and leaving the channel then stands. A readable `pending` with every write failing sends no join, and joins once storage recovers. A failed save after a join forgets the marker, and the following leave stands. All four fail on the previous module; the last one also fails with stop-only handling. The "claim already added" case now uses an unsent join, the unconfirmed row moves out of the "keeps one owed" matrix, and the abort case is renamed to say it is a pre-publication cancellation. Module comment and `channels.md` describe the new settling and storage rules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 One new P2 is inline: a cancelled native fallback can overwrite its successor’s completed state and later rejoin a channel the member left. The prior persistence finding is fixed.
Star Lord’s automated follow-up via Wes’s account, with independent Rocket, Mantis, and Groot coverage. Head c89e511a9bafec582e5f59f98876455844a2b4a3, base 11dd17bb7775a2932ca2466235354105edf2041b. Source-only: no tests or apps executed; CI was still running, native/live-relay acceptance remains unverified, and demo inspection used sampled frames only.
…'s marker Addresses the PR #1624 review thread on `src/features/channels/default-channels.ts:104`. Since c89e511 a join cancelled after publishing settles, and its save was unconditional. A native run could owe [A, B], publish A, and be cancelled during session replacement while `relay_channel_publish` was still pending. The successor saw A joined, joined B and saved `done`. When the old IPC returned, its `ChannelLifecycleUnconfirmed` settled A and the old run then wrote `{ owed: [B] }` over `done`, so the next run undid a leave of B. Every save now goes through `replaceView`, comparing against the revision this run last read or stored. When another run has changed the marker the old run stops without writing; with no successor, the settlement is saved as before. A storage failure still forgets the marker. The first snapshot uses the same fence, so a stale run whose directory read finished late cannot overwrite a successor's snapshot either. Test: a held two-run case. The old run publishes #general and is aborted. The successor joins #welcome and marks done, and only then is the old publish released as unconfirmed. The marker stays `done`, and leaving #welcome causes no third-run join. By trace it fails on the previous module, which writes `{ owed: ["welcome"] }`. Not run locally: the machine is too loaded to test reliably, and the commit and push skipped hooks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 One P2 remains in the fallback’s leave-preservation contract: the revision guard can discard a late settlement when its successor makes partial progress (inline). The overwrite of a successor’s done marker is fixed.
Star Lord’s automated source review via Wes’s account, with independent Rocket, Mantis and Groot coverage. Head f83e506628f403e3a3e50498f037940a97d37d3b, base 11dd17bb7775a2932ca2466235354105edf2041b. No tests or apps executed; CI was running, native/live-relay acceptance remains unverified, and demo inspection used sampled frames only. The latest commit reports skipped hooks and no local test run.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One confirmed P2 remains: overlapping fallback runs can discard a settled join and undo a later explicit leave. Added Chromium/WebKit reproduction evidence and the equal-value revision ordering to the existing anchored finding: #1624 (comment). Fix before merge; no duplicate finding opened.
…stored Addresses the PR #1624 review thread on `src/features/channels/default-channels.ts:122` (Wes's partial-successor ordering and Kalvin's equal-value ordering). f83e506 fenced every save on the revision this run last read or stored, and a run whose save found another value stopped without writing. That discarded settlements whenever runs overlapped. A session replacement aborts the old run, but a native publish still waits on IPC, so its result can arrive after the successor has started: - Partial successor: the old run's #general publish is in flight. The successor fails #general before publishing, joins #welcome and stores `{ owed: [general] }`. The old publish then lands, and its settlement saw a changed marker and was dropped, so a later leave of #general was undone. - Equal value: the successor rewrote the byte-identical `{ owed: [A, B] }`, so the old run's settlement of A saved `{ owed: [B] }`. The successor's own B settlement then saw a changed marker and was dropped. Storage now holds what is owed, not either run. A settlement removes only its id from the owed set stored at that moment and writes it under that revision. A marker that is `done`, cleared, or a new admission's `pending` is left alone. Before each join a run checks that storage still owes the id and stops once it owes nothing, so neither run sends a join for a channel the other settled. The first snapshot keeps its fence, so a late directory read cannot overwrite a successor's snapshot. A save that storage cannot record still forgets the marker. Tests, using a small `gate()` helper that the existing held test now uses too: - the partial-successor ordering, then leaving both channels triggers no further join; - the equal-value ordering, then leaving #welcome triggers no further join; - the old run settling #welcome while the successor is still on #general, after which the member leaves #welcome and the successor skips it. All three fail on the previous module, and the third also fails without the pre-join check. The existing case where the successor reaches `done` is unchanged. The module comment and `channels.md` describe the new rule. Verification: all 25 tests in `default-channels.test.ts` pass, `tsc --noEmit` is clean, and Biome passes on the changed files. Hooks were skipped (`--no-verify`) because the machine is too loaded for the pre-commit and pre-push suites to run reliably. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 No changes requested on this follow-up. The prior late-settlement finding is addressed: each result removes only its channel from the current owed set, and the added regressions cover partial-successor, equal-value, and settled-meanwhile orderings.
Star Lord’s automated source review via Wes’s account, with independent Rocket, Mantis and Groot coverage; head d8a397b4cb067f08b413e4ff57e4d5ae80892e74, base 11dd17bb7775a2932ca2466235354105edf2041b. No tests or apps executed; CI was still running, the latest commit reports skipped hooks, and native/live-relay acceptance remains unverified. The two demo videos were not re-inspected this cycle.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 The prior same-context settlement finding is addressed in Chromium and WebKit app/broker reproductions. Remaining observations below distinguish a modeled cross-window race from a reproduced retry delay and a regression-test gap.
… and reconcile after the directory read Addresses the three PR #1624 review threads on `src/features/channels/default-channels.ts` (lines 62, 113 and 121). Line 62 (P2, windows lose a settled removal): `replaceView` is a foreground comparison, so two same-origin windows could both read `{ owed: [A, B] }`, and A's settlement writing `[B]` would overwrite B's `[A]`. The snapshot and each settlement now run their read-modify-write under a per-scope Web Lock, `buzz-default-channels:<scope>`, as `bestie/setup.ts` and `read-state.ts` already do across windows. Network waits (the directory read and each join) stay outside the lock, so a stalled publish never holds back another run. Without Web Locks the update runs unguarded, as before. Line 113 (P3, a successor gives up after a predecessor settles during its directory read): the snapshot compared against the revision read before the query, so any settlement in between made it return, and the remaining owed channels waited for a later mount. The snapshot now reconciles with the marker stored after the read: an owed set becomes its intersection with the directory's candidates. Intersecting never owes again what another run settled. `pending` counts only if it is the same marker read before the query, and `done`, a cleared marker or a new admission's `pending` stands, so those fences are unchanged. Line 121 (P3, the settled-meanwhile guard was untested): the regression now owes a third channel (#news), so the successor reaches the #welcome the other run settled while #news is still owed, instead of stopping at `done` first. Tests: - the settled-meanwhile case with #general, #welcome and #news; it fails without the pre-join guard; - a successor whose directory read is held while the other run settles #general still joins #welcome; fails on the previous module; - an owed set replaced by `done`, a cleared marker or a new admission's `pending` during the read stands and sends nothing; the `pending` case fails if the reconcile drops the revision check; - with a `navigator.locks` stub, every marker write happens inside the scope's lock; fails on the previous module and if only the settlement loses the lock. All 30 tests in `default-channels.test.ts` pass, `tsc --noEmit` is clean, and Biome passes on the changed files. Hooks were skipped (`--no-verify`) because the machine is too loaded for the pre-commit and pre-push suites to run reliably. Not done: a two-page browser run with Leave and reload. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 No changes requested on this follow-up: scoped Web Locks, post-read reconciliation, and the three-channel regression address the prior observations without a new source-demonstrated defect.
Star Lord’s automated source review via Wes’s account, with independent Rocket, Mantis and Groot coverage, at head 997028b70eaf7a3796d76b0542172048d9a1095b / base 11dd17bb7775a2932ca2466235354105edf2041b. Source only: no tests or apps run; cross-window/native behavior remains unverified despite the successful CI-required snapshot. Both demo attachments returned HTTP 403 and were not inspected.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Two remaining findings: a delayed automatic join can reverse Leave, and the lock regression does not detect stale reads.
…k join Addresses the two PR #1624 review threads on `src/features/channels/default-channels.{ts,test.ts}` from review 997028b. `default-channels.ts:143` (P2, a join held while signing undoes a leave): the pre-join guard ran before `lifecycle.run`, but signing can wait. Window A passed the guard and waited on the signer; window B joined the same channel, saved `done`, and the member left; A's post-signing authorization then saw no membership and published another 9021. The fallback now passes the lifecycle owner's existing `active` callback, which it checks right before publishing, so storage must still owe the id at that moment or nothing is sent. Both checks use one `owes(scope, id)` predicate. A declined join throws the owner's pre-publication `AbortError`, which does not settle, so the run moves to the next candidate. The loop now skips (rather than returns) once nothing is owed; no candidate is then sent, so the effect is unchanged. `default-channels.test.ts:466` (P3, stale reads while waiting for the lock): the lock test showed only that writes happen under the lock, so reading the marker before taking the lock survived the suite. A new case runs once for the snapshot and once for a settlement: storage owes #general and #welcome, and another window settles #welcome while this run waits for the lock. Only #general joins and the marker becomes `done`. Tests: - the harness's `run` now models the owner's last check: after the hook returns, a declined `active` throws `AbortError` and publishes nothing; `members()` exposes the roster; - a join held while signing, while another window joins #general, marks `done` and the member leaves, publishes nothing, so the leave stands; - the two lock-wait cases above. Fail-before, each by mutation against this module: without the `active` argument, the signing case fails; reading the snapshot's marker before the lock fails the snapshot case; reading `drop`'s marker (or its marker and revision) before the lock fails the settlement case. Each mutation fails only its targeted test. Verification: all 33 tests in `default-channels.test.ts` pass, `tsc --noEmit` is clean, and Biome passes on the changed files. Hooks were skipped (`--no-verify`) because the machine is too loaded for the pre-commit and pre-push suites to run reliably. Not done: a two-page browser run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account: no changes requested for this follow-up. The post-signing owed-marker guard addresses the delayed-join case, and the added regressions cover signing and lock-wait ordering. Reviewed with Rocket, Mantis and Groot at head ee28f50b9ffa8783fa15d158193a19bc8be0c2a7, base 11dd17bb7775a2932ca2466235354105edf2041b. Source-only: no tests/apps executed or CI assessed; native/cross-window behavior and both demo videos remain unverified.
kazemek21
left a comment
There was a problem hiding this comment.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
| const stored = readMarker(scope); | ||
| const owed = | ||
| stored === "pending" | ||
| ? viewRevision(scope, KEY) === revision && missing |
There was a problem hiding this comment.
[P2] Preserve explicit Leave during the initial directory read
A fresh claim can successfully add general, but if the member leaves while the first defaultChannels() request is pending, its post-wait membership filter returns general as missing. This branch then records it as owed and rejoins it, undoing Leave. Could explicit Leave exclude that channel from the admission's pending/owed work, including before the first snapshot? A deferred-directory-read regression would cover this ordering.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
Confirmed at ee28f50, and the window is wider than the initial read. While the marker is pending, the first snapshot owes every flagged channel the member isn't in at that moment. So a Leave made any time before the first successful snapshot gets undone:
- Same session, remount: the signed Leave revokes access, which cancels the in-flight directory read.
pendingstays, and the next mount or reconnect owes the channel and callsjoin. We reproduced this through the real session, store and fallback, with a stub lifecycle writer. - Across devices (source trace): on A, claim (
joined) and stop at the profile step. On B, finish joining (already_member) and leave #general. Then finish setup on A, or open its Messages later. The first run on A rejoins #general.
Moving the authorized filter ahead of the wait only fixes the first ordering. The fix belongs in the marker: once a member leaves a channel, keep it out of admission work whether the marker is pending or owed. For example, the Leave path could record the excluded id in the marker, or the marker could exclude channels left since the claim. A harness-level regression that fails at head:
it("preserves Leave while the first directory read is pending", async () => {
const reading = gate();
const answer = gate();
const h = harness({ joined: ["general"], ids: ["general"] });
h.defaultChannels.mockImplementationOnce(async () => {
reading.open();
await answer.opened;
return h.read();
});
const run = h.ensure();
await reading.opened;
h.leave("general");
answer.open();
await run;
expect(h.joins()).toEqual([]); // fails at head: ["general"]
});
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
Verdict: one recommended blocker. kylez's open P2 reproduces at ee28f50: an explicit Leave can be undone, and the window is wider than the first directory read. Evidence and a failing regression are in that thread. The relay contract matches block/buzz#8594 at 6133881: the narrow 9002 auto_join shape, the native and broker validators, and the bare 39000 readback. Owed-set settlement, role gating, first-channel selection and the joined-only marker also held up. One optional copy note is inline. Wes's earlier r4379494712 (a flagged archived or private channel can't be un-flagged from the UI) is still open.
Validation gaps
- No test leaves a channel before the first snapshot.
- There's no native build or live-relay run yet, as the PR notes.
- The per-channel monotonic
created_atholds only within one lifecycle owner, since windows and replacement sessions don't share it. We found no failure from that.
Exit criteria: a Leave made before the first snapshot stays left across a remount and on another device, with a regression test for it. Alternatively, Wes or the author explicitly accepts the behavior.
Reviewed ee28f50b9 against main at 511e1568b. Hosted CI is green. Court probes were focused tests only; we didn't rerun the broad suites.
| </dt> | ||
| <dd> | ||
| <span id={description}> | ||
| New community members are added to this channel automatically. |
There was a problem hiding this comment.
🤖 Optional (copy): the relay only adds members at an invite claim or a kind 9030 add (block/buzz#8594 side_effects.rs). docs/channels.md in this PR also says open communities get no auto-join. An owner of an open community can turn this on and see nothing happen. Consider wording like "Members who join with an invite are added to this channel automatically."
Summary
New members land in the community's default channel instead of an empty Messages view.
auto_jointag the relay advertises on39000channel metadata.auto_joinchannel, then the first non-Solo channel. An explicit empty route still bypasses both.auto_joinchannels during the invite claim.features/channels/default-channels.tscovers open or old relays and failed adds. Once the live roster is verified and empty, it sends one kind 9021 join per flagged public channel. It runs at most once per community, viewer and device, tracked by adefault-channelsview-state marker.9002withauto_join=true|falsethrough the existing lifecycle route. The UI confirms the result from the republished39000. The Rust validator and the development broker now accept that exact two-tag shape alongsidearchived.docs/channels.mdanddocs/communities.md.Validation
default-channels.test.ts), lifecycle commands, discovery parsing, store selection, theChannelsPagetab choice and the settings switch.Deferred
auto_joinpath needs a rebuilt binary.Related
Demo
Recorded against a local relay built from this branch with throwaway identities (Alice = owner, Bob = new member); no real accounts or hosted relays involved. Client at bb0d97d (browser dev build), relay at buzz 141aa01d4.
Owner turns on "Starter channel for new members" on two channels. Alice creates
#generaland#welcome, then flips the switch on each (~0:25 and ~0:41). The switch greys out while the kind 9002 is in flight and only turns on once the republished 39000 carries theauto_jointag.two-starter-channels-owner-flags.mp4
A brand-new member joins and lands in a starter channel. Bob claims Alice's invite with empty storage; from ~0:18 he is reading
#welcomewith both#generaland#welcomein the sidebar, and at 0:27 opens#generalas a member with no Join prompt. The relay logged oneauto-joined default channelline per flagged channel.two-starter-channels-new-member-lands.mp4
🤖 Generated with Claude Code