fix(desktop): channel topic and membership metadata cleanup (#3642)

First slice of #2216, scoped to the system/status lines in the chat
timeline.

## Why

Two problems on the same surface.

**Clearing a channel topic renders as empty quotes.** The relay reports
a clear as a `topic_changed` event carrying an empty string — there's no
separate "cleared" event type. So the timeline printed:

> Alice
> changed the topic to “”

which reads as if the topic were *set to* two quote marks. Same for
purpose.

**The membership caption reads like a headline, not a metadata line.**
`title` and `action` render on separate lines — the member's name sits
in the header row with the avatar and timestamp, and the caption sits
beneath it. So the caption was "was added by Alice Chen" standing alone
under a name, while its siblings on that same line are "joined the
channel" and "left the channel".

## What

- Blank, missing, or whitespace-only topic/purpose now reads **"cleared
the channel topic"** / **"cleared the channel purpose"**.
- Membership captions drop "was": **"added by Alice Chen"**, matching
"joined the channel" and "left the channel".
- The wording moves to `lib/systemEventCopy.ts` as a pure function, so
it's assertable in a unit test instead of only reachable through the
DOM. That also removes two JSX fragments from `SystemMessageRow.tsx`,
taking it 911 → 900 lines.

## Two E2E assertions this exposed

Both were measuring something other than what they claimed, and the copy
change tipped them over. Neither is a product bug, but both would have
failed the next person too.

1. **`mentions.spec.ts:1245`** asserted a button was un-underlined while
the mouse was still parked from a previous `hover()`. Any reflow — new
rows, scroll-to-bottom, a different text wrap — can slide that button
under the stationary pointer, so the assertion measured *where the mouse
happened to be* rather than the resting style. Dropping four characters
changed the text wrap, changed the row height, changed the scroll
offset, and the pointer landed on it. Now parks the pointer off-target
first.
2. **`mentions.spec.ts:1253`** used a bare `role=tooltip` lookup. Once
the first tooltip animates out while the second opens, two elements
match and strict mode trips. Now scopes to the open tooltip via
`:not([data-state="closed"])`.

## Deliberately out of scope

- **Timestamps.** The day divider, per-message clock times, the Inbox
thread pane, and the inbox list have three divergent date
implementations and none fully match the writing standard's
Today/Yesterday/weekday/date progression. That's its own slice of #2216.
- **Whose avatar shows.** An addition puts the *added* member in the
header; a removal puts the *remover* there. Possibly intentional, but
it's a design question, not copy.
- **`the channel` vs `this channel`.** joined/left/removed say "the
channel"; created/archived/unarchived say "this channel". Worth
normalizing, but it touches lines this PR otherwise leaves alone.

## Validation

- `pnpm check`, `pnpm typecheck` — clean
- Unit: **3781/3781**, including 6 new tests in
`systemEventCopy.test.mjs` covering set/blank/undefined/null/whitespace
for both fields, plus a guard that no variant can emit empty quotes
- Smoke E2E `mentions` + `messaging`: **85/85**
- The previously fragile test run with `--repeat-each=5`: **5/5**

Signed-off-by: Clay Delk <clay.delk@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Clay Delk
2026-07-30 18:53:04 -04:00
committed by GitHub
parent ede2686334
commit 9e8fcfda09
8 changed files with 311 additions and 21 deletions

View File

@@ -0,0 +1,107 @@
import assert from "node:assert/strict";
import test from "node:test";
import {
describeChannelTextFieldChange,
toInlineName,
} from "./systemEventCopy.ts";
test("a set topic is quoted verbatim", () => {
assert.equal(
describeChannelTextFieldChange("topic", "Release planning"),
"changed the topic to “Release planning”",
);
});
test("a set purpose names the purpose, not the topic", () => {
assert.equal(
describeChannelTextFieldChange("purpose", "Where we ship from"),
"changed the purpose to “Where we ship from”",
);
});
// The relay reports a clear as a change carrying an empty string, so without
// this branch the timeline reads: changed the topic to “”.
test("an empty value reads as cleared, not as a change to empty quotes", () => {
for (const blank of ["", undefined, null]) {
assert.equal(
describeChannelTextFieldChange("topic", blank),
"cleared the topic",
);
assert.equal(
describeChannelTextFieldChange("purpose", blank),
"cleared the purpose",
);
}
});
test("a whitespace-only value reads as cleared", () => {
assert.equal(
describeChannelTextFieldChange("topic", " \n\t "),
"cleared the topic",
);
});
test("surrounding whitespace is trimmed out of the quotes", () => {
assert.equal(
describeChannelTextFieldChange("topic", " Release planning "),
"changed the topic to “Release planning”",
);
});
test("no caption announces empty quotes", () => {
for (const value of ["", " ", null, undefined, "Real topic"]) {
for (const field of ["topic", "purpose"]) {
assert.doesNotMatch(
describeChannelTextFieldChange(field, value),
/“”|""/,
`${field} with ${JSON.stringify(value)} must not render empty quotes`,
);
}
}
});
test("the reader's own name is lowercase mid-sentence", () => {
// "added by You" next to an agent's "managed by you" was the inconsistency.
assert.equal(toInlineName("You", true), "you");
});
test("cleared and changed captions use the same noun", () => {
// Not "cleared the channel topic" against "changed the topic to …".
assert.match(describeChannelTextFieldChange("topic", ""), /\bthe topic\b/);
assert.match(
describeChannelTextFieldChange("topic", "Ship it"),
/\bthe topic\b/,
);
for (const value of ["", "Ship it"]) {
assert.doesNotMatch(
describeChannelTextFieldChange("topic", value),
/channel topic/,
);
}
});
test("every other name keeps its own capitalization", () => {
for (const name of [
"Alice Chen",
"you-know-who",
"Someone",
"npub1abc…def",
]) {
assert.equal(toInlineName(name, false), name);
}
});
test("someone else whose display name is literally You is left alone", () => {
// The decisive case: the label is user-controlled, identity is not. Matching
// on the string would rewrite this person's name as if they were the reader.
assert.equal(toInlineName("You", false), "You");
assert.equal(toInlineName("Youssef", false), "Youssef");
assert.equal(toInlineName("You Know Who", false), "You Know Who");
});
test("the reader is lowercased whatever their profile name says", () => {
// Self resolution never consults the profile, but the rule keys on identity,
// so it does not matter what the label happens to be.
assert.equal(toInlineName("Alice Chen", true), "you");
});

View File

@@ -0,0 +1,59 @@
/**
* Copy for channel system events (the "joined", "added by", "changed the
* topic" captions in the message timeline).
*
* These live outside `SystemMessageRow` so the wording is a pure function of
* the payload and can be asserted directly in tests. Only cases whose caption
* is plain text belong here — cases that interpolate a profile link build their
* JSX in the component.
*/
/** Curly quotes, so the caption matches the typography used elsewhere in chat. */
const OPEN_QUOTE = "“";
const CLOSE_QUOTE = "”";
export type ChannelTextField = "topic" | "purpose";
/**
* Caption for a channel topic or purpose change.
*
* Bare "the topic" rather than "the channel topic": this row only ever renders
* in a channel timeline, under that channel's own header, so naming the channel
* again is redundant — and it keeps the cleared and changed captions on the same
* noun instead of one saying "channel topic" and the other "topic".
*
* A blank value means the field was cleared: the relay reports a clear as a
* `topic_changed` / `purpose_changed` event carrying an empty string, not as a
* separate event type. Without this branch the timeline renders `changed the
* topic to ""`, which reads like the topic was set to two quote marks.
* Whitespace-only values are treated as cleared for the same reason.
*/
export function describeChannelTextFieldChange(
field: ChannelTextField,
value: string | null | undefined,
): string {
const trimmed = value?.trim();
if (!trimmed) {
return `cleared the ${field}`;
}
return `changed the ${field} to ${OPEN_QUOTE}${trimmed}${CLOSE_QUOTE}`;
}
/**
* Adjusts a resolved display name for use inside a sentence rather than in the
* name slot at the top of a row — "added by you", "removed you from the channel".
*
* `resolveUserLabel` returns "You" for the current user, which is right standing
* alone and wrong mid-phrase. Agent ownership already draws the same distinction
* from the other side: `formatOwnerLabel` returns lowercase "you" because it is
* only ever read as "managed by you".
*
* `isSelf` is the caller's pubkey comparison, not an inspection of `label`.
* Matching on the string would also rewrite a different person whose display
* name happens to be "You" — the label is user-controlled, identity is not.
* Every name that isn't the reader's own is a proper noun and is returned
* untouched.
*/
export function toInlineName(label: string, isSelf: boolean): string {
return isSelf ? "you" : label;
}

View File

@@ -28,6 +28,10 @@ import {
import { Popover, PopoverContent, PopoverTrigger } from "@/shared/ui/popover";
import { Tooltip, TooltipContent, TooltipTrigger } from "@/shared/ui/tooltip";
import { UserAvatar } from "@/shared/ui/UserAvatar";
import {
describeChannelTextFieldChange,
toInlineName,
} from "../lib/systemEventCopy";
import { MessageAgentOwner } from "./MessageAgentOwner";
import { MessageAuthorText, MessageHeaderRow } from "./MessageHeader";
import { MessageTimestamp } from "./MessageTimestamp";
@@ -180,6 +184,29 @@ function resolveDisplayLabel(
return resolveLabel(pubkey, currentPubkey, profiles);
}
function isSelfPubkey(
pubkey: string | undefined,
currentPubkey: string | undefined,
): boolean {
return Boolean(
pubkey &&
currentPubkey &&
normalizePubkey(pubkey) === normalizePubkey(currentPubkey),
);
}
/** Same label as `resolveDisplayLabel`, adjusted for mid-sentence use. */
function resolveInlineDisplayLabel(
pubkey: string | undefined,
currentPubkey: string | undefined,
profiles: UserProfileLookup | undefined,
): string {
return toInlineName(
resolveLabel(pubkey, currentPubkey, profiles),
isSelfPubkey(pubkey, currentPubkey),
);
}
function isKnownAgentPubkey(
pubkey: string | undefined,
profiles: UserProfileLookup | undefined,
@@ -386,7 +413,7 @@ function MembershipPersonName({
pubkey={pubkey}
underlineOnHover
>
{resolveDisplayLabel(pubkey, currentPubkey, profiles)}
{resolveInlineDisplayLabel(pubkey, currentPubkey, profiles)}
</ProfileName>
);
}
@@ -497,12 +524,17 @@ function describeSystemEvent(
currentPubkey,
profiles,
);
const inlineTargetLabel = resolveInlineDisplayLabel(
payload.target,
currentPubkey,
profiles,
);
const actorName = (
<ProfileName pubkey={payload.actor}>{actorLabel}</ProfileName>
);
const targetName = (
<ProfileName highlight isAgent={isTargetAgent} pubkey={payload.target}>
{targetLabel}
{inlineTargetLabel}
</ProfileName>
);
const membershipTitle = (
@@ -522,9 +554,13 @@ function describeSystemEvent(
title: membershipTitle,
action: (
<>
was added by{" "}
added by{" "}
<ProfileName pubkey={payload.actor} underlineOnHover>
{resolveDisplayLabel(payload.actor, currentPubkey, profiles)}
{resolveInlineDisplayLabel(
payload.actor,
currentPubkey,
profiles,
)}
</ProfileName>
, along with{" "}
<MemberNamesInlineList
@@ -566,9 +602,13 @@ function describeSystemEvent(
title: membershipTitle,
action: (
<>
was added by{" "}
added by{" "}
<ProfileName pubkey={payload.actor} underlineOnHover>
{resolveDisplayLabel(payload.actor, currentPubkey, profiles)}
{resolveInlineDisplayLabel(
payload.actor,
currentPubkey,
profiles,
)}
</ProfileName>
</>
),
@@ -587,12 +627,12 @@ function describeSystemEvent(
case "topic_changed":
return {
title: actorName,
action: <>changed the topic to &ldquo;{payload.topic}&rdquo;</>,
action: describeChannelTextFieldChange("topic", payload.topic),
};
case "purpose_changed":
return {
title: actorName,
action: <>changed the purpose to &ldquo;{payload.purpose}&rdquo;</>,
action: describeChannelTextFieldChange("purpose", payload.purpose),
};
case "channel_created":
return {

View File

@@ -1114,7 +1114,7 @@ test("system add rows use plain names while remove rows retain agent mention sty
const addedRow = page
.getByTestId("system-message-row")
.filter({ hasText: "portal" })
.filter({ hasText: "was added by" });
.filter({ hasText: "added by" });
const removedRow = page
.getByTestId("system-message-row")
.filter({ hasText: "removed portal from the channel" });
@@ -1178,7 +1178,7 @@ test("groups member additions and joins with hidden names in the standard toolti
const groupedRow = page
.getByTestId("system-message-row")
.filter({ hasText: "was added by Alice Chen" });
.filter({ hasText: "added by Alice Chen" });
for (const visibleName of [
"Erica Chapman",
"Peter Griffin",
@@ -1188,9 +1188,9 @@ test("groups member additions and joins with hidden names in the standard toolti
await expect(groupedRow).toContainText(visibleName);
}
await expect(
groupedRow.locator("p").filter({ hasText: "was added by" }),
groupedRow.locator("p").filter({ hasText: "added by" }),
).toContainText(
"was added by Alice Chen, along with Peter Griffin, Marcia Thomas, Jordan Lee, and 2 others",
"added by Alice Chen, along with Peter Griffin, Marcia Thomas, Jordan Lee, and 2 others",
);
await expect(groupedRow.locator("[data-mention]")).toHaveCount(0);
@@ -1200,6 +1200,11 @@ test("groups member additions and joins with hidden names in the standard toolti
await expect(visibleName).toHaveCSS("text-decoration-line", "underline");
const othersTrigger = groupedRow.getByRole("button", { name: "2 others" });
// Park the pointer off-target first: the previous hover leaves the mouse at a
// fixed viewport point, and any later reflow (new rows, scroll-to-bottom, a
// different text wrap) can slide this button under it. Without this the
// assertion measures where the mouse happens to be, not the resting style.
await page.mouse.move(0, 0);
await expect(othersTrigger).toHaveCSS("text-decoration-line", "none");
await othersTrigger.hover();
await expect(othersTrigger).toHaveCSS("text-decoration-line", "underline");
@@ -1242,10 +1247,17 @@ test("groups member additions and joins with hidden names in the standard toolti
const joinedOthersTrigger = joinedRow.getByRole("button", {
name: "2 others",
});
await page.mouse.move(0, 0);
await expect(joinedOthersTrigger).toHaveCSS("text-decoration-line", "none");
await joinedOthersTrigger.hover();
await expect(page.getByRole("tooltip")).toContainText("Olivia Park");
await expect(page.getByRole("tooltip")).toContainText("Sam Rivera");
// Scope to the *open* tooltip: the first row's tooltip stays mounted with
// data-state="closed" while it animates out, so a bare role=tooltip lookup
// matches two elements and trips strict mode.
const joinedTooltip = page.locator(
'[role="tooltip"]:not([data-state="closed"])',
);
await expect(joinedTooltip).toContainText("Olivia Park");
await expect(joinedTooltip).toContainText("Sam Rivera");
});
test("system agent profile only exposes message action", async ({ page }) => {
@@ -1277,7 +1289,7 @@ test("system agent profile only exposes message action", async ({ page }) => {
const joinedRow = page
.getByTestId("system-message-row")
.filter({ hasText: "mira" })
.filter({ hasText: "was added by" });
.filter({ hasText: "added by" });
const agentName = joinedRow.getByText("mira", { exact: true });
await expect(agentName).toHaveText("mira");
await expect(agentName).not.toHaveAttribute("data-mention");

View File

@@ -279,7 +279,12 @@ class _MembershipSystemMessageContent extends StatelessWidget {
TextSpan(
text: event.isSelfJoin
? 'joined the channel'
: 'was added by ${resolveLabel(event.actorPubkey)}',
// No "was": the name renders on the line above via
// MessageAuthorMeta, so this reads as a status line rather than a
// sentence continuing across the metadata row. Matches desktop's
// SystemMessageRow. `SystemEvent.describe` keeps "was added by"
// because it builds subject and predicate into one string.
: 'added by ${resolveLabel(event.actorPubkey)}',
),
if (additionalTargets.isNotEmpty)
TextSpan(text: event.isSelfJoin ? ' along with ' : ', along with '),

View File

@@ -101,9 +101,10 @@ class SystemEvent {
final target = resolveLabel(targetPubkey);
return '$actor removed $target from the channel';
}(),
SystemEventType.topicChanged => '$actor changed the topic to "$topic"',
SystemEventType.topicChanged =>
'$actor ${_describeTextFieldChange('topic', topic)}',
SystemEventType.purposeChanged =>
'$actor changed the purpose to "$purpose"',
'$actor ${_describeTextFieldChange('purpose', purpose)}',
SystemEventType.channelCreated => '$actor created this channel',
SystemEventType.channelArchived => '$actor archived this channel',
SystemEventType.channelUnarchived => '$actor unarchived this channel',
@@ -113,6 +114,25 @@ class SystemEvent {
}
}
/// Caption fragment for a channel topic or purpose change, e.g.
/// `changed the topic to "Release planning"` or `cleared the topic`.
///
/// A blank value means the field was cleared: the relay reports a clear as a
/// `topic_changed` / `purpose_changed` event carrying an empty string, not as a
/// separate event type. Without this branch the timeline renders
/// `changed the topic to ""`, which reads as if the topic were set to two quote
/// marks. Whitespace-only values are treated as cleared for the same reason.
///
/// Mirrors `describeChannelTextFieldChange` in
/// `desktop/src/features/messages/lib/systemEventCopy.ts`.
String _describeTextFieldChange(String field, String? value) {
final trimmed = value?.trim();
if (trimmed == null || trimmed.isEmpty) {
return 'cleared the $field';
}
return 'changed the $field to "$trimmed"';
}
@immutable
class TimelineReaction {
final String emoji;

View File

@@ -1348,7 +1348,7 @@ void main() {
await tester.pumpAndSettle();
expect(find.text('Bob'), findsOneWidget);
final addedAction = findRichText('was added by Alice');
final addedAction = findRichText('added by Alice');
expect(addedAction, findsOneWidget);
expect(find.text('Alice added Bob to the channel'), findsNothing);
expect(
@@ -1366,7 +1366,7 @@ void main() {
expect(timestampRect.left, greaterThan(nameRect.right));
final addedText = tester.widget<RichText>(addedAction);
expect(
effectiveFontSizeForText(addedText.text, 'was added by Alice'),
effectiveFontSizeForText(addedText.text, 'added by Alice'),
systemMessageBodyTextStyle.fontSize,
);
});
@@ -1435,7 +1435,7 @@ void main() {
expect(find.text('Bob'), findsOneWidget);
expect(
findRichText('was added by Alice, along with Carol, Dave, Erin, and '),
findRichText('added by Alice, along with Carol, Dave, Erin, and '),
findsOneWidget,
);
expect(find.byKey(const Key('membership-overflow')), findsOneWidget);

View File

@@ -287,6 +287,53 @@ void main() {
);
});
// The relay reports a clear as a change carrying an empty string, so
// without the cleared branch this reads: changed the topic to "".
test('a blank topic or purpose reads as cleared', () {
for (final blank in [null, '', ' \n\t ']) {
expect(
SystemEvent(
type: SystemEventType.topicChanged,
actorPubkey: 'pk1',
topic: blank,
).describe(resolve),
'Alice cleared the topic',
);
expect(
SystemEvent(
type: SystemEventType.purposeChanged,
actorPubkey: 'pk1',
purpose: blank,
).describe(resolve),
'Alice cleared the purpose',
);
}
});
test('no caption announces empty quotes', () {
for (final value in [null, '', ' ', 'Real topic']) {
expect(
SystemEvent(
type: SystemEventType.topicChanged,
actorPubkey: 'pk1',
topic: value,
).describe(resolve),
isNot(contains('""')),
);
}
});
test('surrounding whitespace is trimmed out of the quotes', () {
expect(
SystemEvent(
type: SystemEventType.topicChanged,
actorPubkey: 'pk1',
topic: ' Release v2 ',
).describe(resolve),
'Alice changed the topic to "Release v2"',
);
});
test('channel_created', () {
final event = SystemEvent(
type: SystemEventType.channelCreated,