From a02afae472ba07bcd036ad79eed5a7f965855222 Mon Sep 17 00:00:00 2001 From: Simon Hamp Date: Fri, 23 Oct 2026 18:16:37 +0100 Subject: [PATCH 1/4] feat: pin messages in orgs Maintainers can pin up to 25 messages in an org. Everyone in the org sees pinned messages, including people added without the history, and can reply and react to them. A one-line banner at the top of the thread opens a panel with each pinned message. Co-Authored-By: Claude Opus 5.5 --- KNOWLEDGE_BASE.md | 8 +- app/Actions/DeleteMessagePin.php | 34 + app/Actions/UpdateMessagePin.php | 35 + app/Actions/UpdateMessageReactionsVersion.php | 5 +- app/Http/Controllers/MessagePinController.php | 38 + app/Http/Requests/DeleteMessagePinRequest.php | 41 + app/Http/Requests/UpdateMessagePinRequest.php | 44 + app/Http/Resources/OrgThreadResource.php | 7 +- app/Models/Conversation.php | 5 + app/Models/Message.php | 10 +- app/Queries/ConversationThreadQuery.php | 10 + app/Queries/PinnedMessagesQuery.php | 31 + app/Rules/RepliableMessage.php | 6 +- database/factories/MessageFactory.php | 7 + ...120712_add_pinned_at_to_messages_table.php | 24 + .../components/chat/conversation-thread.tsx | 159 +++- .../js/components/chat/message-parts.tsx | 181 +++- .../js/components/chat/message-reactions.tsx | 36 +- .../js/components/chat/pinned-messages.tsx | 859 ++++++++++++++++++ resources/js/lib/chat.ts | 208 ++++- resources/js/lib/leaving-rows.ts | 30 + resources/js/lib/pinned-messages.ts | 100 ++ resources/js/types/conversation.ts | 7 + routes/web.php | 10 + .../Api/OrgMessageControllerTest.php | 18 + .../MessageImageControllerTest.php | 12 + .../Controllers/MessagePinControllerTest.php | 139 +++ .../MessageReactionControllerTest.php | 18 + .../Controllers/OrgMessageControllerTest.php | 71 ++ .../Controllers/UserTimeoutControllerTest.php | 7 +- tests/Feature/RateLimitTest.php | 9 + tests/Unit/ChatNavigation.test.tsx | 2 +- tests/Unit/LeavingRows.test.ts | 61 ++ tests/Unit/PinnedMessages.test.ts | 136 +++ tests/Unit/RemoteNotifications.test.tsx | 2 +- tests/Unit/RemotePreferences.test.tsx | 2 +- 36 files changed, 2286 insertions(+), 86 deletions(-) create mode 100644 app/Actions/DeleteMessagePin.php create mode 100644 app/Actions/UpdateMessagePin.php create mode 100644 app/Http/Controllers/MessagePinController.php create mode 100644 app/Http/Requests/DeleteMessagePinRequest.php create mode 100644 app/Http/Requests/UpdateMessagePinRequest.php create mode 100644 app/Queries/PinnedMessagesQuery.php create mode 100644 database/migrations/2026_10_23_120712_add_pinned_at_to_messages_table.php create mode 100644 resources/js/components/chat/pinned-messages.tsx create mode 100644 resources/js/lib/leaving-rows.ts create mode 100644 resources/js/lib/pinned-messages.ts create mode 100644 tests/Feature/Controllers/MessagePinControllerTest.php create mode 100644 tests/Unit/LeavingRows.test.ts create mode 100644 tests/Unit/PinnedMessages.test.ts diff --git a/KNOWLEDGE_BASE.md b/KNOWLEDGE_BASE.md index 8516a18c6..516a50cc5 100644 --- a/KNOWLEDGE_BASE.md +++ b/KNOWLEDGE_BASE.md @@ -374,11 +374,11 @@ Sources: [Remotes setting](resources/js/components/remotes-setting.tsx), [contac An **Org** is a private group chat within Remotes, not a synchronized GitHub organization. Create one with other eligible developers; a group can contain up to **100 people including you**. Members need Remotes enabled, must follow the person adding them (unless that person is an admin), and must have no block with anyone in the org. A group's optional name can be up to **60 characters**. -The creator starts as a **Maintainer**, and invited people start as **Contributors**. Maintainers can rename the group, add or remove members, and change roles. Contributors can read and send messages. When adding members, a maintainer can share existing history or only new messages. Members can leave or stash the group for themselves. At least one maintainer remains while the group has members: the last maintainer cannot be demoted, and if they leave, the longest-standing remaining member takes over. An org that everyone has left is deleted. +The creator starts as a **Maintainer**, and invited people start as **Contributors**. Maintainers can rename the group, add or remove members, change roles, and pin up to **25 messages**. Contributors can read and send messages. When adding members, a maintainer can share existing history or only new messages. Members can leave or stash the group for themselves. At least one maintainer remains while the group has members: the last maintainer cannot be demoted, and if they leave, the longest-standing remaining member takes over. An org that everyone has left is deleted. -Group messages support the same text, images, voice notes, replies, link cards, reactions, and typing indicators as direct messages. A member's read position shows only when both you and that member have pull receipts on. Any new message pops the org back for members who stashed it. Membership and history access determine what a person can read. +Group messages support the same text, images, voice notes, replies, link cards, reactions, and typing indicators as direct messages. A member's read position shows only when both you and that member have pull receipts on. Any new message pops the org back for members who stashed it. Membership and history access determine what a person can read, except pinned messages: on the web, every member sees those in the pinned messages bar at the top of the chat, including people added without the history, and can reply and react to them. -Sources: [group creation](app/Http/Requests/CreateOrgRequest.php), [invitation eligibility](app/Queries/RemoteCandidatesQuery.php), [history sharing](app/Actions/CreateOrgMembers.php), [role changes](app/Http/Requests/UpdateOrgMemberRequest.php), [leaving a group](app/Actions/DeleteOrgMember.php). +Sources: [group creation](app/Http/Requests/CreateOrgRequest.php), [invitation eligibility](app/Queries/RemoteCandidatesQuery.php), [history sharing](app/Actions/CreateOrgMembers.php), [pinning](app/Http/Requests/UpdateMessagePinRequest.php), [role changes](app/Http/Requests/UpdateOrgMemberRequest.php), [leaving a group](app/Actions/DeleteOrgMember.php). ### Muting and blocking @@ -555,7 +555,7 @@ Posts awaiting automated checks stay pending if those checks fail; a failed chec Before you first commit, reply, quote, or schedule a commit, the web composer shows the commit rules, and keeps showing them on every attempt until you send with "Got it, commit": no engagement bait, no low-effort posts, no AI-generated posts, no spam or self-promotion, and no harassment, hate, or private information, followed by a link to the terms. Your first submission must come from the web so you can accept these rules; terminal, GitHub Action, and app posting become available afterwards. -During a **timeout**, you can still read and react, but cannot commit, reply, quote, schedule, send remotes, or start, rename, or add people to orgs from any surface; each attempt says how long is left and why. Scheduled commits wait until the timeout ends. Like bans, timeouts belong to the GitHub account: deleting the /dev/null account and signing in again does not end a timeout or erase its history. +During a **timeout**, you can still read and react, but cannot commit, reply, quote, schedule, send remotes, or start, rename, add people to, or pin messages in orgs from any surface; each attempt says how long is left and why. Scheduled commits wait until the timeout ends. Like bans, timeouts belong to the GitHub account: deleting the /dev/null account and signing in again does not end a timeout or erase its history. A **ban** sends you to a page explaining why, where you can only sign out or delete your account; the app refuses everything but deleting the account, and the terminal refuses posting. A ban belongs to the GitHub account, so deleting the /dev/null account and signing in again does not lift it. Banning someone cancels their scheduled commits, so lifting the ban later does not publish them all at once. diff --git a/app/Actions/DeleteMessagePin.php b/app/Actions/DeleteMessagePin.php new file mode 100644 index 000000000..b92e9e502 --- /dev/null +++ b/app/Actions/DeleteMessagePin.php @@ -0,0 +1,34 @@ +whereKey($message->conversation_id)->lockForUpdate()->first(); + + $unpinned = Message::query()->whereKey($message->id)->whereNotNull('pinned_at')->toBase()->update(['pinned_at' => null]); + + if ($unpinned > 0) { + $this->updateMessageReactionsVersion->handle($message); + } + }); + } +} diff --git a/app/Actions/UpdateMessagePin.php b/app/Actions/UpdateMessagePin.php new file mode 100644 index 000000000..d21279f4b --- /dev/null +++ b/app/Actions/UpdateMessagePin.php @@ -0,0 +1,35 @@ +whereKey($message->conversation_id)->lockForUpdate()->first(); + + $pinned = Message::query()->whereKey($message->id)->whereNull('pinned_at')->toBase()->update(['pinned_at' => now()]); + + if ($pinned > 0) { + $this->updateMessageReactionsVersion->handle($message); + } + }); + } +} diff --git a/app/Actions/UpdateMessageReactionsVersion.php b/app/Actions/UpdateMessageReactionsVersion.php index 012ef9b36..c0d8b5d26 100644 --- a/app/Actions/UpdateMessageReactionsVersion.php +++ b/app/Actions/UpdateMessageReactionsVersion.php @@ -11,8 +11,9 @@ { /** * Polls ask for the reactions changed since a version, so each change takes the conversation's next version and - * marks its message with it. Bumping the conversation locks its row until the change commits, keeping versions in - * the order they become visible. + * marks its message with it. Pinning and unpinning move the version too, so polls hand an org's pinned messages + * over again. Bumping the conversation locks its row until the change commits, keeping versions in the order they + * become visible. */ public function handle(Message $message): void { diff --git a/app/Http/Controllers/MessagePinController.php b/app/Http/Controllers/MessagePinController.php new file mode 100644 index 000000000..c4940d2a5 --- /dev/null +++ b/app/Http/Controllers/MessagePinController.php @@ -0,0 +1,38 @@ +handle($message); + + $org = $message->conversation()->firstOrFail(); + + return response()->json(['pinned' => $pinnedMessages->get($org), 'reactions_version' => $org->reactions_version]); + } + + public function destroy(DeleteMessagePinRequest $request, Message $message, DeleteMessagePin $action, PinnedMessagesQuery $pinnedMessages): JsonResponse + { + $action->handle($message); + + $org = $message->conversation()->firstOrFail(); + + return response()->json(['pinned' => $pinnedMessages->get($org), 'reactions_version' => $org->reactions_version]); + } +} diff --git a/app/Http/Requests/DeleteMessagePinRequest.php b/app/Http/Requests/DeleteMessagePinRequest.php new file mode 100644 index 000000000..6b051bfa4 --- /dev/null +++ b/app/Http/Requests/DeleteMessagePinRequest.php @@ -0,0 +1,41 @@ +user(); + assert($user instanceof User); + + $message = $this->route('message'); + assert($message instanceof Message); + + $org = $message->conversation; + + return match (true) { + ! $user->remotes_enabled || ! $message->isVisibleTo($user) || ! $org->isOrg() => Response::denyAsNotFound(), + ! $org->isMaintainedBy($user) => Response::deny('Only maintainers can unpin messages.'), + default => Response::allow(), + }; + } + + /** + * @return array{} + */ + public function rules(): array + { + return []; + } +} diff --git a/app/Http/Requests/UpdateMessagePinRequest.php b/app/Http/Requests/UpdateMessagePinRequest.php new file mode 100644 index 000000000..462b855f0 --- /dev/null +++ b/app/Http/Requests/UpdateMessagePinRequest.php @@ -0,0 +1,44 @@ +user(); + assert($user instanceof User); + + $message = $this->route('message'); + assert($message instanceof Message); + + $org = $message->conversation; + + return match (true) { + ! $user->remotes_enabled || $message->event !== null || ! $message->isVisibleTo($user) || ! $org->isOrg() => Response::denyAsNotFound(), + ! $org->isMaintainedBy($user) => Response::deny('Only maintainers can pin messages.'), + $message->pinned_at === null && $org->messages()->whereNotNull('pinned_at')->count() >= Conversation::MAX_PINNED_MESSAGES => Response::deny('An org can pin up to '.Conversation::MAX_PINNED_MESSAGES.' messages. Unpin one first.'), + default => Response::allow(), + }; + } + + /** + * @return array{} + */ + public function rules(): array + { + return []; + } +} diff --git a/app/Http/Resources/OrgThreadResource.php b/app/Http/Resources/OrgThreadResource.php index 1aaf1864a..5b3313fa4 100644 --- a/app/Http/Resources/OrgThreadResource.php +++ b/app/Http/Resources/OrgThreadResource.php @@ -11,8 +11,9 @@ /** * An org as the web's thread loads and polls it: its name, a page of messages from where the user's history starts, - * the reactions changed since the version a poll last saw, its members with their roles and, where both share pulls, - * how far each has pulled, who is typing, and the user's own role. + * the reactions changed since the version a poll last saw, the messages it pinned unless they are unchanged since that + * version, its members with their roles and, where both share pulls, how far each has pulled, who is typing, and the + * user's own role. * * @phpstan-import-type OrgThread from ConversationThreadQuery * @@ -22,6 +23,7 @@ * has_older: OrgThread['has_older'], * reactions_version: OrgThread['reactions_version'], * reaction_changes: OrgThread['reaction_changes'], + * pinned: OrgThread['pinned'], * read_through_id: OrgThread['read_through_id'], * members: OrgThread['members'], * typing: OrgThread['typing'], @@ -46,6 +48,7 @@ public function toArray(Request $request): array ...$thread, 'org' => ['id' => $thread['org']->id, 'name' => $thread['org']->name], 'messages' => MessageResource::collection($thread['messages']), + 'pinned' => $thread['pinned'] === null ? null : MessageResource::collection($thread['pinned']), 'members' => $thread['members']->map(fn (array $member): array => [ 'role' => $member['role'], 'read_through_id' => $member['read_through_id'], diff --git a/app/Models/Conversation.php b/app/Models/Conversation.php index 6996af619..597044bdc 100644 --- a/app/Models/Conversation.php +++ b/app/Models/Conversation.php @@ -53,6 +53,11 @@ final class Conversation extends Model */ public const int MAX_MEMBERS = 100; + /** + * An org pins up to twenty-five messages at a time. + */ + public const int MAX_PINNED_MESSAGES = 25; + /** * Two people share exactly one conversation, whichever of them started it. */ diff --git a/app/Models/Message.php b/app/Models/Message.php index 9c9ecd543..8f72c2180 100644 --- a/app/Models/Message.php +++ b/app/Models/Message.php @@ -9,6 +9,7 @@ use Carbon\CarbonInterface; use Database\Factories\MessageFactory; use Illuminate\Database\Eloquent\Attributes\Visible; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; @@ -31,6 +32,7 @@ * @property-read OrgEvent|null $event * @property-read string|null $subject_id * @property-read int $reactions_version + * @property-read CarbonInterface|null $pinned_at * @property-read User $user * @property-read User|null $subject * @property-read Message|null $replyTo @@ -42,7 +44,7 @@ * @property-read CarbonInterface $created_at * @property-read CarbonInterface $updated_at */ -#[Visible(['id', 'user_id', 'client_id', 'body', 'reply_to_id', 'mentions', 'event', 'subject_id', 'created_at', 'user', 'subject', 'linkPreview', 'images', 'voice', 'reactions'])] +#[Visible(['id', 'user_id', 'client_id', 'body', 'reply_to_id', 'mentions', 'event', 'subject_id', 'pinned_at', 'created_at', 'user', 'subject', 'linkPreview', 'images', 'voice', 'reactions'])] final class Message extends Model { /** @use HasFactory */ @@ -73,6 +75,7 @@ public function casts(): array 'event' => OrgEvent::class, 'subject_id' => 'string', 'reactions_version' => 'integer', + 'pinned_at' => 'datetime', 'created_at' => 'datetime', 'updated_at' => 'datetime', ]; @@ -147,14 +150,15 @@ public function reactions(): HasMany } /** - * Only the people in the conversation see a message, and in an org only from where their history starts. + * Only the people in the conversation see a message, and in an org only from where their history starts, unless + * the org pinned it for everyone. */ public function isVisibleTo(User $user): bool { return ConversationParticipant::query() ->where('conversation_id', $this->conversation_id) ->where('user_id', $user->id) - ->where('visible_from_id', '<', $this->id) + ->when($this->pinned_at === null, fn (Builder $query): Builder => $query->where('visible_from_id', '<', $this->id)) ->exists(); } diff --git a/app/Queries/ConversationThreadQuery.php b/app/Queries/ConversationThreadQuery.php index c993cab90..5209f9e84 100644 --- a/app/Queries/ConversationThreadQuery.php +++ b/app/Queries/ConversationThreadQuery.php @@ -39,6 +39,7 @@ * has_older: bool, * reactions_version: int, * reaction_changes: Collection, + * pinned: Collection|null, * read_through_id: int, * members: Collection, * typing: array, @@ -53,6 +54,11 @@ { public const int PAGE = 50; + public function __construct(private PinnedMessagesQuery $pinnedMessages) + { + // + } + /** * A one-to-one conversation. Reads only show as pulled when both people share them, and only once the partner * has merged the conversation. @@ -90,6 +96,9 @@ public function get(User $user, User $partner, ?int $after = null, ?int $before /** * An org the user is in, from where their history starts. Each member's reads show only when both share pulls. + * The messages the org pinned come whatever history the user was given, and polls get them again only once the + * reactions version moved past the one they sent, as pinning and unpinning move it. They are null when unchanged, + * and on older pages, where the thread keeps the ones it has. * * @return OrgThread */ @@ -118,6 +127,7 @@ public function org(User $user, Conversation $org, ?int $after = null, ?int $bef 'has_older' => $hasOlder, 'reactions_version' => $org->reactions_version, 'reaction_changes' => $this->reactionChanges($org, $mine->visible_from_id, $reactionsAfter), + 'pinned' => $before === null && ($reactionsAfter === null || $org->reactions_version > $reactionsAfter) ? $this->pinnedMessages->get($org) : null, 'read_through_id' => $mine->read_through_id, 'members' => $participants->map(fn (ConversationParticipant $participant): array => [ 'user' => $participant->user, diff --git a/app/Queries/PinnedMessagesQuery.php b/app/Queries/PinnedMessagesQuery.php new file mode 100644 index 000000000..8628e7339 --- /dev/null +++ b/app/Queries/PinnedMessagesQuery.php @@ -0,0 +1,31 @@ + + */ + public function get(Conversation $org): Collection + { + return $org->messages() + ->whereNotNull('pinned_at') + ->with(['images', 'voice', 'reactions', 'user']) + ->orderByDesc('pinned_at') + ->orderByDesc('id') + ->get() + ->makeHidden('reply_to_id') + ->toBase(); + } +} diff --git a/app/Rules/RepliableMessage.php b/app/Rules/RepliableMessage.php index c6b34bbcc..b793cecc3 100644 --- a/app/Rules/RepliableMessage.php +++ b/app/Rules/RepliableMessage.php @@ -7,9 +7,11 @@ use App\Models\Conversation; use Closure; use Illuminate\Contracts\Validation\ValidationRule; +use Illuminate\Database\Eloquent\Builder; /** - * A reply goes to a message in the same conversation, within the history the sender can see, and never to an event. + * A reply goes to a message in the same conversation, within the history the sender can see or pinned for everyone in + * it, and never to an event. */ final readonly class RepliableMessage implements ValidationRule { @@ -24,7 +26,7 @@ public function validate(string $attribute, mixed $value, Closure $fail): void && is_numeric($value) && $this->conversation->messages() ->whereKey((int) $value) - ->where('id', '>', $this->visibleFrom) + ->where(fn (Builder $query): Builder => $query->where('id', '>', $this->visibleFrom)->orWhereNotNull('pinned_at')) ->whereNull('event') ->exists(); diff --git a/database/factories/MessageFactory.php b/database/factories/MessageFactory.php index e89b9647e..0c93341fa 100644 --- a/database/factories/MessageFactory.php +++ b/database/factories/MessageFactory.php @@ -27,4 +27,11 @@ public function definition(): array 'body' => fake()->sentence(), ]; } + + public function pinned(): self + { + return $this->state(fn (array $attributes): array => [ + 'pinned_at' => now(), + ]); + } } diff --git a/database/migrations/2026_10_23_120712_add_pinned_at_to_messages_table.php b/database/migrations/2026_10_23_120712_add_pinned_at_to_messages_table.php new file mode 100644 index 000000000..6eb3d2d50 --- /dev/null +++ b/database/migrations/2026_10_23_120712_add_pinned_at_to_messages_table.php @@ -0,0 +1,24 @@ +timestamp('pinned_at')->nullable(); + + $table->index(['conversation_id', 'pinned_at']); + }); + } +}; diff --git a/resources/js/components/chat/conversation-thread.tsx b/resources/js/components/chat/conversation-thread.tsx index aeb2c2dea..fcc6649f9 100644 --- a/resources/js/components/chat/conversation-thread.tsx +++ b/resources/js/components/chat/conversation-thread.tsx @@ -18,12 +18,15 @@ import { } from 'react'; import { toast } from 'sonner'; import { + LiftedMessage, MessageImages, MessageLinkCard, MessageText, BubbleActions, Repliable, ReplyGhost, + replyOf, + timeOf, } from '@/components/chat/message-parts'; import type { Author } from '@/components/chat/message-parts'; import { @@ -34,6 +37,7 @@ import { } from '@/components/chat/message-reactions'; import { MessageStatusGlyph } from '@/components/chat/message-status'; import { EventLine, typingText } from '@/components/chat/org'; +import { PinnedBanner, usePinMessage } from '@/components/chat/pinned-messages'; import { VoiceNote, VoiceNoteComposer } from '@/components/chat/voice-note'; import { ImageDropOverlay, @@ -257,6 +261,33 @@ export function ConversationThread({ } }, [messages, isLoaded, user.id, mutedUntil, isVisible]); + const pinnedMessages = thread?.pinned ?? noMessages; + const canPin = isOrg && thread?.role === 'maintainer' && isOpen; + // Polls hand over a new list whenever any reaction changes, so the bubbles only hear about the pins themselves. + const pinnedKey = pinnedMessages.map((message) => message.id).join(); + const pinnedIds = useMemo( + () => new Set(pinnedKey === '' ? [] : pinnedKey.split(',').map(Number)), + [pinnedKey], + ); + // Only who wrote the pinned messages matters for names, so a reaction on one doesn't re-render every row. + const pinnedAuthorIds = [ + ...new Set(pinnedMessages.map((message) => message.user_id)), + ] + .toSorted() + .join(); + const pinnedAuthors = useMemo( + () => pinnedMessages.flatMap((message) => message.user ?? []), + // eslint-disable-next-line react-hooks/exhaustive-deps + [pinnedAuthorIds], + ); + const pins = usePinMessage(target, user.id); + const { pin, unpin } = pins; + const togglePin = useCallback( + (message: Message, isPinned: boolean) => + isPinned ? unpin(message) : pin(message), + [pin, unpin], + ); + const updateScrollPosition = useCallback(() => { setIsScrolledBack( scroller.current !== null && @@ -299,7 +330,8 @@ export function ConversationThread({ : (members?.find((member) => member.user.id === userId) ?.user ?? messages.find((message) => message.user_id === userId) - ?.user); + ?.user ?? + pinnedAuthors.find((person) => person.id === userId)); return { name: person?.name ?? 'Someone', @@ -307,7 +339,7 @@ export function ConversationThread({ isMe: false, }; }, - [user, current, members, messages], + [user, current, members, messages, pinnedAuthors], ); useEffect(() => () => jumpRequest.current?.abort(), []); @@ -622,6 +654,26 @@ export function ConversationThread({ > {header} + {isOrg && ( + { + if (!window.matchMedia('(pointer: coarse)').matches) { + draft.input.current?.focus({ preventScroll: true }); + } + }} + /> + )} + {pins.dialog} +
0 && + (isCompact ? 'pt-12' : 'pt-13'), )} > {thread.isLoadingOlder && ( @@ -702,6 +757,8 @@ export function ConversationThread({ onReply={isOpen ? replyTo : null} usage={usage} onReact={isOpen ? react : null} + pinnedIds={pinnedIds} + onPin={canPin ? togglePin : null} /> ))} @@ -903,6 +960,8 @@ const ThreadItem = memo(function ThreadItem({ onReply, usage, onReact, + pinnedIds, + onPin, }: { item: Item; pulls: Pulls; @@ -916,6 +975,11 @@ const ThreadItem = memo(function ThreadItem({ onReply: ((reply: MessageReply) => void) | null; usage: ReactionUsage[]; onReact: ((messageId: number, emoji: string) => void) | null; + pinnedIds: Set; + /** + * Pins or unpins a message, for maintainers only. + */ + onPin: ((message: Message, isPinned: boolean) => void) | null; }) { if (item.kind === 'day') { return ( @@ -1003,6 +1067,11 @@ const ThreadItem = memo(function ThreadItem({ onReply={onReply} usage={usage} onReact={onReact} + isPinned={ + bubble.kind === 'message' && + pinnedIds.has(bubble.message.id) + } + onPin={onPin} /> ); })} @@ -1024,6 +1093,8 @@ function BubbleWithMeta({ onReply, usage, onReact, + isPinned, + onPin, }: { bubble: Bubble; isMine: boolean; @@ -1038,6 +1109,8 @@ function BubbleWithMeta({ onReply: ((reply: MessageReply) => void) | null; usage: ReactionUsage[]; onReact: ((messageId: number, emoji: string) => void) | null; + isPinned: boolean; + onPin: ((message: Message, isPinned: boolean) => void) | null; }) { const [isReacting, setIsReacting] = useState(false); const [isTrayOpen, setIsTrayOpen] = useState(false); @@ -1082,15 +1155,20 @@ function BubbleWithMeta({ const side = isMine ? 'mine' : 'theirs'; const replyToThis = onReply !== null && bubble.kind === 'message' - ? () => - onReply({ - id: bubble.message.id, - user_id: bubble.message.user_id, - body: bubble.message.body, - images_count: images.length, - has_voice: voice !== null, - }) + ? () => onReply(replyOf(bubble.message)) : null; + // Only org messages pin. Contributors see the pin of a pinned one, and can't press it. + const pin = + target.kind !== 'org' || bubble.kind !== 'message' + ? null + : onPin !== null + ? { + isPinned, + onToggle: () => onPin(bubble.message, isPinned), + } + : isPinned + ? { isPinned, onToggle: null } + : null; const reactions = bubble.kind === 'message' ? bubble.message.reactions : undefined; @@ -1141,6 +1219,12 @@ function BubbleWithMeta({ isReacting={isTrayOpen} reactRef={reactButton} trayId={trayId} + pin={pin} + // Whichever part of the message comes first sits under the reply's quote. + isBelowReply={ + reply !== null && + (images.length === 0 || (message.body === '' && voice === null)) + } /> ); @@ -1155,9 +1239,16 @@ function BubbleWithMeta({ } className={cn( 'relative flex max-w-[85%] min-w-0 flex-col gap-1', + // Org bubbles leave room for a third action, the pin, whether or not it shows, so pushing a + // message or a pin arriving never rewraps one. isCompact - ? 'max-w-[88%] pointer-fine:max-w-[calc(100%-3.5rem)]' - : 'sm:max-w-[78%]', + ? target.kind === 'org' + ? 'max-w-[88%] pointer-fine:max-w-[calc(100%-5.5rem)]' + : 'max-w-[88%] pointer-fine:max-w-[calc(100%-3.5rem)]' + : cn('sm:max-w-[78%]', { + 'pointer-fine:max-sm:max-w-[calc(100%-5rem)]': + target.kind === 'org', + }), isMine ? 'items-end' : 'items-start', { '[-webkit-touch-callout:none] pointer-coarse:select-none': @@ -1242,7 +1333,7 @@ function BubbleWithMeta({
- {reactToThis !== null && ( + {bubble.kind === 'message' && reactToThis !== null && ( - - - ) : message.body === '' ? null : ( -
- -
- ) + } onReact={reactToThis} + pin={ + pin?.onToggle + ? { isPinned, onToggle: pin.onToggle } + : null + } /> )} @@ -1395,13 +1469,6 @@ function MetaLine({ ); } -function timeOf(date: string): string { - return new Date(date).toLocaleTimeString([], { - hour: '2-digit', - minute: '2-digit', - }); -} - /** * The draft being written to a conversation: its body and images, kept per conversation across pages and the dock, * and the link chosen for its card. diff --git a/resources/js/components/chat/message-parts.tsx b/resources/js/components/chat/message-parts.tsx index b15be6700..604c0758b 100644 --- a/resources/js/components/chat/message-parts.tsx +++ b/resources/js/components/chat/message-parts.tsx @@ -1,9 +1,10 @@ -import { Image, Mic, Reply, SmilePlus } from 'lucide-react'; +import { Image, Mic, Pin, Reply, SmilePlus } from 'lucide-react'; import type { ReactNode, RefObject } from 'react'; import { useEffect, useRef, useState } from 'react'; import { show as showMessageImage } from '@/actions/App/Http/Controllers/MessageImageController'; import MessageLinkPreviewController from '@/actions/App/Http/Controllers/MessageLinkPreviewController'; import { preloadEmojiGroups } from '@/components/chat/message-reactions'; +import { VoiceNote } from '@/components/chat/voice-note'; import { CommitBody } from '@/components/commit-body'; import { objectUrlFor } from '@/components/commit-image-picker'; import { CommitImages } from '@/components/commit-images'; @@ -57,6 +58,63 @@ const isUnsent = (images: MessageImage[] | File[]): images is File[] => const downloadUrl = (image: ShownImage): string => showMessageImage.url(image.id, { query: { download: 1 } }); +/** + * A message as the composer quotes it when replying to it. + */ +export function replyOf(message: Message): MessageReply { + return { + id: message.id, + user_id: message.user_id, + body: message.body, + images_count: message.images?.length ?? 0, + has_voice: (message.voice ?? null) !== null, + }; +} + +export function timeOf(date: string): string { + return new Date(date).toLocaleTimeString([], { + hour: '2-digit', + minute: '2-digit', + }); +} + +/** + * The message a long press lifts above the thread: its voice note, or its text. + */ +export function LiftedMessage({ + message, + isMine, +}: { + message: Message; + isMine: boolean; +}) { + const voice = message.voice ?? null; + + if (voice === null && message.body === '') { + return null; + } + + return ( +
+ {voice === null ? ( + + ) : ( + + )} +
+ ); +} + export function MessageImages({ images }: { images: MessageImage[] | File[] }) { const [viewedIndex, setViewedIndex] = useState(null); @@ -105,7 +163,10 @@ export function MessageLinkCard({ message }: { message: Message }) { ); } -function snippetOf(reply: MessageReply): ReactNode { +/** + * A message's first words on one line, or what it carries when it has none. + */ +export function snippetOf(reply: MessageReply): ReactNode { const body = reply.body .replace(/```[\s\S]*?(```|$)/g, ' code ') .replace(/`/g, '') @@ -319,8 +380,10 @@ export function Repliable({ } /** - * Reply and react, just outside the end of the bubble they belong to and vertically centred on it, so they take no - * room in the thread. They show on hover or focus, and stay while the reaction tray under the message is open. + * Pin, reply and react, just outside the end of the bubble they belong to and vertically centred on it, so they take + * no room in the thread. They show on hover or focus, and stay while the reaction tray under the message is open. A + * pinned message's pin always shows, nearest the bubble, so the others fade in beyond it without moving it, and only + * maintainers can press it. */ export function BubbleActions({ isMine, @@ -329,6 +392,8 @@ export function BubbleActions({ isReacting, reactRef, trayId, + pin = null, + isBelowReply = false, }: { isMine: boolean; onReply: (() => void) | null; @@ -336,32 +401,130 @@ export function BubbleActions({ isReacting: boolean; reactRef: RefObject; trayId: string; + pin?: { isPinned: boolean; onToggle: (() => void) | null } | null; + /** + * The reply's quote overlaps the top 10px of the bubble, so the actions centre on what it leaves showing. + */ + isBelowReply?: boolean; }) { - if (onReply === null && onReact === null) { + // The pointer is still on the pin right after pinning, so it doesn't act as Unpin until it leaves, as Follow does. + const [isJustPinned, setIsJustPinned] = useState(false); + const pinButton = useRef(null); + const isPinned = pin?.isPinned ?? false; + const togglePin = pin?.onToggle ?? null; + const isPinShown = isPinned || togglePin !== null; + const pinLabel = isPinned + ? isJustPinned + ? 'Pinned message' + : 'Unpin message' + : 'Pin message'; + + // Only the pointer moving off the pin counts as leaving it: the warning's overlay opening over a resting pointer + // doesn't, so the pin stays safe through the warning too. + useEffect(() => { + if (!isJustPinned) { + return; + } + + const onPointerMove = (event: PointerEvent) => { + const box = pinButton.current?.getBoundingClientRect(); + + if ( + box === undefined || + event.clientX < box.left || + event.clientX > box.right || + event.clientY < box.top || + event.clientY > box.bottom + ) { + setIsJustPinned(false); + } + }; + + document.addEventListener('pointermove', onPointerMove); + + return () => document.removeEventListener('pointermove', onPointerMove); + }, [isJustPinned]); + + if (onReply === null && onReact === null && !isPinShown) { return null; } const button = - 'grid size-7 place-items-center rounded-md text-muted-foreground transition-colors hover:bg-accent hover:text-foreground focus-visible:ring-2 focus-visible:ring-ring focus-visible:outline-none'; + 'grid size-7 place-items-center rounded-md text-muted-foreground hover:bg-accent hover:text-foreground focus-visible:ring-2 focus-visible:ring-ring focus-visible:outline-none'; + const transient = cn( + button, + 'opacity-0 transition-[color,background-color,opacity] group-hover/message:opacity-100 group-has-focus-visible/actions:opacity-100 group-data-open/actions:opacity-100 pointer-coarse:hidden', + ); return (
+ {isPinShown && + (togglePin === null ? ( + + + + ) : ( + + ))} {onReply !== null && ( )} + {pin !== null && !isPicking && ( + + )}
{isPicking && ( setIsPicking(false)}> diff --git a/resources/js/components/chat/pinned-messages.tsx b/resources/js/components/chat/pinned-messages.tsx new file mode 100644 index 000000000..018f37e31 --- /dev/null +++ b/resources/js/components/chat/pinned-messages.tsx @@ -0,0 +1,859 @@ +import { Link } from '@inertiajs/react'; +import { + ChevronDown, + Image, + Pin, + PinOff, + Reply, + SmilePlus, + X, +} from 'lucide-react'; +import type { CSSProperties, RefObject } from 'react'; +import { + useCallback, + useEffect, + useId, + useLayoutEffect, + useMemo, + useRef, + useState, +} from 'react'; +import { toast } from 'sonner'; +import { + LiftedMessage, + MessageText, + replyOf, + snippetOf, + timeOf, +} from '@/components/chat/message-parts'; +import type { Author } from '@/components/chat/message-parts'; +import { + preloadEmojiGroups, + ReactionChips, + ReactionSheet, + ReactionTray, +} from '@/components/chat/message-reactions'; +import { VoiceNote } from '@/components/chat/voice-note'; +import { Button } from '@/components/ui/button'; +import { Checkbox } from '@/components/ui/checkbox'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogTitle, +} from '@/components/ui/dialog'; +import { Label } from '@/components/ui/label'; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from '@/components/ui/popover'; +import { UserAvatar } from '@/components/user-avatar'; +import { UserPreview } from '@/components/user-preview'; +import { ChatRequestError, setMessagePinned, targetKey } from '@/lib/chat'; +import type { ChatTarget } from '@/lib/chat'; +import { withLeavingRows } from '@/lib/leaving-rows'; +import type { ReactionUsage } from '@/lib/reactions'; +import { readStorage, writeStorage } from '@/lib/storage'; +import { cn, formatDayLabel, laneColor } from '@/lib/utils'; +import { show } from '@/routes/user'; +import type { Message, MessageReply, User } from '@/types'; + +const warningDismissedKeyFor = (userId: string): string => + `pin-warning-dismissed:${userId}`; + +/** + * On touch screens each action takes taps across 44px around its 28px look, spaced so those areas don't overlap. + */ +const action = + 'relative grid size-7 place-items-center rounded-md text-muted-foreground transition-colors hover:bg-accent hover:text-foreground focus-visible:ring-2 focus-visible:ring-ring focus-visible:outline-none pointer-coarse:before:absolute pointer-coarse:before:-inset-2'; + +/** + * What a message says in a line of plain text, for titles and labels. + */ +function textOf(message: Message): string { + const images = message.images?.length ?? 0; + + if (message.body !== '') { + return message.body; + } + + if ((message.voice ?? null) !== null) { + return 'Voice note'; + } + + return images > 1 ? `${images} images` : 'Image'; +} + +/** + * Pinning and unpinning, for the thread's bubbles and the pinned panel. Pinning asks first, every time until the user + * says not to, since everyone in the org sees what is pinned, whatever history they were given. Returns the dialog + * that asks, to render once in the thread. + */ +export function usePinMessage(target: ChatTarget, userId: string) { + const key = targetKey(target); + const warningDismissedKey = warningDismissedKeyFor(userId); + const [confirming, setConfirming] = useState(null); + const [isDismissing, setIsDismissing] = useState(false); + const checkboxId = useId(); + + // Resolves to whether the server took it. + const save = useCallback( + (message: Message, isPinned: boolean) => + setMessagePinned(target, message, isPinned).then( + () => true, + (error: unknown) => { + toast.error( + error instanceof ChatRequestError && + error.status === 403 && + error.body.message + ? error.body.message + : isPinned + ? 'Could not pin. Try again.' + : 'Could not unpin. Try again.', + ); + + return false; + }, + ), + // eslint-disable-next-line react-hooks/exhaustive-deps + [key], + ); + + const pin = useCallback( + (message: Message) => { + if (readStorage(warningDismissedKey) === '1') { + void save(message, true); + + return; + } + + setIsDismissing(false); + setConfirming(message); + }, + [warningDismissedKey, save], + ); + + const unpin = useCallback( + (message: Message) => save(message, false), + [save], + ); + + const confirm = () => { + if (confirming === null) { + return; + } + + if (isDismissing) { + writeStorage(warningDismissedKey, '1'); + } + + setConfirming(null); + void save(confirming, true); + }; + + const dialog = ( + { + if (!isOpen) { + setConfirming(null); + } + }} + > + +
+ + Pin for the whole org? + + + Everyone in the org sees pinned messages, including + people added without Share history, who can't read + anything pushed before they joined. + +
+ + setIsDismissing(checked === true) + } + data-test="pin-warning-dismiss" + /> + +
+
+
+ + +
+
+
+ ); + + return { pin, unpin, dialog }; +} + +/** + * The org's pinned messages folded into one row over the top of the thread: the latest one, and how many there are. + * It takes no height of its own and lies over the thread instead of pushing it down, so a pin arriving never moves + * what is being read. Pressing it opens the panel listing them all. When the last one goes, the banner stays until + * the panel has closed. + */ +export function PinnedBanner({ + messages, + user, + authorOf, + boundary, + isCompact, + usage, + onReply, + onReact, + onUnpin, + onEmptied, +}: { + messages: Message[]; + user: User; + authorOf: (userId: string) => Author; + /** + * What the panel stays inside: the thread's scroller, between its header and the composer. + */ + boundary: RefObject; + isCompact: boolean; + usage: ReactionUsage[]; + onReply: ((reply: MessageReply) => void) | null; + onReact: ((messageId: number, emoji: string) => void) | null; + onUnpin: ((message: Message) => Promise) | null; + /** + * Called once the panel has closed because nothing is pinned any more, to hand focus on. + */ + onEmptied: () => void; +}) { + const [isOpen, setIsOpen] = useState(false); + const [isPanelShown, setIsPanelShown] = useState(false); + const [collisionBoundary, setCollisionBoundary] = + useState(null); + const [lastShown, setLastShown] = useState(messages); + const headingId = useId(); + const isEmpty = messages.length === 0; + + if (!isEmpty && messages !== lastShown) { + setLastShown(messages); + } + + if (isEmpty && isOpen) { + setIsOpen(false); + } + + const shown = isEmpty ? lastShown : messages; + const latest = shown.at(0); + + if (latest === undefined || (isEmpty && !isPanelShown)) { + return null; + } + + const author = authorOf(latest.user_id); + const count = shown.length; + + return ( +
+ { + setIsOpen(open); + + if (open) { + setIsPanelShown(true); + setCollisionBoundary(boundary.current); + } + }} + > + + + + { + event.preventDefault(); + (event.currentTarget as HTMLElement).focus(); + }} + onCloseAutoFocus={(event) => { + setIsPanelShown(false); + + // The banner is about to go, so focus can't go back to it. + if (isEmpty) { + event.preventDefault(); + onEmptied(); + } + }} + onEscapeKeyDown={(event) => { + // Escape in a message's reaction tray folds the tray, not the panel. + if ( + event.target instanceof Element && + event.target.closest('[data-reaction-tray]') + ) { + event.preventDefault(); + } + }} + className="flex max-h-[min(var(--radix-popover-content-available-height),calc(var(--visual-viewport-height,100dvh)-2rem))] w-(--radix-popover-trigger-width) flex-col overflow-hidden rounded-none border-x-0 border-t-0 p-0 shadow-[0_12px_32px_-12px_rgb(0_0_0/0.35)]" + data-test="pinned-panel" + > + setIsOpen(false)} + onReply={ + onReply === null + ? null + : (reply) => { + setIsOpen(false); + onReply(reply); + } + } + onReact={onReact} + onUnpin={onUnpin} + /> + + +
+ ); +} + +/** + * The panel's sticky header over its own scrolling list. An unpinned message collapses where it was before it goes. + */ +function PinnedPanel({ + messages, + count, + headingId, + user, + authorOf, + isCompact, + usage, + onClose, + onReply, + onReact, + onUnpin, +}: { + messages: Message[]; + count: number; + headingId: string; + user: User; + authorOf: (userId: string) => Author; + isCompact: boolean; + usage: ReactionUsage[]; + onClose: () => void; + onReply: ((reply: MessageReply) => void) | null; + onReact: ((messageId: number, emoji: string) => void) | null; + onUnpin: ((message: Message) => Promise) | null; +}) { + const [rowsOf, setRowsOf] = useState(messages); + const [rows, setRows] = useState(() => withLeavingRows([], messages)); + const [reactingTo, setReactingTo] = useState(null); + const handedFocus = useRef(new Map()); + + if (rowsOf !== messages) { + setRowsOf(messages); + setRows((current) => withLeavingRows(current, messages)); + } + + return ( + <> +
+

+ Pinned messages +

+ + {count} + + +
+
    + {rows.map(({ item: message, isLeaving }) => ( + + setReactingTo((current) => + isReacting + ? message.id + : current === message.id + ? null + : current, + ) + } + onReply={onReply} + onReact={onReact} + onUnpin={onUnpin} + onNavigate={onClose} + handedFocus={handedFocus} + onGone={() => + setRows((current) => + current.filter( + (row) => + !row.isLeaving || + row.item.id !== message.id, + ), + ) + } + /> + ))} +
+ + ); +} + +/** + * Moves focus off an unpinned message before it goes inert: to the next message's unpin button, or the one before. + * Returns where it went. + */ +function focusNeighbour(button: HTMLElement): HTMLElement | null { + const row = button.closest('li'); + const step = ( + from: Element | null | undefined, + to: 'nextElementSibling' | 'previousElementSibling', + ) => { + let sibling = from?.[to]; + + while (sibling?.hasAttribute('data-leaving')) { + sibling = sibling[to]; + } + + return sibling; + }; + + const neighbour = + ( + step(row, 'nextElementSibling') ?? + step(row, 'previousElementSibling') + )?.querySelector('[data-unpin]') ?? null; + + neighbour?.focus({ preventScroll: true }); + + return neighbour; +} + +/** + * One pinned message: who wrote it and when, its text, a voice note to play or how many images it has, and its + * reactions. Its actions always show. Reacting brings up the quick reactions beside them, or on a touch screen opens + * the sheet over the panel, which stays open behind it. + */ +function PinnedItem({ + message, + isLeaving, + user, + authorOf, + isCompact, + usage, + isReacting, + onReactingChange, + onReply, + onReact, + onUnpin, + onNavigate, + handedFocus, + onGone, +}: { + message: Message; + isLeaving: boolean; + user: User; + authorOf: (userId: string) => Author; + isCompact: boolean; + usage: ReactionUsage[]; + isReacting: boolean; + onReactingChange: (isReacting: boolean) => void; + onReply: ((reply: MessageReply) => void) | null; + onReact: ((messageId: number, emoji: string) => void) | null; + onUnpin: ((message: Message) => Promise) | null; + /** + * Called as a plain click on the author leaves for their profile. + */ + onNavigate: () => void; + /** + * Where focus went as each message was unpinned here, kept until the unpin goes through. + */ + handedFocus: RefObject>; + onGone: () => void; +}) { + const reactButton = useRef(null); + const unpinButton = useRef(null); + const trayId = useId(); + const [isReactingOnTouch, setIsReactingOnTouch] = useState(false); + const author = authorOf(message.user_id); + const isMine = message.user_id === user.id; + const images = message.images?.length ?? 0; + const voice = message.voice ?? null; + const reactions = message.reactions; + const reacted = useMemo( + () => + new Set( + (reactions ?? []) + .filter((reaction) => reaction.user_id === user.id) + .map((reaction) => reaction.emoji), + ), + [reactions, user.id], + ); + const reactToThis = + onReact === null ? null : (emoji: string) => onReact(message.id, emoji); + const avatar = ( + + ); + + // A message back from a failed unpin takes focus back, unless the user has moved on from where it went. + useEffect(() => { + if (isLeaving) { + return; + } + + const neighbour = handedFocus.current.get(message.id); + handedFocus.current.delete(message.id); + + if (neighbour !== undefined && document.activeElement === neighbour) { + unpinButton.current?.focus({ preventScroll: true }); + } + }, [isLeaving, handedFocus, message.id]); + + return ( +
  • { + if ( + isLeaving && + event.target === event.currentTarget && + event.propertyName === 'grid-template-rows' + ) { + onGone(); + } + }} + data-test="pinned-message" + > +
    +
    +
    + {message.user === undefined ? ( + <> + {avatar} + + {author.name} + + + ) : ( + // As in the thread, the author links to their profile with a hovercard. + + { + // The dock stays on every page, so its panel closes as a plain click leaves. + if ( + event.button === 0 && + !event.metaKey && + !event.ctrlKey && + !event.shiftKey && + !event.altKey + ) { + onNavigate(); + } + }} + className="flex min-w-0 items-center gap-2 rounded-sm" + title={author.name} + > + {avatar} + + {author.name} + + + + )} + + + {onReply !== null && ( + + )} + {reactToThis !== null && ( + + )} + {reactToThis !== null && ( + + 0 + } + isMine + onReact={reactToThis} + placement="beside" + /> + + )} + {onUnpin !== null && ( + + )} + +
    + {message.body !== '' && } + {voice !== null && ( +
    + +
    + )} + {images > 0 && ( + + + {images > 1 ? `${images} images` : 'Image'} + + )} + authorOf(userId).name} + onToggle={reactToThis} + /> +
    +
    + + {reactToThis !== null && ( + + } + onReact={reactToThis} + /> + )} +
  • + ); +} + +/** + * Up to three lines of a pinned message, with the rest a press away. The toggle only shows when there is more. + */ +function PinnedText({ message }: { message: Message }) { + const body = useRef(null); + const [isLong, setIsLong] = useState(false); + const [isExpanded, setIsExpanded] = useState(false); + + // Measured before paint, so the toggle is there from the first frame. + useLayoutEffect(() => { + const element = body.current; + + if (element === null || isExpanded) { + return; + } + + const measure = () => + setIsLong(element.scrollHeight > element.clientHeight + 1); + const observer = new ResizeObserver(measure); + + measure(); + observer.observe(element); + + if (element.firstElementChild !== null) { + observer.observe(element.firstElementChild); + } + + return () => observer.disconnect(); + }, [isExpanded]); + + return ( + <> +
    + +
    + {isLong && ( + + )} + + ); +} diff --git a/resources/js/lib/chat.ts b/resources/js/lib/chat.ts index 62f4f0e50..5fe3c253c 100644 --- a/resources/js/lib/chat.ts +++ b/resources/js/lib/chat.ts @@ -11,6 +11,7 @@ import ConversationPingController from '@/actions/App/Http/Controllers/Conversat import ConversationReadController from '@/actions/App/Http/Controllers/ConversationReadController'; import ConversationTypingController from '@/actions/App/Http/Controllers/ConversationTypingController'; import InboxController from '@/actions/App/Http/Controllers/InboxController'; +import MessagePinController from '@/actions/App/Http/Controllers/MessagePinController'; import MessageReactionController from '@/actions/App/Http/Controllers/MessageReactionController'; import OrgArchiveController from '@/actions/App/Http/Controllers/OrgArchiveController'; import OrgController from '@/actions/App/Http/Controllers/OrgController'; @@ -24,6 +25,15 @@ import OrgTypingController from '@/actions/App/Http/Controllers/OrgTypingControl import RemoteCandidateController from '@/actions/App/Http/Controllers/RemoteCandidateController'; import UserBlockController from '@/actions/App/Http/Controllers/UserBlockController'; import { showTimeout, timeoutFrom } from '@/components/timeout-dialog'; +import { + acceptedPinnedMessages, + reactionsAfterOf, + withPinnedMessage, + withPinnedMessageRestored, + withoutPinnedMessage, + withShownReactions, +} from '@/lib/pinned-messages'; +import type { PinnedMessages } from '@/lib/pinned-messages'; import type { ReactionUsage } from '@/lib/reactions'; import { withReactionUse } from '@/lib/reactions'; import { @@ -92,6 +102,15 @@ export type ThreadState = { members: OrgMember[]; typing: string[]; role: OrgRole | null; + /** + * The org's pinned messages, latest pinned first, which everyone in the org sees whatever history they were given. + * Whether the user pinned the conversation itself in their inbox is pinned_at. + */ + pinned: Message[]; + /** + * The reactions version the pinned messages are current with, which only moves when a list of them is taken. + */ + pinnedMessagesVersion: number | null; is_archived: boolean; pinned_at: string | null; muted_until_at: string | null; @@ -173,6 +192,14 @@ const lastActivity = new Map(); const lastTypingPing = new Map(); const reactionsInFlight = new Map(); const reactionsSettledAt = new Map(); +const messagePinsInFlight = new Map(); +/** + * The newest pinned messages a pin request handed back while others in the org were still in flight. + */ +const newestPinnedMessages = new Map< + string, + { pinned: Message[]; version: number; sentAt: number } +>(); let inboxWatchers = 0; let inboxTimer: number | undefined; let isLoadingReactionUsage = false; @@ -431,7 +458,7 @@ function withReactionChanges( .map((change) => [change.id, change.reactions]), ); - if (changed.size === 0) { + if (!messages.some((message) => changed.has(message.id))) { return messages; } @@ -442,6 +469,52 @@ function withReactionChanges( ); } +/** + * Whether a message's reactions are the user's own to settle: they are reacting to it right now, or reacted to it after + * the request was sent. + */ +function isReactionHeld(sentAt: number): (id: number) => boolean { + return (id) => + reactionsInFlight.has(id) || + (reactionsSettledAt.get(id) ?? 0) >= sentAt; +} + +/** + * The thread's pinned messages once the server sent a list with the reactions version it read before it, or none. + * A list the thread doesn't take still gets the reaction changes sent with it. + */ +function pinnedMessagesFrom( + key: string, + thread: ThreadState, + pinned: Message[] | null, + version: number, + changes: MessageReactionChange[], + sentAt: number, +): PinnedMessages { + const accepted = acceptedPinnedMessages( + thread, + pinned, + version, + messagePinsInFlight.has(key), + ); + + if (accepted === null) { + return { + pinned: withReactionChanges(thread.pinned, changes, sentAt), + pinnedMessagesVersion: thread.pinnedMessagesVersion, + }; + } + + return { + pinned: withShownReactions( + accepted.pinned, + [...thread.messages, ...thread.pinned], + isReactionHeld(sentAt), + ), + pinnedMessagesVersion: accepted.pinnedMessagesVersion, + }; +} + function unreadOf(conversations: ConversationSummary[]): number { return conversations.reduce( (total, conversation) => total + conversation.ahead, @@ -723,6 +796,8 @@ function emptyThread(target: ChatTarget): ThreadState { members: [], typing: [], role: null, + pinned: [], + pinnedMessagesVersion: null, is_archived: false, pinned_at: null, muted_until_at: null, @@ -779,6 +854,14 @@ export function seedThread( ...fromServer(thread), target, messages: mergeMessages(current.messages, thread.messages), + ...pinnedMessagesFrom( + key, + current, + 'pinned' in thread ? thread.pinned : null, + thread.reactions_version, + [], + performance.now(), + ), has_older: current.isLoaded ? current.has_older : thread.has_older, @@ -1071,7 +1154,14 @@ async function pollThread(target: ChatTarget): Promise { const thread = state.threads[key]; const after = thread?.isLoaded ? (thread.messages.at(-1)?.id ?? 0) : null; const reactionsAfter = - after === null ? null : (thread?.reactions_version ?? null); + after === null + ? null + : reactionsAfterOf( + thread?.reactions_version ?? null, + target.kind === 'org' + ? (thread?.pinnedMessagesVersion ?? 0) + : null, + ); const polledAt = performance.now(); const version = ++muteVersion; const pinsVersion = ++pinVersion; @@ -1127,6 +1217,14 @@ async function pollThread(target: ChatTarget): Promise { ), fresh.messages, ), + ...pinnedMessagesFrom( + key, + current, + 'pinned' in fresh ? fresh.pinned : null, + fresh.reactions_version, + fresh.reaction_changes, + polledAt, + ), reactions_version: Math.max( current.reactions_version ?? 0, fresh.reactions_version, @@ -1432,17 +1530,30 @@ function countReactionUse(emoji: string, change: 1 | -1): void { } } +/** + * A message can show twice, in the thread and among the pinned ones, or only among the pinned ones for a member added + * after it without its history. + */ function setReactions( key: string, messageId: number, change: (reactions: MessageReaction[]) => MessageReaction[], ): void { + const changed = (messages: Message[]) => + messages.some((message) => message.id === messageId) + ? messages.map((message) => + message.id === messageId + ? { + ...message, + reactions: change(message.reactions ?? []), + } + : message, + ) + : messages; + updateThread(key, (thread) => ({ - messages: thread.messages.map((message) => - message.id === messageId - ? { ...message, reactions: change(message.reactions ?? []) } - : message, - ), + messages: changed(thread.messages), + pinned: changed(thread.pinned), })); } @@ -1457,9 +1568,11 @@ export async function toggleReaction( userId: string, ): Promise { const key = targetKey(target); - const message = state.threads[key]?.messages.find( - (item) => item.id === messageId, - ); + const thread = state.threads[key]; + const message = [ + ...(thread?.messages ?? []), + ...(thread?.pinned ?? []), + ].find((item) => item.id === messageId); if (message === undefined) { return false; @@ -1518,6 +1631,81 @@ export async function toggleReaction( } } +/** + * Pins a message for the whole org, or unpins it, straight away, then settles on the org's pinned messages as the + * server holds them, once no other pin request in the org is in flight. Puts the message back where it was and throws + * when the server refuses. + */ +export async function setMessagePinned( + target: ChatTarget, + message: Message, + isPinned: boolean, +): Promise { + const key = targetKey(target); + const shown = state.threads[key]?.pinned ?? []; + const index = shown.findIndex((item) => item.id === message.id); + const sentAt = performance.now(); + + if (isPinned === (index !== -1)) { + return; + } + + messagePinsInFlight.set(key, (messagePinsInFlight.get(key) ?? 0) + 1); + updateThread(key, (thread) => ({ + pinned: isPinned + ? withPinnedMessage(thread.pinned, message) + : withoutPinnedMessage(thread.pinned, message), + })); + + try { + const { pinned, reactions_version: version } = await request<{ + pinned: Message[]; + reactions_version: number; + }>( + isPinned + ? MessagePinController.update.url({ message: message.id }) + : MessagePinController.destroy.url({ message: message.id }), + isPinned ? 'PUT' : 'DELETE', + ); + + // Responses can arrive out of order, so the newest one wins. + if ((newestPinnedMessages.get(key)?.version ?? -1) <= version) { + newestPinnedMessages.set(key, { pinned, version, sentAt }); + } + } catch (error) { + updateThread(key, (thread) => ({ + pinned: isPinned + ? withoutPinnedMessage(thread.pinned, message) + : withPinnedMessageRestored(thread.pinned, shown[index], index), + })); + + throw error; + } finally { + const left = (messagePinsInFlight.get(key) ?? 1) - 1; + const newest = newestPinnedMessages.get(key); + + if (left > 0) { + messagePinsInFlight.set(key, left); + } else { + messagePinsInFlight.delete(key); + newestPinnedMessages.delete(key); + + if (newest !== undefined) { + updateThread(key, (thread) => + pinnedMessagesFrom( + key, + thread, + newest.pinned, + newest.version, + [], + newest.sentAt, + ), + ); + } + } + } +} + export function setReplyingTo( target: ChatTarget, reply: MessageReply | null, diff --git a/resources/js/lib/leaving-rows.ts b/resources/js/lib/leaving-rows.ts new file mode 100644 index 000000000..9b16736a4 --- /dev/null +++ b/resources/js/lib/leaving-rows.ts @@ -0,0 +1,30 @@ +/** + * A row of a list as it shows: leaving once its item is gone from the list, until it has collapsed. + */ +export type ListRow = { item: T; isLeaving: boolean }; + +/** + * The rows for the next items, with each item that left kept where it was, after the row it followed, so it can + * collapse in place instead of the rows under it jumping up. + */ +export function withLeavingRows( + previous: ListRow[], + next: T[], +): ListRow[] { + const nextIds = new Set(next.map((item) => item.id)); + const rows = next.map((item) => ({ item, isLeaving: false })); + let at = 0; + + previous.forEach((row) => { + if (nextIds.has(row.item.id)) { + at = rows.findIndex(({ item }) => item.id === row.item.id) + 1; + + return; + } + + rows.splice(at, 0, { item: row.item, isLeaving: true }); + at++; + }); + + return rows; +} diff --git a/resources/js/lib/pinned-messages.ts b/resources/js/lib/pinned-messages.ts new file mode 100644 index 000000000..21a8307b8 --- /dev/null +++ b/resources/js/lib/pinned-messages.ts @@ -0,0 +1,100 @@ +import type { Message } from '@/types'; + +/** + * An org's pinned messages as a thread shows them, with the reactions version they are current with. Pinning and + * unpinning move that version, so a list sent with an older one is from before a change the thread already shows. + */ +export type PinnedMessages = { + pinned: Message[]; + pinnedMessagesVersion: number | null; +}; + +/** + * The pinned messages the server sent along with the reactions version it read just before them, unless the user is + * pinning or unpinning in the org right now, as their own request hands back a fresher list, or the list is older than + * the one shown. Null when the thread keeps what it has. + */ +export function acceptedPinnedMessages( + current: PinnedMessages, + pinned: Message[] | null, + version: number, + isPinning: boolean, +): PinnedMessages | null { + if ( + pinned === null || + isPinning || + (current.pinnedMessagesVersion !== null && + version < current.pinnedMessagesVersion) + ) { + return null; + } + + return { pinned, pinnedMessagesVersion: version }; +} + +/** + * The reactions version a poll asks for the changes after. Once a poll's pinned messages were dropped they lag behind + * the thread, so the next poll asks from theirs and gets them again. Changes sent twice are whole lists per message, + * so taking them again changes nothing. + */ +export function reactionsAfterOf( + reactionsVersion: number | null, + pinnedMessagesVersion: number | null, +): number | null { + return reactionsVersion === null || pinnedMessagesVersion === null + ? reactionsVersion + : Math.min(reactionsVersion, pinnedMessagesVersion); +} + +/** + * Keeps the reactions shown on the messages the user is reacting to right now, or reacted to since the list was sent: + * their own request hands back fresher ones. + */ +export function withShownReactions( + incoming: Message[], + shown: Message[], + isHeld: (id: number) => boolean, +): Message[] { + if (!incoming.some((message) => isHeld(message.id))) { + return incoming; + } + + const reactions = new Map( + shown.map((message) => [message.id, message.reactions]), + ); + + return incoming.map((message) => + isHeld(message.id) && reactions.has(message.id) + ? { ...message, reactions: reactions.get(message.id) } + : message, + ); +} + +export function withPinnedMessage( + pinned: Message[], + message: Message, +): Message[] { + return pinned.some((item) => item.id === message.id) + ? pinned + : [{ ...message, pinned_at: new Date().toISOString() }, ...pinned]; +} + +export function withoutPinnedMessage( + pinned: Message[], + message: Message, +): Message[] { + return pinned.filter((item) => item.id !== message.id); +} + +/** + * Puts an unpinned message back where it was, when the server refused to unpin it. + */ +export function withPinnedMessageRestored( + pinned: Message[], + message: Message, + index: number, +): Message[] { + return pinned.some((item) => item.id === message.id) + ? pinned + : pinned.toSpliced(Math.min(index, pinned.length), 0, message); +} diff --git a/resources/js/types/conversation.ts b/resources/js/types/conversation.ts index 9b1389d7f..5ee575ff3 100644 --- a/resources/js/types/conversation.ts +++ b/resources/js/types/conversation.ts @@ -60,6 +60,7 @@ export type Message = { subject_id?: string | null; user?: User; subject?: User | null; + pinned_at?: string | null; created_at: string; }; @@ -137,6 +138,12 @@ export type OrgThread = { muted_until_at: string | null; role: OrgRole | null; access: PushAccess; + /** + * The org's pinned messages, latest pinned first, unlike pinned_at, which is when the user pinned the org in their + * inbox. Null when they haven't changed since the reactions version the poll asked after, so the ones already shown + * stay. + */ + pinned: Message[] | null; }; /** diff --git a/routes/web.php b/routes/web.php index 7b6e9e094..ad2bb8301 100644 --- a/routes/web.php +++ b/routes/web.php @@ -62,6 +62,7 @@ use App\Http\Controllers\MentionSuggestionController; use App\Http\Controllers\MessageImageController; use App\Http\Controllers\MessageLinkPreviewController; +use App\Http\Controllers\MessagePinController; use App\Http\Controllers\MessageReactionController; use App\Http\Controllers\MessageVoiceController; use App\Http\Controllers\MobileAuthorizationController; @@ -397,6 +398,15 @@ ->name('message-reaction.destroy'); }); + Route::put('remotes/messages/{message}/pin', [MessagePinController::class, 'update']) + ->whereNumber('message') + ->middleware([PreventPostingInTimeout::class, 'throttle:pins']) + ->name('message-pin.update'); + Route::delete('remotes/messages/{message}/pin', [MessagePinController::class, 'destroy']) + ->whereNumber('message') + ->middleware('throttle:pins') + ->name('message-pin.destroy'); + Route::post('remotes/@{partner:username}/messages', [ConversationMessageController::class, 'store']) ->middleware([PreventPostingInTimeout::class, 'throttle:messages']) ->name('conversation-message.store'); diff --git a/tests/Feature/Controllers/Api/OrgMessageControllerTest.php b/tests/Feature/Controllers/Api/OrgMessageControllerTest.php index d9bb8e60b..f640a552e 100644 --- a/tests/Feature/Controllers/Api/OrgMessageControllerTest.php +++ b/tests/Feature/Controllers/Api/OrgMessageControllerTest.php @@ -65,6 +65,24 @@ ->assertJsonPath('data.role', 'contributor'); }); +it('hands over the pinned messages as the thread shows them, until they are unchanged', function (): void { + [$nuno] = followersOf($this->user, 1); + $org = createOrg($this->user, [$nuno]); + $message = pushToOrg($nuno, $org, 'deploy runbook'); + $this->actingAs($this->user)->putJson(route('message-pin.update', $message))->assertOk(); + + $version = signedApiRequest($this->keyPair, 'GET', route('api-org-message.index', $org)) + ->assertOk() + ->assertJsonPath('data.pinned.0.id', $message->id) + ->assertJsonPath('data.pinned.0.body', 'deploy runbook') + ->assertJsonPath('data.pinned.0.user.lane', $nuno->lane()) + ->assertJsonPath('data.pinned.0.created_at', $message->created_at->toIso8601ZuluString()) + ->json('data.reactions_version'); + + expect(signedApiRequest($this->keyPair, 'GET', route('api-org-message.index', ['org' => $org, 'after' => $message->id, 'reactions_after' => $version]))->json('data')) + ->toHaveKey('pinned', null); +}); + it('keeps people out of orgs they are not in', function (): void { $maintainer = User::factory()->withRemotes()->create(); [$nuno] = followersOf($maintainer, 1); diff --git a/tests/Feature/Controllers/MessageImageControllerTest.php b/tests/Feature/Controllers/MessageImageControllerTest.php index 54ae763ff..7692d7ed5 100644 --- a/tests/Feature/Controllers/MessageImageControllerTest.php +++ b/tests/Feature/Controllers/MessageImageControllerTest.php @@ -70,6 +70,18 @@ function storedMessageImage(Message $message): MessageImage $this->get(route('message-image.show', $after))->assertOk(); }); +it('serves the images of a pinned message to every org member, whatever history they were given', function (): void { + $mark = User::factory()->create(); + [$nuno, $taylor] = followersOf($mark, 2); + $org = createOrg($mark, [$nuno]); + $message = pushToOrg($nuno, $org); + $image = storedMessageImage($message); + $this->actingAs($mark)->postJson(route('org-member.store', $org), ['members' => [$taylor->username], 'share_history' => false])->assertNoContent(); + $this->putJson(route('message-pin.update', $message))->assertOk(); + + $this->actingAs($taylor)->get(route('message-image.show', $image))->assertOk(); +}); + it('requires authentication', function (): void { $image = storedMessageImage(Message::factory()->create()); diff --git a/tests/Feature/Controllers/MessagePinControllerTest.php b/tests/Feature/Controllers/MessagePinControllerTest.php new file mode 100644 index 000000000..ad6b2b2f2 --- /dev/null +++ b/tests/Feature/Controllers/MessagePinControllerTest.php @@ -0,0 +1,139 @@ +create(); + [$nuno] = followersOf($mark, 1); + $org = createOrg($mark, [$nuno]); + $message = pushToOrg($nuno, $org, 'deploys go out on fridays now'); + $version = Conversation::query()->findOrFail($org)->reactions_version; + + $this->actingAs($mark)->putJson(route('message-pin.update', $message)) + ->assertOk() + ->assertJsonCount(1, 'pinned') + ->assertJsonPath('pinned.0.id', $message->id) + ->assertJsonPath('pinned.0.body', 'deploys go out on fridays now') + ->assertJsonPath('pinned.0.user.username', $nuno->username) + ->assertJsonPath('reactions_version', $version + 1); + + $pinnedAt = $message->fresh()?->pinned_at; + + expect($pinnedAt)->not->toBeNull() + ->and(Conversation::query()->findOrFail($org)->reactions_version)->toBe($version + 1); + + $this->travel(5)->minutes(); + + $this->putJson(route('message-pin.update', $message)) + ->assertOk() + ->assertJsonPath('pinned.*.id', [$message->id]) + ->assertJsonPath('reactions_version', $version + 1); + + expect($message->fresh()?->pinned_at?->equalTo($pinnedAt))->toBeTrue(); +}); + +it('lets maintainers unpin a message, however often', function (): void { + $mark = User::factory()->create(); + [$nuno] = followersOf($mark, 1); + $org = createOrg($mark, [$nuno]); + $kept = pushToOrg($nuno, $org, 'kept'); + $dropped = pushToOrg($nuno, $org, 'dropped'); + $this->actingAs($mark)->putJson(route('message-pin.update', $kept))->assertOk(); + $this->putJson(route('message-pin.update', $dropped))->assertOk(); + $version = Conversation::query()->findOrFail($org)->reactions_version; + + $this->deleteJson(route('message-pin.destroy', $dropped)) + ->assertOk() + ->assertJsonPath('pinned.*.id', [$kept->id]) + ->assertJsonPath('reactions_version', $version + 1); + + expect($dropped->fresh()?->pinned_at)->toBeNull() + ->and(Conversation::query()->findOrFail($org)->reactions_version)->toBe($version + 1); + + $this->deleteJson(route('message-pin.destroy', $dropped)) + ->assertOk() + ->assertJsonPath('pinned.*.id', [$kept->id]) + ->assertJsonPath('reactions_version', $version + 1); +}); + +it('only lets maintainers pin and unpin', function (): void { + $mark = User::factory()->create(); + [$nuno] = followersOf($mark, 1); + $org = createOrg($mark, [$nuno]); + $message = pushToOrg($nuno, $org); + + $this->actingAs($nuno)->putJson(route('message-pin.update', $message)) + ->assertForbidden() + ->assertJsonPath('message', 'Only maintainers can pin messages.'); + $this->actingAs($mark)->putJson(route('message-pin.update', $message))->assertOk(); + $this->actingAs($nuno)->deleteJson(route('message-pin.destroy', $message)) + ->assertForbidden() + ->assertJsonPath('message', 'Only maintainers can unpin messages.'); + + expect($message->fresh()?->pinned_at)->not->toBeNull(); +}); + +it('is not there for people outside the org', function (): void { + $mark = User::factory()->create(); + [$nuno] = followersOf($mark, 1); + $org = createOrg($mark, [$nuno]); + $message = pushToOrg($nuno, $org); + + $this->actingAs(User::factory()->create())->putJson(route('message-pin.update', $message))->assertNotFound(); + $this->deleteJson(route('message-pin.destroy', $message))->assertNotFound(); +}); + +it('is not there while the maintainer has Remotes off', function (): void { + $mark = User::factory()->create(); + [$nuno] = followersOf($mark, 1); + $org = createOrg($mark, [$nuno]); + $message = pushToOrg($nuno, $org); + $mark->update(['remotes_enabled' => false]); + + $this->actingAs($mark)->putJson(route('message-pin.update', $message))->assertNotFound(); + $this->deleteJson(route('message-pin.destroy', $message))->assertNotFound(); +}); + +it('pins neither direct messages nor org events', function (): void { + $mark = User::factory()->create(); + [$nuno] = followersOf($mark, 1); + $direct = sendChatMessage($mark, $nuno); + $org = createOrg($mark, [$nuno]); + $event = Message::query()->where('conversation_id', $org)->whereNotNull('event')->sole(); + + $this->actingAs($mark)->putJson(route('message-pin.update', $direct))->assertNotFound(); + $this->deleteJson(route('message-pin.destroy', $direct))->assertNotFound(); + $this->putJson(route('message-pin.update', $event))->assertNotFound(); +}); + +it('pins only within the history the maintainer can see, but unpins whatever the org pinned', function (): void { + $mark = User::factory()->create(); + [$nuno, $taylor] = followersOf($mark, 2); + $org = createOrg($mark, [$nuno]); + $before = pushToOrg($nuno, $org, 'from before taylor joined'); + $this->actingAs($mark)->postJson(route('org-member.store', $org), ['members' => [$taylor->username], 'share_history' => false])->assertNoContent(); + $this->putJson(route('org-member.update', [$org, $taylor]), ['role' => 'maintainer'])->assertNoContent(); + + $this->actingAs($taylor)->putJson(route('message-pin.update', $before))->assertNotFound(); + $this->actingAs($mark)->putJson(route('message-pin.update', $before))->assertOk(); + $this->actingAs($taylor)->deleteJson(route('message-pin.destroy', $before))->assertOk()->assertJsonPath('pinned', []); +}); + +it('caps how many messages an org pins', function (): void { + $mark = User::factory()->create(); + [$nuno] = followersOf($mark, 1); + $org = createOrg($mark, [$nuno]); + $pinned = Message::factory()->count(Conversation::MAX_PINNED_MESSAGES)->pinned()->create(['conversation_id' => $org, 'user_id' => $nuno->id]); + $message = pushToOrg($nuno, $org); + + $this->actingAs($mark)->putJson(route('message-pin.update', $message)) + ->assertForbidden() + ->assertJsonPath('message', 'An org can pin up to 25 messages. Unpin one first.'); + $this->putJson(route('message-pin.update', $pinned->first()))->assertOk()->assertJsonCount(Conversation::MAX_PINNED_MESSAGES, 'pinned'); + $this->deleteJson(route('message-pin.destroy', $pinned->first()))->assertOk(); + $this->putJson(route('message-pin.update', $message))->assertOk()->assertJsonPath('pinned.0.id', $message->id); +}); diff --git a/tests/Feature/Controllers/MessageReactionControllerTest.php b/tests/Feature/Controllers/MessageReactionControllerTest.php index efc91fc2d..d3f81b046 100644 --- a/tests/Feature/Controllers/MessageReactionControllerTest.php +++ b/tests/Feature/Controllers/MessageReactionControllerTest.php @@ -111,6 +111,24 @@ expect($messages->firstWhere('id', $after->id)['reactions'])->toBe([['user_id' => $taylor->id, 'emoji' => 'πŸ‘‹']]); }); +it('lets org members react to a pinned message from before their history, until it is unpinned', function (): void { + $mark = User::factory()->create(); + [$nuno, $taylor] = followersOf($mark, 2); + $org = createOrg($mark, [$nuno]); + $runbook = pushToOrg($nuno, $org, 'deploy runbook'); + $this->actingAs($mark)->postJson(route('org-member.store', $org), ['members' => [$taylor->username], 'share_history' => false])->assertNoContent(); + $this->putJson(route('message-pin.update', $runbook))->assertOk(); + + $this->actingAs($taylor)->postJson(route('message-reaction.store', $runbook), ['emoji' => 'πŸ™']) + ->assertOk() + ->assertJsonPath('reactions', [['user_id' => $taylor->id, 'emoji' => 'πŸ™']]); + $this->getJson(route('org-message.index', $org))->assertJsonPath('pinned.0.reactions', [['user_id' => $taylor->id, 'emoji' => 'πŸ™']]); + + $this->actingAs($mark)->deleteJson(route('message-pin.destroy', $runbook))->assertOk(); + + $this->actingAs($taylor)->postJson(route('message-reaction.store', $runbook), ['emoji' => 'πŸ‘€'])->assertNotFound(); +}); + it('hands polls the reactions that changed since the last one, wherever the message is', function (): void { $mark = User::factory()->withRemotes()->create(); $nuno = User::factory()->withRemotes()->create(); diff --git a/tests/Feature/Controllers/OrgMessageControllerTest.php b/tests/Feature/Controllers/OrgMessageControllerTest.php index 59492c08c..24b8d4c26 100644 --- a/tests/Feature/Controllers/OrgMessageControllerTest.php +++ b/tests/Feature/Controllers/OrgMessageControllerTest.php @@ -202,3 +202,74 @@ ->assertJsonPath('members.*.user.id', [$viewer->id, $joining->id]) ->assertJsonPath('members.1.suggests_mentions', true); }); + +it('hands every member the pinned messages, latest pinned first, whatever history they were given', function (): void { + $mark = User::factory()->create(); + [$nuno, $taylor] = followersOf($mark, 2); + $org = createOrg($mark, [$nuno]); + $runbook = pushToOrg($nuno, $org, 'deploy runbook'); + $this->actingAs($mark)->postJson(route('org-member.store', $org), ['members' => [$taylor->username], 'share_history' => false])->assertNoContent(); + $welcome = pushToOrg($nuno, $org, 'welcome taylor'); + $this->actingAs($mark)->putJson(route('message-pin.update', $welcome))->assertOk(); + $this->travel(1)->minutes(); + $this->putJson(route('message-pin.update', $runbook))->assertOk(); + + $thread = $this->actingAs($taylor)->getJson(route('org-message.index', $org)) + ->assertOk() + ->assertJsonPath('pinned.*.id', [$runbook->id, $welcome->id]) + ->assertJsonPath('pinned.0.body', 'deploy runbook') + ->assertJsonPath('pinned.0.user.username', $nuno->username) + ->assertJsonPath('pinned.0.pinned_at', now()->startOfSecond()->toJSON()); + + expect($thread->json('messages.*.id'))->toContain($welcome->id)->not->toContain($runbook->id); +}); + +it('hands polls the pinned messages only once they changed, and older pages none', function (): void { + $mark = User::factory()->create(); + [$nuno] = followersOf($mark, 1); + $org = createOrg($mark, [$nuno]); + $message = pushToOrg($nuno, $org, 'deploy runbook'); + $version = $this->actingAs($mark)->getJson(route('org-message.index', $org))->assertJsonPath('pinned', [])->json('reactions_version'); + + expect($this->getJson(route('org-message.index', [$org, 'after' => $message->id, 'reactions_after' => $version]))->json()) + ->toHaveKey('pinned', null); + + $this->putJson(route('message-pin.update', $message))->assertOk(); + + $this->getJson(route('org-message.index', [$org, 'after' => $message->id, 'reactions_after' => $version])) + ->assertJsonPath('pinned.*.id', [$message->id]); + + expect($this->getJson(route('org-message.index', [$org, 'before' => $message->id]))->json()) + ->toHaveKey('pinned', null); +}); + +it('leaves out what a pinned message replies to, which may be from before the member history', function (): void { + $mark = User::factory()->create(); + [$nuno, $taylor] = followersOf($mark, 2); + $org = createOrg($mark, [$nuno]); + $question = pushToOrg($nuno, $org, 'where does the deploy key live?'); + $answer = $this->actingAs($mark)->postJson(route('org-message.store', $org), ['body' => 'in the vault', 'client_id' => Str::uuid()->toString(), 'reply_to_id' => $question->id]) + ->assertCreated() + ->json('message.id'); + $this->postJson(route('org-member.store', $org), ['members' => [$taylor->username], 'share_history' => false])->assertNoContent(); + + expect($this->putJson(route('message-pin.update', $answer))->assertOk()->json('pinned.0'))->not->toHaveKeys(['reply_to', 'reply_to_id']); + + $thread = $this->actingAs($taylor)->getJson(route('org-message.index', $org))->assertJsonPath('pinned.0.id', $answer); + + expect($thread->json('pinned.0'))->not->toHaveKeys(['reply_to', 'reply_to_id']) + ->and($thread->getContent())->not->toContain('where does the deploy key live?'); +}); + +it('lets members reply to a pinned message from before their history', function (): void { + $mark = User::factory()->create(); + [$nuno, $taylor] = followersOf($mark, 2); + $org = createOrg($mark, [$nuno]); + $runbook = pushToOrg($nuno, $org, 'deploy runbook'); + $this->actingAs($mark)->postJson(route('org-member.store', $org), ['members' => [$taylor->username], 'share_history' => false])->assertNoContent(); + $this->putJson(route('message-pin.update', $runbook))->assertOk(); + + $this->actingAs($taylor)->postJson(route('org-message.store', $org), ['body' => 'thanks!', 'client_id' => Str::uuid()->toString(), 'reply_to_id' => $runbook->id]) + ->assertCreated() + ->assertJsonPath('message.reply_to.body', 'deploy runbook'); +}); diff --git a/tests/Feature/Controllers/UserTimeoutControllerTest.php b/tests/Feature/Controllers/UserTimeoutControllerTest.php index c8c82fc38..232c2050c 100644 --- a/tests/Feature/Controllers/UserTimeoutControllerTest.php +++ b/tests/Feature/Controllers/UserTimeoutControllerTest.php @@ -174,10 +174,11 @@ ->assertJsonPath('timeout.reason', 'Spam or self-promotion'); }); -it('stops a user in timeout from starting, renaming, or growing orgs and messaging them', function (): void { +it('stops a user in timeout from starting, renaming, or growing orgs, and from messaging or pinning in them', function (): void { $user = User::factory()->create(); [$member, $newcomer] = followersOf($user, 2); $org = createOrg($user, [$member]); + $message = pushToOrg($member, $org); UserTimeout::factory()->for($user)->create(); $this->actingAs($user) @@ -188,10 +189,12 @@ $this->putJson(route('org.update', $org), ['name' => 'buy my course'])->assertForbidden(); $this->postJson(route('org-member.store', $org), ['members' => [$newcomer->username], 'share_history' => false])->assertForbidden(); $this->postJson(route('org-message.store', $org), ['body' => 'hello', 'client_id' => Str::uuid()->toString()])->assertForbidden(); + $this->putJson(route('message-pin.update', $message))->assertForbidden(); expect(Conversation::query()->count())->toBe(1) ->and(Conversation::query()->sole()->name)->toBe('dev/null core') - ->and(Conversation::query()->sole()->participants()->count())->toBe(2); + ->and(Conversation::query()->sole()->participants()->count())->toBe(2) + ->and($message->fresh()?->pinned_at)->toBeNull(); }); it('tells the user how long is left of the timeout that ends last, rounded up', function (string $endsIn, string $remaining): void { diff --git a/tests/Feature/RateLimitTest.php b/tests/Feature/RateLimitTest.php index f764d4909..22fdba3d1 100644 --- a/tests/Feature/RateLimitTest.php +++ b/tests/Feature/RateLimitTest.php @@ -3,6 +3,7 @@ declare(strict_types=1); use App\Models\Commit; +use App\Models\Message; use App\Models\User; dataset('rate limits', [ @@ -22,6 +23,14 @@ 20, 60, ], + 'message pins' => [ + fn (): array => [ + ['PUT', route('message-pin.update', $message = Message::factory()->create())], + ['DELETE', route('message-pin.destroy', $message)], + ], + 20, + 60, + ], 'follows' => [ fn (): array => [ ['POST', route('user-follow.store', $user = User::factory()->create())], diff --git a/tests/Unit/ChatNavigation.test.tsx b/tests/Unit/ChatNavigation.test.tsx index 0f7e51589..4cb07a133 100644 --- a/tests/Unit/ChatNavigation.test.tsx +++ b/tests/Unit/ChatNavigation.test.tsx @@ -17,7 +17,7 @@ function thread(kind: 'direct' | 'org' = 'direct'): ChatTarget { ? { kind, partner: { id, username: id } as User } : { kind, org: { id, name: 'Navigation' } }; seedThread(target, kind === 'direct' ? page([101, 102]) : { - ...page([101, 102]), members: [], typing: [], role: 'contributor', + ...page([101, 102]), members: [], typing: [], role: 'contributor', pinned: [], }); return target; } diff --git a/tests/Unit/LeavingRows.test.ts b/tests/Unit/LeavingRows.test.ts new file mode 100644 index 000000000..fe2dfaf32 --- /dev/null +++ b/tests/Unit/LeavingRows.test.ts @@ -0,0 +1,61 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { withLeavingRows } from '../../resources/js/lib/leaving-rows.ts'; +import type { ListRow } from '../../resources/js/lib/leaving-rows.ts'; + +type Item = { id: number; label?: string }; + +const rows = (...ids: number[]): ListRow[] => + ids.map((id) => ({ item: { id }, isLeaving: false })); + +const shown = (list: ListRow[]) => + list.map(({ item, isLeaving }) => (isLeaving ? `-${item.id}` : item.id)); + +await test('keeps a removed row where it was, leaving', () => { + assert.deepEqual( + shown(withLeavingRows(rows(1, 2, 3), [{ id: 1 }, { id: 3 }])), + [1, '-2', 3], + ); +}); + +await test('keeps removed rows next to each other in their order', () => { + assert.deepEqual( + shown(withLeavingRows(rows(1, 2, 3, 4), [{ id: 1 }, { id: 4 }])), + [1, '-2', '-3', 4], + ); +}); + +await test('keeps a removed first row first', () => { + assert.deepEqual(shown(withLeavingRows(rows(1, 2), [{ id: 2 }])), [ + '-1', + 2, + ]); +}); + +await test('keeps a removed row after the row it followed when new ones arrive first', () => { + assert.deepEqual( + shown(withLeavingRows(rows(1, 2), [{ id: 5 }, { id: 1 }])), + [5, 1, '-2'], + ); +}); + +await test('keeps rows that are still leaving until they are dropped', () => { + const leaving = withLeavingRows(rows(1, 2), [{ id: 1 }]); + + assert.deepEqual(shown(withLeavingRows(leaving, [{ id: 1 }])), [1, '-2']); +}); + +await test('brings back a row whose item returns', () => { + const leaving = withLeavingRows(rows(1, 2), [{ id: 1 }]); + + assert.deepEqual( + shown(withLeavingRows(leaving, [{ id: 1 }, { id: 2 }])), + [1, 2], + ); +}); + +await test('shows the next version of the items that stay', () => { + const next = withLeavingRows(rows(1), [{ id: 1, label: 'fresh' }]); + + assert.equal(next[0]?.item.label, 'fresh'); +}); diff --git a/tests/Unit/PinnedMessages.test.ts b/tests/Unit/PinnedMessages.test.ts new file mode 100644 index 000000000..fe12908c9 --- /dev/null +++ b/tests/Unit/PinnedMessages.test.ts @@ -0,0 +1,136 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { + acceptedPinnedMessages, + reactionsAfterOf, + withPinnedMessage, + withPinnedMessageRestored, + withoutPinnedMessage, + withShownReactions, +} from '../../resources/js/lib/pinned-messages.ts'; +import type { Message } from '../../resources/js/types'; + +const message = (id: number, reactions: Message['reactions'] = []) => + ({ + id, + user_id: 'ana', + client_id: `c${id}`, + body: '', + reactions, + }) as Message; + +const ids = (messages: Message[]) => messages.map(({ id }) => id); + +await test('takes a list as new as the one shown when nothing is being pinned', () => { + const accepted = acceptedPinnedMessages( + { pinned: [message(1)], pinnedMessagesVersion: 4 }, + [message(2), message(1)], + 4, + false, + ); + + assert.deepEqual(ids(accepted?.pinned ?? []), [2, 1]); + assert.equal(accepted?.pinnedMessagesVersion, 4); +}); + +await test('takes the first list a thread gets', () => { + assert.equal( + acceptedPinnedMessages( + { pinned: [], pinnedMessagesVersion: null }, + [message(1)], + 2, + false, + )?.pinnedMessagesVersion, + 2, + ); +}); + +await test('drops a list while the user is pinning or unpinning', () => { + assert.equal( + acceptedPinnedMessages( + { pinned: [], pinnedMessagesVersion: 1 }, + [message(1)], + 9, + true, + ), + null, + ); +}); + +await test('drops a list older than the one shown, like a poll sent before a pin that lands after it', () => { + assert.equal( + acceptedPinnedMessages( + { pinned: [message(1)], pinnedMessagesVersion: 7 }, + [], + 6, + false, + ), + null, + ); +}); + +await test('keeps the list when the server sends none, as nothing changed', () => { + assert.equal( + acceptedPinnedMessages( + { pinned: [message(1)], pinnedMessagesVersion: 3 }, + null, + 3, + false, + ), + null, + ); +}); + +await test('asks for the changes after the thread version while the pinned messages are current', () => { + assert.equal(reactionsAfterOf(8, 8), 8); +}); + +await test('asks again from the pinned messages once a dropped list leaves them behind', () => { + assert.equal(reactionsAfterOf(9, 6), 6); +}); + +await test('asks from the thread version when there are no pinned messages to keep current', () => { + assert.equal(reactionsAfterOf(5, null), 5); + assert.equal(reactionsAfterOf(null, 5), null); +}); + +await test('keeps the reactions shown on messages the user is reacting to', () => { + const shown = [message(1, [{ user_id: 'me', emoji: 'πŸ‘' }]), message(2)]; + const incoming = [ + message(1), + message(2, [{ user_id: 'ben', emoji: 'πŸŽ‰' }]), + ]; + const kept = withShownReactions(incoming, shown, (id) => id === 1); + + assert.deepEqual(kept[0]?.reactions, [{ user_id: 'me', emoji: 'πŸ‘' }]); + assert.deepEqual(kept[1]?.reactions, [{ user_id: 'ben', emoji: 'πŸŽ‰' }]); +}); + +await test('hands back the same list when no reaction is held', () => { + const incoming = [message(1)]; + + assert.equal( + withShownReactions(incoming, [], () => false), + incoming, + ); +}); + +await test('pins a message first, once', () => { + const pinned = withPinnedMessage([message(1)], message(2)); + + assert.deepEqual(ids(pinned), [2, 1]); + assert.notEqual(pinned[0]?.pinned_at ?? null, null); + assert.equal(withPinnedMessage(pinned, message(2)), pinned); +}); + +await test('unpins a message and puts it back where it was', () => { + const pinned = [message(3), message(2), message(1)]; + const unpinned = withoutPinnedMessage(pinned, message(2)); + + assert.deepEqual(ids(unpinned), [3, 1]); + assert.deepEqual( + ids(withPinnedMessageRestored(unpinned, message(2), 1)), + [3, 2, 1], + ); + assert.deepEqual(ids(withPinnedMessageRestored([], message(2), 1)), [2]); +}); diff --git a/tests/Unit/RemoteNotifications.test.tsx b/tests/Unit/RemoteNotifications.test.tsx index 63018dae9..727e475df 100644 --- a/tests/Unit/RemoteNotifications.test.tsx +++ b/tests/Unit/RemoteNotifications.test.tsx @@ -791,7 +791,7 @@ test('an unfocused window does not mark incoming messages read before it can ale messages: [conversation(50).last_message!], has_older: false, reactions_version: 0, reaction_changes: [], read_through_id: 0, members: [], typing: [], is_archived: false, pinned_at: null, muted_until_at: null, - role: 'contributor', access: 'open', + role: 'contributor', access: 'open', pinned: [], }); markThreadRead(target); diff --git a/tests/Unit/RemotePreferences.test.tsx b/tests/Unit/RemotePreferences.test.tsx index edb712d1d..bb2394843 100644 --- a/tests/Unit/RemotePreferences.test.tsx +++ b/tests/Unit/RemotePreferences.test.tsx @@ -35,7 +35,7 @@ const orgSummary: ConversationSummary = { const thread: OrgThread & { org: typeof org } = { org, messages: [], has_older: false, reactions_version: 0, reaction_changes: [], read_through_id: 0, members: [], typing: [], is_archived: true, pinned_at: null, muted_until_at: null, - role: 'contributor', access: 'open', + role: 'contributor', access: 'open', pinned: [], }; function elements(node: ReactNode): ReactElement>[] { From 7d0dc3148f9d1aaba2cfa8cb0a8579fe43594627 Mon Sep 17 00:00:00 2001 From: Simon Hamp Date: Sat, 24 Oct 2026 19:56:47 +0100 Subject: [PATCH 2/4] feat: pin and unpin org messages from the API The app gets the same pin and unpin endpoints as the web, signed like its other Remotes calls. They share the web's checks and answer with the pinned list in the shape the API thread already uses, plus the reactions version. Co-Authored-By: Claude Opus 5.5 --- KNOWLEDGE_BASE.md | 2 +- .../Controllers/Api/MessagePinController.php | 40 +++++ .../Requests/DeleteApiMessagePinRequest.php | 35 ++++ app/Http/Requests/DeleteMessagePinRequest.php | 21 ++- .../Requests/UpdateApiMessagePinRequest.php | 35 ++++ app/Http/Requests/UpdateMessagePinRequest.php | 21 ++- routes/api.php | 12 ++ .../Api/MessagePinControllerTest.php | 168 ++++++++++++++++++ 8 files changed, 317 insertions(+), 17 deletions(-) create mode 100644 app/Http/Controllers/Api/MessagePinController.php create mode 100644 app/Http/Requests/DeleteApiMessagePinRequest.php create mode 100644 app/Http/Requests/UpdateApiMessagePinRequest.php create mode 100644 tests/Feature/Controllers/Api/MessagePinControllerTest.php diff --git a/KNOWLEDGE_BASE.md b/KNOWLEDGE_BASE.md index 516a50cc5..2d9ded45b 100644 --- a/KNOWLEDGE_BASE.md +++ b/KNOWLEDGE_BASE.md @@ -480,7 +480,7 @@ A signed request sees what the web shows you, such as your own pending commits. - follow or unfollow, mute or unmute, and block or unblock, and list who you muted and blocked; - star or unstar; - create polls on commits that are neither replies nor quotes, and vote on polls; -- change Remotes settings, read and send messages with images, link cards, and voice notes in conversations and orgs, mark them pulled, react to messages and list your reactions, find people, and start orgs; +- change Remotes settings, read and send messages with images, link cards, and voice notes in conversations and orgs, mark them pulled, react to messages and list your reactions, pin and unpin org messages as a maintainer, find people, and start orgs; - rename orgs, add people to them, change their roles, remove them, and leave an org, with the same maintainer rules as the web; - stash or reopen conversations and orgs, pin and mute them, merge or drop message requests, and send typing indicators; - switch PastΓ©is mode and give pastΓ©is; diff --git a/app/Http/Controllers/Api/MessagePinController.php b/app/Http/Controllers/Api/MessagePinController.php new file mode 100644 index 000000000..1044e4cca --- /dev/null +++ b/app/Http/Controllers/Api/MessagePinController.php @@ -0,0 +1,40 @@ +handle($message); + + $org = $message->conversation()->firstOrFail(); + + return response()->json(['pinned' => MessageResource::collection($pinnedMessages->get($org)), 'reactions_version' => $org->reactions_version]); + } + + public function destroy(DeleteApiMessagePinRequest $request, Message $message, DeleteMessagePin $action, PinnedMessagesQuery $pinnedMessages): JsonResponse + { + $action->handle($message); + + $org = $message->conversation()->firstOrFail(); + + return response()->json(['pinned' => MessageResource::collection($pinnedMessages->get($org)), 'reactions_version' => $org->reactions_version]); + } +} diff --git a/app/Http/Requests/DeleteApiMessagePinRequest.php b/app/Http/Requests/DeleteApiMessagePinRequest.php new file mode 100644 index 000000000..d10764a68 --- /dev/null +++ b/app/Http/Requests/DeleteApiMessagePinRequest.php @@ -0,0 +1,35 @@ +user(); + assert($user instanceof User); + + $message = $this->route('message'); + assert($message instanceof Message); + + return DeleteMessagePinRequest::authorization($user, $message); + } + + /** + * @return array{} + */ + public function rules(): array + { + return []; + } +} diff --git a/app/Http/Requests/DeleteMessagePinRequest.php b/app/Http/Requests/DeleteMessagePinRequest.php index 6b051bfa4..c3d2053f7 100644 --- a/app/Http/Requests/DeleteMessagePinRequest.php +++ b/app/Http/Requests/DeleteMessagePinRequest.php @@ -12,16 +12,10 @@ final class DeleteMessagePinRequest extends FormRequest { /** - * Maintainers unpin an org's messages for everyone in it. + * Maintainers unpin an org's messages for everyone in it. The app unpins the same way. */ - public function authorize(): Response + public static function authorization(User $user, Message $message): Response { - $user = $this->user(); - assert($user instanceof User); - - $message = $this->route('message'); - assert($message instanceof Message); - $org = $message->conversation; return match (true) { @@ -31,6 +25,17 @@ public function authorize(): Response }; } + public function authorize(): Response + { + $user = $this->user(); + assert($user instanceof User); + + $message = $this->route('message'); + assert($message instanceof Message); + + return self::authorization($user, $message); + } + /** * @return array{} */ diff --git a/app/Http/Requests/UpdateApiMessagePinRequest.php b/app/Http/Requests/UpdateApiMessagePinRequest.php new file mode 100644 index 000000000..0c943eec3 --- /dev/null +++ b/app/Http/Requests/UpdateApiMessagePinRequest.php @@ -0,0 +1,35 @@ +user(); + assert($user instanceof User); + + $message = $this->route('message'); + assert($message instanceof Message); + + return UpdateMessagePinRequest::authorization($user, $message); + } + + /** + * @return array{} + */ + public function rules(): array + { + return []; + } +} diff --git a/app/Http/Requests/UpdateMessagePinRequest.php b/app/Http/Requests/UpdateMessagePinRequest.php index 462b855f0..fc8fab724 100644 --- a/app/Http/Requests/UpdateMessagePinRequest.php +++ b/app/Http/Requests/UpdateMessagePinRequest.php @@ -14,16 +14,10 @@ final class UpdateMessagePinRequest extends FormRequest { /** * Maintainers pin an org's messages for everyone in it, up to its limit, and only within the history they can see. - * Events can't be pinned. + * Events can't be pinned. The app pins the same way. */ - public function authorize(): Response + public static function authorization(User $user, Message $message): Response { - $user = $this->user(); - assert($user instanceof User); - - $message = $this->route('message'); - assert($message instanceof Message); - $org = $message->conversation; return match (true) { @@ -34,6 +28,17 @@ public function authorize(): Response }; } + public function authorize(): Response + { + $user = $this->user(); + assert($user instanceof User); + + $message = $this->route('message'); + assert($message instanceof Message); + + return self::authorization($user, $message); + } + /** * @return array{} */ diff --git a/routes/api.php b/routes/api.php index aaf14381c..afa1bc829 100644 --- a/routes/api.php +++ b/routes/api.php @@ -26,6 +26,7 @@ use App\Http\Controllers\Api\MacAppController; use App\Http\Controllers\Api\MessageImageController; use App\Http\Controllers\Api\MessageLinkPreviewController; +use App\Http\Controllers\Api\MessagePinController; use App\Http\Controllers\Api\MessageReactionController; use App\Http\Controllers\Api\MessageVoiceController; use App\Http\Controllers\Api\NotificationController; @@ -274,6 +275,17 @@ ->name('api-message-reaction.destroy'); }); + // Remote Pins: an org's maintainers pinning and unpinning messages for everyone in it... + Route::middleware('throttle:pins')->group(function (): void { + Route::put('remotes/messages/{message}/pin', [MessagePinController::class, 'update']) + ->whereNumber('message') + ->middleware(PreventPostingInTimeout::class) + ->name('api-message-pin.update'); + Route::delete('remotes/messages/{message}/pin', [MessagePinController::class, 'destroy']) + ->whereNumber('message') + ->name('api-message-pin.destroy'); + }); + // Notifications... Route::get('notifications', [NotificationController::class, 'index'])->name('api-notification.index'); Route::get('notifications/{notification}/group', [NotificationGroupController::class, 'show'])->name('api-notification-group.show'); diff --git a/tests/Feature/Controllers/Api/MessagePinControllerTest.php b/tests/Feature/Controllers/Api/MessagePinControllerTest.php new file mode 100644 index 000000000..a17e3ba1b --- /dev/null +++ b/tests/Feature/Controllers/Api/MessagePinControllerTest.php @@ -0,0 +1,168 @@ + true, 'remotes.everyone' => true]); + + $this->user = User::factory()->withRemotes()->create(['username' => 'octocat']); + $this->keyPair = apiKeyPairFor($this->user); + $this->nuno = User::factory()->withRemotes()->create(['username' => 'nunomaduro']); +}); + +it('pins a message for the whole org once, and answers with the pinned messages as the thread shows them', function (): void { + [$taylor] = followersOf($this->user, 1); + $org = createOrg($this->user, [$taylor]); + $message = pushToOrg($taylor, $org, 'deploys go out on fridays now'); + $version = Conversation::query()->findOrFail($org)->reactions_version; + + $pinned = signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $message)) + ->assertOk() + ->assertJsonCount(1, 'pinned') + ->assertJsonPath('pinned.0.id', $message->id) + ->assertJsonPath('pinned.0.body', 'deploys go out on fridays now') + ->assertJsonPath('pinned.0.user.lane', $taylor->lane()) + ->assertJsonPath('pinned.0.created_at', $message->created_at->toIso8601ZuluString()) + ->assertJsonPath('reactions_version', $version + 1) + ->json('pinned'); + + signedApiRequest($this->keyPair, 'GET', route('api-org-message.index', $org))->assertJsonPath('data.pinned', $pinned); + + $pinnedAt = $message->fresh()?->pinned_at; + $this->travel(5)->minutes(); + + signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $message)) + ->assertOk() + ->assertJsonPath('pinned.*.id', [$message->id]) + ->assertJsonPath('reactions_version', $version + 1); + + expect($pinnedAt)->not->toBeNull() + ->and($message->fresh()?->pinned_at?->equalTo($pinnedAt))->toBeTrue(); +}); + +it('unpins a message, however often', function (): void { + [$taylor] = followersOf($this->user, 1); + $org = createOrg($this->user, [$taylor]); + $kept = pushToOrg($taylor, $org, 'kept'); + $dropped = pushToOrg($taylor, $org, 'dropped'); + signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $kept))->assertOk(); + signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $dropped))->assertOk(); + + $version = Conversation::query()->findOrFail($org)->reactions_version; + + signedApiRequest($this->keyPair, 'DELETE', route('api-message-pin.destroy', $dropped)) + ->assertOk() + ->assertJsonPath('pinned.*.id', [$kept->id]) + ->assertJsonPath('reactions_version', $version + 1); + + expect($dropped->fresh()?->pinned_at)->toBeNull(); + + signedApiRequest($this->keyPair, 'DELETE', route('api-message-pin.destroy', $dropped)) + ->assertOk() + ->assertJsonPath('pinned.*.id', [$kept->id]) + ->assertJsonPath('reactions_version', $version + 1); +}); + +it('only lets maintainers pin and unpin', function (): void { + $this->user->following()->attach($this->nuno); + $org = createOrg($this->nuno, [$this->user]); + $message = pushToOrg($this->nuno, $org); + + signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $message)) + ->assertForbidden() + ->assertJsonPath('message', 'Only maintainers can pin messages.'); + + $this->actingAs($this->nuno)->putJson(route('message-pin.update', $message))->assertOk(); + + signedApiRequest($this->keyPair, 'DELETE', route('api-message-pin.destroy', $message)) + ->assertForbidden() + ->assertJsonPath('message', 'Only maintainers can unpin messages.'); + + expect($message->fresh()?->pinned_at)->not->toBeNull(); +}); + +it('is not there outside the orgs the user is in, or while they have Remotes off', function (string $case, string $method): void { + [$taylor] = followersOf($this->user, 1); + $message = match ($case) { + 'not a member' => pushToOrg($this->nuno, createOrg($this->nuno, followersOf($this->nuno, 1))), + 'direct message' => sendChatMessage($this->nuno, $this->user), + default => pushToOrg($taylor, createOrg($this->user, [$taylor])), + }; + + if ($case === 'remotes off') { + $this->user->update(['remotes_enabled' => false]); + } + + signedApiRequest($this->keyPair, $method, route($method === 'PUT' ? 'api-message-pin.update' : 'api-message-pin.destroy', $message))->assertNotFound(); + + expect($message->fresh()?->pinned_at)->toBeNull(); +})->with(['not a member', 'direct message', 'remotes off'])->with(['PUT', 'DELETE']); + +it('pins no org events', function (): void { + [$taylor] = followersOf($this->user, 1); + $org = createOrg($this->user, [$taylor]); + $event = Message::query()->where('conversation_id', $org)->whereNotNull('event')->sole(); + + signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $event))->assertNotFound(); +}); + +it('pins only within the history the maintainer can see, but unpins whatever the org pinned', function (): void { + [$taylor] = followersOf($this->nuno, 1); + $this->user->following()->attach($this->nuno); + $org = createOrg($this->nuno, [$taylor]); + $before = pushToOrg($taylor, $org, 'from before octocat joined'); + $this->actingAs($this->nuno)->postJson(route('org-member.store', $org), ['members' => [$this->user->username], 'share_history' => false])->assertNoContent(); + $this->putJson(route('org-member.update', [$org, $this->user]), ['role' => 'maintainer'])->assertNoContent(); + + signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $before))->assertNotFound(); + + $this->actingAs($this->nuno)->putJson(route('message-pin.update', $before))->assertOk(); + + signedApiRequest($this->keyPair, 'DELETE', route('api-message-pin.destroy', $before))->assertOk()->assertJsonPath('pinned', []); +}); + +it('caps how many messages an org pins', function (): void { + [$taylor] = followersOf($this->user, 1); + $org = createOrg($this->user, [$taylor]); + $pinned = Message::factory()->count(Conversation::MAX_PINNED_MESSAGES)->pinned()->create(['conversation_id' => $org, 'user_id' => $taylor->id]); + $message = pushToOrg($taylor, $org); + + signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $message)) + ->assertForbidden() + ->assertJsonPath('message', 'An org can pin up to 25 messages. Unpin one first.'); + + signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $pinned->first())) + ->assertOk() + ->assertJsonCount(Conversation::MAX_PINNED_MESSAGES, 'pinned'); +}); + +it('refuses pins from timed out and banned users', function (string $restriction): void { + [$taylor] = followersOf($this->user, 1); + $message = pushToOrg($taylor, createOrg($this->user, [$taylor])); + + if ($restriction === 'timeout') { + UserTimeout::factory()->for($this->user)->create(); + } else { + resolve(CreateUserBan::class)->handle($this->user, ModerationReason::Spam); + } + + signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $message)) + ->assertForbidden() + ->assertJsonPath('code', $restriction === 'timeout' ? 'timed_out' : 'banned'); + + expect($message->fresh()?->pinned_at)->toBeNull(); +})->with(['timeout', 'banned']); + +it('rejects requests that are not signed', function (): void { + [$taylor] = followersOf($this->user, 1); + $message = pushToOrg($taylor, createOrg($this->user, [$taylor])); + + $this->putJson(route('api-message-pin.update', $message))->assertUnauthorized(); +}); From 1dde919dde068252021d4470850d0e0bee42567c Mon Sep 17 00:00:00 2001 From: Simon Hamp Date: Sun, 25 Oct 2026 01:03:47 +0100 Subject: [PATCH 3/4] fix: keep the pin limit under concurrent pins and leave reply ids out of API pins The pin action now checks the limit again after it takes the conversation lock, so two maintainers pinning the 25th message at once can't end up with 26. The request's check still gives the usual message; the losing pin just doesn't happen, and the web treats a pin missing from the answer as failed. API pins no longer carry reply_to_id, matching the web, so a member without history doesn't get the id of a message they can't read. Co-Authored-By: Claude Opus 5.5 --- app/Actions/UpdateMessagePin.php | 10 ++++-- app/Http/Resources/MessageResource.php | 4 +-- resources/js/lib/chat.ts | 7 ++++- resources/js/lib/pinned-messages.ts | 11 +++++++ .../Api/MessagePinControllerTest.php | 31 +++++++++++++++++++ .../Controllers/MessagePinControllerTest.php | 21 +++++++++++++ tests/Unit/Actions/UpdateMessagePinTest.php | 31 +++++++++++++++++++ tests/Unit/PinnedMessages.test.ts | 6 ++++ 8 files changed, 115 insertions(+), 6 deletions(-) create mode 100644 tests/Unit/Actions/UpdateMessagePinTest.php diff --git a/app/Actions/UpdateMessagePin.php b/app/Actions/UpdateMessagePin.php index d21279f4b..7af659a1c 100644 --- a/app/Actions/UpdateMessagePin.php +++ b/app/Actions/UpdateMessagePin.php @@ -18,16 +18,20 @@ public function __construct(private UpdateMessageReactionsVersion $updateMessage /** * Pins the message for everyone in its org, keeping when it was first pinned however often it is pinned again. The * conversation is locked before the message is updated, as reactions lock it, so a pin and a reaction on the same - * message can't deadlock. + * message can't deadlock. The limit is checked again under that lock, as two maintainers pinning at once can both + * pass the request's check: the second pin then just doesn't happen. */ public function handle(Message $message): void { DB::transaction(function () use ($message): void { Conversation::query()->whereKey($message->conversation_id)->lockForUpdate()->first(); - $pinned = Message::query()->whereKey($message->id)->whereNull('pinned_at')->toBase()->update(['pinned_at' => now()]); + $hasRoom = Message::query() + ->where('conversation_id', $message->conversation_id) + ->whereNotNull('pinned_at') + ->count() < Conversation::MAX_PINNED_MESSAGES; - if ($pinned > 0) { + if ($hasRoom && Message::query()->whereKey($message->id)->whereNull('pinned_at')->toBase()->update(['pinned_at' => now()]) > 0) { $this->updateMessageReactionsVersion->handle($message); } }); diff --git a/app/Http/Resources/MessageResource.php b/app/Http/Resources/MessageResource.php index b995a4b68..5dd9f5e70 100644 --- a/app/Http/Resources/MessageResource.php +++ b/app/Http/Resources/MessageResource.php @@ -20,7 +20,7 @@ * A message as the web's thread shows it: its body, also split into prose, inline code and highlighted code blocks, * where it mentions people the message links to, the message it replies to only as much as its ghost shows, and when * they were loaded its link card, images, voice note and reactions. In an org it also says who wrote it and the event - * it records. + * it records. Which message it replies to is left out wherever the model hides it, as pinned messages do. * * The app's image views and audio players can't sign requests, so each image and voice note comes with a URL signed * for the user, good until midnight UTC the day after tomorrow, which stays the same all day so the app can cache it. @@ -43,7 +43,7 @@ public function toArray(Request $request): array 'mentions' => collect(resolve(Mentions::class)->positions($this->body)) ->filter(fn (array $mention): bool => array_key_exists(Str::lower($mention['username']), $this->mentions ?? [])) ->values(), - 'reply_to_id' => $this->reply_to_id, + 'reply_to_id' => $this->when(! in_array('reply_to_id', $this->getHidden(), true), $this->reply_to_id), 'reply_to' => $this->whenLoaded('replyTo', fn (Message $replyTo): array => [ 'id' => $replyTo->id, 'user_id' => $replyTo->user_id, diff --git a/resources/js/lib/chat.ts b/resources/js/lib/chat.ts index 5fe3c253c..077aeb36d 100644 --- a/resources/js/lib/chat.ts +++ b/resources/js/lib/chat.ts @@ -27,6 +27,7 @@ import UserBlockController from '@/actions/App/Http/Controllers/UserBlockControl import { showTimeout, timeoutFrom } from '@/components/timeout-dialog'; import { acceptedPinnedMessages, + isPinnedIn, reactionsAfterOf, withPinnedMessage, withPinnedMessageRestored, @@ -1634,7 +1635,7 @@ export async function toggleReaction( /** * Pins a message for the whole org, or unpins it, straight away, then settles on the org's pinned messages as the * server holds them, once no other pin request in the org is in flight. Puts the message back where it was and throws - * when the server refuses. + * when the server refuses, or answers without the message among the pinned ones. */ export async function setMessagePinned( target: ChatTarget, @@ -1672,6 +1673,10 @@ export async function setMessagePinned( if ((newestPinnedMessages.get(key)?.version ?? -1) <= version) { newestPinnedMessages.set(key, { pinned, version, sentAt }); } + + if (isPinned && !isPinnedIn(pinned, message)) { + throw new Error('Another pin took the last place first.'); + } } catch (error) { updateThread(key, (thread) => ({ pinned: isPinned diff --git a/resources/js/lib/pinned-messages.ts b/resources/js/lib/pinned-messages.ts index 21a8307b8..3393ebc1f 100644 --- a/resources/js/lib/pinned-messages.ts +++ b/resources/js/lib/pinned-messages.ts @@ -70,6 +70,17 @@ export function withShownReactions( ); } +/** + * Whether a pin went through. When two maintainers take the org's last place at once, the server still answers the one + * it turned away with the org's pinned messages, just without theirs. + */ +export function isPinnedIn( + pinned: Message[], + message: Pick, +): boolean { + return pinned.some((item) => item.id === message.id); +} + export function withPinnedMessage( pinned: Message[], message: Message, diff --git a/tests/Feature/Controllers/Api/MessagePinControllerTest.php b/tests/Feature/Controllers/Api/MessagePinControllerTest.php index a17e3ba1b..e7a556d04 100644 --- a/tests/Feature/Controllers/Api/MessagePinControllerTest.php +++ b/tests/Feature/Controllers/Api/MessagePinControllerTest.php @@ -8,6 +8,8 @@ use App\Models\Message; use App\Models\User; use App\Models\UserTimeout; +use Illuminate\Support\Arr; +use Illuminate\Support\Str; beforeEach(function (): void { config(['remotes.requests' => true, 'remotes.everyone' => true]); @@ -128,6 +130,35 @@ signedApiRequest($this->keyPair, 'DELETE', route('api-message-pin.destroy', $before))->assertOk()->assertJsonPath('pinned', []); }); +it('leaves out what a pinned message replies to, which may be from before the member history', function (): void { + [$nuno, $taylor] = followersOf($this->user, 2); + $org = createOrg($this->user, [$nuno]); + $question = pushToOrg($nuno, $org, 'where does the deploy key live?'); + $answer = $this->actingAs($this->user)->postJson(route('org-message.store', $org), ['body' => 'in the vault', 'client_id' => Str::uuid()->toString(), 'reply_to_id' => $question->id]) + ->assertCreated() + ->json('message.id'); + $this->postJson(route('org-member.store', $org), ['members' => [$taylor->username], 'share_history' => false])->assertNoContent(); + $this->putJson(route('org-member.update', [$org, $taylor]), ['role' => 'maintainer'])->assertNoContent(); + $welcome = pushToOrg($nuno, $org, 'welcome taylor'); + signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $answer))->assertOk(); + $taylorKeys = apiKeyPairFor($taylor); + + $lists = [ + signedApiRequest($taylorKeys, 'PUT', route('api-message-pin.update', $welcome))->assertOk()->json('pinned'), + signedApiRequest($taylorKeys, 'DELETE', route('api-message-pin.destroy', $welcome))->assertOk()->json('pinned'), + signedApiRequest($taylorKeys, 'GET', route('api-org-message.index', $org))->assertOk()->json('data.pinned'), + ]; + + foreach ($lists as $pinned) { + expect(array_column($pinned, 'id'))->toContain($answer); + + foreach ($pinned as $message) { + expect($message)->not->toHaveKeys(['reply_to', 'reply_to_id']) + ->and(Arr::dot(Arr::except($message, 'user')))->not->toContain($question->id); + } + } +}); + it('caps how many messages an org pins', function (): void { [$taylor] = followersOf($this->user, 1); $org = createOrg($this->user, [$taylor]); diff --git a/tests/Feature/Controllers/MessagePinControllerTest.php b/tests/Feature/Controllers/MessagePinControllerTest.php index ad6b2b2f2..14d8e501d 100644 --- a/tests/Feature/Controllers/MessagePinControllerTest.php +++ b/tests/Feature/Controllers/MessagePinControllerTest.php @@ -2,6 +2,7 @@ declare(strict_types=1); +use App\Actions\UpdateMessagePin; use App\Models\Conversation; use App\Models\Message; use App\Models\User; @@ -137,3 +138,23 @@ $this->deleteJson(route('message-pin.destroy', $pinned->first()))->assertOk(); $this->putJson(route('message-pin.update', $message))->assertOk()->assertJsonPath('pinned.0.id', $message->id); }); + +it('leaves a message unpinned when another pin fills the org after the request was allowed', function (): void { + $mark = User::factory()->create(); + [$nuno] = followersOf($mark, 1); + $org = createOrg($mark, [$nuno]); + Message::factory()->count(Conversation::MAX_PINNED_MESSAGES - 1)->pinned()->create(['conversation_id' => $org, 'user_id' => $nuno->id]); + $theirs = pushToOrg($nuno, $org, 'pinned by another maintainer meanwhile'); + $mine = pushToOrg($nuno, $org, 'mine'); + $version = Conversation::query()->findOrFail($org)->reactions_version; + + app()->resolving(UpdateMessagePin::class, fn () => Message::query()->whereKey($theirs->id)->update(['pinned_at' => now()])); + + $response = $this->actingAs($mark)->putJson(route('message-pin.update', $mine)) + ->assertOk() + ->assertJsonCount(Conversation::MAX_PINNED_MESSAGES, 'pinned') + ->assertJsonPath('reactions_version', $version); + + expect($response->json('pinned.*.id'))->toContain($theirs->id)->not->toContain($mine->id) + ->and($mine->fresh()?->pinned_at)->toBeNull(); +}); diff --git a/tests/Unit/Actions/UpdateMessagePinTest.php b/tests/Unit/Actions/UpdateMessagePinTest.php new file mode 100644 index 000000000..c0926690b --- /dev/null +++ b/tests/Unit/Actions/UpdateMessagePinTest.php @@ -0,0 +1,31 @@ +create(); + [$member] = followersOf($maintainer, 1); + $org = Conversation::query()->findOrFail(createOrg($maintainer, [$member])); + Message::factory()->count(Conversation::MAX_PINNED_MESSAGES - 1)->pinned()->create(['conversation_id' => $org->id, 'user_id' => $member->id]); + $first = pushToOrg($member, $org->id, 'first'); + $second = pushToOrg($member, $org->id, 'second'); + + expect(UpdateMessagePinRequest::authorization($maintainer, $first)->allowed())->toBeTrue() + ->and(UpdateMessagePinRequest::authorization($maintainer, $second)->allowed())->toBeTrue(); + + $version = $org->fresh()?->reactions_version; + + resolve(UpdateMessagePin::class)->handle($first); + resolve(UpdateMessagePin::class)->handle($second); + + expect($org->messages()->whereNotNull('pinned_at')->count())->toBe(Conversation::MAX_PINNED_MESSAGES) + ->and($first->fresh()?->pinned_at)->not->toBeNull() + ->and($second->fresh()?->pinned_at)->toBeNull() + ->and($org->fresh()?->reactions_version)->toBe($version + 1); +}); diff --git a/tests/Unit/PinnedMessages.test.ts b/tests/Unit/PinnedMessages.test.ts index fe12908c9..dd30a1aba 100644 --- a/tests/Unit/PinnedMessages.test.ts +++ b/tests/Unit/PinnedMessages.test.ts @@ -2,6 +2,7 @@ import assert from 'node:assert/strict'; import { test } from 'node:test'; import { acceptedPinnedMessages, + isPinnedIn, reactionsAfterOf, withPinnedMessage, withPinnedMessageRestored, @@ -134,3 +135,8 @@ await test('unpins a message and puts it back where it was', () => { ); assert.deepEqual(ids(withPinnedMessageRestored([], message(2), 1)), [2]); }); + +await test('counts a pin as done only when the settled list has the message', () => { + assert.equal(isPinnedIn([message(2), message(1)], message(1)), true); + assert.equal(isPinnedIn([message(2)], message(1)), false); +}); From f22635693b976a9fcd9f74780e2d98b7596967e1 Mon Sep 17 00:00:00 2001 From: Simon Hamp Date: Mon, 26 Oct 2026 09:20:57 +0000 Subject: [PATCH 4/4] fix: include pinned_at in API messages The web's message shape has pinned_at; the API's didn't, so the app couldn't show when a pinned message was pinned. MessageResource now includes it, in the same ISO 8601 UTC form as created_at. Co-Authored-By: Claude Opus 5.5 --- app/Http/Resources/MessageResource.php | 1 + tests/Feature/Controllers/Api/MessagePinControllerTest.php | 3 +++ 2 files changed, 4 insertions(+) diff --git a/app/Http/Resources/MessageResource.php b/app/Http/Resources/MessageResource.php index 5dd9f5e70..ed54e1726 100644 --- a/app/Http/Resources/MessageResource.php +++ b/app/Http/Resources/MessageResource.php @@ -76,6 +76,7 @@ public function toArray(Request $request): array 'subject_id' => $this->subject_id, 'user' => $this->whenLoaded('user', fn (User $user): UserResource => UserResource::make($user)), 'subject' => $this->whenLoaded('subject', fn (User $subject): UserResource => UserResource::make($subject)), + 'pinned_at' => $this->pinned_at?->toIso8601ZuluString(), 'created_at' => $this->created_at->toIso8601ZuluString(), ]; } diff --git a/tests/Feature/Controllers/Api/MessagePinControllerTest.php b/tests/Feature/Controllers/Api/MessagePinControllerTest.php index e7a556d04..973bd9e95 100644 --- a/tests/Feature/Controllers/Api/MessagePinControllerTest.php +++ b/tests/Feature/Controllers/Api/MessagePinControllerTest.php @@ -38,6 +38,9 @@ signedApiRequest($this->keyPair, 'GET', route('api-org-message.index', $org))->assertJsonPath('data.pinned', $pinned); $pinnedAt = $message->fresh()?->pinned_at; + + expect($pinned[0]['pinned_at'])->toBe($pinnedAt?->toIso8601ZuluString()); + $this->travel(5)->minutes(); signedApiRequest($this->keyPair, 'PUT', route('api-message-pin.update', $message))