feat(history): write pinned_at to messages_by_room on pin/unpin#301
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a Changespinned_at mirror into messages_by_room
Sequence Diagram(s)sequenceDiagram
participant Client
participant PinMessage
participant hasRoomTimelineRow
participant CassandraBatch
Client->>PinMessage: PinMessage(msg)
PinMessage->>hasRoomTimelineRow: check msg.ThreadParent & msg.TShow
hasRoomTimelineRow-->>PinMessage: withRoomRow (bool)
PinMessage->>CassandraBatch: UPDATE messages_by_id SET pinned_at
PinMessage->>CassandraBatch: INSERT pinned_messages_by_room
alt withRoomRow == true
PinMessage->>CassandraBatch: UPDATE messages_by_room SET pinned_at
end
CassandraBatch-->>Client: nil or error(pinBatchTables(withRoomRow))
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pin state was only stored on messages_by_id and pinned_messages_by_room, so channel history loaded from messages_by_room never populated pinnedAt and timeline pin indicators disappeared after reload. - Schema: add pinned_at TIMESTAMP to messages_by_room (canonical doc, local bootstrap cql, new prod migration) - Write: PinMessage/UnpinMessage gain a third batch statement mirroring pinned_at onto messages_by_room, guarded by hasRoomTimelineRow so pinning a tshow=false thread reply does not upsert a ghost timeline row - Read: add pinned_at to the room-history base column list pinned_by is deliberately not mirrored (open question on the issue). Refs hmchangw#298 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
51e082c to
426d227
Compare
Summary
Pin state was only stored on
messages_by_idandpinned_messages_by_room, so channel history served frommessages_by_roomnever populatedpinnedAt— timeline pin indicators disappeared after a reload. This mirrorspinned_atontomessages_by_roomend-to-end:pinned_at TIMESTAMPadded to the canonical schema indocs/cassandra_message_model.md, the local bootstrapdocker-local/cassandra/init/10-table-messages_by_room.cql(ALTER, following theenc_payloadprecedent), and a new prod migrationdocker-local/cassandra/migrations/2026-06-pinnedat-messages-by-room.cql. The integration-test bootstrap schema inhistory-service/internal/cassrepo/integration_test.gowas updated to match.PinMessage/UnpinMessagegain a third statement in the existing UnloggedBatch:pinned_at = null). The bucket is derived frommsg.CreatedAtvia the Repository's existingmsgbucket.Sizer— the same sizer already used by the room-history read walk and the reactions mirror, so no new wiring.pinned_atadded to the room-history base column list; the struct already had thecql:"pinned_at"tag, so scanning works unchanged.Upsert guard (tshow=false thread replies)
A thread-only reply (
thread_parent_idset,tshow=false) has nomessages_by_roomrow, and a Cassandra UPDATE is an upsert — an unguarded pin would create a ghost timeline row containing nothing but the primary key and pin column. The newhasRoomTimelineRowhelper (msg.ThreadParentID == "" || msg.TShow) gates the third batch statement on both pin and unpin; this mirrors the routing convention already used by the reactions mirror incassrepo/reactions.go. Covered byTestRepository_PinThreadOnlyReply_NoGhostRoomRow.The rejected alternative was accept-and-document (let the ghost row exist; history reads would still never surface it as a real message only if filtered) — rejected because ghost rows would surface in room-history scans as empty messages and pollute the timeline.
Scope
pinned_byis deliberately excluded — it is an explicit open question on #298, not approved. Onlypinned_atis mirrored; richer pin metadata remains a point lookup onmessages_by_id. Addingpinned_bylater is a cheap independent ALTER + batch-statement change.Rows pinned before the migration keep a null
pinned_atonmessages_by_roomuntil re-pinned; a backfill recipe is noted in the migration file header.Test evidence
go build ./...andgo vet -tags=integration ./history-service/...clean.go test ./history-service/...green (includes new unit tests for the guard helper).go test -tags=integration ./history-service/internal/cassrepo/→ok ... 360.7s, including:TestRepository_PinAndUnpinMessage: room row'spinned_atmirrors the pin time, room-historyGetMessagesBeforesurfacesPinnedAt, unpin clears it while the timeline row survives;TestRepository_PinThreadOnlyReply_NoGhostRoomRow: no ghost row on pin or unpin, whilemessages_by_id+pinned_messages_by_roomstill record the pin.Refs #298 (kept open intentionally — the
pinned_byopen question on the issue is undecided; closing is the maintainer's call once that's resolved).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation