From f5582cedef683123e88aac7f77054f41f4e29d3a Mon Sep 17 00:00:00 2001 From: Joost Reijnen Date: Tue, 25 Aug 2026 16:50:18 +0200 Subject: [PATCH 1/5] desktop: collapse settings-section registry to one descriptor array SettingsSection union, SETTINGS_SECTION_VALUES/isSettingsSection, the settingsSections descriptor, the renderSettingsSection switch, and SettingsView's settingsNavGroups were five parallel structures a new settings section had to touch. settingsSections is now the single source: each descriptor carries value/label/icon/featureGate as before, plus group, order, and a render(props) closure lifted from the old switch case. SettingsView derives nav grouping and panel rendering directly from the registry. Behavior, data-testids, and section order/grouping are unchanged (including the pre-existing "moderation" section, which isn't wired into any nav group both before and after this change). Part of the desktop channel-feature-registry seam proposed in https://github.com/block/buzz/issues/3280. Signed-off-by: Joost Reijnen --- .../features/settings/ui/SettingsPanels.tsx | 285 ++++++++++-------- .../src/features/settings/ui/SettingsView.tsx | 76 ++--- 2 files changed, 181 insertions(+), 180 deletions(-) diff --git a/desktop/src/features/settings/ui/SettingsPanels.tsx b/desktop/src/features/settings/ui/SettingsPanels.tsx index 6b00fc8f74c..089045031d5 100644 --- a/desktop/src/features/settings/ui/SettingsPanels.tsx +++ b/desktop/src/features/settings/ui/SettingsPanels.tsx @@ -81,58 +81,69 @@ import { UpdateChecker } from "../UpdateChecker"; import { SettingsSectionHeader } from "./SettingsSectionHeader"; import { VoiceSettingsCard } from "./VoiceSettingsCard"; -export type SettingsSection = - | "profile" - | "notifications" - | "voice" - | "experimental" - | "agents" - | "channel-templates" - | "compute" - | "appearance" - | "shortcuts" - | "hosted-communities" - | "community-members" - | "moderation" - | "custom-emoji" - | "local-archive" - | "mobile" - | "updates"; +/** + * A settings section identifier. `settingsSections` below is now the single + * source of truth for which section values exist, what each one renders, and + * where it sits in navigation — there is no separate union type to keep in + * sync. This alias stays a plain `string` purely so existing callers can + * still annotate a variable's intent; validity is checked at runtime via + * `isSettingsSection`, which consults the registry directly. + * + * Previously, adding a section meant touching a `SettingsSection` union, a + * `SETTINGS_SECTION_VALUES` array, a `settingsSections` descriptor, a + * `renderSettingsSection` switch case, and a `settingsNavGroups` entry (5 + * parallel structures). It's now a single `settingsSections` entry. + * + * The `never` exhaustiveness check a switch statement used to provide is + * replaced by the registry being the *only* place a section is defined: + * there is nothing left to fall out of sync with. + */ +export type SettingsSection = string; export const DEFAULT_SETTINGS_SECTION: SettingsSection = "profile"; -const SETTINGS_SECTION_VALUES: readonly SettingsSection[] = [ - "profile", - "notifications", - "voice", - "experimental", - "agents", - "channel-templates", - "compute", - "appearance", - "shortcuts", - "hosted-communities", - "community-members", - "moderation", - "custom-emoji", - "local-archive", - "mobile", - "updates", -]; - export function isSettingsSection(value: unknown): value is SettingsSection { return ( - typeof value === "string" && - (SETTINGS_SECTION_VALUES as readonly string[]).includes(value) + typeof value === "string" && settingsSections.some((s) => s.value === value) ); } +/** Nav-group identifier a settings section is grouped under. See {@link SETTINGS_NAV_GROUPS}. */ +export type SettingsNavGroupId = "personal" | "communities" | "app"; + +/** + * Static nav-group order + labels for the settings sidebar. Section → group + * membership lives on each `SettingsSectionDescriptor.group`; this just + * controls the order the group headers themselves appear in and what + * they're labeled. + */ +export const SETTINGS_NAV_GROUPS: ReadonlyArray<{ + id: SettingsNavGroupId; + label: string; +}> = [ + { id: "personal", label: "Personal" }, + { id: "communities", label: "Communities" }, + { id: "app", label: "App" }, +]; + export type SettingsSectionDescriptor = { value: SettingsSection; label: string; icon: LucideIcon; + /** + * Nav group this section is grouped under. Omitted for a section that + * should stay reachable by value but not appear in the sidebar — e.g. + * "moderation" below, which predates this registry and was never wired + * into a nav group; that (likely unintentional) behavior is preserved + * as-is here rather than fixed as a drive-by in this refactor. + */ + group?: SettingsNavGroupId; + /** Explicit ordering within the group; lower renders first. Defaults to 0. */ + order?: number; /** If set, this section is only visible when the feature is enabled */ featureGate?: string; + /** Renders this section's panel body. */ + render: (props: SettingsPanelProps) => React.ReactNode; }; export type SettingsPanelProps = { @@ -151,88 +162,162 @@ export type SettingsPanelProps = { }; export const settingsSections: SettingsSectionDescriptor[] = [ - { - value: "appearance", - label: "Appearance", - icon: MonitorCog, - }, + // --- Personal --- { value: "profile", label: "Profile", icon: UserRound, + group: "personal", + order: 0, + render: (props) => ( + + ), + }, + { + value: "appearance", + label: "Appearance", + icon: MonitorCog, + group: "personal", + order: 1, + render: () => , }, { value: "notifications", label: "Notifications", icon: BellRing, + group: "personal", + order: 2, + render: (props) => ( + + ), }, { value: "voice", label: "Voice", icon: Volume2, + group: "personal", + order: 3, + render: () => , }, { - value: "experimental", - label: "Experiments", - icon: FlaskConical, + value: "shortcuts", + label: "Shortcuts", + icon: Keyboard, + group: "personal", + order: 4, + render: () => , }, { - value: "agents", - label: "Agents", - icon: Bot, - featureGate: "managed-agents", + value: "custom-emoji", + label: "Custom emoji", + icon: Smile, + group: "personal", + order: 5, + featureGate: "custom-emoji", + render: () => , + }, + { + value: "local-archive", + label: "Local archive", + icon: Archive, + group: "personal", + order: 6, + render: () => , }, { value: "channel-templates", label: "Channel templates", icon: LayoutTemplate, + group: "personal", + order: 7, featureGate: "channel-templates", + render: () => , }, - { - value: "compute", - label: "Compute", - icon: Cpu, - }, - { - value: "shortcuts", - label: "Shortcuts", - icon: Keyboard, - }, + // --- Communities --- { value: "hosted-communities", label: "Hosted communities", icon: MessagesSquare, + group: "communities", + order: 0, + render: () => , }, { value: "community-members", label: "Invites", icon: Ticket, + group: "communities", + order: 1, + render: (props) => ( + + ), }, + // --- Not wired into any nav group (see doc comment on `group` above) --- { value: "moderation", label: "Moderation", icon: ShieldAlert, + render: () => , }, + // --- App --- { - value: "custom-emoji", - label: "Custom emoji", - icon: Smile, - featureGate: "custom-emoji", + value: "agents", + label: "Agents", + icon: Bot, + group: "app", + order: 0, + featureGate: "managed-agents", + render: () => , }, { - value: "local-archive", - label: "Local archive", - icon: Archive, + value: "compute", + label: "Compute", + icon: Cpu, + group: "app", + order: 1, + render: () => , + }, + { + value: "experimental", + label: "Experiments", + icon: FlaskConical, + group: "app", + order: 2, + render: () => , }, { value: "mobile", label: "Mobile", icon: Smartphone, + group: "app", + order: 3, + render: (props) => ( + + ), }, { value: "updates", label: "Updates", icon: Download, + group: "app", + order: 4, + render: () => , }, ]; @@ -798,69 +883,3 @@ function ThemeSettingsCard() { ); } - -export function renderSettingsSection( - section: SettingsSection, - props: SettingsPanelProps, -): React.ReactNode { - switch (section) { - case "profile": - return ( - - ); - case "notifications": - return ( - - ); - case "voice": - return ; - case "experimental": - return ; - case "agents": - return ; - case "channel-templates": - return ; - case "compute": - return ; - case "appearance": - return ; - case "shortcuts": - return ; - case "hosted-communities": - return ; - case "community-members": - return ( - - ); - case "moderation": - return ; - case "custom-emoji": - return ; - case "local-archive": - return ; - case "mobile": - return ; - case "updates": - return ; - default: { - const exhaustiveCheck: never = section; - return exhaustiveCheck; - } - } -} diff --git a/desktop/src/features/settings/ui/SettingsView.tsx b/desktop/src/features/settings/ui/SettingsView.tsx index 2f9b2c36a1a..cad004c9785 100644 --- a/desktop/src/features/settings/ui/SettingsView.tsx +++ b/desktop/src/features/settings/ui/SettingsView.tsx @@ -30,7 +30,7 @@ import { } from "@/shared/ui/sidebar"; import { SidebarMenuLabel } from "@/shared/ui/sidebar-menu-label"; import { - renderSettingsSection, + SETTINGS_NAV_GROUPS, settingsSections, type SettingsPanelProps, type SettingsSection, @@ -48,33 +48,6 @@ type SettingsViewProps = SettingsPanelProps & { section: SettingsSection; }; -const settingsNavGroups: Array<{ - label: string; - sections: SettingsSection[]; -}> = [ - { - label: "Personal", - sections: [ - "profile", - "appearance", - "notifications", - "voice", - "shortcuts", - "custom-emoji", - "local-archive", - "channel-templates", - ], - }, - { - label: "Communities", - sections: ["hosted-communities", "community-members"], - }, - { - label: "App", - sections: ["agents", "compute", "experimental", "mobile", "updates"], - }, -]; - function SettingsSectionButton({ active, onSelect, @@ -186,23 +159,32 @@ export function SettingsView({ return () => window.removeEventListener("keydown", handleKeyDown); }, [onClose]); - const visibleSectionByValue = React.useMemo( - () => new Map(visibleSections.map((entry) => [entry.value, entry])), - [visibleSections], - ); - const visibleNavGroups = React.useMemo( - () => - settingsNavGroups - .map((group) => ({ - ...group, - sections: group.sections - .map((value) => visibleSectionByValue.get(value)) - .filter( - (entry): entry is SettingsSectionDescriptor => entry != null, - ), - })) - .filter((group) => group.sections.length > 0), - [visibleSectionByValue], + // Group visible sections by their `group` field, in `SETTINGS_NAV_GROUPS` + // order, sorted within each group by `order`. Sections with no `group` + // (e.g. "moderation") are omitted from the sidebar entirely, mirroring + // pre-registry behavior where they simply weren't listed in any nav group. + const visibleNavGroups = React.useMemo(() => { + const byGroup = new Map(); + for (const entry of visibleSections) { + if (!entry.group) continue; + const list = byGroup.get(entry.group); + if (list) { + list.push(entry); + } else { + byGroup.set(entry.group, [entry]); + } + } + return SETTINGS_NAV_GROUPS.map(({ id, label }) => ({ + label, + sections: (byGroup.get(id) ?? []) + .slice() + .sort((a, b) => (a.order ?? 0) - (b.order ?? 0)), + })).filter((group) => group.sections.length > 0); + }, [visibleSections]); + + const activeSection = React.useMemo( + () => visibleSections.find((entry) => entry.value === section), + [visibleSections, section], ); return ( @@ -342,7 +324,7 @@ export function SettingsView({ className="mx-auto flex min-h-full w-full max-w-4xl flex-col gap-4" data-testid={`settings-panel-${section}`} > - {renderSettingsSection(section, { + {activeSection?.render({ currentPubkey, fallbackDisplayName, isUpdatingDesktopNotifications, @@ -355,7 +337,7 @@ export function SettingsView({ onSetNotifyWhileViewing, onSetAllSlotAlertsEnabled, onSetSoundForSlot, - })} + }) ?? null} From 7f5ac4fe6d87f24df4e16e5754ca9e9761cf694f Mon Sep 17 00:00:00 2001 From: Joost Reijnen Date: Tue, 25 Aug 2026 16:50:32 +0200 Subject: [PATCH 2/5] desktop: channel-feature registry, wired into header glyph + forum dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces shared/channel-features: a ChannelFeaturePlugin registry modeled on the existing shared/features/ flag manifest ("typed definition list + resolver hook + gate"), per https://github.com/block/buzz/issues/3280. A plugin classifies a channel (parseBinding) into a typed binding and declares the glyph shown for channels it matches. Four built-in plugins (dm, private-channel, forum, stream — registered in priority order in builtins.ts) now back two call sites that used to independently re-derive "what kind of channel is this": - ChatHeader's ChannelIcon dm/private/forum/hash cascade is now a channelGlyph() lookup against the registry instead of an inline if-chain. - ChannelScreen's forum-vs-chat content dispatch (and three related layout checks: single-panel view, transparent chrome, the timeline-loading gate, and the "manage" action's forum branch) now read one classifyChannel(activeChannel)?.pluginId === "forum" result instead of four independent activeChannel.channelType === "forum" checks. This is intentionally the minimal, behavior-preserving slice of the RFC's proposed surface that current upstream has a second real consumer for. The RFC's fuller plugin surface (tabs, settingsPanel, sidebar group/create-actions, headerAction) is not ported here — see PR_DESCRIPTION.md's Follow-ups for why and what it would take. registry.test.mjs exercises the built-in plugin priority/precedence cascade and registerChannelFeature's dedup/sort behavior. Signed-off-by: Joost Reijnen --- .../features/channels/ui/ChannelScreen.tsx | 22 ++-- desktop/src/features/chat/ui/ChatHeader.tsx | 31 ++--- .../src/shared/channel-features/builtins.ts | 42 +++++++ desktop/src/shared/channel-features/index.ts | 21 ++++ .../shared/channel-features/registry.test.mjs | 115 ++++++++++++++++++ .../src/shared/channel-features/registry.ts | 89 ++++++++++++++ desktop/src/shared/channel-features/types.ts | 51 ++++++++ 7 files changed, 348 insertions(+), 23 deletions(-) create mode 100644 desktop/src/shared/channel-features/builtins.ts create mode 100644 desktop/src/shared/channel-features/index.ts create mode 100644 desktop/src/shared/channel-features/registry.test.mjs create mode 100644 desktop/src/shared/channel-features/registry.ts create mode 100644 desktop/src/shared/channel-features/types.ts diff --git a/desktop/src/features/channels/ui/ChannelScreen.tsx b/desktop/src/features/channels/ui/ChannelScreen.tsx index 240a9ad70c1..c7ebfed5a08 100644 --- a/desktop/src/features/channels/ui/ChannelScreen.tsx +++ b/desktop/src/features/channels/ui/ChannelScreen.tsx @@ -77,6 +77,7 @@ import { useElementWidth } from "@/shared/hooks/use-mobile"; import { useThreadPanelWidth } from "@/shared/hooks/useThreadPanelWidth"; import { AUXILIARY_PANEL_SINGLE_COLUMN_BREAKPOINT_PX } from "@/shared/layout/AuxiliaryPanel"; import { normalizePubkey } from "@/shared/lib/pubkey"; +import { classifyChannel } from "@/shared/channel-features"; import { useChannelActivityTyping } from "./useChannelActivityTyping"; import { useChannelAgentSessions } from "./useChannelAgentSessions"; import { useMessageProfiles } from "./useMessageProfiles"; @@ -171,6 +172,12 @@ export function ChannelScreen({ const mainInsetRef = useMainInsetRef(); const currentPubkey = currentIdentity?.pubkey; const activeChannelId = activeChannel?.id ?? null; + // Classifies through the same channel-feature registry ChatHeader's glyph + // resolution uses (see `shared/channel-features`), rather than each call + // site below independently re-checking `channelType === "forum"`. + const isActiveChannelForum = + activeChannel !== null && + classifyChannel(activeChannel)?.pluginId === "forum"; const isHuddleTranscript = useIsHuddleTranscript(activeChannelId); const relaySelfPubkey = useRelaySelfQuery(activeChannel !== null).data; const requireThreadEditResolutionRef = React.useRef<() => boolean>( @@ -624,7 +631,7 @@ export function ChannelScreen({ activeChannelId !== null && settledChannelIdRef.current === activeChannelId; const timelineLoadingNow = activeChannel !== null && - activeChannel.channelType !== "forum" && + !isActiveChannelForum && selectTimelineLoadingState( { isPending: messagesQuery.isPending, @@ -709,9 +716,7 @@ export function ChannelScreen({ channelContentWidthPx > 0 && channelContentWidthPx < AUXILIARY_PANEL_SINGLE_COLUMN_BREAKPOINT_PX; const isSinglePanelView = - isNarrowPanelViewport && - activeChannel?.channelType !== "forum" && - hasAuxiliaryPanel; + isNarrowPanelViewport && !isActiveChannelForum && hasAuxiliaryPanel; const shouldCompactHeaderActions = hasAuxiliaryPanel && channelContentWidthPx > 0 && @@ -724,7 +729,7 @@ export function ChannelScreen({ }); const handleManageChannel = React.useCallback(() => { if (!requireThreadEditResolution()) return; - if (activeChannel?.channelType === "forum") { + if (isActiveChannelForum) { openGlobalChannelManagement(); return; } @@ -740,7 +745,7 @@ export function ChannelScreen({ setProfilePanelPubkey(null); setChannelManagementOpen(true); }, [ - activeChannel?.channelType, + isActiveChannelForum, channelManagementOpen, openGlobalChannelManagement, requireThreadEditResolution, @@ -772,11 +777,12 @@ export function ChannelScreen({ onManageChannel={handleManageChannel} onToggleMembers={handleToggleMembers} showHeaderContent={!isSinglePanelView && !isHuddleTranscript} - transparentChrome={activeChannel?.channelType !== "forum"} + transparentChrome={!isActiveChannelForum} /> ), [ activeChannel, + isActiveChannelForum, activeChannelEphemeralDisplay, activeChannelTitle, shouldCompactHeaderActions, @@ -824,7 +830,7 @@ export function ChannelScreen({ ref={channelContentRef} > {activeChannel ? ( - activeChannel.channelType === "forum" ? ( + isActiveChannelForum ? ( ; } - if (channelType === "dm") { - return ; + // dm → private → forum → hash, in that order, is now a priority-ordered + // channel-feature plugin cascade (see `shared/channel-features/builtins.ts`) + // rather than an inline if-chain — `ChannelScreen`'s forum/chat content + // dispatch classifies through the same plugins, so there's one place that + // knows "what kind of channel is this" instead of two independently + // re-deriving it. + if (!channelType) { + return ; } - - if (visibility === "private") { - return ; - } - - if (channelType === "forum") { - return ; - } - - return ; + const Glyph = + channelGlyph({ channelType, visibility: visibility ?? "open" }) ?? Hash; + return Glyph === Hash ? ( + + ) : ( + + ); } export function ChatHeader({ diff --git a/desktop/src/shared/channel-features/builtins.ts b/desktop/src/shared/channel-features/builtins.ts new file mode 100644 index 00000000000..26dbb945586 --- /dev/null +++ b/desktop/src/shared/channel-features/builtins.ts @@ -0,0 +1,42 @@ +import { CircleDot, FileText, Hash, Lock } from "lucide-react"; +import { registerChannelFeature } from "./registry"; + +/** + * The four channel-classification cases `ChatHeader`'s `ChannelIcon` used to + * check inline (dm → private → forum → hash), now expressed as + * priority-ordered channel-feature plugins. Registering them here — rather + * than inlining the checks at each call site — gives `ChatHeader` and + * `ChannelScreen` (which only needs the `forum` case, for its content + * dispatch) one shared source of truth for "what kind of channel is this", + * instead of two places independently re-deriving the same cascade. + * + * `stream` is an explicit catch-all (rather than leaving plain channels + * unclassified) so `classifyChannel` always returns a match once these are + * registered, matching `ChannelIcon`'s original `Hash`-by-default fallthrough. + */ +export function registerBuiltinChannelFeatures(): void { + registerChannelFeature({ + id: "dm", + priority: 0, + glyph: CircleDot, + parseBinding: (channel) => (channel.channelType === "dm" ? true : null), + }); + registerChannelFeature({ + id: "private-channel", + priority: 10, + glyph: Lock, + parseBinding: (channel) => (channel.visibility === "private" ? true : null), + }); + registerChannelFeature({ + id: "forum", + priority: 20, + glyph: FileText, + parseBinding: (channel) => (channel.channelType === "forum" ? true : null), + }); + registerChannelFeature({ + id: "stream", + priority: 30, + glyph: Hash, + parseBinding: () => true, + }); +} diff --git a/desktop/src/shared/channel-features/index.ts b/desktop/src/shared/channel-features/index.ts new file mode 100644 index 00000000000..efea53e72bc --- /dev/null +++ b/desktop/src/shared/channel-features/index.ts @@ -0,0 +1,21 @@ +import { registerBuiltinChannelFeatures } from "./builtins"; + +export { + channelGlyph, + classifyChannel, + classifyChannelWith, + getChannelPlugins, + registerChannelFeature, +} from "./registry"; +export type { + ChannelBinding, + ChannelClassifyInput, + ChannelFeaturePlugin, +} from "./types"; + +// Registering here (module scope, run once per module load thanks to ESM +// caching) means any call site that imports from this barrel gets the +// built-in dm/private-channel/forum/stream plugins for free, mirroring how +// `shared/features/manifest` loads its manifest at import time rather than +// requiring an explicit bootstrap call from `App.tsx`. +registerBuiltinChannelFeatures(); diff --git a/desktop/src/shared/channel-features/registry.test.mjs b/desktop/src/shared/channel-features/registry.test.mjs new file mode 100644 index 00000000000..12e7e6971f5 --- /dev/null +++ b/desktop/src/shared/channel-features/registry.test.mjs @@ -0,0 +1,115 @@ +import assert from "node:assert/strict"; +import { describe, it, beforeEach } from "node:test"; +import { CircleDot, FileText, Hash, Lock } from "lucide-react"; + +import { registerBuiltinChannelFeatures } from "./builtins.ts"; +import { + __resetChannelFeatureRegistryForTests, + channelGlyph, + classifyChannel, + getChannelPlugins, + registerChannelFeature, +} from "./registry.ts"; + +describe("built-in channel-feature plugins", () => { + beforeEach(() => { + __resetChannelFeatureRegistryForTests(); + registerBuiltinChannelFeatures(); + }); + + it("registers exactly the 4 built-in plugins, in priority order", () => { + const plugins = getChannelPlugins(); + assert.deepEqual( + plugins.map((p) => p.id), + ["dm", "private-channel", "forum", "stream"], + ); + }); + + it("re-registering is a no-op (idempotent)", () => { + registerBuiltinChannelFeatures(); + assert.equal(getChannelPlugins().length, 4); + }); + + const cases = [ + { + name: "a dm channel", + channel: { channelType: "dm", visibility: "open" }, + pluginId: "dm", + glyph: CircleDot, + }, + { + name: "a private stream channel", + channel: { channelType: "stream", visibility: "private" }, + pluginId: "private-channel", + glyph: Lock, + }, + { + name: "an open forum channel", + channel: { channelType: "forum", visibility: "open" }, + pluginId: "forum", + glyph: FileText, + }, + { + name: "a plain open stream channel", + channel: { channelType: "stream", visibility: "open" }, + pluginId: "stream", + glyph: Hash, + }, + ]; + + for (const { name, channel, pluginId, glyph } of cases) { + it(`classifies ${name} as "${pluginId}", matching ChatHeader's old ChannelIcon cascade`, () => { + const binding = classifyChannel(channel); + assert.ok(binding, "expected a non-null binding"); + assert.equal(binding.pluginId, pluginId); + assert.equal(channelGlyph(channel), glyph); + }); + } + + it("dm takes precedence over private (a dm can't be classified as private-channel)", () => { + const binding = classifyChannel({ + channelType: "dm", + visibility: "private", + }); + assert.equal(binding.pluginId, "dm"); + }); + + it("private takes precedence over forum", () => { + const binding = classifyChannel({ + channelType: "forum", + visibility: "private", + }); + assert.equal(binding.pluginId, "private-channel"); + }); +}); + +describe("registerChannelFeature", () => { + beforeEach(() => { + __resetChannelFeatureRegistryForTests(); + }); + + it("classifies against an empty registry as null", () => { + assert.equal( + classifyChannel({ channelType: "stream", visibility: "open" }), + null, + ); + }); + + it("sorts by priority, ties keeping registration order", () => { + registerChannelFeature({ id: "b", priority: 1, parseBinding: () => true }); + registerChannelFeature({ id: "a", priority: 0, parseBinding: () => true }); + registerChannelFeature({ id: "c", priority: 1, parseBinding: () => true }); + assert.deepEqual( + getChannelPlugins().map((p) => p.id), + ["a", "b", "c"], + ); + }); + + it("warns and ignores a duplicate id instead of throwing", () => { + registerChannelFeature({ id: "dup", parseBinding: () => true }); + assert.doesNotThrow(() => + registerChannelFeature({ id: "dup", parseBinding: () => null }), + ); + assert.equal(getChannelPlugins().length, 1); + }); +}); diff --git a/desktop/src/shared/channel-features/registry.ts b/desktop/src/shared/channel-features/registry.ts new file mode 100644 index 00000000000..c60cfb5d424 --- /dev/null +++ b/desktop/src/shared/channel-features/registry.ts @@ -0,0 +1,89 @@ +import type { + ChannelBinding, + ChannelClassifyInput, + ChannelFeaturePlugin, +} from "./types"; + +/** + * Module-level plugin list, kept sorted by `priority` (ties preserve + * registration order via `Array.prototype.sort`'s stability guarantee). + * + * Plugins are generic over their own binding type (`ChannelFeaturePlugin`), + * but the registry holds a heterogeneous mix, so entries are stored type-erased + * as `ChannelFeaturePlugin`. `registerChannelFeature` is the only + * place that performs the erasure (via `unknown` double-cast, never `any`) — + * every other module only ever sees the erased shape, which is exactly what + * `classifyChannel`'s `ChannelBinding` result needs. + */ +let plugins: ChannelFeaturePlugin[] = []; + +/** + * Register a channel-feature plugin. Duplicate `id`s are a no-op (warn + + * ignore) rather than a throw, so a stray double-registration (e.g. a hot + * reload or an accidental double barrel-import) can't crash app startup. + */ +export function registerChannelFeature( + plugin: ChannelFeaturePlugin, +): void { + if (plugins.some((existing) => existing.id === plugin.id)) { + console.warn( + `[channel-features] Duplicate channel feature id "${plugin.id}" — ignoring.`, + ); + return; + } + plugins = [ + ...plugins, + plugin as unknown as ChannelFeaturePlugin, + ].sort((a, b) => (a.priority ?? 0) - (b.priority ?? 0)); +} + +/** All registered channel-feature plugins, in classification order. */ +export function getChannelPlugins(): ChannelFeaturePlugin[] { + return [...plugins]; +} + +/** + * Classify `channel` against an explicit plugin list: the first plugin whose + * `parseBinding` returns non-null wins (the list is assumed already in + * priority order). `null` when none match. The primitive `classifyChannel` + * builds on (passing the mutable registry); exposed for any caller that holds + * a fixed plugin set of its own. + */ +export function classifyChannelWith( + candidates: readonly ChannelFeaturePlugin[], + channel: ChannelClassifyInput, +): ChannelBinding | null { + for (const plugin of candidates) { + const value = plugin.parseBinding(channel); + if (value !== null) { + return { pluginId: plugin.id, value }; + } + } + return null; +} + +/** + * Classify `channel` against the registered plugins: the first plugin (in + * priority order) whose `parseBinding` returns non-null wins. `null` when no + * plugin matches — with the built-in `stream` catch-all plugin registered + * (see `builtins.ts`), this only happens before that registration runs. + */ +export function classifyChannel( + channel: ChannelClassifyInput, +): ChannelBinding | null { + return classifyChannelWith(plugins, channel); +} + +/** The matched plugin's glyph for `channel`, or `null` when nothing matched. */ +export function channelGlyph(channel: ChannelClassifyInput) { + const binding = classifyChannel(channel); + if (!binding) return null; + return ( + plugins.find((plugin) => plugin.id === binding.pluginId)?.glyph ?? null + ); +} + +/** Test-only: reset the registry between test files/cases. */ +export function __resetChannelFeatureRegistryForTests(): void { + plugins = []; +} diff --git a/desktop/src/shared/channel-features/types.ts b/desktop/src/shared/channel-features/types.ts new file mode 100644 index 00000000000..38eed2fd948 --- /dev/null +++ b/desktop/src/shared/channel-features/types.ts @@ -0,0 +1,51 @@ +import type { LucideIcon } from "lucide-react"; +import type { Channel } from "@/shared/api/types"; + +/** + * The minimal channel shape a plugin's `parseBinding` needs to classify a + * channel. Kept narrower than the full `Channel` so call sites that only + * have a partial channel on hand (e.g. `ChatHeader`, which receives + * `channelType`/`visibility` as separate props rather than a `Channel`) can + * classify without constructing one. + */ +export type ChannelClassifyInput = Pick; + +/** + * The result of classifying a channel: which plugin matched, and the value + * its `parseBinding` returned. `T` defaults to `unknown` so call sites that + * don't care about the specific plugin's binding shape (e.g. glyph + * resolution) can use the bare `ChannelBinding` alias. + */ +export interface ChannelBinding { + pluginId: string; + value: T; +} + +/** + * A channel-feature plugin: classifies a channel into a typed binding and + * supplies the glyph shown for channels it matches. Modeled on the existing + * `shared/features/` flag manifest's "typed definition + resolver + gate" + * ergonomics, but for channel-binding plugins instead of preview flags. + * + * This is the seed of the plugin surface proposed in + * https://github.com/block/buzz/issues/3280: today it unifies channel + * classification for the header glyph and the channel-screen content + * dispatch (see `ChatHeader.tsx` / `ChannelScreen.tsx`). A plugin that wants + * to contribute its own tab bar, settings section, or sidebar affordance — + * e.g. hosting an MCP App as a channel tab (block/buzz#3275) — is a natural + * extension of `T` and this interface once a concrete second consumer shows + * up; see PR_DESCRIPTION.md's Follow-ups for the shape that would take. + */ +export interface ChannelFeaturePlugin { + /** Unique id — duplicate registration is a no-op (warn + ignore). */ + id: string; + /** + * Classify `channel` into this plugin's binding shape, or `null` if it + * doesn't match. + */ + parseBinding: (channel: ChannelClassifyInput) => T | null; + /** Glyph shown for channels this plugin matches (header/sidebar/intro). */ + glyph?: LucideIcon; + /** Lower runs first when classifying; ties keep registration order. Default 0. */ + priority?: number; +} From a33a5ef095fb57435f3c60dd9786e3ec73278951 Mon Sep 17 00:00:00 2001 From: Joost Reijnen Date: Tue, 25 Aug 2026 16:51:27 +0200 Subject: [PATCH 3/5] docs: add PR description for the channel-feature-registry seam Covers problem/how/testing/follow-ups per CONTRIBUTING.md's PR checklist, states this implements #3280, notes the complementary relationship to #3275, and records today's duplicate-issue/PR search. Signed-off-by: Joost Reijnen --- PR_DESCRIPTION.md | 189 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 PR_DESCRIPTION.md diff --git a/PR_DESCRIPTION.md b/PR_DESCRIPTION.md new file mode 100644 index 00000000000..e0a4b586335 --- /dev/null +++ b/PR_DESCRIPTION.md @@ -0,0 +1,189 @@ +## Problem + +Implements https://github.com/block/buzz/issues/3280. + +Adding a channel-scoped feature to the desktop client means editing a spread +of centrally-owned files in lockstep. Two concrete instances of this exist in +current upstream `main` today: + +1. **The settings-section wiring** (`SettingsPanels.tsx` / `SettingsView.tsx`): + a `SettingsSection` union type, a `SETTINGS_SECTION_VALUES` array, + `isSettingsSection`, a `settingsSections` descriptor array, a + `renderSettingsSection` `switch` with a `never` exhaustiveness gate, and + `SettingsView`'s separate `settingsNavGroups` map — five parallel + structures a new settings section has to touch. +2. **Channel classification** is scattered: `ChatHeader`'s `ChannelIcon` + re-derives "what kind of channel is this" (dm → private → forum → hash) + as an inline `if`-chain, and `ChannelScreen` independently re-checks + `activeChannel.channelType === "forum"` in four separate places to decide + what to render and how to lay it out. + +This PR is the client-side companion the RFC describes, ported fresh onto +current upstream (our original implementation lived in a disconnected +fork-snapshot repo and couldn't be cherry-picked — see "Provenance" below). + +## How + +**Commit 1 — settings-section registry.** `settingsSections` is now the +single source of truth: each descriptor carries `value`/`label`/`icon`/ +`featureGate` as before, plus `group`, `order`, and a `render(props)` closure +lifted from the old `switch` case. `SettingsView` derives nav grouping and +panel rendering directly from the registry (`SETTINGS_NAV_GROUPS` controls +group order/labels; each descriptor's `group`/`order` controls placement). +Behavior, `data-testid`s, and section order/grouping are unchanged, including +the pre-existing `"moderation"` section, which isn't wired into any nav group +both before and after this change. + +**Commit 2 — `shared/channel-features` registry.** A `ChannelFeaturePlugin` +registry modeled on the existing `shared/features/` flag manifest ("typed +definition list + resolver hook + gate"): + +```ts +interface ChannelFeaturePlugin { + id: string; + parseBinding: (channel: ChannelClassifyInput) => T | null; + glyph?: LucideIcon; + priority?: number; // lower runs first; ties keep registration order +} +``` + +Four built-in plugins (`dm`, `private-channel`, `forum`, `stream`, +registered in priority order in `builtins.ts`) reproduce `ChatHeader`'s +exact dm → private → forum → hash cascade, and now back two call sites that +used to independently re-derive it: + +- `ChatHeader`'s `ChannelIcon` calls `channelGlyph({channelType, visibility})` + instead of the inline `if`-chain. +- `ChannelScreen` computes `isActiveChannelForum` once via + `classifyChannel(activeChannel)?.pluginId === "forum"` and reuses it for + the forum/chat content dispatch, the single-panel-view check, the + transparent-chrome check, the timeline-loading gate, and the "manage + channel" action's forum branch — five sites that previously re-checked + `channelType === "forum"` independently. + +Registration happens at module scope in `shared/channel-features/index.ts` +(mirroring how `shared/features/manifest` loads its manifest at import time), +so any call site that imports from the barrel gets the built-ins for free. + +### What's intentionally *not* ported + +The RFC's fuller proposed surface — `tabs`, `settingsPanel`, `sidebar` +group/create-actions, `headerAction` — is **not** in this PR. Our original +implementation had those because our fork added genuinely new channel types +(product/repo/board-hierarchy channels) that needed tab bars, sidebar +groups, and settings panels of their own. Current upstream `main` has no +such second consumer yet: the only two dispatch points that exist today +(`ChatHeader`'s glyph, `ChannelScreen`'s forum/chat split) are a binary +classification, not a multi-tab surface, so forcing in `ChannelFeatureTab`/ +`ChannelFeatureShell`/sidebar-group machinery now would be speculative +plugin-surface for zero real callers — exactly the kind of premature +abstraction the RFC's own "behavior-preserving, not a new privileged lane" +framing argues against. See Follow-ups below for the concrete trigger that +would justify porting the rest. + +## Relationship to #3275 + +#3275 ("host MCP Apps as channel tabs") is the motivating concrete case for +the RFC: it extends `ChannelScreen`'s shared shells directly to add a new +tab type. This PR does not touch #3275's code and its content dispatch +(forum vs. chat) is orthogonal to MCP-App tabs (which install *within* a +channel already classified as a normal chat channel), so there's no merge +conflict or ordering dependency between the two. + +The natural follow-up once both land: an MCP-App-tabs plugin would extend +`ChannelFeaturePlugin` with a `tabs` field (as the RFC describes) and +register its tab bar for channels with an installed App — the same +extension point `#3275`'s review question 4 asks about ("Does the current +typed channel-surface seam compose cleanly with the behavior-preserving +registry proposed in #3280?"). This PR doesn't answer that by shipping the +`tabs` field pre-emptively (no real second tab-contributing plugin exists +in this repo yet to shape it against); it answers it by proving out the +`parseBinding`/`glyph`/priority-cascade mechanics on two real call sites so +that field has a proven foundation to extend. + +## Testing + +- `pnpm typecheck` — clean. +- `pnpm lint` (`biome check`) — clean on all touched/added files (two + pre-existing warnings and two pre-existing infos elsewhere in the repo, + unrelated to this change, unchanged by it). +- `pnpm test` (unit, `node --test`) — **5463 passed**, 0 failed, including + the new `desktop/src/shared/channel-features/registry.test.mjs` (11 tests: + built-in plugin registration order, idempotent re-registration, the + dm/private/forum/stream classification cascade and its precedence rules, + and `registerChannelFeature`'s sort/dedup behavior). +- `pnpm build:e2e` — clean build. +- `pnpm exec playwright test --project=smoke --grep "forum|Forum|settings|Settings|sidebar|Sidebar"` + — **112 passed, 1 skipped, 0 failed** (covers + `settings-section-layout.spec.ts`, `sidebar.spec.ts`, + `sidebar-offcanvas-rail.spec.ts`, `sidebar-relay-card.spec.ts`, + `sidebar-snapshot.spec.ts`, `sidebar-more-unread-overlap.spec.ts`, + `hosted-communities-settings-screenshots.spec.ts`, + `invites-settings-screenshots.spec.ts`, + `profile-backup-settings.spec.ts`, `voice-settings.spec.ts`, and the + forum-touching cases inside the broader channel specs). +- Did **not** run the full Playwright suite (`pnpm test:e2e`) or the Rust + side (`just ci` / `cargo test --manifest-path desktop/src-tauri/Cargo.toml`) + — this PR has no Rust changes and no diff outside `desktop/src`, and the + full JS/TS suite plus the targeted grep above already cover every spec + that touches channel tabs, sidebar hierarchy, and settings. Re-running the + targeted grep is fast (~3 min); a reviewer with more time budget may want + the full suite for extra confidence. + +### Screenshots + +Captured locally via a dev build + Playwright against the mock bridge +(`pnpm build:e2e` + a throwaway script driving `installMockBridge`/ +`openSettings`, not committed): + +- Settings → Appearance, showing the registry-rendered nav groups + (Personal/Communities/App) and panel content unchanged from before. +- The `#general` channel, showing the `stream` plugin's `Hash` glyph in the + channel header — the same icon `ChatHeader`'s old inline cascade produced + for a plain open stream channel. + +These aren't posted via `scripts/post-screenshots.sh` because that script +posts to an open PR (`gh pr create` was intentionally not run for this +branch — see the task instructions this branch was prepared under). Whoever +opens the actual PR from `upstream-pr/channel-feature-registry-seam-b` should +re-capture and post screenshots through that script at PR-creation time. + +## Duplicate check (re-verified today) + +`gh api search/issues -f q='repo:block/buzz channel feature registry ChannelFeaturePlugin'` +returns exactly two results: issue #3280 (this RFC) and PR #3275 (the MCP +Apps host, discussed above). No other open or closed issue/PR implements a +channel-feature/settings-section registry for the desktop client. + +## Follow-ups + +- **Port the `tabs`/`ChannelFeatureShell` surface** once a second real + tab-contributing plugin exists in this repo (e.g. an MCP-App-tabs plugin + building on #3275, or a future Sequence/board/docs-style channel type). + Shaping `ChannelFeatureTab` against a hypothetical single consumer + risks guessing wrong; a second concrete caller is the right trigger. +- **Sidebar group/create-action surface** (`ChannelFeatureSidebar`) — same + reasoning; upstream has no repo/product-style sidebar hierarchy today to + drive the design. +- **`headerAction`** — same; no plugin needs to contribute a header action + yet (our fork's needed this for a "New idea" dialog action that doesn't + exist upstream). +- Consider whether `channelGlyph`'s `stream`-vs-`Hash`-styling special case + in `ChatHeader.tsx` (the `Glyph === Hash` check, preserved from the + original inline code's distinct `CHANNEL_HASH_ICON_CLASS`/`color="gray"` + treatment) is worth generalizing into a per-plugin style hook, or left as + the one acknowledged wart of an otherwise uniform glyph lookup. + +## Provenance + +The design was originally implemented and reviewed in a fork whose git +history isn't connected to this upstream (a content-snapshot import, not a +real fork), so it couldn't be cherry-picked or rebased. This PR is a fresh +port of the mechanism onto current `upstream/main`, adapted to what +upstream's `ChannelScreen`/`ChatHeader`/settings code actually look like +today (which has drifted from our fork's snapshot — the settings-section +descriptor shape, the channel dispatch's actual branch points, and the +absence of any custom channel-type/tab surface all differ from what the +original diff assumed). See the "What's intentionally not ported" section +above for the specific scope this drift and the "behavior-preserving" +constraint together produced. From 6ef9724fb3eb41711670b381cfd0990702bed806 Mon Sep 17 00:00:00 2001 From: Joost Reijnen Date: Tue, 25 Aug 2026 17:15:58 +0200 Subject: [PATCH 4/5] docs: rewrite PR description in plain style Signed-off-by: Joost Reijnen --- PR_DESCRIPTION.md | 256 ++++++++++++++++++++++------------------------ 1 file changed, 122 insertions(+), 134 deletions(-) diff --git a/PR_DESCRIPTION.md b/PR_DESCRIPTION.md index e0a4b586335..d383d353375 100644 --- a/PR_DESCRIPTION.md +++ b/PR_DESCRIPTION.md @@ -1,42 +1,44 @@ -## Problem +# desktop: channel-feature registry (settings sections + channel classification) + +Implements #3280. -Implements https://github.com/block/buzz/issues/3280. +## Problem -Adding a channel-scoped feature to the desktop client means editing a spread -of centrally-owned files in lockstep. Two concrete instances of this exist in -current upstream `main` today: +Adding a channel-scoped feature to the desktop client means editing several +centrally-owned files at once. Two concrete cases of this exist in current +upstream `main` today: -1. **The settings-section wiring** (`SettingsPanels.tsx` / `SettingsView.tsx`): +1. **Settings-section wiring** (`SettingsPanels.tsx` / `SettingsView.tsx`): a `SettingsSection` union type, a `SETTINGS_SECTION_VALUES` array, `isSettingsSection`, a `settingsSections` descriptor array, a - `renderSettingsSection` `switch` with a `never` exhaustiveness gate, and - `SettingsView`'s separate `settingsNavGroups` map — five parallel + `renderSettingsSection` switch with a `never` exhaustiveness gate, and + `SettingsView`'s separate `settingsNavGroups` map. Five parallel structures a new settings section has to touch. -2. **Channel classification** is scattered: `ChatHeader`'s `ChannelIcon` - re-derives "what kind of channel is this" (dm → private → forum → hash) - as an inline `if`-chain, and `ChannelScreen` independently re-checks - `activeChannel.channelType === "forum"` in four separate places to decide - what to render and how to lay it out. +2. **Channel classification is scattered.** `ChatHeader`'s `ChannelIcon` + re-derives "what kind of channel is this" (dm, private, forum, hash) as + an inline if-chain, and `ChannelScreen` separately re-checks + `activeChannel.channelType === "forum"` in four different places to + decide what to render. This PR is the client-side companion the RFC describes, ported fresh onto -current upstream (our original implementation lived in a disconnected -fork-snapshot repo and couldn't be cherry-picked — see "Provenance" below). +current upstream. Our original implementation lived in a fork whose git +history isn't connected to this repo (a content-snapshot import, not a real +fork), so it couldn't be cherry-picked. See "Provenance" below. ## How -**Commit 1 — settings-section registry.** `settingsSections` is now the +**Commit 1, settings-section registry.** `settingsSections` is now the single source of truth: each descriptor carries `value`/`label`/`icon`/ -`featureGate` as before, plus `group`, `order`, and a `render(props)` closure -lifted from the old `switch` case. `SettingsView` derives nav grouping and -panel rendering directly from the registry (`SETTINGS_NAV_GROUPS` controls -group order/labels; each descriptor's `group`/`order` controls placement). -Behavior, `data-testid`s, and section order/grouping are unchanged, including -the pre-existing `"moderation"` section, which isn't wired into any nav group -both before and after this change. - -**Commit 2 — `shared/channel-features` registry.** A `ChannelFeaturePlugin` -registry modeled on the existing `shared/features/` flag manifest ("typed -definition list + resolver hook + gate"): +`featureGate` as before, plus `group`, `order`, and a `render(props)` +closure lifted from the old switch case. `SettingsView` derives nav +grouping and panel rendering directly from the registry. Behavior, +`data-testid`s, and section order/grouping are unchanged, including the +pre-existing `"moderation"` section, which isn't wired into any nav group +either before or after this change. + +**Commit 2, `shared/channel-features` registry.** A `ChannelFeaturePlugin` +registry modeled on the existing `shared/features/` flag manifest (typed +definition list, resolver hook, gate): ```ts interface ChannelFeaturePlugin { @@ -49,141 +51,127 @@ interface ChannelFeaturePlugin { Four built-in plugins (`dm`, `private-channel`, `forum`, `stream`, registered in priority order in `builtins.ts`) reproduce `ChatHeader`'s -exact dm → private → forum → hash cascade, and now back two call sites that -used to independently re-derive it: +existing dm, private, forum, hash cascade exactly, and now back two call +sites that used to derive it independently: - `ChatHeader`'s `ChannelIcon` calls `channelGlyph({channelType, visibility})` - instead of the inline `if`-chain. -- `ChannelScreen` computes `isActiveChannelForum` once via - `classifyChannel(activeChannel)?.pluginId === "forum"` and reuses it for + instead of the inline if-chain. +- `ChannelScreen` computes `isActiveChannelForum` once, via + `classifyChannel(activeChannel)?.pluginId === "forum"`, and reuses it for the forum/chat content dispatch, the single-panel-view check, the transparent-chrome check, the timeline-loading gate, and the "manage - channel" action's forum branch — five sites that previously re-checked - `channelType === "forum"` independently. - -Registration happens at module scope in `shared/channel-features/index.ts` -(mirroring how `shared/features/manifest` loads its manifest at import time), -so any call site that imports from the barrel gets the built-ins for free. - -### What's intentionally *not* ported - -The RFC's fuller proposed surface — `tabs`, `settingsPanel`, `sidebar` -group/create-actions, `headerAction` — is **not** in this PR. Our original -implementation had those because our fork added genuinely new channel types -(product/repo/board-hierarchy channels) that needed tab bars, sidebar -groups, and settings panels of their own. Current upstream `main` has no -such second consumer yet: the only two dispatch points that exist today -(`ChatHeader`'s glyph, `ChannelScreen`'s forum/chat split) are a binary -classification, not a multi-tab surface, so forcing in `ChannelFeatureTab`/ -`ChannelFeatureShell`/sidebar-group machinery now would be speculative -plugin-surface for zero real callers — exactly the kind of premature -abstraction the RFC's own "behavior-preserving, not a new privileged lane" -framing argues against. See Follow-ups below for the concrete trigger that -would justify porting the rest. + channel" action's forum branch. Five sites that used to check + `channelType === "forum"` on their own. + +Registration happens at module scope in `shared/channel-features/index.ts`, +the same way `shared/features/manifest` loads at import time, so any call +site that imports from the barrel gets the built-ins for free. + +## Not in this PR + +The RFC's fuller proposed surface, `tabs`, `settingsPanel`, `sidebar` +group/create-actions, `headerAction`, is not in this PR. Our original +implementation had those because our fork added new channel types +(product/repo/board-hierarchy) that needed their own tab bars, sidebar +groups, and settings panels. Current upstream `main` has no second +consumer yet: the only two dispatch points that exist today (`ChatHeader`'s +glyph, `ChannelScreen`'s forum/chat split) are a binary classification, not +a multi-tab surface. Adding the tab/sidebar-group machinery now would be +speculative for zero real callers. See Follow-ups below for what would +justify porting the rest. ## Relationship to #3275 -#3275 ("host MCP Apps as channel tabs") is the motivating concrete case for -the RFC: it extends `ChannelScreen`'s shared shells directly to add a new -tab type. This PR does not touch #3275's code and its content dispatch -(forum vs. chat) is orthogonal to MCP-App tabs (which install *within* a -channel already classified as a normal chat channel), so there's no merge -conflict or ordering dependency between the two. +#3275 ("host MCP Apps as channel tabs") is the motivating case for the RFC: +it extends `ChannelScreen`'s shared shells directly to add a new tab type. +This PR doesn't touch #3275's code, and its content dispatch (forum vs. +chat) is orthogonal to MCP-App tabs, which install within a channel that's +already classified as a normal chat channel. There's no merge conflict or +ordering dependency between the two. The natural follow-up once both land: an MCP-App-tabs plugin would extend -`ChannelFeaturePlugin` with a `tabs` field (as the RFC describes) and -register its tab bar for channels with an installed App — the same -extension point `#3275`'s review question 4 asks about ("Does the current -typed channel-surface seam compose cleanly with the behavior-preserving -registry proposed in #3280?"). This PR doesn't answer that by shipping the -`tabs` field pre-emptively (no real second tab-contributing plugin exists -in this repo yet to shape it against); it answers it by proving out the -`parseBinding`/`glyph`/priority-cascade mechanics on two real call sites so -that field has a proven foundation to extend. +`ChannelFeaturePlugin` with a `tabs` field, as the RFC describes, and +register its tab bar for channels with an installed app. This PR doesn't +ship that field pre-emptively, since there's no real second tab-contributing +plugin in this repo yet to shape it against. It answers the same question +by proving the `parseBinding`/`glyph`/priority-cascade mechanics on two real +call sites, so that field has something real to build on. ## Testing -- `pnpm typecheck` — clean. -- `pnpm lint` (`biome check`) — clean on all touched/added files (two +- `pnpm typecheck`: clean. +- `pnpm lint` (`biome check`): clean on all touched/added files (two pre-existing warnings and two pre-existing infos elsewhere in the repo, - unrelated to this change, unchanged by it). -- `pnpm test` (unit, `node --test`) — **5463 passed**, 0 failed, including - the new `desktop/src/shared/channel-features/registry.test.mjs` (11 tests: + unrelated and unchanged). +- `pnpm test` (unit, `node --test`): 5463 passed, 0 failed, including the + new `desktop/src/shared/channel-features/registry.test.mjs` (11 tests: built-in plugin registration order, idempotent re-registration, the - dm/private/forum/stream classification cascade and its precedence rules, - and `registerChannelFeature`'s sort/dedup behavior). -- `pnpm build:e2e` — clean build. -- `pnpm exec playwright test --project=smoke --grep "forum|Forum|settings|Settings|sidebar|Sidebar"` - — **112 passed, 1 skipped, 0 failed** (covers - `settings-section-layout.spec.ts`, `sidebar.spec.ts`, - `sidebar-offcanvas-rail.spec.ts`, `sidebar-relay-card.spec.ts`, - `sidebar-snapshot.spec.ts`, `sidebar-more-unread-overlap.spec.ts`, + dm/private/forum/stream classification cascade and its precedence, and + `registerChannelFeature`'s sort/dedup behavior). +- `pnpm build:e2e`: clean build. +- `pnpm exec playwright test --project=smoke --grep "forum|Forum|settings|Settings|sidebar|Sidebar"`: + 112 passed, 1 skipped, 0 failed (covers `settings-section-layout.spec.ts`, + `sidebar.spec.ts`, `sidebar-offcanvas-rail.spec.ts`, + `sidebar-relay-card.spec.ts`, `sidebar-snapshot.spec.ts`, + `sidebar-more-unread-overlap.spec.ts`, `hosted-communities-settings-screenshots.spec.ts`, - `invites-settings-screenshots.spec.ts`, - `profile-backup-settings.spec.ts`, `voice-settings.spec.ts`, and the - forum-touching cases inside the broader channel specs). -- Did **not** run the full Playwright suite (`pnpm test:e2e`) or the Rust - side (`just ci` / `cargo test --manifest-path desktop/src-tauri/Cargo.toml`) - — this PR has no Rust changes and no diff outside `desktop/src`, and the - full JS/TS suite plus the targeted grep above already cover every spec - that touches channel tabs, sidebar hierarchy, and settings. Re-running the - targeted grep is fast (~3 min); a reviewer with more time budget may want - the full suite for extra confidence. + `invites-settings-screenshots.spec.ts`, `profile-backup-settings.spec.ts`, + `voice-settings.spec.ts`, and the forum-touching cases in the broader + channel specs). +- Did not run the full Playwright suite or the Rust side (`just ci`). This + PR has no Rust changes and no diff outside `desktop/src`, and the full + JS/TS suite plus the targeted grep above already cover every spec that + touches channel tabs, sidebar hierarchy, and settings. A reviewer with + more time may want the full suite for extra confidence. ### Screenshots -Captured locally via a dev build + Playwright against the mock bridge -(`pnpm build:e2e` + a throwaway script driving `installMockBridge`/ -`openSettings`, not committed): - -- Settings → Appearance, showing the registry-rendered nav groups - (Personal/Communities/App) and panel content unchanged from before. -- The `#general` channel, showing the `stream` plugin's `Hash` glyph in the - channel header — the same icon `ChatHeader`'s old inline cascade produced - for a plain open stream channel. - -These aren't posted via `scripts/post-screenshots.sh` because that script -posts to an open PR (`gh pr create` was intentionally not run for this -branch — see the task instructions this branch was prepared under). Whoever -opens the actual PR from `upstream-pr/channel-feature-registry-seam-b` should -re-capture and post screenshots through that script at PR-creation time. +Captured locally against a dev build and the mock bridge: -## Duplicate check (re-verified today) +- Settings, Appearance panel, showing the registry-rendered nav groups + (Personal/Communities/App) and unchanged panel content. +- The `#general` channel, showing the `stream` plugin's Hash glyph in the + channel header, the same icon the old inline cascade produced for a + plain open stream channel. -`gh api search/issues -f q='repo:block/buzz channel feature registry ChannelFeaturePlugin'` -returns exactly two results: issue #3280 (this RFC) and PR #3275 (the MCP -Apps host, discussed above). No other open or closed issue/PR implements a -channel-feature/settings-section registry for the desktop client. +These weren't posted through `scripts/post-screenshots.sh`, since that +script needs an open PR and none was opened for this branch. Whoever opens +the PR from `upstream-pr/channel-feature-registry-seam-b` should recapture +and post screenshots through that script at PR-creation time. ## Follow-ups -- **Port the `tabs`/`ChannelFeatureShell` surface** once a second real - tab-contributing plugin exists in this repo (e.g. an MCP-App-tabs plugin - building on #3275, or a future Sequence/board/docs-style channel type). - Shaping `ChannelFeatureTab` against a hypothetical single consumer - risks guessing wrong; a second concrete caller is the right trigger. -- **Sidebar group/create-action surface** (`ChannelFeatureSidebar`) — same - reasoning; upstream has no repo/product-style sidebar hierarchy today to +- Port the `tabs`/`ChannelFeatureShell` surface once a second real + tab-contributing plugin exists (an MCP-App-tabs plugin building on + #3275, or a future Sequence/board/docs-style channel type). A second + concrete caller is the right trigger, shaping it against one hypothetical + consumer risks guessing wrong. +- Sidebar group/create-action surface (`ChannelFeatureSidebar`), same + reasoning. Upstream has no repo/product-style sidebar hierarchy today to drive the design. -- **`headerAction`** — same; no plugin needs to contribute a header action - yet (our fork's needed this for a "New idea" dialog action that doesn't - exist upstream). -- Consider whether `channelGlyph`'s `stream`-vs-`Hash`-styling special case - in `ChatHeader.tsx` (the `Glyph === Hash` check, preserved from the - original inline code's distinct `CHANNEL_HASH_ICON_CLASS`/`color="gray"` - treatment) is worth generalizing into a per-plugin style hook, or left as - the one acknowledged wart of an otherwise uniform glyph lookup. +- `headerAction`, same reasoning. No plugin needs to contribute a header + action yet (our fork needed this for a "New idea" dialog action that + doesn't exist upstream). +- Consider whether `channelGlyph`'s stream-vs-Hash styling special case in + `ChatHeader.tsx` is worth generalizing into a per-plugin style hook, or + left as the one acknowledged wart of an otherwise uniform glyph lookup. + +## Duplicate check + +`gh api search/issues -f q='repo:block/buzz channel feature registry ChannelFeaturePlugin'` +returns exactly two results: issue #3280 (this RFC) and PR #3275 (the MCP +Apps host, discussed above). No other open or closed issue or PR implements +a channel-feature or settings-section registry for the desktop client. ## Provenance The design was originally implemented and reviewed in a fork whose git -history isn't connected to this upstream (a content-snapshot import, not a -real fork), so it couldn't be cherry-picked or rebased. This PR is a fresh -port of the mechanism onto current `upstream/main`, adapted to what +history isn't connected to this upstream repo (a content-snapshot import, +not a real fork), so it couldn't be cherry-picked or rebased. This PR is a +fresh port of the mechanism onto current `upstream/main`, adapted to what upstream's `ChannelScreen`/`ChatHeader`/settings code actually look like -today (which has drifted from our fork's snapshot — the settings-section +today, which has drifted from our fork's snapshot: the settings-section descriptor shape, the channel dispatch's actual branch points, and the absence of any custom channel-type/tab surface all differ from what the -original diff assumed). See the "What's intentionally not ported" section -above for the specific scope this drift and the "behavior-preserving" -constraint together produced. +original diff assumed. See "Not in this PR" above for the scope that drift +and the behavior-preserving constraint together produced. From b3f07887459fb284988ae89826b911561129e13c Mon Sep 17 00:00:00 2001 From: Joost Reijnen Date: Wed, 26 Aug 2026 08:56:32 +0200 Subject: [PATCH 5/5] docs: add review focus questions and pin testing to commit SHA Signed-off-by: Joost Reijnen --- PR_DESCRIPTION.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/PR_DESCRIPTION.md b/PR_DESCRIPTION.md index d383d353375..5962920e0f3 100644 --- a/PR_DESCRIPTION.md +++ b/PR_DESCRIPTION.md @@ -97,8 +97,28 @@ plugin in this repo yet to shape it against. It answers the same question by proving the `parseBinding`/`glyph`/priority-cascade mechanics on two real call sites, so that field has something real to build on. +## Review focus + +1. Is the reduced scope (settings-section registry plus a two-plugin + channel classifier, no `tabs`/`sidebar`/`headerAction` surface) the + right size for a first PR, or should more of the RFC's surface be + included speculatively even without a second consumer? +2. Does this typed classification seam compose cleanly with #3275's own + review question about the same relationship ("does the current typed + channel-surface seam compose cleanly with the behavior-preserving + registry proposed in #3280")? This PR doesn't answer that by adding a + `tabs` field, it answers it by proving the underlying mechanics: does + that stand on its own, or do you want the `tabs` field sketched here + too even without a plugin to use it yet? +3. `channelGlyph`'s stream-vs-Hash styling special case in `ChatHeader.tsx` + is the one place the port kept a pre-existing wart rather than + generalizing it. Worth fixing here, or fine as a named follow-up? + ## Testing +At commit `31861518d` (the last code commit on this branch; later commits +only touch this description): + - `pnpm typecheck`: clean. - `pnpm lint` (`biome check`): clean on all touched/added files (two pre-existing warnings and two pre-existing infos elsewhere in the repo,