ENG-270, ENG-328 Creation of the supabase embedding schema, integration with turbo and CI/CD #165
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update introduces a new database package for a discourse graph system, including a full PostgreSQL schema, Supabase configuration, migration scripts, LinkML and PlantUML schema definitions, TypeScript types, and supporting documentation. It also adds CI/CD workflow automation, linter configuration, and deployment scripts, establishing a comprehensive foundation for managing discourse data, concepts, content, and synchronization tasks. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant Supabase CLI
participant Supabase DB
Developer->>GitHub Actions: Push to main / Manual dispatch
GitHub Actions->>Supabase CLI: Checkout code, setup CLI
GitHub Actions->>Supabase CLI: Run turbo deploy command
Supabase CLI->>Supabase DB: Deploy database schema/functions
Supabase DB-->>GitHub Actions: Deployment status
sequenceDiagram
participant Client
participant Supabase DB
participant PL/pgSQL Function
Client->>Supabase DB: Call propose_sync_task(target, function, worker, timeout, interval)
Supabase DB->>PL/pgSQL Function: Execute logic (insert/lock/check/backoff)
PL/pgSQL Function-->>Supabase DB: Return interval/null (task acquired or wait)
Supabase DB-->>Client: Response (interval or task ownership)
sequenceDiagram
participant Client
participant Supabase DB
participant PL/pgSQL Function
Client->>Supabase DB: Call match_content_embeddings(query_embedding, threshold, count, doc_id)
Supabase DB->>PL/pgSQL Function: Vector similarity search
PL/pgSQL Function-->>Supabase DB: Return matching content rows
Supabase DB-->>Client: Embedding search results
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI see that you've tagged me, but I don't have a specific question or instruction to respond to. Let me gather more information about this PR to better assist you. 🏁 Script executed: Length of output: 2439 @maparent Hi there! I see you've tagged me, but I'm not sure what specific help you're looking for. I can see this PR is focused on setting up Supabase embedding schema and integration with Turbo/CI/CD. Looking at the changes, you've added:
How can I assist you with this PR? I can:
Just let me know what you need! (_/) |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🔭 Outside diff range comments (1)
packages/database/supabase/.gitignore (1)
1-9:⚠️ Potential issuePrevent committing base
.envfiles
Currently, only.env.keys,.env.local, and.env.*.localare ignored. The primary.envfile (which may contain secrets) isn’t excluded. Add:+.envto avoid leaking credentials.
♻️ Duplicate comments (2)
packages/database/supabase/migrations/20250513173724_content_concept_key.sql (2)
47-62:match_content_embeddingsfunction duplicated here.
This logic overlaps withschemas/embedding.sql. Ensure both definitions stay in sync or consolidate into a single migration file.
63-90:match_embeddings_for_subset_nodesduplicated—keep DRY.
Same as above; consider centralizing function creation.
🧹 Nitpick comments (32)
packages/database/supabase/migrations/20250506174523_content_idx_id.sql (1)
1-2: Optimize index creation and naming
- To avoid blocking writes, consider using
CONCURRENTLY:- CREATE UNIQUE INDEX "Content_space_and_id" ON "Content" (space_id, source_local_id) WHERE source_local_id IS NOT NULL; + CREATE UNIQUE INDEX CONCURRENTLY "Content_space_and_id" ON "Content" (space_id, source_local_id) WHERE source_local_id IS NOT NULL;
- For consistency with PostgreSQL conventions, you may opt for snake_case naming (e.g.,
content_space_and_source_local_id_idx).
Ensure your migration runner supports non-transactional operations or document the expected locking behavior..github/workflows/database-deploy.yaml (1)
3-3: Make theworkflow_dispatchmapping explicit.
Although GitHub Actions acceptsworkflow_dispatch:without a body, adding an explicit empty mapping improves readability and future extensibility.packages/database/package.json (2)
14-14: Split and simplify thebuildscript.
The current"build"chains many operations with&&, which makes troubleshooting harder. Extract subtasks (lint, type gen, copy, diff) into separate npm scripts and orchestrate them with a runner (e.g.,npm-run-all). This will improve maintainability and logs.
19-20: Clarifydbdiff:saveusage.
The script is defined as"dbdiff:save": "supabase stop && supabase db diff -f ", but it's unclear how to pass the filename. Typically users run:npm run dbdiff:save -- <migration_name>.sqlConsider updating the script or documenting this pattern in the README.
packages/database/README.md (3)
1-2: Fix period placement in intro.
Change:All CLI commands below should be run in this directory (`packages/database`.)to
All CLI commands below should be run in this directory (`packages/database`).This places the period outside the code parentheses.
13-13: Correct typo in work session instruction.
Change:End you work session with `npm run stop`to:
End your work session with `npm run stop`This fixes the pronoun.
🧰 Tools
🪛 LanguageTool
[grammar] ~13-~13: There is an agreement error between ‘work’ and ‘session’. Insert ‘a(n)’ or change the noun to plural.
Context: ... deployed while you work. 3. End you work session withnpm run stop(alias for `supabas...(PRP_VB_NN)
18-18: Rephrase for clarity and punctuation.
Instead of:4. `turbo build`, which will do the following:use:
Run `turbo build` (the following steps will be executed):This reads more naturally.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: Loose punctuation mark.
Context: ...n placing your file. 4.turbo build, which will do the following: 1. C...(UNLIKELY_OPENING_PUNCTUATION)
packages/database/example.md (2)
1-2: Update the heading and remove trailing ellipsis.
Rename# example...to a descriptive title, for example:# Example Dataset for Discourse GraphThis improves clarity for readers.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Trailing punctuation in heading
Punctuation: '...'(MD026, no-trailing-punctuation)
8-11: Fix Markdown list indentation.
Nested list items under(dgpgid) roam/js/discourse-graphshould indent consistently (4 spaces per level). For example:-* (dgpgid) roam/js/discourse-graph - * (et1bkid) Opposes +* (dgpgid) roam/js/discourse-graph + * (et1bkid) OpposesThis ensures proper rendering in Markdown.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Unordered list indentation
Expected: 4; Actual: 6(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 4; Actual: 6(MD007, ul-indent)
11-11: Inconsistent indentation for list items at the same level
Expected: 6; Actual: 4(MD005, list-indent)
packages/database/supabase/schemas/space.sql (1)
34-39: Add index ondiscourse_platform_idfor performance
Foreign key constraints don’t auto-index referencing columns. For faster lookups:+CREATE INDEX ON public."DiscourseSpace"(discourse_platform_id);packages/database/supabase/schemas/contributor.sql (1)
1-4: Inconsistent table naming convention
The join tablescontent_contributorsandconcept_contributorsuse snake_case unquoted, while other tables follow PascalCase with double quotes. For consistency, consider renaming or quoting them (e.g.,public."ContentContributors").Also applies to: 24-27
packages/database/supabase/schemas/account.sql (1)
17-24: Add indexes on foreign key columns inAccount
Neitherperson_idnorplatform_idis indexed, which can degrade join performance. Consider:+CREATE INDEX ON public."Account"(person_id); +CREATE INDEX ON public."Account"(platform_id);packages/database/supabase/config.toml (6)
20-23: Optional: Enable HTTPS for local API.
You’ve disabled TLS locally (enabled = false). For scenarios testing auth flows or secure cookies, consider toggling this on and providing dev certificates.
24-32: Database and pooler settings are appropriate.
Pinningmajor_version = 15aligns with your production DB. You’ve disabled the pooler; if concurrent connections grow, enabling it could improve performance.Also applies to: 33-44
48-63: Migrations ordering is explicit but verbose.
You list each schema file individually. Consider using a glob pattern (e.g.,"./schemas/*.sql") to simplify maintenance as new schemas are added.
226-233: Twilio secret in ENV, good—but check variable naming.
You useSUPABASE_AUTH_SMS_TWILIO_AUTH_TOKEN; ensure this matches your CI/CD secret and documentation.
272-294: Third-party auth blocks are placeholders.
Good that all are disabled by default. Remove unused stubs once providers go live to reduce config noise.
308-313: Analytics backend set to Postgres.
Ensure you have required retention and cleanup policies in place to manage table growth.packages/database/supabase/schemas/embedding.sql (1)
21-29: Privileges granted broadly.
GRANT ALLtoanonandauthenticatedis convenient for local dev but violates least privilege. ConsiderSELECT/INSERT/UPDATEonly as needed.packages/database/supabase/schemas/content.sql (2)
107-121: Indexing strategy: B-tree + full-text.
You’ve added GIN/Btree for ID lookups and a pgroonga index ontext. Confirmpgroongaextension is loaded in migrations. Consider adding trigram or GIN onmetadataif querying JSON.
142-149: Broad grants on tables.
As with embeddings, limit grants to necessary operations in production.packages/database/types.gen.ts (1)
572-580:match_embeddings_for_subset_nodessignature aligns.
Good to see thestring[]Roam UID mapping. Consider overloading with numeric IDs for efficiency.packages/database/supabase/migrations/20250513173724_content_concept_key.sql (1)
25-28: Data migration usesrepresents_id.
Double-check that all rows inContenthadrepresents_idset to avoid nulls. You may want a quick validation query before dropping the old column.packages/database/supabase/migrations/20250517154122_plpgsql_linting.sql (3)
11-13: Handle missingsync_infoentries gracefully
UsingSELECT ... INTO STRICTwill raise an exception if no matching row exists insync_info. Consider adding a controlled exception block or an explicit check to provide clearer diagnostic messages or fallback behavior when no record is found.
57-57: Remove or translate non-English comment
The comment-- zut il renvoie null...is in French and unclear to non-French readers. Replace it with a clear English explanation or remove it to maintain consistency.
64-92: Consider using transaction-scoped advisory locks
Switch from session-level locks (pg_advisory_lock/unlock) to transaction-scoped locks (pg_advisory_xact_lock) to ensure locks are automatically released at the end of the transaction, reducing the risk of leaks on errors.packages/database/supabase/migrations/20250512142307_sync_table.sql (2)
38-38: Remove or translate non-English comment
The comment-- zut il renvoie null...is in French and not descriptive. Replace it with an English comment or remove it.
45-46: Use transaction-scoped advisory locks
Replace the pair:PERFORM pg_advisory_lock(s_id); ... PERFORM pg_advisory_unlock(s_id);with:
PERFORM pg_advisory_xact_lock(s_id);to have locks automatically released at transaction end.
Also applies to: 91-92
packages/database/supabase/schemas/sync.sql (1)
95-157: Avoid duplication of function definitions
The PL/pgSQL functionspropose_sync_taskandend_sync_taskappear both here and in the migration scripts. Maintaining two sources risks drift. Consider centralizing these definitions or generating them in one place to ensure consistency.packages/database/supabase/schemas/concept.sql (1)
58-66: Standardize index naming conventions
The index"Concept_content"is quoted and in CamelCase. For consistency with the rest of the schema, consider using unquoted snake_case, e.g.,idx_concept_content_jsonb.packages/database/schema.puml (1)
80-91: Remove(i)suffix from inherited fields in ConceptSchema.
The(i)notation clutters the diagram—use inheritance arrows instead of inline markers.-class "ConceptSchema" [[{None}]] { - {field} id(i) : integer - {field} epistemic_status(i) : EpistemicStatus - ... -} +class "ConceptSchema" [[{None}]] { + {field} id : integer + {field} epistemic_status : EpistemicStatus + ... +}packages/database/supabase/migrations/20250504202930_content_tables.sql (1)
186-193: Unify table naming conventions.
concept_contributorsis lowercase and unquoted, whereas other tables use quoted PascalCase. Consider standardizing to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/database/schema.svgis excluded by!**/*.svg
📒 Files selected for processing (27)
.github/workflows/database-deploy.yaml(1 hunks)packages/database/.sqruff(1 hunks)packages/database/README.md(1 hunks)packages/database/example.md(1 hunks)packages/database/package.json(1 hunks)packages/database/schema.puml(1 hunks)packages/database/schema.yaml(1 hunks)packages/database/supabase/.gitignore(1 hunks)packages/database/supabase/config.toml(1 hunks)packages/database/supabase/migrations/20250504195841_remote_schema.sql(1 hunks)packages/database/supabase/migrations/20250504202930_content_tables.sql(1 hunks)packages/database/supabase/migrations/20250506174523_content_idx_id.sql(1 hunks)packages/database/supabase/migrations/20250512142307_sync_table.sql(1 hunks)packages/database/supabase/migrations/20250513173724_content_concept_key.sql(1 hunks)packages/database/supabase/migrations/20250517154122_plpgsql_linting.sql(1 hunks)packages/database/supabase/schemas/account.sql(1 hunks)packages/database/supabase/schemas/agent.sql(1 hunks)packages/database/supabase/schemas/base.sql(1 hunks)packages/database/supabase/schemas/concept.sql(1 hunks)packages/database/supabase/schemas/content.sql(1 hunks)packages/database/supabase/schemas/contributor.sql(1 hunks)packages/database/supabase/schemas/embedding.sql(1 hunks)packages/database/supabase/schemas/extensions.sql(1 hunks)packages/database/supabase/schemas/space.sql(1 hunks)packages/database/supabase/schemas/sync.sql(1 hunks)packages/database/types.gen.ts(1 hunks)turbo.json(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/database/example.md
1-1: Trailing punctuation in heading
Punctuation: '...'
(MD026, no-trailing-punctuation)
9-9: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
11-11: Inconsistent indentation for list items at the same level
Expected: 6; Actual: 4
(MD005, list-indent)
🪛 LanguageTool
packages/database/README.md
[grammar] ~13-~13: There is an agreement error between ‘work’ and ‘session’. Insert ‘a(n)’ or change the noun to plural.
Context: ... deployed while you work. 3. End you work session with npm run stop (alias for `supabas...
(PRP_VB_NN)
[style] ~16-~16: Consider shortening or rephrasing this to strengthen your wording.
Context: ...u're working on a feature branch. 2. Make changes to the schema, by editing files in `projec...
(MAKE_CHANGES)
[uncategorized] ~18-~18: Loose punctuation mark.
Context: ...n placing your file. 4. turbo build, which will do the following: 1. C...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (60)
packages/database/.sqruff (1)
1-8: SQL lint configuration looks solid
The PostgreSQL dialect is specified, unwanted rules (CP05, LT05) are excluded, and indentation settings are clear. This aligns well with your schema management workflow.turbo.json (1)
35-38: Supabase environment variables integrated into Turbo deploy
The newSUPABASE_PROJECT_ID,SUPABASE_DB_PASSWORD, andSUPABASE_ACCESS_TOKENentries look correct. Please verify that these names match the secrets defined in your GitHub Actions workflow (.github/workflows/database-deploy.yaml).packages/database/supabase/schemas/base.sql (1)
17-17: Verify publication existence or use IF EXISTS
The statementALTER PUBLICATION supabase_realtime OWNER TO postgres;will error if the
supabase_realtimepublication isn’t defined beforehand. Either ensure you create the publication in an earlier migration or make this idempotent:-ALTER PUBLICATION supabase_realtime OWNER TO postgres; +ALTER PUBLICATION IF EXISTS supabase_realtime OWNER TO postgres;packages/database/supabase/schemas/space.sql (1)
18-18: Consider enforcing NOT NULL onurl
Theurlcolumn inDiscourseSpaceis nullable. If every space must have a URL, change it to:url character varying NOT NULL,to maintain data integrity.
packages/database/supabase/schemas/account.sql (1)
36-37: Ensurespace_idis NOT NULL if required
space_idinSpaceAccessis currently nullable. If every access entry must reference a valid space, declare it asSPACE_ID bigint NOT NULL.packages/database/supabase/config.toml (10)
1-6: Project Identifier is explicit and clear.
Usingproject_id = "discourse-graphs"matches the repo name, ensuring isolation of Supabase artifacts.
7-19: API configuration looks solid.
Exposingpublicandgraphql_publicschemas with a sensiblemax_rowslimit guards against large payloads. Consider bumpingmax_rowsif your use case evolves.
64-70: Caution: Automatic seeding enabled.
enabled = truewill run seed scripts on every reset, which can slow CI runs or leak test data. Verify this behavior is desired in CI/CD workflows.
78-86: Studio configuration is correct.
You’ve wired upopenai_api_keyvia ENV, which is secure. Make sure the key has only necessary scopes.
147-160: Rate limits look reasonable.
Throttling email, SMS, and anonymous sign-in should help mitigate abuse.
167-183: Email auth settings are comprehensive.
Requiring double confirmations and OTP expiry ensures strong security.
199-207: SMS auth disabled by default.
That's consistent with your policies. If SMS sign-up is ever required, revisitenable_signup.
234-242: MFA and phone MFA tuned.
MFA is enabled via TOTP and disabled via phone, matching a balanced security model.Also applies to: 243-250
257-271: OAuth providers scaffolded correctly.
You’ve stubbed Apple OAuth; ensure credentials are provisioned before enabling.
295-304: Edge runtime enabled for Deno.
Great for edge functions; pinningdeno_version = 1is safe. Verify compatibility when upgrading.packages/database/supabase/schemas/embedding.sql (2)
1-8: EnumEmbeddingNameis comprehensive.
All targeted models are listed. Naming is clear and consistent with Supabase vector conventions.
12-18: Embedding table key design: single-column PK.
Usingtarget_idas the primary key enforces one embedding per content. If you later support multiple models per content, you may need a composite PK.packages/database/supabase/schemas/content.sql (3)
1-12:Scaleenum covers all granularity levels.
This matches your LinkML model and provides clear content hierarchies.
16-28:Documenttable schema is sound.
Using an OID for raw content is appropriate for large blobs. Ensure clients correctly fetch large objects.
60-75:Contenttable supports nesting and metadata.
Foreign keys forcreator_idvs.author_idand self‐referencingpart_of_idare well defined.packages/database/types.gen.ts (3)
1-8:Jsontype is correctly recursive.
This covers all JSON structures you’ll encounter.
316-325: Enum constants mirror DB definitions.
Great for static validation. Keep this in sync with schema SQL.
558-571: Functionmatch_content_embeddingsArgs/Returns mismatch.
The TS signature omitscurrent_document_idfrom the return but includes it as optional arg—OK. However, ensure the type ofquery_embedding(string) matches actual binary/vector type; client libs may prefernumber[].packages/database/supabase/migrations/20250513173724_content_concept_key.sql (6)
3-6: Constraint rename is precise.
UpdatingAutomatedAgentto referenceAgent(id)cascades correctly.
9-14: Droppinglast_syncedcolumns—confirm no dependencies.
Ensure any downstream logic or views aren’t broken. Check for leftover references in sync scripts.
17-22:represented_by_idrelationship refined.
Moving representation logic toConceptand adding a unique index is a clean data model improvement.
31-36: Dropping old constraint and column is straightforward.
Looks correct.
39-46: Disabling function body checks for<=>is necessary.
You noted the vector operator issue; this is the right workaround.
91-92: Re-enabling function body checks.
Restoringcheck_function_bodiesis correct hygiene after DDL.packages/database/supabase/migrations/20250517154122_plpgsql_linting.sql (1)
14-16: Verify enum ordering fortask_status
The comparisonASSERT s_status > 'active'depends on the internal ordering of thetask_statusenum. Please confirm that'active'is defined first so that all subsequent statuses (timeout,failed,complete) correctly satisfy this check.packages/database/supabase/schemas/concept.sql (1)
16-32: Ensure sequence usage privileges forentity_id_seq
TheConcept.idcolumn relies onnextval('public.entity_id_seq'), but this migration does not grantUSAGEon that sequence. Verify that the roles inserting intoConcepthave permission, or addGRANT USAGE ON SEQUENCE public.entity_id_seq TO ....packages/database/schema.puml (13)
8-12: UMLAccountclass is well-defined.
Fields map correctly to the SQL table, and relationships toPersonandDiscoursePlatformare drawn below.
13-17: UMLDiscourseSpaceclass matches the SQL schema.
Fieldsid,url, andnameare correct.
18-19: Verify optionalspacecardinality for SpaceAccess.
The diagram shows0..1for thespacelink, but in SQLspace_idis nullable. Confirm if entries without a space are intended, or ifspace_idshould beNOT NULL.
20-29: UMLDocumentclass aligns with the migration.
All fields match the SQL definition (including thecontentsblob mapped from OID).
30-33: AbstractAgentis correctly modeled.
Fieldsidandtypemirror the base table.
34-43: UMLContentclass is accurate.
Attributes correspond to the SQLContenttable.
44-47: Document–Agent and Document–Space relationships look good.
Cardinalities reflect the DB constraints.
48-52: UMLDiscoursePlatformclass matches the SQL schema.
Fieldsid,name, andurlare correct.
53-60: UMLConceptclass fields are consistent.
All properties align with the SQLConcepttable.
61-67: Concept and Content space relationships are correct.
The0..1and1cardinalities match the migration.
74-79: Embedding and content relationships are correct.
Links toContent,Concept, andAgentuse the proper cardinalities.
96-102: UMLPersonsubclass is accurate.
Fields beyond the inherited ones map cleanly to the SQLPersontable.
110-112: Agent inheritance and account→agent link are correct.
Subclasses and associations reflect the DB constraints.packages/database/supabase/migrations/20250504202930_content_tables.sql (16)
2-8: Sequence creation is correct.
Theentity_id_seqwith BIGINT and caching aligns with global ID requirements.
11-11:EntityTypeenum looks complete.
All intended entity variants are included.
13-13:Scaleenum covers content granularity.
Values match the design doc.
15-15:EmbeddingNameenum is correct.
All supported embedding identifiers are listed.
17-17:EpistemicStatusenum matches requirements.
Statuses are exhaustive.
19-23:Agenttable aligned with LinkML and UML.
Basic PK and type field are correct.
26-34:Persontable FK toAgentis correct.
Constraints and metadata align.
49-55:DiscoursePlatformdefinition is solid.
PK, name, and URL enforced as NOT NULL.
57-66:Accounttable correctly links platform and person.
FKs, permissions, andactiveflag look good.
69-76:DiscourseSpacetable matches UML.
FK toDiscoursePlatformis present and cascades as expected.
79-87:SpaceAccesstable definition is correct.
Unique constraint, FKs, and editor flag align with design.
92-105:Documenttable and comments are accurate.
All metadata fields, OID type, and author FK are in place.
114-135:Concepttable, indexes, and comments look good.
Referential behavior and GIN index on JSONB are appropriate.
145-166:Contenttable, indexes, and comments align with requirements.
Nested content, search index, and FK cascades are correct.
195-203: Content embedding table is correctly defined.
Model default, vector type, and PK/FK align.
205-211:⚠️ Potential issueMissing semicolon at end of
content_contributors.
The finalCREATE TABLElacks a terminating;, which will cause a syntax error.CREATE TABLE content_contributors ( content_id BIGINT, contributor_id BIGINT, PRIMARY KEY (content_id, contributor_id), FOREIGN KEY(content_id) REFERENCES "Content" (id) ON DELETE CASCADE ON UPDATE CASCADE, FOREIGN KEY(contributor_id) REFERENCES "Agent" (id) ON DELETE CASCADE ON UPDATE CASCADE -) +);Likely an incorrect or invalid review comment.
|
This is quite large, so I think it'll be best to walk through together at our next meeting. |
maparent
force-pushed
the
feature/supabase
branch
from
692e100 to
bbc7099
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
🔥
Just some small things we discussed and only the sqruff to eslint is blocking.
Approving so that when the change is done you can merge. Ping me if you'd like a double check.
| ALTER TABLE ONLY public."DiscoursePlatform" | ||
| ADD CONSTRAINT "DiscoursePlatform_pkey" PRIMARY KEY (id); | ||
|
|
||
| ALTER TABLE ONLY public."DiscourseSpace" |
There was a problem hiding this comment.
@maparent renaming this to just Space (and Platform) instead of DiscourseSpace keeps nagging at me. "Discourse` feels redundant. Thoughts? Or is it named this way for a specific purpose (eg: to avoid a clash of some sort)?
There was a problem hiding this comment.
No anticipated conflict, and no objection. I'll do that rename later today.
Thank you. Reminder that the deploy mechanics also has to be made specific to main. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
.github/workflows/database-deploy.yaml (1)
16-18: 🛠️ Refactor suggestionUpdate actions/setup-node to v4
The current workflow uses actions/setup-node@v3, which is outdated according to the static analysis. Update to v4 for better compatibility with GitHub Actions runners.
- - uses: actions/setup-node@v3 + - uses: actions/setup-node@v4 with: node-version: "20"🧰 Tools
🪛 actionlint (1.7.7)
16-16: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
packages/database/supabase/schemas/sync.sql (1)
207-258: Restrict privileges on sync_info table and related functionsGranting
ALLon thesync_infotable, its sequence, and these functions to theanonandauthenticatedroles allows any user to propose or end sync tasks arbitrarily. This poses a security risk.
🧹 Nitpick comments (24)
packages/database/supabase/migrations/20250520133551_nodes_needing_sync.sql (3)
13-14: Ensure proper JSONB field existence validationThe code currently assumes that each node in the array contains 'uid' and 'roam_edit_time' fields. Consider adding validation to handle cases where these fields might be missing or null.
- roam_node_uid := (node_info->>'uid')::text; - roam_node_edit_epoch_ms := (node_info->>'roam_edit_time')::bigint; + -- Skip nodes with missing uid or edit time + IF node_info->>'uid' IS NULL OR node_info->>'roam_edit_time' IS NULL THEN + CONTINUE; + END IF; + + roam_node_uid := (node_info->>'uid')::text; + roam_node_edit_epoch_ms := (node_info->>'roam_edit_time')::bigint;
20-21: Consider adding an index for performanceIf this function will be run frequently with large datasets, consider adding an index on the
source_local_idcolumn in the Content table to improve query performance.
28-33: Consider logging for nodes requiring syncFor debugging and audit purposes, consider adding logging for nodes that need synchronization, especially when handling large datasets.
.github/workflows/database-deploy.yaml (3)
10-13: Add environment variable validationConsider adding a step to validate that all required environment variables are set before proceeding with the deployment. This can help catch configuration issues early in the workflow.
env: SUPABASE_ACCESS_TOKEN: ${{ secrets.SUPABASE_ACCESS_TOKEN }} SUPABASE_PROJECT_ID: ${{ secrets.SUPABASE_PROJECT_ID_PROD }} SUPABASE_DB_PASSWORD: ${{ secrets.SUPABASE_DB_PASSWORD_PROD }} + steps: + - name: Validate environment variables + run: | + if [ -z "$SUPABASE_ACCESS_TOKEN" ] || [ -z "$SUPABASE_PROJECT_ID" ] || [ -z "$SUPABASE_DB_PASSWORD" ]; then + echo "Missing required environment variables" + exit 1 + fi
19-23: Add dependency cachingAdding a caching step for Node.js dependencies can significantly improve workflow run times on repeated executions.
- uses: actions/setup-node@v4 with: node-version: "20" + cache: 'npm' - run: npm ci - uses: supabase/setup-cli@v1
4-6: Add path filtering to the workflow triggerConsider adding path filters to the push trigger to only run this workflow when relevant database files change.
push: branches: - main + paths: + - 'packages/database/**' + - '.github/workflows/database-deploy.yaml'packages/database/scripts/lint.ts (3)
11-12: Add support for additional CLI argumentsThe script only supports a
-fflag for fixing issues, but it would be more flexible to support passing additional arguments tosqruff.- const command = - process.argv.length == 3 && process.argv[2] == "-f" ? "fix" : "lint"; + // Default to lint, use fix if -f flag is present + let command = "lint"; + let additionalArgs = ""; + + // Process command line arguments + for (let i = 2; i < process.argv.length; i++) { + if (process.argv[i] === "-f") { + command = "fix"; + } else { + // Pass through other arguments to sqruff + additionalArgs += " " + process.argv[i]; + } + }
13-13: Make the target directory configurableThe script hardcodes the
supabase/schemasdirectory. Consider making this configurable via an environment variable or command-line argument.- exec(`sqruff ${command} supabase/schemas`, {}, (err, stdout, stderr) => { + // Use directory from environment variable or default to supabase/schemas + const targetDir = process.env.SQL_SCHEMAS_DIR || "supabase/schemas"; + exec(`sqruff ${command} ${targetDir}${additionalArgs}`, {}, (err, stdout, stderr) => {
13-17: Use promises for better error handlingThe nested callback structure makes error handling more complex. Consider using promisified versions of exec or spawn for better control flow.
- exec(`sqruff ${command} supabase/schemas`, {}, (err, stdout, stderr) => { - console.log(`${stdout}`); - console.log(`${stderr}`); - process.exit(err ? err.code : 0); - }); + // Use promisified exec for better error handling + import { promisify } from "node:util"; + const execPromise = promisify(exec); + + execPromise(`sqruff ${command} supabase/schemas`) + .then(({ stdout, stderr }) => { + if (stdout) console.log(stdout); + if (stderr) console.log(stderr); + process.exit(0); + }) + .catch((err) => { + console.error("Linting failed:", err.message); + process.exit(err.code || 1); + });packages/database/scripts/deploy.ts (3)
41-53: Add documentation for the -f flagThe script deploys functions when the
-fflag is provided, but there's no documentation explaining this. Add a comment or help message explaining the purpose of this flag.+ // Check if we should also deploy functions if (process.argv.length == 3 && process.argv[2] == "-f") { - // Also push functions + console.log("Deploying Supabase functions as requested with -f flag"); exec(
6-62: Refactor to use async/await for better readabilityThe nested callback structure creates a "callback pyramid" that's hard to read and maintain. Consider refactoring using async/await for a more linear and readable flow.
Here's a partial example of how this could be improved:
import { promisify } from "node:util"; import { exec as execCallback } from "node:child_process"; import dotenv from "dotenv"; const exec = promisify(execCallback); dotenv.config(); const main = async () => { try { // Check if we're on the main branch const { stdout: gitStatus } = await exec("git status -s -b"); const branchMatch = gitStatus.match(/## ([^\.]+)/); if (!branchMatch || branchMatch[1] !== "main") { console.log("Not on main branch, not deploying database"); return; } // Verify environment variables const { SUPABASE_PROJECT_ID, SUPABASE_DB_PASSWORD } = process.env; if (!SUPABASE_PROJECT_ID) { console.log("Please define SUPABASE_PROJECT_ID"); process.exit(1); } if (!SUPABASE_DB_PASSWORD) { console.log("Please define SUPABASE_DB_PASSWORD"); process.exit(1); } // Link Supabase project try { const { stdout, stderr } = await exec( `supabase link --project-ref ${SUPABASE_PROJECT_ID}`, { env: { ...process.env, SUPABASE_DB_PASSWORD } } ); console.log(stdout); console.error(stderr); } catch (err) { console.error("Failed to link Supabase project"); process.exit(1); } // Push database changes try { const { stdout, stderr } = await exec("supabase db push"); console.log(stdout); console.error(stderr); } catch (err) { console.error("Failed to push database changes"); process.exit(1); } // Deploy functions if requested if (process.argv.length == 3 && process.argv[2] == "-f") { console.log("Deploying Supabase functions as requested with -f flag"); try { const { stdout, stderr } = await exec( `supabase functions deploy --project-ref ${SUPABASE_PROJECT_ID}` ); console.log(stdout); console.error(stderr); } catch (err) { console.error("Failed to deploy Supabase functions"); process.exit(1); } } } catch (error) { console.error("error:", error); process.exit(1); } };
3-5: Add TypeScript types for better code qualityConsider adding TypeScript types for the imported modules and function parameters/returns to improve code quality and maintainability.
import { exec } from "node:child_process"; import dotenv from "dotenv"; +import { ExecException } from "child_process"; dotenv.config(); -const main = () => { +const main = (): void => {packages/database/README.md (4)
7-7: Clarify TODO for group access token.The instruction on line 7 includes a
TODO: Create a group access token.Consider tracking this in your issue tracker or linking to a specific task, since team members will need correct credentials forsupabase login.
13-13: Fix grammar in step description.Change “End you work session” to “End your work session” for subject-verb agreement and clarity:
- 3. End you work session with `npm run stop` + 3. End your work session with `npm run stop`🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: “you” seems less likely than “your” (belonging to you).
Context: ... are deployed while you work. 3. End you work session withnpm run stop(alias...(AI_HYDRA_LEO_CP_YOU_YOUR)
[grammar] ~13-~13: There is an agreement error between ‘work’ and ‘session’. Insert ‘a(n)’ or change the noun to plural.
Context: ... deployed while you work. 3. End you work session withnpm run stop(alias for `supabas...(PRP_VB_NN)
16-17: Tighten phrasing by removing unnecessary comma.The phrase “Make changes to the schema, by editing files…” can be streamlined:
- 2. Make changes to the schema, by editing files in `packages/database/supabase/schemas` + 2. Make changes to the schema by editing files in `packages/database/supabase/schemas`🧰 Tools
🪛 LanguageTool
[style] ~16-~16: Consider shortening or rephrasing this to strengthen your wording.
Context: ...u're working on a feature branch. 2. Make changes to the schema, by editing files in `packag...(MAKE_CHANGES)
25-26: Normalize list numbering under “Development.”Steps 10 and 11 follow after step 6, which can confuse readers. Consider using nested numbering (e.g., 3.7, 3.8) or rely on Markdown’s auto-numbering for subitems to maintain a clear sequence.
packages/database/example.md (2)
1-1: Remove trailing punctuation in heading.Change
# example...to a more conventional# Example(capitalized and without ellipsis) to adhere to Markdown style guidelines.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Trailing punctuation in heading
Punctuation: '...'(MD026, no-trailing-punctuation)
8-17: Normalize list indentation.Nested bullet points are inconsistently indented (some use 4 spaces, others 6). Align subitems using two spaces per level, for example:
- * (et1bkid) Opposes + * (et1bkid) OpposesRepeat for all nested entries under this block.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Unordered list indentation
Expected: 4; Actual: 6(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 4; Actual: 6(MD007, ul-indent)
11-11: Inconsistent indentation for list items at the same level
Expected: 6; Actual: 4(MD005, list-indent)
packages/database/supabase/migrations/20250520132747_restrict_search_by_document.sql (1)
21-22: Clarify or remove subjective comment.The remark
-- Supabase wants to replace this function for no obvious reason. Letting it.may be unclear to future maintainers. Either explain the context or remove the comment to keep the migration focused.
packages/database/supabase/schemas/concept.sql (1)
55-57: Refine comments for concept timestamps.The comments on
createdandlast_modifiedrefer to “content.” Since this is theConcepttable, consider adjusting to:COMMENT ON COLUMN public."Concept".created IS 'Time when the concept was created in the remote source'; COMMENT ON COLUMN public."Concept".last_modified IS 'Time when the concept was last modified in the remote source';packages/database/supabase/schemas/sync.sql (1)
121-121: Remove French debug commentThere's a French comment "zut il renvoie null..." that appears to be debugging-related. Consider removing it or translating to English for consistency.
- -- zut il renvoie null... + -- Note: returns null for new rowspackages/database/schema.yaml (3)
122-122: Consider implementing the TODO for known skillsThere's a TODO comment about implementing "known skills" to track what processes agents can confirm. Consider addressing this in a future update.
Would you like me to generate a draft implementation for the agent skills modeling?
310-338: Review commented Reference-related classes for future implementationThe commented-out
Reference,ContentLink, andOccurrenceclasses appear to be planned for future implementation. Consider creating a ticket to track the implementation of these classes if they're still relevant to your roadmap.Would you like me to create a draft implementation for these reference-related classes based on the comments?
296-306: Consider converting inline comments to proper documentationThe inline comments on lines 296-306 contain valuable design considerations about concepts. Consider converting these into proper documentation (perhaps in a separate markdown file) to preserve this knowledge for future developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/database/schema.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
.github/workflows/database-deploy.yaml(1 hunks)packages/database/.sqruff(1 hunks)packages/database/README.md(1 hunks)packages/database/example.md(1 hunks)packages/database/package.json(1 hunks)packages/database/schema.puml(1 hunks)packages/database/schema.yaml(1 hunks)packages/database/scripts/deploy.ts(1 hunks)packages/database/scripts/lint.ts(1 hunks)packages/database/supabase/.gitignore(1 hunks)packages/database/supabase/config.toml(1 hunks)packages/database/supabase/migrations/20250504195841_remote_schema.sql(1 hunks)packages/database/supabase/migrations/20250504202930_content_tables.sql(1 hunks)packages/database/supabase/migrations/20250506174523_content_idx_id.sql(1 hunks)packages/database/supabase/migrations/20250512142307_sync_table.sql(1 hunks)packages/database/supabase/migrations/20250513173724_content_concept_key.sql(1 hunks)packages/database/supabase/migrations/20250517154122_plpgsql_linting.sql(1 hunks)packages/database/supabase/migrations/20250520132747_restrict_search_by_document.sql(1 hunks)packages/database/supabase/migrations/20250520133551_nodes_needing_sync.sql(1 hunks)packages/database/supabase/migrations/20250522193823_rename_discourse_space.sql(1 hunks)packages/database/supabase/schemas/account.sql(1 hunks)packages/database/supabase/schemas/agent.sql(1 hunks)packages/database/supabase/schemas/base.sql(1 hunks)packages/database/supabase/schemas/concept.sql(1 hunks)packages/database/supabase/schemas/content.sql(1 hunks)packages/database/supabase/schemas/contributor.sql(1 hunks)packages/database/supabase/schemas/embedding.sql(1 hunks)packages/database/supabase/schemas/extensions.sql(1 hunks)packages/database/supabase/schemas/space.sql(1 hunks)packages/database/supabase/schemas/sync.sql(1 hunks)packages/database/types.gen.ts(1 hunks)turbo.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/database/supabase/migrations/20250522193823_rename_discourse_space.sql
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/database/supabase/migrations/20250506174523_content_idx_id.sql
- packages/database/supabase/migrations/20250504195841_remote_schema.sql
- packages/database/.sqruff
- packages/database/supabase/.gitignore
- packages/database/package.json
- turbo.json
- packages/database/supabase/schemas/account.sql
- packages/database/supabase/schemas/extensions.sql
- packages/database/supabase/config.toml
- packages/database/supabase/schemas/space.sql
- packages/database/supabase/schemas/base.sql
- packages/database/types.gen.ts
- packages/database/supabase/schemas/contributor.sql
- packages/database/supabase/schemas/agent.sql
- packages/database/supabase/migrations/20250512142307_sync_table.sql
- packages/database/supabase/schemas/embedding.sql
- packages/database/supabase/schemas/content.sql
- packages/database/supabase/migrations/20250517154122_plpgsql_linting.sql
- packages/database/supabase/migrations/20250504202930_content_tables.sql
🧰 Additional context used
🧠 Learnings (2)
packages/database/supabase/migrations/20250520132747_restrict_search_by_document.sql (1)
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/schemas/embedding.sql:66-95
Timestamp: 2025-05-20T14:04:19.622Z
Learning: In the `match_embeddings_for_subset_nodes` SQL function in packages/database/supabase/schemas/embedding.sql, the number of results is implicitly limited by the length of the input array parameter `p_subset_roam_uids` since the function filters content using `WHERE c.source_local_id = ANY(p_subset_roam_uids)`.
packages/database/schema.yaml (1)
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/schema.yaml:116-121
Timestamp: 2025-05-20T03:06:16.564Z
Learning: In the discourse-graph project's LinkML schema (packages/database/schema.yaml), attributes and slots are equivalent constructs. Items can be defined either as slots or attributes without needing to duplicate them in both sections.
🪛 actionlint (1.7.7)
.github/workflows/database-deploy.yaml
16-16: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 LanguageTool
packages/database/README.md
[uncategorized] ~13-~13: “you” seems less likely than “your” (belonging to you).
Context: ... are deployed while you work. 3. End you work session with npm run stop (alias...
(AI_HYDRA_LEO_CP_YOU_YOUR)
[grammar] ~13-~13: There is an agreement error between ‘work’ and ‘session’. Insert ‘a(n)’ or change the noun to plural.
Context: ... deployed while you work. 3. End you work session with npm run stop (alias for `supabas...
(PRP_VB_NN)
[style] ~16-~16: Consider shortening or rephrasing this to strengthen your wording.
Context: ...u're working on a feature branch. 2. Make changes to the schema, by editing files in `packag...
(MAKE_CHANGES)
[uncategorized] ~18-~18: Loose punctuation mark.
Context: ...n placing your file. 4. turbo build, which will do the following: 1. C...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
packages/database/example.md
1-1: Trailing punctuation in heading
Punctuation: '...'
(MD026, no-trailing-punctuation)
9-9: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
11-11: Inconsistent indentation for list items at the same level
Expected: 6; Actual: 4
(MD005, list-indent)
🔇 Additional comments (11)
packages/database/supabase/migrations/20250520133551_nodes_needing_sync.sql (1)
1-38: Function design is suitable for node synchronization with RoamThe
get_nodes_needing_syncfunction effectively identifies nodes that need synchronization by comparing timestamps between Roam and the Content table. This is a clean implementation that follows the principle of single responsibility.packages/database/supabase/schemas/sync.sql (6)
1-6: LGTM: Well-defined task status enumThe enum definition with appropriate states ('active', 'timeout', 'complete', 'failed') provides a clear structure for tracking synchronization task status.
10-20: Well-structured sync_info table with appropriate fieldsGood job on defining a comprehensive table for tracking synchronization tasks. The table includes all necessary fields for effective task management: identifiers, status tracking, worker attribution, failure counting, and timestamps for task lifecycle events.
43-45: Good use of unique index for task identificationThe unique index on
(sync_target, sync_function)ensures that only one synchronization task can exist for a given target-function pair, which is essential for preventing duplicate tasks.
49-85: LGTM: Robust end_sync_task function with proper validationsThis function correctly handles task completion with appropriate validations:
- Ensures status progression (line 68)
- Verifies worker identity (line 67)
- Resets failure count on completion (line 71)
- Increments failure count on non-completion status changes (line 74)
95-157: Well-implemented concurrency control in propose_sync_taskExcellent implementation of the task proposal system with:
- Advisory locks for concurrency control (line 128)
- Parameter validation (lines 113-115)
- Timeout detection logic (lines 134-137)
- Exponential backoff for failures (line 139)
168-204: LGTM: Effective implementation of get_nodes_needing_syncThis function properly identifies nodes needing synchronization by comparing timestamps between external data and local content records. The logic correctly handles both new nodes and updated nodes.
packages/database/schema.yaml (3)
1-8: LGTM: Well-defined schema header with appropriate identifiersThe schema header correctly defines the namespace, prefixes, and imports needed for the LinkML schema.
10-79: Comprehensive enumeration definitionsThe enumerations are well-structured with clear permissible values and helpful descriptions. This provides a solid foundation for the schema's type system.
80-105: Well-defined custom types with appropriate annotationsThe custom types are properly defined with proper URI references, base types, and SQL-specific annotations where needed. The vector type annotation for pgvector is particularly valuable for embedding storage.
packages/database/schema.puml (1)
1-110: LGTM: Comprehensive UML diagram of the database schemaThe PlantUML diagram provides a clear visual representation of the entities, their attributes, and relationships. This is valuable for understanding the overall structure of the database schema.
Since this file is auto-generated as noted in previous review comments, no direct edits are suggested.
| const main = () => { | ||
| try { | ||
| exec("git status -s -b", (err, stdout, stderr) => { | ||
| if (err) { | ||
| console.error("Is git installed?"); | ||
| process.exit(1); | ||
| } | ||
| const lines = stdout.split("\n"); | ||
| if (lines[0] != "## main...main") { | ||
| console.log("Not on main branch, not deploying database"); | ||
| process.exit(0); | ||
| } | ||
| const { SUPABASE_PROJECT_ID, SUPABASE_DB_PASSWORD } = process.env; | ||
| if (!SUPABASE_PROJECT_ID) { | ||
| console.log("Please define SUPABASE_PROJECT_ID"); | ||
| process.exit(1); | ||
| } | ||
| if (!SUPABASE_DB_PASSWORD) { | ||
| console.log("Please define SUPABASE_DB_PASSWORD"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve branch verification logic
The current branch check is very specific ("## main...main") and might not work in all Git setups. Consider a more robust approach that extracts the branch name from the Git status output.
- const lines = stdout.split("\n");
- if (lines[0] != "## main...main") {
+ // Extract branch name using regex for more robust parsing
+ const branchMatch = stdout.match(/## ([^\.]+)/);
+ if (!branchMatch || branchMatch[1] !== "main") {
console.log("Not on main branch, not deploying database");
process.exit(0);
}📝 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.
| const main = () => { | |
| try { | |
| exec("git status -s -b", (err, stdout, stderr) => { | |
| if (err) { | |
| console.error("Is git installed?"); | |
| process.exit(1); | |
| } | |
| const lines = stdout.split("\n"); | |
| if (lines[0] != "## main...main") { | |
| console.log("Not on main branch, not deploying database"); | |
| process.exit(0); | |
| } | |
| const { SUPABASE_PROJECT_ID, SUPABASE_DB_PASSWORD } = process.env; | |
| if (!SUPABASE_PROJECT_ID) { | |
| console.log("Please define SUPABASE_PROJECT_ID"); | |
| process.exit(1); | |
| } | |
| if (!SUPABASE_DB_PASSWORD) { | |
| console.log("Please define SUPABASE_DB_PASSWORD"); | |
| // Extract branch name using regex for more robust parsing | |
| const branchMatch = stdout.match(/## ([^\.]+)/); | |
| if (!branchMatch || branchMatch[1] !== "main") { | |
| console.log("Not on main branch, not deploying database"); | |
| process.exit(0); | |
| } |
🤖 Prompt for AI Agents
In packages/database/scripts/deploy.ts around lines 6 to 24, the branch
verification logic is too specific by checking for the exact string "##
main...main" from git status output, which may fail in different Git setups.
Modify the code to parse the first line of the git status output more robustly
by extracting the current branch name dynamically, for example by splitting the
line and retrieving the branch name portion, then compare it to "main" to decide
whether to proceed with deployment.
There was a problem hiding this comment.
The goal is not just to extract the branch name, but to ensure that the branch is pristine, with no un-pushed changes and ideally no uncommitted changes. (I pushed another change to handle the latter case.)
There was a problem hiding this comment.
@maparent I understand your point now, thank you for the clarification. You're absolutely right that the goal is more comprehensive than just checking the branch name - ensuring the branch is pristine with no uncommitted or unpushed changes is crucial for a deployment script.
I see in the full file you've already implemented the check for uncommitted changes with:
if (lines.length > 1) {
console.log(
"You seem to have uncommitted changes, not deploying database",
);
process.exit(0);
}This approach of using git status -s -b -uno and checking lines.length is an effective way to verify both the branch name and its pristine state. Sorry for not seeing the full context in my initial suggestion.
✏️ Learnings added
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/scripts/deploy.ts:0-0
Timestamp: 2025-05-22T23:48:45.419Z
Learning: The database deployment script in packages/database/scripts/deploy.ts requires that the branch is 'main' and pristine (no uncommitted or unpushed changes) before allowing deployment to proceed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
packages/database/supabase/migrations/20250520132747_restrict_search_by_document.sql
Show resolved
Hide resolved
packages/database/supabase/migrations/20250513173724_content_concept_key.sql
Show resolved
Hide resolved
packages/database/supabase/migrations/20250513173724_content_concept_key.sql
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
First expression of the database schema. Machinery to automate the steps.
Summary by CodeRabbit
New Features
Documentation
Chores