From 560224f5161eb593602c8d450ae4e4e353cee199 Mon Sep 17 00:00:00 2001 From: Jamie <113370520+jamiebuilds-signal@users.noreply.github.com> Date: Tue, 13 Jan 2026 12:01:07 -0800 Subject: [PATCH] Pinned messages UI fixes --- stylesheets/_modules.scss | 74 ++----------------- .../conversation/ExpireTimer.dom.stories.tsx | 35 --------- .../conversation/ExpireTimer.dom.tsx | 20 +---- .../conversation/MessageMetadata.dom.tsx | 4 - .../conversation/Quote.dom.stories.tsx | 2 +- .../conversation/Timeline.dom.stories.tsx | 2 +- .../TimelineMessage.dom.stories.tsx | 2 +- .../conversation/TimelineMessage.dom.tsx | 12 ++- .../PinnedMessagesBar.dom.stories.tsx | 24 +++++- .../pinned-messages/PinnedMessagesBar.dom.tsx | 65 ++++++++-------- ts/messageModifiers/PinnedMessages.preload.ts | 7 ++ ts/sql/server/pinnedMessages.std.ts | 2 +- ts/state/selectors/message.preload.ts | 15 +++- ts/state/smart/PinnedMessagesBar.preload.tsx | 56 ++++++++++---- 14 files changed, 133 insertions(+), 187 deletions(-) diff --git a/stylesheets/_modules.scss b/stylesheets/_modules.scss index e07b3f412b..e23107ff86 100644 --- a/stylesheets/_modules.scss +++ b/stylesheets/_modules.scss @@ -1874,81 +1874,17 @@ $message-padding-horizontal: 12px; display: inline-block; margin-inline-start: 6px; margin-bottom: 2px; - - & { - @include mixins.dark-theme { - @include mixins.color-svg( - '../images/icons/v3/message_timer/messagetimer-60.svg', - variables.$color-white-alpha-80 - ); - } - } - & { - @include mixins.light-theme { - @include mixins.color-svg( - '../images/icons/v3/message_timer/messagetimer-60.svg', - variables.$color-white-alpha-80 - ); - } - } } $timer-icons: - '55', '50', '45', '40', '35', '30', '25', '20', '15', '10', '05', '00'; + '60', '55', '50', '45', '40', '35', '30', '25', '20', '15', '10', '05', '00'; @each $timer-icon in $timer-icons { .module-expire-timer--#{$timer-icon} { - & { - @include mixins.dark-theme { - @include mixins.color-svg( - '../images/icons/v3/message_timer/messagetimer-#{$timer-icon}.svg', - variables.$color-white-alpha-80 - ); - } - } - & { - @include mixins.light-theme { - @include mixins.color-svg( - '../images/icons/v3/message_timer/messagetimer-#{$timer-icon}.svg', - variables.$color-white-alpha-80 - ); - } - } - } -} - -.module-expire-timer--incoming { - background-color: variables.$color-white-alpha-80; - - @include mixins.light-theme { - background-color: variables.$color-gray-60; - } - @include mixins.dark-theme { - background-color: variables.$color-gray-25; - } -} -.module-expire-timer--with-sticker { - @include mixins.light-theme { - background-color: variables.$color-gray-60; - } -} - -// When status indicators are overlaid on top of an image, they use different colors -.module-expire-timer--with-image-no-caption { - @include mixins.light-theme { - background-color: variables.$color-white; - } - @include mixins.dark-theme { - background-color: variables.$color-gray-02; - } -} - -.module-expire-timer--outline-only-bubble { - @include mixins.light-theme { - background-color: variables.$color-gray-60; - } - @include mixins.dark-theme { - background-color: variables.$color-gray-25; + @include mixins.color-svg( + '../images/icons/v3/message_timer/messagetimer-#{$timer-icon}.svg', + currentColor + ); } } diff --git a/ts/components/conversation/ExpireTimer.dom.stories.tsx b/ts/components/conversation/ExpireTimer.dom.stories.tsx index 21a9280d45..b321077c42 100644 --- a/ts/components/conversation/ExpireTimer.dom.stories.tsx +++ b/ts/components/conversation/ExpireTimer.dom.stories.tsx @@ -11,12 +11,9 @@ export default { } satisfies Meta; const createProps = (overrideProps: Partial = {}): Props => ({ - direction: overrideProps.direction || 'outgoing', expirationLength: overrideProps.expirationLength || 30 * 1000, expirationTimestamp: overrideProps.expirationTimestamp || Date.now() + 30 * 1000, - withImageNoCaption: overrideProps.withImageNoCaption || false, - withSticker: overrideProps.withSticker || false, }); export const _30Seconds = (): React.JSX.Element => { @@ -51,38 +48,6 @@ export function Expired(): React.JSX.Element { return ; } -export function Sticker(): React.JSX.Element { - const props = createProps({ - withSticker: true, - }); - - return ; -} - -export function ImageNoCaption(): React.JSX.Element { - const props = createProps({ - withImageNoCaption: true, - }); - - return ( -
- -
- ); -} - -export function Incoming(): React.JSX.Element { - const props = createProps({ - direction: 'incoming', - }); - - return ( -
- -
- ); -} - export function ExpirationTooFarOut(): React.JSX.Element { const props = createProps({ expirationTimestamp: Date.now() + 150 * 1000, diff --git a/ts/components/conversation/ExpireTimer.dom.tsx b/ts/components/conversation/ExpireTimer.dom.tsx index ffa114d149..878ed367ee 100644 --- a/ts/components/conversation/ExpireTimer.dom.tsx +++ b/ts/components/conversation/ExpireTimer.dom.tsx @@ -6,22 +6,14 @@ import classNames from 'classnames'; import { getIncrement, getTimerBucket } from '../../util/timer.std.js'; -export type Props = { - direction?: 'incoming' | 'outgoing'; +export type Props = Readonly<{ expirationLength: number; expirationTimestamp?: number; - isOutlineOnlyBubble?: boolean; - withImageNoCaption?: boolean; - withSticker?: boolean; -}; +}>; export function ExpireTimer({ - direction, expirationLength, expirationTimestamp, - isOutlineOnlyBubble, - withImageNoCaption, - withSticker, }: Props): React.JSX.Element { const [, forceUpdate] = useReducer(() => ({}), {}); @@ -40,13 +32,7 @@ export function ExpireTimer({
); diff --git a/ts/components/conversation/MessageMetadata.dom.tsx b/ts/components/conversation/MessageMetadata.dom.tsx index 2dec39e2d9..dd512eada2 100644 --- a/ts/components/conversation/MessageMetadata.dom.tsx +++ b/ts/components/conversation/MessageMetadata.dom.tsx @@ -220,12 +220,8 @@ export const MessageMetadata = forwardRef>( ) : null} {expirationLength ? ( ) : null} {textPending ? ( diff --git a/ts/components/conversation/Quote.dom.stories.tsx b/ts/components/conversation/Quote.dom.stories.tsx index 5e3de36d5e..23c3ba653a 100644 --- a/ts/components/conversation/Quote.dom.stories.tsx +++ b/ts/components/conversation/Quote.dom.stories.tsx @@ -77,7 +77,7 @@ const defaultMessageProps: TimelineMessagesProps = { canEditMessage: true, canEndPoll: false, canForward: true, - canPinMessages: true, + canPinMessage: true, canReact: true, canReply: true, canRetry: true, diff --git a/ts/components/conversation/Timeline.dom.stories.tsx b/ts/components/conversation/Timeline.dom.stories.tsx index f248e147c0..5ecf7f81c8 100644 --- a/ts/components/conversation/Timeline.dom.stories.tsx +++ b/ts/components/conversation/Timeline.dom.stories.tsx @@ -47,7 +47,7 @@ function mockMessageTimelineItem( canEditMessage: true, canEndPoll: false, canForward: true, - canPinMessages: true, + canPinMessage: true, canReact: true, canReply: true, canRetry: true, diff --git a/ts/components/conversation/TimelineMessage.dom.stories.tsx b/ts/components/conversation/TimelineMessage.dom.stories.tsx index 41b2e1e24f..417b7c0190 100644 --- a/ts/components/conversation/TimelineMessage.dom.stories.tsx +++ b/ts/components/conversation/TimelineMessage.dom.stories.tsx @@ -239,7 +239,7 @@ const createProps = (overrideProps: Partial = {}): Props => ({ canCopy: true, canEditMessage: true, canEndPoll: overrideProps.direction === 'outgoing', - canPinMessages: overrideProps.canPinMessages ?? true, + canPinMessage: overrideProps.canPinMessage ?? true, canReact: true, canReply: true, canDownload: true, diff --git a/ts/components/conversation/TimelineMessage.dom.tsx b/ts/components/conversation/TimelineMessage.dom.tsx index a17c66b2bf..7fbb9be613 100644 --- a/ts/components/conversation/TimelineMessage.dom.tsx +++ b/ts/components/conversation/TimelineMessage.dom.tsx @@ -54,7 +54,7 @@ export type PropsData = { canRetryDeleteForEveryone: boolean; canReact: boolean; canReply: boolean; - canPinMessages: boolean; + canPinMessage: boolean; hasMaxPinnedMessages: boolean; selectedReaction?: string; isTargeted?: boolean; @@ -114,7 +114,7 @@ export function TimelineMessage(props: Props): React.JSX.Element { canReply, canRetry, canRetryDeleteForEveryone, - canPinMessages, + canPinMessage, containerElementRef, containerWidthBreakpoint, conversationId, @@ -368,11 +368,9 @@ export function TimelineMessage(props: Props): React.JSX.Element { }); }} onPinMessage={ - canPinMessages && !isPinned ? handleOpenPinMessageDialog : null - } - onUnpinMessage={ - canPinMessages && isPinned ? handleUnpinMessage : null + canPinMessage && !isPinned ? handleOpenPinMessageDialog : null } + onUnpinMessage={canPinMessage && isPinned ? handleUnpinMessage : null} onMoreInfo={() => pushPanelForConversation({ type: PanelType.MessageDetails, @@ -388,7 +386,7 @@ export function TimelineMessage(props: Props): React.JSX.Element { canCopy, canEditMessage, canForward, - canPinMessages, + canPinMessage, canRetry, canSelect, canEndPoll, diff --git a/ts/components/conversation/pinned-messages/PinnedMessagesBar.dom.stories.tsx b/ts/components/conversation/pinned-messages/PinnedMessagesBar.dom.stories.tsx index d56c2e4ff6..98be000336 100644 --- a/ts/components/conversation/pinned-messages/PinnedMessagesBar.dom.stories.tsx +++ b/ts/components/conversation/pinned-messages/PinnedMessagesBar.dom.stories.tsx @@ -182,10 +182,28 @@ export function Variants(): React.JSX.Element { title="Poll" message={{ poll: { question: `${SHORT_TEXT}?` } }} /> - - + + - + ); } diff --git a/ts/components/conversation/pinned-messages/PinnedMessagesBar.dom.tsx b/ts/components/conversation/pinned-messages/PinnedMessagesBar.dom.tsx index a51a62f055..a8114566c0 100644 --- a/ts/components/conversation/pinned-messages/PinnedMessagesBar.dom.tsx +++ b/ts/components/conversation/pinned-messages/PinnedMessagesBar.dom.tsx @@ -191,7 +191,7 @@ function TabsList(props: { return ( - {props.pins.toReversed().map((pin, pinIndex) => { + {props.pins.map((pin, pinIndex) => { return ( ; } - // 3. And everything else... + // 3. Check specific types of messages (before checking attachments) + if (message.payment) { + text ??= i18n('icu:PinnedMessagesBar__MessagePreview__Text--Payment'); + icon ??= { + symbol: 'creditcard', + label: i18n( + 'icu:PinnedMessagesBar__MessagePreview__SymbolLabel--Payment' + ), + }; + } else if (message.poll != null) { + text ??= ; + icon ??= { + symbol: 'poll', + label: i18n('icu:PinnedMessagesBar__MessagePreview__SymbolLabel--Poll'), + }; + } else if (message.sticker) { + text ??= i18n('icu:PinnedMessagesBar__MessagePreview__Text--Sticker'); + icon ??= { + symbol: 'sticker', + label: i18n( + 'icu:PinnedMessagesBar__MessagePreview__SymbolLabel--Sticker' + ), + }; + } else if (message.contact != null) { + const name = message.contact.name ?? i18n('icu:unknownContact'); + text ??= ; + icon ??= { + symbol: 'person-circle', + label: name, + }; + } + + // 4. Check attachments (make sure to check types like sticker/contact first) if (message.attachment != null) { if (message.attachment.type === 'image') { text ??= i18n('icu:PinnedMessagesBar__MessagePreview__Text--Photo'); @@ -426,35 +458,6 @@ function getMessagePreview(i18n: LocalizerType, message: PinMessage): Preview { } else { throw missingCaseError(message.attachment); } - } else if (message.contact != null) { - const name = message.contact.name ?? i18n('icu:unknownContact'); - text ??= ; - icon ??= { - symbol: 'person-circle', - label: name, - }; - } else if (message.payment) { - text ??= i18n('icu:PinnedMessagesBar__MessagePreview__Text--Payment'); - icon ??= { - symbol: 'creditcard', - label: i18n( - 'icu:PinnedMessagesBar__MessagePreview__SymbolLabel--Payment' - ), - }; - } else if (message.poll != null) { - text ??= ; - icon ??= { - symbol: 'poll', - label: i18n('icu:PinnedMessagesBar__MessagePreview__SymbolLabel--Poll'), - }; - } else if (message.sticker) { - text ??= i18n('icu:PinnedMessagesBar__MessagePreview__Text--Sticker'); - icon ??= { - symbol: 'sticker', - label: i18n( - 'icu:PinnedMessagesBar__MessagePreview__SymbolLabel--Sticker' - ), - }; } return { icon, text }; diff --git a/ts/messageModifiers/PinnedMessages.preload.ts b/ts/messageModifiers/PinnedMessages.preload.ts index c7a583b9c5..d85902d937 100644 --- a/ts/messageModifiers/PinnedMessages.preload.ts +++ b/ts/messageModifiers/PinnedMessages.preload.ts @@ -16,6 +16,7 @@ import { pinnedMessagesCleanupService } from '../services/expiring/pinnedMessage import { drop } from '../util/drop.std.js'; import type { AppendPinnedMessageResult } from '../sql/server/pinnedMessages.std.js'; import * as Errors from '../types/errors.std.js'; +import { isGiftBadge } from '../state/selectors/message.preload.js'; const { AccessRequired } = Proto.AccessControl; const { Role } = Proto.Member; @@ -209,6 +210,8 @@ function validatePinnedMessageTarget( target: MessageModifierTarget, sourceAci: AciString ): { error: string } | null { + const message = target.targetMessage.attributes; + if (!isValidSenderAciForConversation(target.targetConversation, sourceAci)) { return { error: 'Sender cannot send to target conversation' }; } @@ -217,5 +220,9 @@ function validatePinnedMessageTarget( return { error: 'Sender does not have access to edit group attributes' }; } + if (isGiftBadge(message)) { + return { error: 'Cannot pin gift badge messages' }; + } + return null; } diff --git a/ts/sql/server/pinnedMessages.std.ts b/ts/sql/server/pinnedMessages.std.ts index 639900513b..5a23bc90df 100644 --- a/ts/sql/server/pinnedMessages.std.ts +++ b/ts/sql/server/pinnedMessages.std.ts @@ -64,7 +64,7 @@ export function getPinnedMessagesPreloadDataForConversation( const [query, params] = sql` SELECT * FROM pinnedMessages WHERE conversationId = ${conversationId} - ORDER BY pinnedAt DESC + ORDER BY pinnedAt ASC `; return db diff --git a/ts/state/selectors/message.preload.ts b/ts/state/selectors/message.preload.ts index 3ffa41aa0e..128f71d4dc 100644 --- a/ts/state/selectors/message.preload.ts +++ b/ts/state/selectors/message.preload.ts @@ -946,7 +946,7 @@ export const getPropsForMessage = ( canDownload: canDownload(message, conversationSelector), canEndPoll: canEndPoll(message), canForward: canForward(message), - canPinMessages: canPinMessages(conversation), + canPinMessage: canPinMessage(conversation, message), canReact: canReact(message, ourConversationId, conversationSelector), canReply: canReply(message, ourConversationId, conversationSelector), canRetry: hasErrors(message), @@ -2414,6 +2414,19 @@ export function canPinMessages(conversation: ConversationType): boolean { return conversation.type === 'direct' || canEditGroupInfo(conversation); } +export function canPinMessage( + conversation: ConversationType, + message: ReadonlyMessageAttributesType +): boolean { + if (!canPinMessages(conversation)) { + return false; + } + if (isGiftBadge(message)) { + return false; + } + return true; +} + function getHasMaxPinnedMessages( pinnedMessagesMessageIds: ReadonlyArray ) { diff --git a/ts/state/smart/PinnedMessagesBar.preload.tsx b/ts/state/smart/PinnedMessagesBar.preload.tsx index f77e491573..2ed521416a 100644 --- a/ts/state/smart/PinnedMessagesBar.preload.tsx +++ b/ts/state/smart/PinnedMessagesBar.preload.tsx @@ -132,6 +132,10 @@ function getPinSender(props: MessagePropsType): PinSender { }; } +function getLastPinId(pins: ReadonlyArray): PinnedMessageId | null { + return pins.at(-1)?.id ?? null; +} + function getPrevPinId( pins: ReadonlyArray, pinnedMessageId: PinnedMessageId @@ -375,26 +379,45 @@ export const SmartPinnedMessagesBar = memo(function SmartPinnedMessagesBar() { useConversationsActions(); const [current, setCurrent] = useState(() => { - return pins.at(0)?.id ?? null; + return getLastPinId(pins); }); - const isCurrentOutOfDate = useMemo(() => { + const [prevPins, setPrevPins] = useState(pins); + if (pins !== prevPins) { + // Needed for `expectedCurrent` which might update `current` in the same render + setPrevPins(pins); + } + + const expectedCurrent = useMemo(() => { + const latestPinId = getLastPinId(pins); + + // If `current` is null, use the latest pin id if we have one. if (current == null) { - if (pins.length > 0) { - return true; - } - return false; + return latestPinId; } - const hasMatch = pins.some(pin => { - return pin.id === current; - }); + // If `current` is already the latest pin id, leave it. + if (current === latestPinId) { + return current; + } - return !hasMatch; - }, [current, pins]); + // Update `current` if it no longer exists. + const hasCurrent = pins.some(pin => pin.id === current); + if (!hasCurrent) { + return latestPinId; + } - if (isCurrentOutOfDate) { - setCurrent(pins.at(0)?.id ?? null); + // Update `current` if it was previously the latest and there's a new latest. + const prevLatestPinId = getLastPinId(prevPins); + if (prevLatestPinId === current && latestPinId != null) { + return latestPinId; + } + + return current; + }, [current, pins, prevPins]); + + if (current !== expectedCurrent) { + setCurrent(expectedCurrent); } const handleCurrentChange = useCallback( @@ -410,9 +433,10 @@ export const SmartPinnedMessagesBar = memo(function SmartPinnedMessagesBar() { if (current == null) { return; } - const prevPinId = getPrevPinId(pins, current); - if (prevPinId != null) { - setCurrent(prevPinId); + + const updatedCurrent = getPrevPinId(pins, current) ?? getLastPinId(pins); + if (updatedCurrent != null) { + setCurrent(updatedCurrent); } }, [scrollToMessage, conversationId, pins, current]