Merged
Conversation
The tags feature (PR #86) added new tables but missed the startup ensureTable migration, causing getMonitorsWithTags() to fail with a SQL error on databases that haven't run drizzle-kit push. https://claude.ai/code/session_012qN2riMykNDMYS26j3TJkY
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new ensureTagTables() function that creates Changes
Sequence Diagram(s)sequenceDiagram
participant Router as Router/registerRoutes
participant Ensure as ensureTagTables
participant DB as Database
Router->>Ensure: call ensureTagTables()
Ensure->>DB: execute CREATE TABLE IF NOT EXISTS tags...
DB-->>Ensure: success / error
Ensure->>DB: execute CREATE TABLE IF NOT EXISTS monitor_tags...
DB-->>Ensure: success / error
Ensure-->>Router: resolves (or logs error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Security note: runtime schema creation requires DB user privileges; confirm least-privilege policies and consider idempotency/race conditions when multiple instances start concurrently. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes.migration.test.ts (1)
170-189:⚠️ Potential issue | 🟡 MinorMake the new tag migration assertions specific.
includes("tags")is too weak here because everymonitor_tagsstatement also satisfies it. This test can still pass if the standalonetagsDDL disappears, and it won't catch drift on required columns/indexes likename_lower,colour, ortags_user_name_lower_uniq.Suggested fix
- expect(callStrings.some((s: string) => s.includes("tags"))).toBe(true); - expect(callStrings.some((s: string) => s.includes("monitor_tags"))).toBe(true); + const tagsCreate = callStrings.find((s: string) => + s.includes("CREATE TABLE IF NOT EXISTS tags") + ); + expect(tagsCreate).toBeDefined(); + expect(tagsCreate).toContain("user_id"); + expect(tagsCreate).toContain("name"); + expect(tagsCreate).toContain("name_lower"); + expect(tagsCreate).toContain("colour"); + expect(callStrings.some((s: string) => s.includes("tags_user_idx"))).toBe(true); + expect(callStrings.some((s: string) => s.includes("tags_user_name_lower_uniq"))).toBe(true); + + const monitorTagsCreate = callStrings.find((s: string) => + s.includes("CREATE TABLE IF NOT EXISTS monitor_tags") + ); + expect(monitorTagsCreate).toBeDefined(); + expect(monitorTagsCreate).toContain("monitor_id"); + expect(monitorTagsCreate).toContain("tag_id"); + expect(monitorTagsCreate).toContain("ON DELETE CASCADE"); + expect(callStrings.some((s: string) => s.includes("monitor_tags_monitor_tag_uniq"))).toBe(true);As per coding guidelines,
**/*.test.ts: Assertions are specific (not just truthy/falsy).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes.migration.test.ts` around lines 170 - 189, The test currently uses expect(callStrings.some(s => s.includes("tags"))) which is too broad and matches "monitor_tags"; update the assertions that inspect callStrings (derived from mockDbExecute.mock.calls) to specifically look for the standalone tags table and its required columns/indexes: assert a call contains "CREATE TABLE tags" (or a word-boundary match for "tags"), and separately assert the DDL includes the specific column names "name_lower" and "colour" and the unique index name "tags_user_name_lower_uniq" (keep existing checks for "monitor_tags" untouched); use explicit expect(callStrings.some(s => s.includes("CREATE TABLE tags"))) and expect(callStrings.some(s => s.includes("name_lower"))) etc., so the test fails if the standalone tags DDL or required columns/indexes are removed or changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/services/ensureTables.ts`:
- Around line 104-130: ensureTagTables currently catches errors and only logs
them, allowing startup to proceed despite failed DDL; change ensureTagTables
(the function in server/services/ensureTables.ts) to rethrow the caught error
(or remove the try/catch) so failures propagate to the caller (e.g.,
server/routes.ts) and prevent mounting tag/monitor routes when the tables
couldn't be created; ensure you preserve the logging but then throw the original
error (or a wrapped Error) after logging.
---
Outside diff comments:
In `@server/routes.migration.test.ts`:
- Around line 170-189: The test currently uses expect(callStrings.some(s =>
s.includes("tags"))) which is too broad and matches "monitor_tags"; update the
assertions that inspect callStrings (derived from mockDbExecute.mock.calls) to
specifically look for the standalone tags table and its required
columns/indexes: assert a call contains "CREATE TABLE tags" (or a word-boundary
match for "tags"), and separately assert the DDL includes the specific column
names "name_lower" and "colour" and the unique index name
"tags_user_name_lower_uniq" (keep existing checks for "monitor_tags" untouched);
use explicit expect(callStrings.some(s => s.includes("CREATE TABLE tags"))) and
expect(callStrings.some(s => s.includes("name_lower"))) etc., so the test fails
if the standalone tags DDL or required columns/indexes are removed or changed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73cc6d2f-a79b-4be9-82bc-07b96902bed0
📒 Files selected for processing (3)
server/routes.migration.test.tsserver/routes.tsserver/services/ensureTables.ts
Comment on lines
+104
to
+130
| export async function ensureTagTables(): Promise<void> { | ||
| try { | ||
| await db.execute(sql` | ||
| CREATE TABLE IF NOT EXISTS tags ( | ||
| id SERIAL PRIMARY KEY, | ||
| user_id TEXT NOT NULL REFERENCES users(id), | ||
| name TEXT NOT NULL, | ||
| name_lower TEXT NOT NULL, | ||
| colour TEXT NOT NULL, | ||
| created_at TIMESTAMP NOT NULL DEFAULT NOW() | ||
| ) | ||
| `); | ||
| await db.execute(sql`CREATE INDEX IF NOT EXISTS tags_user_idx ON tags(user_id)`); | ||
| await db.execute(sql`CREATE UNIQUE INDEX IF NOT EXISTS tags_user_name_lower_uniq ON tags(user_id, name_lower)`); | ||
|
|
||
| await db.execute(sql` | ||
| CREATE TABLE IF NOT EXISTS monitor_tags ( | ||
| id SERIAL PRIMARY KEY, | ||
| monitor_id INTEGER NOT NULL REFERENCES monitors(id) ON DELETE CASCADE, | ||
| tag_id INTEGER NOT NULL REFERENCES tags(id) ON DELETE CASCADE, | ||
| created_at TIMESTAMP NOT NULL DEFAULT NOW() | ||
| ) | ||
| `); | ||
| await db.execute(sql`CREATE UNIQUE INDEX IF NOT EXISTS monitor_tags_monitor_tag_uniq ON monitor_tags(monitor_id, tag_id)`); | ||
| } catch (e) { | ||
| console.error("Could not ensure tag tables:", e); | ||
| } |
There was a problem hiding this comment.
Propagate tag-table bootstrap failure instead of only logging it.
This helper always resolves successfully, so server/routes.ts continues mounting /api/monitors and the tag routes even when the DDL fails. In read-only or permission-constrained deployments, that leaves the original missing-table 500 path intact.
Suggested fix
-export async function ensureTagTables(): Promise<void> {
+export async function ensureTagTables(): Promise<boolean> {
try {
await db.execute(sql`
CREATE TABLE IF NOT EXISTS tags (
id SERIAL PRIMARY KEY,
user_id TEXT NOT NULL REFERENCES users(id),
name TEXT NOT NULL,
name_lower TEXT NOT NULL,
colour TEXT NOT NULL,
created_at TIMESTAMP NOT NULL DEFAULT NOW()
)
`);
await db.execute(sql`CREATE INDEX IF NOT EXISTS tags_user_idx ON tags(user_id)`);
await db.execute(sql`CREATE UNIQUE INDEX IF NOT EXISTS tags_user_name_lower_uniq ON tags(user_id, name_lower)`);
await db.execute(sql`
CREATE TABLE IF NOT EXISTS monitor_tags (
id SERIAL PRIMARY KEY,
monitor_id INTEGER NOT NULL REFERENCES monitors(id) ON DELETE CASCADE,
tag_id INTEGER NOT NULL REFERENCES tags(id) ON DELETE CASCADE,
created_at TIMESTAMP NOT NULL DEFAULT NOW()
)
`);
await db.execute(sql`CREATE UNIQUE INDEX IF NOT EXISTS monitor_tags_monitor_tag_uniq ON monitor_tags(monitor_id, tag_id)`);
+ return true;
} catch (e) {
console.error("Could not ensure tag tables:", e);
+ return false;
}
}- await ensureTagTables();
+ const tagTablesReady = await ensureTagTables();
+ if (!tagTablesReady) {
+ throw new Error("Tag tables are unavailable");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/services/ensureTables.ts` around lines 104 - 130, ensureTagTables
currently catches errors and only logs them, allowing startup to proceed despite
failed DDL; change ensureTagTables (the function in
server/services/ensureTables.ts) to rethrow the caught error (or remove the
try/catch) so failures propagate to the caller (e.g., server/routes.ts) and
prevent mounting tag/monitor routes when the tables couldn't be created; ensure
you preserve the logging but then throw the original error (or a wrapped Error)
after logging.
Address CodeRabbit feedback: replace weak `includes("tags")` assertion
(which also matches "monitor_tags") with `includes("CREATE TABLE IF NOT
EXISTS tags")`. Add dedicated test for tag table DDL columns, indexes,
and constraints, following the pattern of existing table tests.
https://claude.ai/code/session_012qN2riMykNDMYS26j3TJkY
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
2 participants
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.
Summary
PR #86 ("Add monitor organisation with tags") introduced
tagsandmonitor_tagsdatabase tables but did not add the corresponding startup migration to create them. On deployments wheredrizzle-kit pushhasn't been re-run,getMonitorsWithTags()queries non-existent tables, causing/api/monitorsto return a 500 error. This completely breaks the dashboard — users see "Error Loading Dashboard / Failed to fetch monitors".Changes
Server startup migration (
server/services/ensureTables.ts):ensureTagTables()function that issuesCREATE TABLE IF NOT EXISTSfor bothtagsandmonitor_tags, along with their indexes (tags_user_idx,tags_user_name_lower_uniq,monitor_tags_monitor_tag_uniq)ensureApiKeysTable()andensureChannelTables()Route registration (
server/routes.ts):ensureTagTables()at startup alongside the otherensure*functionsTest update (
server/routes.migration.test.ts):db.executecall count from 11 to 16 (5 new DDL statements for tag tables)tagsandmonitor_tagsDDL is issuedHow to test
npm run test— all 1123 tests should passnpm run check— no new TypeScript errors (pre-existing scraper.ts errors are unrelated)tags/monitor_tagstables don't exist yethttps://claude.ai/code/session_012qN2riMykNDMYS26j3TJkY
Summary by CodeRabbit
New Features
Tests