feat: add pinned messages - #117
Conversation
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs maintainer review before merge. Reviewed August 1, 2026, 7:47 PM ET / 23:47 UTC. ClawSweeper reviewWhat this changesAdds shared channel-message pins with SQLite and PostgreSQL persistence, three HTTP operations, SDK/OpenAPI types, durable realtime events, and web controls for listing, pinning, and unpinning messages. Merge readinessKeep this PR open for maintainer review. Current Priority: P2 Review scores
Verification
How this fits togetherClickClack persists channel messages in SQLite or PostgreSQL, exposes them through HTTP and the TypeScript SDK, and updates connected web clients through durable realtime events. This PR adds a shared per-channel pin record and a pinned-message panel to that existing message lifecycle. flowchart LR
A[Channel messages] --> B[Pin and unpin API]
B --> C[SQLite or PostgreSQL pin records]
C --> D[Durable pin events]
D --> E[Realtime client refresh]
E --> F[Pinned messages panel]
C --> G[OpenAPI and SDK clients]
Decision needed
Why: The branch is coherent and proof-backed, but choosing to expose and support a persisted collaboration feature and its external contracts is a product decision, not a mechanical correctness judgment. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Sponsor shared channel pins as a documented public contract, retaining the migration, API, SDK, event, export, and realtime behavior together; otherwise pause rather than partially merging parallel pin surfaces. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR proposes a new capability rather than repairing a defined broken behavior. Its body nevertheless supplies a detailed real-server Playwright scenario covering persistence, realtime refresh, thread replies, reloads, and unpinning. Is this the best way to solve the issue? Unclear: the implementation is internally consistent and current main lacks an equivalent capability, but whether shared pins should become a supported persisted API and event contract requires maintainer product direction. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4843afe2e8eb. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (29 earlier review cycles; latest 8 shown)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea11592ede
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return | ||
| } | ||
|
|
||
| pin, event, err := s.store.PinMessage(r.Context(), channelID, body.MessageID, act.user.ID) |
There was a problem hiding this comment.
When a bot identity belongs to multiple workspaces, a token issued for workspace A can reach this store call with a channel from workspace B and read or mutate B's shared pins, because membership checks use only act.user.ID. The existing bot-token isolation test models this exact multi-workspace identity, and other channel handlers call requireBotChannelWorkspace; apply the same guard in all three pin handlers before invoking the store.
Useful? React with 👍 / 👎.
| if (event.type === "message.deleted" && pinnedMessageIDs.has(event.payload.message_id || "")) { | ||
| await loadPinnedMessages(); |
There was a problem hiding this comment.
When a pinned message is edited while the pin panel is open, message.updated reloads the timeline but this invalidation only refreshes pins for message.deleted. Because pinnedMessages contains separately fetched message objects, the panel continues displaying the old body and attachments until another pin event, channel navigation, or manual panel reload; refresh or update the pin cache for message.updated as well.
Useful? React with 👍 / 👎.
| (event.type === "pin.added" || event.type === "pin.removed") && | ||
| event.channel_id === selectedChannelID && | ||
| !selectedDirectID | ||
| ) { | ||
| await loadPinnedMessages(); |
There was a problem hiding this comment.
When the realtime wrapper forces an authoritative resync after an unusable cursor, it advances to the current tail instead of replaying missed events, but the resync reloads messages and channel data without reloading pinnedMessages. Consequently pin changes made during the gap remain stale in an open panel and in message-menu pin labels; the pin-event handler cannot repair this because those events were intentionally skipped, so include the active channel's pins in reconcileRealtimeState.
Useful? React with 👍 / 👎.
| channelID={selectedChannelID} | ||
| {pinnedMessageIDs} | ||
| onTogglePin={toggleMessagePin} |
There was a problem hiding this comment.
The pin state and toggle callback are wired only into MessageList, while channel thread replies are rendered separately by ThreadPanel, which receives neither prop and offers no pin control. The store accepts replies because they retain the channel ID, and the new pinned-panel navigation explicitly handles parent_message_id, so users can view API-created reply pins but cannot create or toggle them through the app; pass the same pin state and callback into the thread surface.
Useful? React with 👍 / 👎.
| for _, row := range rows { | ||
| message, err := getMessage(ctx, s.db, row.MessageID) |
There was a problem hiding this comment.
For a channel at the supported 100-pin limit, every list request performs the initial pin query and then up to 100 sequential getMessage queries; on PostgreSQL this adds a database round trip per pin before attachment and reaction hydration and can make opening the panel disproportionately slow. Return the complete message rows with one typed sqlc query, or batch-fetch all selected IDs, while preserving the pin ordering.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
ea11592 to
7d352a1
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
7d352a1 to
fe4e99c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe4e99c6d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| defer tx.Rollback() | ||
|
|
||
| msg, err := getMessageTx(ctx, tx, messageID) |
There was a problem hiding this comment.
In PostgreSQL, when DeleteMessage commits after this unlocked getMessageTx read but before the pin insert, this transaction can still insert a pin and emit pin.added for the now-deleted message. The new pin is then immediately absent from ListPinnedMessages because that query filters m.deleted_at IS NULL, despite the POST having returned 201. Lock the message row through the validation/insert or make the insert conditional on the message remaining undeleted.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,12 @@ | |||
| CREATE TABLE pinned_messages ( | |||
There was a problem hiding this comment.
When an operator runs clickclack export, both the SQLite and PostgreSQL ExportJSON implementations read from explicit table allowlists that do not include this new pinned_messages table. Every JSON export therefore silently drops all channel pin state, including exports used for migrations or audits; add the table to both export allowlists and cover it in the export tests.
Useful? React with 👍 / 👎.
| LEFT JOIN bot_tombstones quoted_tombstone ON quoted_tombstone.bot_user_id = qu.id | ||
| WHERE p.workspace_id = sqlc.arg(workspace_id) | ||
| AND p.channel_id = sqlc.arg(channel_id) | ||
| AND m.deleted_at IS NULL |
There was a problem hiding this comment.
When a pinned message is soft-deleted, this filter makes it disappear from the authoritative pin list, but DeleteMessage neither deletes its pinned_messages row nor emits pin.removed; it emits only message.deleted. An outgoing subscription configured for exactly pin.added and pin.removed—which the new event registry explicitly supports—will consequently retain the deleted message forever in its replicated pin state. Remove the pin and emit the corresponding event as part of the deletion transaction.
Useful? React with 👍 / 👎.
| try { | ||
| await onTogglePin(message, pinnedMessageIDs.has(message.id)); |
There was a problem hiding this comment.
When pinning from a thread fails—for example because the channel has reached 100 pins, another client already changed the pin, or the request loses connectivity—onTogglePin rejects and this try only resets the saving flag. The toolbar invokes togglePin with void, and the action-sheet callback also has no error boundary, so users receive an unhandled promise rejection and no visible failure feedback; catch the error here and expose it as the timeline and pinned-panel actions already do.
Useful? React with 👍 / 👎.
| <button type="submit" class="search-submit">Search</button> | ||
| </form> | ||
|
|
||
| <button type="button" class="desktop-titlebar-pin" title="Pinned items" aria-label="Pinned items" onclick={onPinnedItems}> |
There was a problem hiding this comment.
When a direct conversation is selected, this title-bar button is still rendered and enabled even though direct messages do not support pins. Its handler immediately returns because selectedDirectID is set, so activating the visible “Pinned items” control does nothing; the regular top bar has the same behavior after it is wired to togglePinnedPanel. Render or enable the control only when a channel is active.
Useful? React with 👍 / 👎.
fe4e99c to
d8591ed
Compare
|
@codex review |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8591ed819
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!channelID || !onTogglePin || pinSaving) return; | ||
| pinSaving = true; | ||
| try { | ||
| await onTogglePin(message, pinnedMessageIDs.has(message.id)); |
There was a problem hiding this comment.
When the pin request is rejected—for example, with the expected 409 after the channel reaches 100 pins—or the network fails, onTogglePin rejects and this try/finally propagates the error. The toolbar explicitly discards that promise, so users receive an unhandled rejection with no visible explanation, while the action-sheet path remains open without reporting why the operation failed. Catch the error and expose a status as MessageRow does for the same action.
Useful? React with 👍 / 👎.
| pinnedPanelOpen = false; | ||
| return; | ||
| } | ||
| if (!selectedChannelID || selectedDirectID) return; |
There was a problem hiding this comment.
When a direct message or an empty workspace is selected, both top-bar variants still present an enabled “Pinned items” button, but this branch silently returns because pins are channel-only. The previous top-bar stub at least reported that there were no pinned items, while the new desktop control also routes here, so these contexts now expose a control that appears operable but has no effect. Hide or disable the control when there is no selected channel, or provide explicit unsupported-state feedback.
Useful? React with 👍 / 👎.
| (event.type === "message.updated" || event.type === "message.deleted") && | ||
| pinnedMessageIDs.has(event.payload.message_id || "") | ||
| ) { | ||
| await loadPinnedMessages(); |
There was a problem hiding this comment.
If this client successfully edits or deletes a pinned message over HTTP while its WebSocket is disconnected or blocked, the local mutation handlers update messages, replies, and selectedThread but never update pinnedMessages. This realtime-only refresh therefore never runs, leaving edited text stale and deleted messages visible in the pinned panel indefinitely until a later reconnect or reload. Update or remove the corresponding pinned entry as part of the successful local mutation path rather than relying exclusively on the echoed event.
Useful? React with 👍 / 👎.
…s-lifecycle # Conflicts: # apps/api/internal/webassets/dist/200.html # apps/api/internal/webassets/dist/_app/immutable/entry/app.C0jTAm08.js # apps/api/internal/webassets/dist/_app/immutable/nodes/0.erneJbxK.js # apps/api/internal/webassets/dist/_app/immutable/nodes/11.BPwHOykE.js # apps/api/internal/webassets/dist/_app/immutable/nodes/11.COwgNK6M.js # apps/api/internal/webassets/dist/_app/immutable/nodes/11.vvKR-XrD.js # apps/api/internal/webassets/dist/_app/immutable/nodes/12.D2jzbKVo.js # apps/api/internal/webassets/dist/_app/immutable/nodes/13.0lkaA9sE.js # apps/api/internal/webassets/dist/_app/immutable/nodes/3.BoPssj0u.js # apps/api/internal/webassets/dist/_app/immutable/nodes/4.B1Q5W4BL.js # apps/api/internal/webassets/dist/_app/immutable/nodes/4.CV83zLxh.js # apps/api/internal/webassets/dist/_app/immutable/nodes/4.D81lYOij.js # apps/api/internal/webassets/dist/_app/immutable/nodes/5.C04PopFT.js # apps/api/internal/webassets/dist/_app/immutable/nodes/5.DW2e0mkr.js # apps/api/internal/webassets/dist/_app/immutable/nodes/5.gnDXXEBJ.js # apps/api/internal/webassets/dist/index.html
fix: complete pinned message lifecycle
clawsweeper
Bot
added
status: 👀 ready for maintainer look
|
The lifecycle assistance branch has been merged into this PR and the latest ClawSweeper review has no actionable code findings. I opened a narrow follow-up for the current PostgreSQL CI failure: jjjhenriksen#2. It adds the missing per-test migrations for the three new isolated-schema tests; once Jacqueline merges it, the upstream CI can rerun on the same feature head. |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
test: migrate isolated PostgreSQL pin stores
clawsweeper
Bot
added
rating: 🐚 platinum hermit
clawsweeper
Bot
added
the
merge-risk: 🚨 session-state
2 participants
Summary
pin.addedandpin.removedrealtime eventsReal behavior proof
The focused Playwright scenario runs against the real ClickClack Go server and embedded web app. It:
201response andpin.addedevent;message.updatedhandling;pin.removedresponse and empty UI state, then verifies the durable API list is empty.Command and result:
The SQLite boundary regression starts with 99 pins, races two requests for the final slot ten times, and proves each run yields exactly one success, one
ErrPinnedMessageLimit, and 100 stored pins. HTTP authorization coverage proves a workspace-bound bot token receives403for pin, unpin, and list operations in another workspace.Validation
The HTTP behavior suite also proves malformed and missing pin payloads return
400, a repeated unpin returns404, and listing after the final unpin serializes a non-null emptymessagesarray. Hosted ChatGPT Codex findings addressed: bot-token workspace guards, refresh on message edits, authoritative resync refresh, thread-reply pin controls, and batched typed PostgreSQL pin listing. ClawSweeper's SQLite concurrency finding is covered by a single conditional write plus the repeated boundary race above. The branch is rebased onto currentmain.Branch head
d8591ed81925f9115ef19e983d0612348fce3f2cis one commit directly onae666669f5ae08e09431d88eded7f012498f12bb(main).