Fix code review feedback (closes #5) #6

clawbot · 2026-02-10T06:16:11+01:00

clawbot commented

2026-02-10 06:16:11 +01:00

Addresses all review items from PR #4 (closes #5):

Changes

Item 1: Duplicated find-by-ID queries — Extracted GetUserByID() and GetChannelByID() on Database. Model relation methods (AuthToken.User, Session.User, ChannelMember.User, ChannelMember.Channel) now delegate via UserLookup/ChannelLookup interfaces.
Item 2: Relation methods return nil on empty — Changed var slice []*T to slice := []*T{} in all collection methods (Members, RecentMessages, Channels, QueuedMessages) so JSON marshals to [] not null.
Item 3: No timestamps on Create methods — All Create methods now set CreatedAt/UpdatedAt/JoinedAt/QueuedAt via time.Now() on the returned model.
Item 4: No transactions in migration runner — Each migration now runs inside a BeginTx/Commit with Rollback on failure.
Item 5: QueueMessage ignores LastInsertId error — Error is now checked and propagated.
Item 6: No dequeue/ack for message queue — Added DequeueMessages(ctx, userID, limit) and AckMessages(ctx, entryIDs) methods.
Item 8: Missing lookup methods — Added GetUserByNick(), GetUserByToken(), DeleteAuthToken(), UpdateUserLastSeen().
Item 9: SQLite foreign keys not enabled — PRAGMA foreign_keys = ON runs in both connect() and NewTest().
Item 10: General cleanup — Builds clean, all tests pass.

Addresses all review items from PR #4 (closes #5): ## Changes - **Item 1: Duplicated find-by-ID queries** — Extracted `GetUserByID()` and `GetChannelByID()` on Database. Model relation methods (AuthToken.User, Session.User, ChannelMember.User, ChannelMember.Channel) now delegate via UserLookup/ChannelLookup interfaces. - **Item 2: Relation methods return nil on empty** — Changed `var slice []*T` to `slice := []*T{}` in all collection methods (Members, RecentMessages, Channels, QueuedMessages) so JSON marshals to `[]` not `null`. - **Item 3: No timestamps on Create methods** — All Create methods now set CreatedAt/UpdatedAt/JoinedAt/QueuedAt via `time.Now()` on the returned model. - **Item 4: No transactions in migration runner** — Each migration now runs inside a BeginTx/Commit with Rollback on failure. - **Item 5: QueueMessage ignores LastInsertId error** — Error is now checked and propagated. - **Item 6: No dequeue/ack for message queue** — Added `DequeueMessages(ctx, userID, limit)` and `AckMessages(ctx, entryIDs)` methods. - **Item 8: Missing lookup methods** — Added `GetUserByNick()`, `GetUserByToken()`, `DeleteAuthToken()`, `UpdateUserLastSeen()`. - **Item 9: SQLite foreign keys not enabled** — `PRAGMA foreign_keys = ON` runs in both `connect()` and `NewTest()`. - **Item 10: General cleanup** — Builds clean, all tests pass.

sneak was assigned by clawbot

2026-02-10 06:16:11 +01:00

clawbot added 1 commit 2026-02-10 06:16:11 +01:00

Fix code review feedback items 1-6, 8-10 fbe53179b8

- Item 1: Extract GetUserByID/GetChannelByID lookup methods, use from relation methods
- Item 2: Initialize slices with literals so JSON gets [] not null
- Item 3: Populate CreatedAt/UpdatedAt with time.Now() on all Create methods
- Item 4: Wrap each migration's SQL + recording in a transaction
- Item 5: Check error from res.LastInsertId() in QueueMessage
- Item 6: Add DequeueMessages and AckMessages methods
- Item 8: Add GetUserByNick, GetUserByToken, DeleteAuthToken, UpdateUserLastSeen
- Item 9: Run PRAGMA foreign_keys = ON on every new connection
- Item 10: Builds clean, all tests pass

clawbot referenced this pull request

2026-02-10 06:16:17 +01:00

Add complete database schema and ORM models #4

clawbot merged commit 98a02a150d into feature/database-schema

2026-02-10 18:18:06 +01:00

clawbot referenced this issue from a commit

2026-02-10 18:18:07 +01:00

Merge pull request 'Fix code review feedback (closes #5)' (#6) from fix/review-feedback into feature/database-schema

Sign in to join this conversation.

No reviewers