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..eeed9d543c --- /dev/null +++ b/desktop/src/features/messages/lib/systemEventCopy.test.mjs @@ -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"); +}); diff --git a/desktop/src/features/messages/lib/systemEventCopy.ts b/desktop/src/features/messages/lib/systemEventCopy.ts new file mode 100644 index 0000000000..bae6abb09b --- /dev/null +++ b/desktop/src/features/messages/lib/systemEventCopy.ts @@ -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; +} diff --git a/desktop/src/features/messages/ui/SystemMessageRow.tsx b/desktop/src/features/messages/ui/SystemMessageRow.tsx index 4410964dd9..c4637d2823 100644 --- a/desktop/src/features/messages/ui/SystemMessageRow.tsx +++ b/desktop/src/features/messages/ui/SystemMessageRow.tsx @@ -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)} ); } @@ -497,12 +524,17 @@ function describeSystemEvent( currentPubkey, profiles, ); + const inlineTargetLabel = resolveInlineDisplayLabel( + payload.target, + currentPubkey, + profiles, + ); const actorName = ( {actorLabel} ); const targetName = ( - {targetLabel} + {inlineTargetLabel} ); const membershipTitle = ( @@ -522,9 +554,13 @@ function describeSystemEvent( title: membershipTitle, action: ( <> - was added by{" "} + added by{" "} - {resolveDisplayLabel(payload.actor, currentPubkey, profiles)} + {resolveInlineDisplayLabel( + payload.actor, + currentPubkey, + profiles, + )} , along with{" "} - was added by{" "} + added by{" "} - {resolveDisplayLabel(payload.actor, currentPubkey, profiles)} + {resolveInlineDisplayLabel( + payload.actor, + currentPubkey, + profiles, + )} ), @@ -587,12 +627,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"); 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,