From 55e1b09c9dd7b988462f6e63d8e41e3c37c39685 Mon Sep 17 00:00:00 2001 From: Clay Delk Date: Wed, 29 Jul 2026 19:18:39 -0400 Subject: [PATCH 1/4] fix(desktop): say when a channel topic or purpose is cleared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clearing a channel topic arrives as a topic_changed event carrying an empty string, not as its own event type, so the timeline rendered `changed the topic to ""` — which reads as if the topic were set to two quote marks. Same for purpose. Both now read "cleared the channel topic" / "cleared the channel purpose", and whitespace-only values count as cleared for the same reason. The wording moves to lib/systemEventCopy.ts as a pure function so it can be asserted directly; that also drops two JSX fragments from SystemMessageRow, which shrinks 911 -> 900 lines. Membership captions lose "was": the caption renders on its own line under the member's name, so "added by Alice" reads as the metadata line it is and matches its siblings "joined the channel" and "left the channel". Two E2E assertions in mentions.spec.ts were measuring the wrong thing and this copy change exposed both: - The un-hovered style check ran with the mouse still parked from the previous hover. Any reflow — new rows, scroll-to-bottom, a different text wrap — could slide the button under that fixed pointer, so it asserted where the mouse happened to be rather than the resting style. It now parks the pointer off-target first. - A bare role=tooltip lookup matched two elements once the first tooltip began animating out while the second opened. It now scopes to the open tooltip. Part of #2216. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Clay Delk --- .../messages/lib/systemEventCopy.test.mjs | 59 +++++++++++++++++++ .../features/messages/lib/systemEventCopy.ts | 35 +++++++++++ .../features/messages/ui/SystemMessageRow.tsx | 9 +-- desktop/tests/e2e/mentions.spec.ts | 26 +++++--- 4 files changed, 118 insertions(+), 11 deletions(-) create mode 100644 desktop/src/features/messages/lib/systemEventCopy.test.mjs create mode 100644 desktop/src/features/messages/lib/systemEventCopy.ts diff --git a/desktop/src/features/messages/lib/systemEventCopy.test.mjs b/desktop/src/features/messages/lib/systemEventCopy.test.mjs new file mode 100644 index 0000000000..ec2d149aa1 --- /dev/null +++ b/desktop/src/features/messages/lib/systemEventCopy.test.mjs @@ -0,0 +1,59 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { describeChannelTextFieldChange } 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 channel topic", + ); + assert.equal( + describeChannelTextFieldChange("purpose", blank), + "cleared the channel purpose", + ); + } +}); + +test("a whitespace-only value reads as cleared", () => { + assert.equal( + describeChannelTextFieldChange("topic", " \n\t "), + "cleared the channel 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`, + ); + } + } +}); diff --git a/desktop/src/features/messages/lib/systemEventCopy.ts b/desktop/src/features/messages/lib/systemEventCopy.ts new file mode 100644 index 0000000000..35541ca779 --- /dev/null +++ b/desktop/src/features/messages/lib/systemEventCopy.ts @@ -0,0 +1,35 @@ +/** + * 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. + * + * 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 channel ${field}`; + } + return `changed the ${field} to ${OPEN_QUOTE}${trimmed}${CLOSE_QUOTE}`; +} diff --git a/desktop/src/features/messages/ui/SystemMessageRow.tsx b/desktop/src/features/messages/ui/SystemMessageRow.tsx index 4410964dd9..d6feee8d7f 100644 --- a/desktop/src/features/messages/ui/SystemMessageRow.tsx +++ b/desktop/src/features/messages/ui/SystemMessageRow.tsx @@ -28,6 +28,7 @@ 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 } from "../lib/systemEventCopy"; import { MessageAgentOwner } from "./MessageAgentOwner"; import { MessageAuthorText, MessageHeaderRow } from "./MessageHeader"; import { MessageTimestamp } from "./MessageTimestamp"; @@ -522,7 +523,7 @@ function describeSystemEvent( title: membershipTitle, action: ( <> - was added by{" "} + added by{" "} {resolveDisplayLabel(payload.actor, currentPubkey, profiles)} @@ -566,7 +567,7 @@ function describeSystemEvent( title: membershipTitle, action: ( <> - was added by{" "} + added by{" "} {resolveDisplayLabel(payload.actor, currentPubkey, profiles)} @@ -587,12 +588,12 @@ function describeSystemEvent( case "topic_changed": return { title: actorName, - action: <>changed the topic to “{payload.topic}”, + action: describeChannelTextFieldChange("topic", payload.topic), }; case "purpose_changed": return { title: actorName, - action: <>changed the purpose to “{payload.purpose}”, + action: describeChannelTextFieldChange("purpose", payload.purpose), }; case "channel_created": return { diff --git a/desktop/tests/e2e/mentions.spec.ts b/desktop/tests/e2e/mentions.spec.ts index 694b5abef5..512eb3d800 100644 --- a/desktop/tests/e2e/mentions.spec.ts +++ b/desktop/tests/e2e/mentions.spec.ts @@ -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"); From 07cd29db5af422f8c45b5b5a9793d613ba7e56d7 Mon Sep 17 00:00:00 2001 From: Clay Delk Date: Thu, 30 Jul 2026 10:30:45 -0400 Subject: [PATCH 2/4] fix(desktop): lowercase "you" inside system message sentences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An agent's metadata reads "managed by you" while the activity line right under it read "added by You" — the same word, two capitalizations, on the same row. `resolveUserLabel` returns "You", which is right where it stands alone as a name (the row title) and wrong once it lands mid-phrase. `toInlineName` adjusts it for the sentence positions: "added by you", "removed you from the channel", "along with you". Every other name is a proper noun and is returned untouched, matched exactly so "Youssef" is unaffected. This is the rule `formatOwnerLabel` already applies from the other side — it returns lowercase because it is only ever read as "managed by you". Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Clay Delk --- .../messages/lib/systemEventCopy.test.mjs | 27 +++++++++++++- .../features/messages/lib/systemEventCopy.ts | 14 ++++++++ .../features/messages/ui/SystemMessageRow.tsx | 35 ++++++++++++++++--- 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/desktop/src/features/messages/lib/systemEventCopy.test.mjs b/desktop/src/features/messages/lib/systemEventCopy.test.mjs index ec2d149aa1..f896bb2fcc 100644 --- a/desktop/src/features/messages/lib/systemEventCopy.test.mjs +++ b/desktop/src/features/messages/lib/systemEventCopy.test.mjs @@ -1,7 +1,10 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { describeChannelTextFieldChange } from "./systemEventCopy.ts"; +import { + describeChannelTextFieldChange, + toInlineName, +} from "./systemEventCopy.ts"; test("a set topic is quoted verbatim", () => { assert.equal( @@ -57,3 +60,25 @@ test("no caption announces empty quotes", () => { } } }); + +test("the current user is lowercase mid-sentence", () => { + // "added by You" next to an agent's "managed by you" was the inconsistency. + assert.equal(toInlineName("You"), "you"); +}); + +test("every other name keeps its own capitalization", () => { + for (const name of [ + "Alice Chen", + "you-know-who", + "Someone", + "npub1abc…def", + ]) { + assert.equal(toInlineName(name), name); + } +}); + +test("only the exact self label is rewritten", () => { + // A person really named "Your Highness" or "Youssef" is not the current user. + assert.equal(toInlineName("Youssef"), "Youssef"); + assert.equal(toInlineName("You Know Who"), "You Know Who"); +}); diff --git a/desktop/src/features/messages/lib/systemEventCopy.ts b/desktop/src/features/messages/lib/systemEventCopy.ts index 35541ca779..d570544d71 100644 --- a/desktop/src/features/messages/lib/systemEventCopy.ts +++ b/desktop/src/features/messages/lib/systemEventCopy.ts @@ -33,3 +33,17 @@ export function describeChannelTextFieldChange( } 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". Every other name is a proper noun and is + * returned untouched. + */ +export function toInlineName(label: string): string { + return label === "You" ? "you" : label; +} diff --git a/desktop/src/features/messages/ui/SystemMessageRow.tsx b/desktop/src/features/messages/ui/SystemMessageRow.tsx index d6feee8d7f..71210a3b0a 100644 --- a/desktop/src/features/messages/ui/SystemMessageRow.tsx +++ b/desktop/src/features/messages/ui/SystemMessageRow.tsx @@ -28,7 +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 } from "../lib/systemEventCopy"; +import { + describeChannelTextFieldChange, + toInlineName, +} from "../lib/systemEventCopy"; import { MessageAgentOwner } from "./MessageAgentOwner"; import { MessageAuthorText, MessageHeaderRow } from "./MessageHeader"; import { MessageTimestamp } from "./MessageTimestamp"; @@ -181,6 +184,15 @@ function resolveDisplayLabel( return resolveLabel(pubkey, currentPubkey, profiles); } +/** 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)); +} + function isKnownAgentPubkey( pubkey: string | undefined, profiles: UserProfileLookup | undefined, @@ -387,7 +399,7 @@ function MembershipPersonName({ pubkey={pubkey} underlineOnHover > - {resolveDisplayLabel(pubkey, currentPubkey, profiles)} + {resolveInlineDisplayLabel(pubkey, currentPubkey, profiles)} ); } @@ -498,12 +510,17 @@ function describeSystemEvent( currentPubkey, profiles, ); + const inlineTargetLabel = resolveInlineDisplayLabel( + payload.target, + currentPubkey, + profiles, + ); const actorName = ( {actorLabel} ); const targetName = ( - {targetLabel} + {inlineTargetLabel} ); const membershipTitle = ( @@ -525,7 +542,11 @@ function describeSystemEvent( <> added by{" "} - {resolveDisplayLabel(payload.actor, currentPubkey, profiles)} + {resolveInlineDisplayLabel( + payload.actor, + currentPubkey, + profiles, + )} , along with{" "} added by{" "} - {resolveDisplayLabel(payload.actor, currentPubkey, profiles)} + {resolveInlineDisplayLabel( + payload.actor, + currentPubkey, + profiles, + )} ), From 563a57326c51d1bc7c39155a639b3dd085ebf26d Mon Sep 17 00:00:00 2001 From: Clay Delk Date: Thu, 30 Jul 2026 13:15:28 -0400 Subject: [PATCH 3/4] fix(desktop): key the inline self-label on identity, not the display string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch on #3642: `toInlineName` compared the resolved label against "You", so a different person whose display name is literally "You" would have been rewritten as if they were the reader. The label is user-controlled; identity is not. The helper now takes an `isSelf` boolean the caller derives by comparing pubkeys through the existing `normalizePubkey` path. Also normalizes the noun: "cleared the channel topic" against "changed the topic to …" used two different nouns for the same field. Both now say "the topic" / "the purpose". This row only renders in a channel timeline, under that channel's own header, so naming the channel again is redundant. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Clay Delk --- .../messages/lib/systemEventCopy.test.mjs | 43 ++++++++++++++----- .../features/messages/lib/systemEventCopy.ts | 20 ++++++--- .../features/messages/ui/SystemMessageRow.tsx | 16 ++++++- 3 files changed, 63 insertions(+), 16 deletions(-) diff --git a/desktop/src/features/messages/lib/systemEventCopy.test.mjs b/desktop/src/features/messages/lib/systemEventCopy.test.mjs index f896bb2fcc..eeed9d543c 100644 --- a/desktop/src/features/messages/lib/systemEventCopy.test.mjs +++ b/desktop/src/features/messages/lib/systemEventCopy.test.mjs @@ -26,11 +26,11 @@ 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 channel topic", + "cleared the topic", ); assert.equal( describeChannelTextFieldChange("purpose", blank), - "cleared the channel purpose", + "cleared the purpose", ); } }); @@ -38,7 +38,7 @@ test("an empty value reads as cleared, not as a change to empty quotes", () => { test("a whitespace-only value reads as cleared", () => { assert.equal( describeChannelTextFieldChange("topic", " \n\t "), - "cleared the channel topic", + "cleared the topic", ); }); @@ -61,9 +61,24 @@ test("no caption announces empty quotes", () => { } }); -test("the current user is lowercase mid-sentence", () => { +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"), "you"); + 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", () => { @@ -73,12 +88,20 @@ test("every other name keeps its own capitalization", () => { "Someone", "npub1abc…def", ]) { - assert.equal(toInlineName(name), name); + assert.equal(toInlineName(name, false), name); } }); -test("only the exact self label is rewritten", () => { - // A person really named "Your Highness" or "Youssef" is not the current user. - assert.equal(toInlineName("Youssef"), "Youssef"); - assert.equal(toInlineName("You Know Who"), "You Know Who"); +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"); }); diff --git a/desktop/src/features/messages/lib/systemEventCopy.ts b/desktop/src/features/messages/lib/systemEventCopy.ts index d570544d71..bae6abb09b 100644 --- a/desktop/src/features/messages/lib/systemEventCopy.ts +++ b/desktop/src/features/messages/lib/systemEventCopy.ts @@ -17,6 +17,11 @@ 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 @@ -29,7 +34,7 @@ export function describeChannelTextFieldChange( ): string { const trimmed = value?.trim(); if (!trimmed) { - return `cleared the channel ${field}`; + return `cleared the ${field}`; } return `changed the ${field} to ${OPEN_QUOTE}${trimmed}${CLOSE_QUOTE}`; } @@ -41,9 +46,14 @@ export function describeChannelTextFieldChange( * `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". Every other name is a proper noun and is - * returned untouched. + * 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): string { - return label === "You" ? "you" : label; +export function toInlineName(label: string, isSelf: boolean): string { + return isSelf ? "you" : label; } diff --git a/desktop/src/features/messages/ui/SystemMessageRow.tsx b/desktop/src/features/messages/ui/SystemMessageRow.tsx index 71210a3b0a..c4637d2823 100644 --- a/desktop/src/features/messages/ui/SystemMessageRow.tsx +++ b/desktop/src/features/messages/ui/SystemMessageRow.tsx @@ -184,13 +184,27 @@ 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)); + return toInlineName( + resolveLabel(pubkey, currentPubkey, profiles), + isSelfPubkey(pubkey, currentPubkey), + ); } function isKnownAgentPubkey( From cadd4266e5ddcf045d6a0b450b99c1e94621e65c Mon Sep 17 00:00:00 2001 From: Clay Delk Date: Thu, 30 Jul 2026 15:06:19 -0400 Subject: [PATCH 4/4] fix(mobile): match desktop channel topic and membership copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the three desktop fixes in this PR apply to the Flutter timeline. **Cleared topic rendered as empty quotes.** `SystemEvent.describe` printed `changed the topic to ""` when the relay reported a clear, same as desktop did — a clear arrives as a `topic_changed` event carrying an empty string, not as its own event type. Now reads "cleared the topic" / "cleared the purpose", and the value is trimmed before it goes inside the quotes. **"was added by" in the two-line membership layout.** Every `member_joined` event renders through `_MembershipSystemMessageContent`, which puts the member's name and timestamp on one line via `MessageAuthorMeta` and the caption beneath — the same structure as desktop's `SystemMessageRow`. So the caption drops "was" for the same reason. `SystemEvent.describe` keeps "$target was added by $actor". That path builds subject and predicate into a single string, so dropping "was" there would read "Bob added by Alice". It is also only a fallback: the membership layout handles every well-formed member_joined event. Left alone: mobile's date formatters have the same divergence desktop just resolved (#3769) — `formatDayHeading` has no weekday band and always prints the year, and `formatThreadSummaryLastReplyTime` still emits ordinals like "on May 19th". Tracked separately rather than folded into a copy change. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Clay Delk --- .../channel_detail_page/system_rows.dart | 7 ++- .../features/channels/timeline_message.dart | 24 +++++++++- .../channels/channel_detail_page_test.dart | 6 +-- .../channels/timeline_message_test.dart | 47 +++++++++++++++++++ 4 files changed, 78 insertions(+), 6 deletions(-) diff --git a/mobile/lib/features/channels/channel_detail_page/system_rows.dart b/mobile/lib/features/channels/channel_detail_page/system_rows.dart index f6019febab..e2b4d378bd 100644 --- a/mobile/lib/features/channels/channel_detail_page/system_rows.dart +++ b/mobile/lib/features/channels/channel_detail_page/system_rows.dart @@ -278,7 +278,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 '), diff --git a/mobile/lib/features/channels/timeline_message.dart b/mobile/lib/features/channels/timeline_message.dart index 253c949703..c8fd96491b 100644 --- a/mobile/lib/features/channels/timeline_message.dart +++ b/mobile/lib/features/channels/timeline_message.dart @@ -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; diff --git a/mobile/test/features/channels/channel_detail_page_test.dart b/mobile/test/features/channels/channel_detail_page_test.dart index 127e6851e6..a371b758f2 100644 --- a/mobile/test/features/channels/channel_detail_page_test.dart +++ b/mobile/test/features/channels/channel_detail_page_test.dart @@ -1333,7 +1333,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( @@ -1351,7 +1351,7 @@ void main() { expect(timestampRect.left, greaterThan(nameRect.right)); final addedText = tester.widget(addedAction); expect( - effectiveFontSizeForText(addedText.text, 'was added by Alice'), + effectiveFontSizeForText(addedText.text, 'added by Alice'), systemMessageBodyTextStyle.fontSize, ); }); @@ -1420,7 +1420,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); diff --git a/mobile/test/features/channels/timeline_message_test.dart b/mobile/test/features/channels/timeline_message_test.dart index 87f39d1c65..c29a12aef4 100644 --- a/mobile/test/features/channels/timeline_message_test.dart +++ b/mobile/test/features/channels/timeline_message_test.dart @@ -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,