feat(store,api): preview + number on conversation summaries (HT-27) - #26
Merged
Conversation
Migration 005 adds the human-facing sequential number: nullable ADD COLUMN, backfill in (created_at, id) order via row_number(), a dedicated sequence setval'd past the backfill, then DEFAULT/NOT NULL/UNIQUE — the 002/004 backfill-before-constraint discipline. The uuid stays the only accepted identifier; number is display-only. preview is derived, not stored: SQL picks the most recent thread with non-null body_text (any direction); derivePreview() collapses whitespace and caps at 120 chars, '' when no thread has text. The API detail handler applies the same exported rule to the threads it already holds, so list and detail can never drift. Per specs/api/agent-inbox-v1.md §2 (v1.1, HT-25). 350/350 tests, including the migration-005 upgrade path over a seeded pre-005 database (out-of-order inserts numbered by creation time; sequence continues at max+1) and preview edge cases (html-only fallback, collapse, cap, empty). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds sequential conversation numbers and normalized previews derived from thread text across the database migration, conversation store, and API list/detail responses, with coverage for backfills, uniqueness, fallback behavior, truncation, and status updates. ChangesConversation summary fields
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConversationAPI
participant ConversationStore
participant PostgreSQL
Client->>ConversationAPI: Request conversation summaries
ConversationAPI->>ConversationStore: listConversations()
ConversationStore->>PostgreSQL: Select number and latest body text
PostgreSQL-->>ConversationStore: Return summary rows
ConversationStore-->>ConversationAPI: Return normalized summaries
ConversationAPI-->>Client: Return number and preview
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/db/migrate.ts (1)
232-239: 🚀 Performance & Scalability | 🔵 TrivialConsider lock duration on large production tables.
ADD COLUMN, backfillUPDATE,SET NOT NULL, andADD CONSTRAINT UNIQUEeach take anACCESS EXCLUSIVElock and/or full table scan onconversations, blocking reads/writes for the duration. Fine for the test-scale fixtures here, but worth confirming this migration's runtime is acceptable against the largest expectedconversationstable size before deploying to a live environment with real traffic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrate.ts` around lines 232 - 239, Review the migration block that adds and backfills conversations.number for production-scale tables, and redesign or sequence the ALTER TABLE, UPDATE, SET NOT NULL, and UNIQUE constraint steps to minimize blocking and full-table lock duration. Preserve the existing row-number backfill, sequence default, non-null requirement, and uniqueness guarantees while ensuring the migration runtime is acceptable for the largest expected conversations table.src/store/conversations.ts (1)
781-781: 🚀 Performance & Scalability | 🔵 TrivialAdd a partial index for
latest_body_text.threads_conversation_id_idxalready coversthread_count, but theORDER BY t.created_at DESC, t.id DESC LIMIT 1lookup still has to sort per conversation. Addthreads (conversation_id, created_at DESC, id DESC) WHERE body_text IS NOT NULLso the preview query stays cheap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/conversations.ts` at line 781, Add a partial index for the threads lookup used by LATEST_BODY_TEXT_SUBQUERY, covering conversation_id, created_at DESC, and id DESC, with a WHERE body_text IS NOT NULL predicate. Preserve the existing thread_count index and ensure the new index supports the ORDER BY and LIMIT 1 access pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/db/migrate.ts`:
- Around line 232-239: Review the migration block that adds and backfills
conversations.number for production-scale tables, and redesign or sequence the
ALTER TABLE, UPDATE, SET NOT NULL, and UNIQUE constraint steps to minimize
blocking and full-table lock duration. Preserve the existing row-number
backfill, sequence default, non-null requirement, and uniqueness guarantees
while ensuring the migration runtime is acceptable for the largest expected
conversations table.
In `@src/store/conversations.ts`:
- Line 781: Add a partial index for the threads lookup used by
LATEST_BODY_TEXT_SUBQUERY, covering conversation_id, created_at DESC, and id
DESC, with a WHERE body_text IS NOT NULL predicate. Preserve the existing
thread_count index and ensure the new index supports the ORDER BY and LIMIT 1
access pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0201322e-64f5-4dab-b6f3-cf9040723e10
📒 Files selected for processing (6)
src/api/conversations.tssrc/api/index.test.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/store/conversations.test.tssrc/store/conversations.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Second v1.1 increment (HT-27) — the inbox-row fields the designed UI renders: a human-facing
#numberand a one-linepreviewexcerpt. Spec:agent-inbox-v1.md§2 (v1.1, merged in #24).What
Migration 005 —
numberADD COLUMN(nullable) → backfill existing rows in(created_at, id)order viarow_number()→CREATE SEQUENCE+setval(max+1, false)→ only thenDEFAULT nextval(...),NOT NULL,UNIQUE.nextvalDEFAULT binds the sequence's OID at ALTER time, so the HT-20 schema option is honored.preview— derived, never storedbody_text(any direction, notes-ready);derivePreview()does the whitespace collapse + 120-char cap in TS.setConversationStatus's RETURNING carries both fields so PATCH responses stay complete summaries.Evidence
''when textless), and wire-shape assertions on list + detail.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes