Merge pull request liam-hq#1435 from liam-hq/modify-github-pull-request-table

hoshinotsuyoshi · web-flow · commit b5c69ba9c33a · 2025-04-22T12:58:52.000Z
♻️(database): Refactor GitHub PR comments into separate table
diff --git a/.liam/schema-override.yml b/.liam/schema-override.yml
@@ -7,6 +7,7 @@ overrides:
         - github_pull_requests
         - github_schema_file_paths
         - github_doc_file_paths
+        - github_pull_request_comments
       comment: "Tables related to GitHub. All tables in this group should have a GitHub prefix. External tables must not depend on these tables (e.g., no pullRequestId foreign keys in tables outside this group)."
     Organization:
       name: Organization
@@ -21,6 +22,8 @@ overrides:
       comment: "GitHub repositories table with github_ prefix pattern"
     github_pull_requests:
       comment: "GitHub pull requests table with github_ prefix pattern. Should be updated to have migration_id instead of migrations having pull_request_id."
+    github_pull_request_comments:
+      comment: "Stores GitHub comment information for pull requests, maintaining a 1:1 relationship with github_pull_requests"
     migrations:
       comment: "Should have project_id instead of pull_request_id to remove GitHub dependency"
     overall_reviews:
diff --git a/docs/migrationOpsContext.md b/docs/migrationOpsContext.md
@@ -63,5 +63,51 @@ Write Postgres-compatible SQL code for Supabase migration files that:
 - Write all SQL in lowercase.
 - Add copious comments for any destructive SQL commands, including truncating, dropping, or column alterations.
 - **Follow the schema design patterns and rules documented in [`docs/schemaPatterns.md`](./schemaPatterns.md)** for consistent database design.
+- **Wrap migrations in a transaction (BEGIN/COMMIT) to ensure atomicity**. This prevents partial migrations if an error occurs.
 
 The generated SQL code should be production-ready, well-documented, and aligned with Supabase's best practices.
+
+## Adding NOT NULL Constraints
+
+When adding a NOT NULL constraint to an existing column:
+
+1. Always update existing rows with appropriate values before applying the constraint.
+2. For columns with foreign key references, derive values from related tables when possible.
+3. Example approach:
+
+```sql
+BEGIN;
+
+-- Add the column as nullable first
+ALTER TABLE "public"."table_name"
+ADD COLUMN "new_column" uuid REFERENCES "public"."referenced_table"("id") ON UPDATE CASCADE ON DELETE RESTRICT;
+
+-- Update existing rows with values from a related table
+UPDATE "public"."table_name" tn
+SET "new_column" = (
+  SELECT rt."id" 
+  FROM "public"."referenced_table" rt
+  JOIN "public"."join_table" jt ON rt."id" = jt."referenced_id"
+  WHERE jt."table_id" = tn."id"
+  LIMIT 1
+);
+
+-- Now make the column NOT NULL
+ALTER TABLE "public"."table_name"
+ALTER COLUMN "new_column" SET NOT NULL;
+
+COMMIT;
+```
+
+## Post-Migration Steps
+
+After applying migrations, always run:
+
+1. Run the combined command to update both schema SQL file and TypeScript types:
+   ```sh
+   cd frontend/packages/db && pnpm supabase:gen
+   ```
+
+2. Test affected functionality to ensure backward compatibility with the previous app version.
+
+3. Update all test files that might be affected by schema changes, especially where strict typing is enforced.
diff --git a/frontend/packages/db/schema/schema.sql b/frontend/packages/db/schema/schema.sql
@@ -163,10 +163,21 @@ CREATE TABLE IF NOT EXISTS "public"."github_doc_file_paths" (
 ALTER TABLE "public"."github_doc_file_paths" OWNER TO "postgres";
 
 
+CREATE TABLE IF NOT EXISTS "public"."github_pull_request_comments" (
+    "id" "uuid" DEFAULT "gen_random_uuid"() NOT NULL,
+    "github_pull_request_id" "uuid" NOT NULL,
+    "github_comment_identifier" integer NOT NULL,
+    "created_at" timestamp(3) with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL,
+    "updated_at" timestamp(3) with time zone NOT NULL
+);
+
+
+ALTER TABLE "public"."github_pull_request_comments" OWNER TO "postgres";
+
+
 CREATE TABLE IF NOT EXISTS "public"."github_pull_requests" (
     "id" "uuid" DEFAULT "gen_random_uuid"() NOT NULL,
     "pull_number" bigint NOT NULL,
-    "comment_id" bigint,
     "created_at" timestamp(3) with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL,
     "updated_at" timestamp(3) with time zone NOT NULL,
     "repository_id" "uuid" NOT NULL
@@ -402,6 +413,21 @@ ALTER TABLE ONLY "public"."github_doc_file_paths"
 
 
 
+ALTER TABLE ONLY "public"."github_pull_request_comments"
+    ADD CONSTRAINT "github_pull_request_comments_github_comment_identifier_key" UNIQUE ("github_comment_identifier");
+
+
+
+ALTER TABLE ONLY "public"."github_pull_request_comments"
+    ADD CONSTRAINT "github_pull_request_comments_github_pull_request_id_key" UNIQUE ("github_pull_request_id");
+
+
+
+ALTER TABLE ONLY "public"."github_pull_request_comments"
+    ADD CONSTRAINT "github_pull_request_comments_pkey" PRIMARY KEY ("id");
+
+
+
 ALTER TABLE ONLY "public"."github_repositories"
     ADD CONSTRAINT "github_repository_github_repository_identifier_organization_id_" UNIQUE ("github_repository_identifier", "organization_id");
 
@@ -568,6 +594,11 @@ ALTER TABLE ONLY "public"."github_doc_file_paths"
 
 
 
+ALTER TABLE ONLY "public"."github_pull_request_comments"
+    ADD CONSTRAINT "github_pull_request_comments_github_pull_request_id_fkey" FOREIGN KEY ("github_pull_request_id") REFERENCES "public"."github_pull_requests"("id") ON UPDATE CASCADE ON DELETE CASCADE;
+
+
+
 ALTER TABLE ONLY "public"."github_pull_requests"
     ADD CONSTRAINT "github_pull_request_repository_id_fkey" FOREIGN KEY ("repository_id") REFERENCES "public"."github_repositories"("id") ON UPDATE CASCADE ON DELETE RESTRICT;
 
@@ -987,6 +1018,12 @@ GRANT ALL ON TABLE "public"."github_doc_file_paths" TO "service_role";
 
 
 
+GRANT ALL ON TABLE "public"."github_pull_request_comments" TO "anon";
+GRANT ALL ON TABLE "public"."github_pull_request_comments" TO "authenticated";
+GRANT ALL ON TABLE "public"."github_pull_request_comments" TO "service_role";
+
+
+
 GRANT ALL ON TABLE "public"."github_pull_requests" TO "anon";
 GRANT ALL ON TABLE "public"."github_pull_requests" TO "authenticated";
 GRANT ALL ON TABLE "public"."github_pull_requests" TO "service_role";
diff --git a/frontend/packages/db/supabase/database.types.ts b/frontend/packages/db/supabase/database.types.ts
@@ -69,25 +69,54 @@ export type Database = {
           },
         ]
       }
+      github_pull_request_comments: {
+        Row: {
+          created_at: string
+          github_comment_identifier: number
+          github_pull_request_id: string
+          id: string
+          updated_at: string
+        }
+        Insert: {
+          created_at?: string
+          github_comment_identifier: number
+          github_pull_request_id: string
+          id?: string
+          updated_at: string
+        }
+        Update: {
+          created_at?: string
+          github_comment_identifier?: number
+          github_pull_request_id?: string
+          id?: string
+          updated_at?: string
+        }
+        Relationships: [
+          {
+            foreignKeyName: 'github_pull_request_comments_github_pull_request_id_fkey'
+            columns: ['github_pull_request_id']
+            isOneToOne: true
+            referencedRelation: 'github_pull_requests'
+            referencedColumns: ['id']
+          },
+        ]
+      }
       github_pull_requests: {
         Row: {
-          comment_id: number | null
           created_at: string
           id: string
           pull_number: number
           repository_id: string
           updated_at: string
         }
         Insert: {
-          comment_id?: number | null
           created_at?: string
           id?: string
           pull_number: number
           repository_id: string
           updated_at: string
         }
         Update: {
-          comment_id?: number | null
           created_at?: string
           id?: string
           pull_number?: number
diff --git a/frontend/packages/db/supabase/migrations/20250422051514_refactor_github_pull_requests.sql b/frontend/packages/db/supabase/migrations/20250422051514_refactor_github_pull_requests.sql
@@ -0,0 +1,62 @@
+/*
+  Migration: Refactor github_pull_requests table and create github_pull_request_comments table
+
+  Purpose:
+  - Remove comment_id from github_pull_requests to avoid record updates, moving it to a separate table
+  - Create new github_pull_request_comments table with a 1:1 relationship to github_pull_requests
+
+  Affected tables:
+  - github_pull_requests
+  - github_pull_request_comments (new)
+*/
+
+BEGIN;
+
+-- Step 1: Create the new github_pull_request_comments table
+CREATE TABLE "public"."github_pull_request_comments" (
+  "id" uuid DEFAULT gen_random_uuid() NOT NULL,
+  "github_pull_request_id" uuid NOT NULL,
+  "github_comment_identifier" integer NOT NULL,
+  "created_at" timestamp(3) with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL,
+  "updated_at" timestamp(3) with time zone NOT NULL,
+
+  -- Define primary key
+  CONSTRAINT "github_pull_request_comments_pkey" PRIMARY KEY ("id"),
+
+  -- Define unique constraint on github_pull_request_id for 1:1 relationship
+  CONSTRAINT "github_pull_request_comments_github_pull_request_id_key" UNIQUE ("github_pull_request_id"),
+
+  -- Define unique constraint on github_comment_identifier
+  CONSTRAINT "github_pull_request_comments_github_comment_identifier_key" UNIQUE ("github_comment_identifier"),
+
+  -- Define foreign key relationship to github_pull_requests
+  CONSTRAINT "github_pull_request_comments_github_pull_request_id_fkey"
+    FOREIGN KEY ("github_pull_request_id")
+    REFERENCES "public"."github_pull_requests"("id")
+    ON UPDATE CASCADE
+    ON DELETE CASCADE
+);
+
+-- Grant permissions on the new table
+GRANT ALL ON TABLE "public"."github_pull_request_comments" TO "anon";
+GRANT ALL ON TABLE "public"."github_pull_request_comments" TO "authenticated";
+GRANT ALL ON TABLE "public"."github_pull_request_comments" TO "service_role";
+
+-- Step 2: Optionally migrate existing comment_id data to the new table
+-- This will preserve any existing comment relationships
+INSERT INTO "public"."github_pull_request_comments"
+("github_pull_request_id", "github_comment_identifier", "updated_at")
+SELECT
+  "id",
+  "comment_id",
+  CURRENT_TIMESTAMP
+FROM "public"."github_pull_requests"
+WHERE "comment_id" IS NOT NULL;
+
+-- Step 3: Finally, drop the comment_id column from github_pull_requests
+-- This comment explains the potentially destructive operation
+-- Note: This is a destructive operation, but we've migrated the data in step 2
+ALTER TABLE "public"."github_pull_requests"
+DROP COLUMN "comment_id";
+
+COMMIT;
diff --git a/frontend/packages/jobs/src/tasks/review/__tests__/postComment.test.ts b/frontend/packages/jobs/src/tasks/review/__tests__/postComment.test.ts
@@ -44,7 +44,6 @@ describe.skip('postComment', () => {
     id: '9999',
     pull_number: 1,
     repository_id: '9999',
-    comment_id: null,
     created_at: new Date().toISOString(),
     updated_at: new Date().toISOString(),
   }
@@ -110,21 +109,23 @@ Migration URL: ${process.env['NEXT_PUBLIC_BASE_URL']}/app/migrations/${testMigra
     )
     expect(createPullRequestComment).toHaveBeenCalledTimes(1)
 
-    const { data: updatedPR } = await supabase
-      .from('github_pull_requests')
-      .select('*')
-      .eq('id', testPullRequest.id)
-      .single()
+    const { data: prComment } = await supabase
+      .from('github_pull_request_comments')
+      .select('github_comment_identifier')
+      .eq('github_pull_request_id', testPullRequest.id)
+      .maybeSingle()
 
-    expect(updatedPR?.comment_id).toBe(mockCommentId)
+    expect(prComment?.github_comment_identifier).toBe(mockCommentId)
   })
 
   it('should update existing comment when comment exists', async () => {
     const existingCommentId = 456
-    await supabase
-      .from('github_pull_requests')
-      .update({ comment_id: existingCommentId })
-      .eq('id', testPullRequest.id)
+    const now = new Date().toISOString()
+    await supabase.from('github_pull_request_comments').insert({
+      github_pull_request_id: testPullRequest.id,
+      github_comment_identifier: existingCommentId,
+      updated_at: now,
+    })
 
     const testPayload = {
       reviewComment: 'Updated review comment',
@@ -183,7 +184,6 @@ Migration URL: ${process.env['NEXT_PUBLIC_BASE_URL']}/app/migrations/${testMigra
       id: '8888',
       pull_number: 2,
       repository_id: testRepository.id,
-      comment_id: null,
       created_at: new Date().toISOString(),
       updated_at: new Date().toISOString(),
     }
diff --git a/frontend/packages/jobs/src/tasks/review/__tests__/processSaveReview.test.ts b/frontend/packages/jobs/src/tasks/review/__tests__/processSaveReview.test.ts
@@ -28,7 +28,6 @@ describe.skip('processSaveReview', () => {
     id: '9999',
     pull_number: 9999,
     repository_id: '9999',
-    comment_id: null,
     created_at: new Date().toISOString(),
     updated_at: new Date().toISOString(),
   }
diff --git a/frontend/packages/jobs/src/tasks/review/postComment.ts b/frontend/packages/jobs/src/tasks/review/postComment.ts
@@ -118,6 +118,13 @@ export async function postComment(
       )
     }
 
+    // Fetch comment ID from github_pull_request_comments if exists
+    const { data: commentRecord } = await supabase
+      .from('github_pull_request_comments')
+      .select('github_comment_identifier')
+      .eq('github_pull_request_id', pullRequestId)
+      .maybeSingle()
+
     const migration = prRecord.migrations[0]
     const migrationUrl = `${process.env['NEXT_PUBLIC_BASE_URL']}/app/projects/${projectId}/ref/${encodeURIComponent(branchName)}/migrations/${migration.id}`
 
@@ -139,12 +146,12 @@ export async function postComment(
 
     const fullComment = `${reviewComment}\n\nMigration URL: ${migrationUrl}${erdLinkText}`
 
-    if (prRecord.comment_id) {
+    if (commentRecord?.github_comment_identifier) {
       await updatePullRequestComment(
         Number(installationId),
         owner,
         repo,
-        Number(prRecord.comment_id),
+        Number(commentRecord.github_comment_identifier),
         fullComment,
       )
     } else {
@@ -156,14 +163,18 @@ export async function postComment(
         fullComment,
       )
 
-      const { error: updateError } = await supabase
-        .from('github_pull_requests')
-        .update({ comment_id: commentResponse.id })
-        .eq('id', pullRequestId)
+      const now = new Date().toISOString()
+      const { error: createCommentError } = await supabase
+        .from('github_pull_request_comments')
+        .insert({
+          github_pull_request_id: pullRequestId,
+          github_comment_identifier: commentResponse.id,
+          updated_at: now,
+        })
 
-      if (updateError) {
+      if (createCommentError) {
         throw new Error(
-          `Failed to update pull request with comment ID: ${updateError.message}`,
+          `Failed to create github_pull_request_comments record: ${createCommentError.message}`,
         )
       }
     }

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,6 @@ describe.skip('processSaveReview', () => {`
`28`	`28`	`id: '9999',`
`29`	`29`	`pull_number: 9999,`
`30`	`30`	`repository_id: '9999',`
`31`		`- comment_id: null,`
`32`	`31`	`created_at: new Date().toISOString(),`
`33`	`32`	`updated_at: new Date().toISOString(),`
`34`	`33`	`}`