test(server,db): Postgres-backed SQL integration harness - #108
Conversation
flyingrobots
added
area/db
Summary by CodeRabbit
WalkthroughYou added the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
db/test/30_rpcs.pgtap (1)
34-44: These tests will explode when you fix the schema.Lines 36-38 and 54-58 use hardcoded UUIDs that don't exist in the database:
r uuid := '00000000-0000-0000-0000-0000000000aa'(round_id)a uuid := '00000000-0000-0000-0000-0000000000bb'(author_id)room uuid := '00000000-0000-0000-0000-0000000000ac'round uuid := '00000000-0000-0000-0000-0000000000ad'voter uuid := '00000000-0000-0000-0000-0000000000ae'These tests pass ONLY because submissions.author_id and votes.voter_id have no FK constraints. When you add the constraints (which you must), these tests will fail with FK violations.
Either:
- Set up proper test data first:
DO $$ DECLARE - r uuid := '00000000-0000-0000-0000-0000000000aa'; - a uuid := '00000000-0000-0000-0000-0000000000bb'; + room_id uuid; + round_id uuid; + author_id uuid; n text := 'nonce-sub-1'; id1 uuid; id2 uuid; BEGIN + -- Create test room, round, and participant + SELECT room_create('Test room', '{}') INTO room_id; + SELECT id INTO round_id FROM rounds WHERE room_id = room_id AND idx = 0; + SELECT id INTO author_id FROM participants WHERE room_id = room_id LIMIT 1; + - SELECT submission_upsert(r, a, 'hello', '[]'::jsonb, '[]'::jsonb, 'deadbeef', n) INTO id1; + SELECT submission_upsert(round_id, author_id, 'hello', '[]'::jsonb, '[]'::jsonb, 'deadbeef', n) INTO id1;Or:
2. Document that FK constraints are intentionally omitted (but this is wrong, per schema review)Also applies to: 52-64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
AGENTS.mdis excluded by!*.md
📒 Files selected for processing (4)
db/rpc.sql(1 hunks)db/schema.sql(1 hunks)db/test/10_rooms_rounds.pgtap(2 hunks)db/test/30_rpcs.pgtap(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T07:24:02.457Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-10-02T07:24:02.457Z
Learning: Applies to db/test/**/*.sql : Maintain pgTAP invariants for rooms, rounds, submissions, and related DB objects
Applied to files:
db/schema.sqldb/test/30_rpcs.pgtapdb/test/10_rooms_rounds.pgtap
🔇 Additional comments (3)
db/test/10_rooms_rounds.pgtap (2)
2-2: Plan count correctly updated.The increment to 7 matches the addition of the participants table existence check. Good.
22-29: LGTM.Participants table existence check follows the same pattern as rooms and rounds. No issues here.
db/schema.sql (1)
28-37: Missing index on FK column.You have a FK constraint on participants.room_id (line 30) but no index on that column. Postgres doesn't auto-create indexes for FK columns.
Every query filtering by room_id (which will be common: "show me participants in this room") will do a full table scan. This is a performance time bomb.
Add an index:
UNIQUE (room_id, anon_name) ); + +CREATE INDEX IF NOT EXISTS idx_participants_room_id ON participants (room_id);Likely an incorrect or invalid review comment.
… cases and stronger assertions
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
db/rpc.sql(1 hunks)db/schema.sql(3 hunks)db/test/20_submissions_votes.pgtap(1 hunks)db/test/30_rpcs.pgtap(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-10-02T07:24:02.457Z
Learning: Applies to db/test/**/*.sql : Maintain pgTAP invariants for rooms, rounds, submissions, and related DB objects
📚 Learning: 2025-10-02T07:24:02.457Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-10-02T07:24:02.457Z
Learning: Applies to db/test/**/*.sql : Maintain pgTAP invariants for rooms, rounds, submissions, and related DB objects
Applied to files:
db/test/30_rpcs.pgtapdb/schema.sqldb/test/20_submissions_votes.pgtap
🔇 Additional comments (12)
db/schema.sql (3)
9-13: LGTM on the rooms table structure.The
client_nonce UNIQUEconstraint enables idempotent room creation. The structure is minimal and correct for M1.
43-44: Good. FK constraints with CASCADE are in place.The foreign keys on
submissions.round_idandsubmissions.author_idwith ON DELETE CASCADE correctly enforce referential integrity and cascade deletes from parents.
64-67: Good. FK constraints with CASCADE are in place.The foreign keys on
votes.room_id,votes.round_id, andvotes.voter_idwith ON DELETE CASCADE correctly enforce referential integrity and cascade deletes from parents.db/test/20_submissions_votes.pgtap (1)
53-68: Smart adaptation to FK constraints.The test now correctly:
- Creates a room via
room_createRPC (line 60)- Fetches
round_idfrom the created round (line 61)- Fetches
author_idfrom seeded participants (line 62)- Uses these valid FKs in the submission INSERT (line 65)
This properly validates the FK relationships and idempotent room creation. The test structure is clean and correct.
Based on learnings: Maintains pgTAP invariants for rooms, rounds, submissions, and related DB objects.
db/rpc.sql (5)
3-7: GOOD. You added the nonce parameter.The function signature now includes
p_client_nonce text DEFAULT NULL(line 6), addressing the critical missing idempotence parameter from past reviews.
19-34: EXCELLENT. Loud validation, no silent coercion.You now:
- Validate topic is non-empty (lines 20-22)
- Check
v_participantsrange [1..64] with explicit errors (lines 23-31)- Check
v_submit_minutesrange [1..1440] with explicit errors (lines 26-34)- Raise exceptions with ERRCODE 22023 and descriptive messages
NO MORE silent
GREATEST()garbage. This is how you validate inputs.
38-48: Idempotency logic is correct.Lines 39-41: Check for existing room by nonce and reuse if found.
Lines 42-48: Insert new room with ON CONFLICT DO UPDATE to handle concurrent inserts.This implements proper idempotent semantics. The
ON CONFLICTclause onclient_nonceensures thread-safe idempotence.
50-56: Participant seeding logic is solid.Line 50-51: Creates initial round 0 with computed deadline.
Lines 53-56: Seedsv_participantsdebaters namedanon_1,anon_2, etc., with ON CONFLICT DO NOTHING for idempotence.This correctly populates the new participants table and creates the initial round as documented in the PR objectives.
53-56: Participants INSERT already handles idempotence. Good.Line 56 has
ON CONFLICT (room_id, anon_name) DO NOTHING, so re-runningroom_createwith the same nonce won't duplicate participants.db/test/30_rpcs.pgtap (3)
17-29: Much better test coverage.This addresses past review complaints. You now verify:
- Room ID returned (line 18)
- Room exists in DB with correct title (line 19)
- Participant count matches config (lines 21-22)
- Participant names follow
anon_Npattern (lines 23-25)- Initial round 0 created with valid phase and deadline (lines 27-28)
This is thorough validation of
room_createbehavior.
86-101: Good. Edge case tests for invalid inputs.You added tests that verify:
- Empty topic raises exception (lines 87-91)
participant_count < 1raises exception (lines 92-96)submit_minutes < 1raises exception (lines 97-101)All expect ERRCODE 22023, matching the validation in
room_create. This properly validates the loud failure behavior.
45-51: These tests DO verify idempotence for submission_upsert and vote_submit.Lines 48-50: Call
submission_upserttwice with same nonce, verify same ID returned.
Lines 70-72: Callvote_submittwice with same nonce, verify same ID returned.Good coverage for those RPCs.
…ts comment; add room_create idempotency test
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
db/rpc.sql (1)
146-148: Redundant CASE; simplify the update.You filter losers with
w.yes <= w.noyet still computeCASE ... ELSE 'final'. It’s always 'final' in that WHERE. Drop the CASE.- UPDATE rounds r SET phase = CASE WHEN w.yes > w.no THEN r.phase ELSE 'final' END - FROM winners w - WHERE r.id = w.round_id AND w.yes <= w.no; + UPDATE rounds r + SET phase = 'final' + FROM winners w + WHERE r.id = w.round_id AND w.yes <= w.no;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
.obsidian/app.json(1 hunks).obsidian/appearance.json(1 hunks).obsidian/community-plugins.json(1 hunks).obsidian/core-plugins.json(1 hunks).obsidian/plugins/chronology/manifest.json(1 hunks).obsidian/plugins/chronology/styles.css(1 hunks).obsidian/plugins/editor-width-slider/data.json(1 hunks).obsidian/plugins/editor-width-slider/main.js(1 hunks).obsidian/plugins/editor-width-slider/manifest.json(1 hunks).obsidian/plugins/editor-width-slider/styles.css(1 hunks).obsidian/plugins/obsidian-plugin-toc/data.json(1 hunks).obsidian/plugins/obsidian-plugin-toc/main.js(1 hunks).obsidian/plugins/obsidian-plugin-toc/manifest.json(1 hunks).obsidian/themes/Flexoki/manifest.json(1 hunks).obsidian/themes/Flexoki/theme.css(1 hunks).obsidian/themes/Light & Bright/manifest.json(1 hunks).obsidian/themes/Light & Bright/theme.css(1 hunks).obsidian/themes/Tokyo Night/manifest.json(1 hunks).obsidian/themes/Tokyo Night/theme.css(1 hunks).obsidian/workspace.json(1 hunks)db/rpc.sql(1 hunks)db/schema.sql(3 hunks)db/test/30_rpcs.pgtap(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-10-02T07:24:02.457Z
Learning: Applies to db/test/**/*.sql : Maintain pgTAP invariants for rooms, rounds, submissions, and related DB objects
📚 Learning: 2025-10-02T07:24:02.457Z
Learnt from: CR
PR: flyingrobots/db8#0
File: AGENTS.md:0-0
Timestamp: 2025-10-02T07:24:02.457Z
Learning: Applies to db/test/**/*.sql : Maintain pgTAP invariants for rooms, rounds, submissions, and related DB objects
Applied to files:
db/schema.sqldb/test/30_rpcs.pgtap
🪛 Biome (2.1.2)
.obsidian/themes/Light & Bright/theme.css
[error] 808-809: Unexpected value or character.
Expected one of:
(parse)
[error] 556-556: Unexpected shorthand property border after border-style
(lint/suspicious/noShorthandPropertyOverrides)
[error] 556-556: Unexpected shorthand property border after border-color
(lint/suspicious/noShorthandPropertyOverrides)
.obsidian/themes/Tokyo Night/theme.css
[error] 165-165: Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally.
--text-error is already defined here.
Remove or rename the duplicate custom property to ensure consistent styling.
(lint/suspicious/noDuplicateCustomProperties)
🔇 Additional comments (7)
.obsidian/themes/Tokyo Night/theme.css (1)
415-416: Compatibility ofcqwunit.
--table-width: 88cqw;relies on container query units; verify Electron/Chromium version in Obsidian supports it. Provide a fallback.Suggested fallback:
-body { - --table-width: 88cqw; -} +body { + /* Fallback first, modern unit second */ + --table-width: 88vw; + --table-width: 88cqw; +}.obsidian/themes/Flexoki/manifest.json (1)
1-7: Manifest looks good..obsidian/plugins/editor-width-slider/styles.css (1)
1-8: No styles present; fine to ship or remove..obsidian/plugins/obsidian-plugin-toc/data.json (1)
1-7: Config defaults look sane..obsidian/plugins/editor-width-slider/data.json (1)
1-5: Looks good..obsidian/plugins/obsidian-plugin-toc/manifest.json (1)
1-10: Manifest is valid.db/rpc.sql (1)
38-58: Idempotency + validation are solid.Nonce reuse, ON CONFLICT on (room_id, idx) and (room_id, anon_name), and explicit input checks are correct. Concurrency-safe path looks good.
| --text-normal: var(--fg); | ||
| --text-faint: var(--comment); | ||
| --text-muted: var(--fg_dark); | ||
| --text-error: var(--red); | ||
| --text-accent: var(--magenta); | ||
| --text-accent-hover: var(--cyan); | ||
| --text-error: var(--red1); | ||
| --text-error-hover: var(--red); | ||
| --text-selection: var(--unknown); |
There was a problem hiding this comment.
Duplicate custom property --text-error.
Defined twice with different values; the latter wins, causing confusion.
Keep the final value; remove the earlier definition:
--text-accent: var(--magenta);
--text-accent-hover: var(--cyan);
- --text-error: var(--red);
--text-error: var(--red1);
--text-error-hover: var(--red);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 165-165: Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally.
--text-error is already defined here.
Remove or rename the duplicate custom property to ensure consistent styling.
(lint/suspicious/noDuplicateCustomProperties)
🤖 Prompt for AI Agents
In .obsidian/themes/Tokyo Night/theme.css around lines 159 to 167, the custom
property --text-error is defined twice with different values; remove the earlier
definition (--text-error: var(--red);) and keep the final definition
(--text-error: var(--red1);) so there is only a single --text-error declaration
to avoid confusion and ensure the intended value is used.
| id uuid PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| title text, | ||
| client_nonce text UNIQUE, | ||
| created_at timestamptz NOT NULL DEFAULT now() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Enforce title invariants at the DB layer.
You validated topic in RPC, but rooms.title is still nullable. Don’t rely on callers for integrity.
- title text,
+ title text NOT NULL CHECK (btrim(title) <> ''),This prevents garbage rows from any non-RPC inserts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id uuid PRIMARY KEY DEFAULT gen_random_uuid(), | |
| title text, | |
| client_nonce text UNIQUE, | |
| created_at timestamptz NOT NULL DEFAULT now() | |
| id uuid PRIMARY KEY DEFAULT gen_random_uuid(), | |
| title text NOT NULL CHECK (btrim(title) <> ''), | |
| client_nonce text UNIQUE, | |
| created_at timestamptz NOT NULL DEFAULT now() |
🤖 Prompt for AI Agents
In db/schema.sql around lines 9 to 12, the rooms.title column is currently
nullable; change its definition to "title text NOT NULL" to enforce the
invariant at the DB layer. If the DB already has data, add a migration that (1)
updates existing NULL/empty titles to a safe default or rejects them (e.g.
UPDATE rooms SET title = 'untitled' WHERE title IS NULL OR trim(title) = ''),
and then (2) runs ALTER TABLE rooms ALTER COLUMN title SET NOT NULL; ensure
schema.sql reflects the final NOT NULL column definition.
…itted .obsidian files from branch
…nore fix removing .obsidian
1 participant
Summary
Introduce a minimal Postgres-backed integration harness for server RPCs to validate the DB path during local runs and CI.
Changes
room_create(topic,cfg,nonce)RPC and seedparticipantsTests
npm run dev:db) and psql smoke callsroom_createreturns a stableroom_idand is idempotent via nonce reuseNext
Notes