ENG-1180 F7: Publish non-text assets referenced by a DG node #708
Conversation
|
Updates to Preview Branch (eng-1180-f7-publish-non-text-assets-referenced-by-a-dg-node) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
maparent
changed the base branch from
main
to
eng-1285-base-user-affordance-to-trigger-node-sharing
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces a file attachment management system by adding database tables (FileReference, file_gc), storage policies with RLS, and a backend upload utility (addFile) to support embedded asset publishing in content, along with automatic garbage collection triggers. Changes
Sequence DiagramsequenceDiagram
participant App as Obsidian App
participant DB as Database Client
participant RPC as Database RPC
participant Storage as Storage Bucket
participant Table as FileReference Table
App->>DB: publishNode(content with attachments)
DB->>DB: Fetch existing Content by source_local_id
DB->>DB: Compare last_modified with file mtime
alt Content up-to-date
DB-->>App: Skip publishing
else Content outdated
App->>App: Resolve embedded links & filter MIME types
loop For each attachment
App->>App: Compute SHA-256 hash of content
App->>RPC: file_exists(hashvalue)
alt File not found
App->>Storage: Upload to assets bucket (hash as key)
end
App->>Table: Insert FileReference record
alt Duplicate filepath+space+source_local_id
Table->>Table: Update existing FileReference
end
end
App->>Table: Delete orphaned FileReferences (not in current attachments)
DB-->>App: Publish complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 8
🤖 Fix all issues with AI agents
In `@apps/obsidian/src/utils/publishNode.ts`:
- Around line 101-108: The cleanup query in the publishNode function is using
the wrong column name: change the .eq("contentId", contentId) filter used on
client.from("FileReference") to use the database column name "content_id" (i.e.
.eq("content_id", contentId)) so the delete actually matches rows; keep the rest
of the logic (the .notIn("filepath", [...new Set(existingFiles)]) and the
non-failing error handling) intact and ensure you reference the same
FileReference/table and existingFiles set when updating the filter.
- Around line 75-76: The current asset extraction misses wikilink-style embeds;
update the logic around assetsRe and fileRefs in publishNode.ts to match both
markdown and wikilink image syntaxes by either replacing assetsRe with a
combined regex that captures paths from both  and ![[path]] patterns
or adding a second regex/pass that finds ![[...]] and extracts the inner path;
ensure the extracted capture group(s) are normalized the same way as existing
markdown captures so downstream code consuming fileContent/fileRefs continues to
work (look for symbols: assetsRe, fileRefs, fileContent).
- Around line 81-83: Replace the URL-based path resolution that constructs a
file URL and substrings it (using new URL(...), asUrl.href.substring, and
plugin.app.vault.getFileByPath) with Obsidian's proper link resolution: call
plugin.app.metadataCache.getFirstLinkpathDest(fname, file.path) to obtain the
attachment; update any logic that expects a Vault file object from getFileByPath
to use the result of getFirstLinkpathDest (which correctly handles relative
links, URL-reserved characters, aliases and fragments) and remove the asUrl/path
substring manipulation.
In `@packages/database/src/lib/files.ts`:
- Around line 30-38: The insert into FileReference via
client.from("FileReference").insert(...) can fail with a duplicate key on the
composite primary key (content_id, filepath); change this to an
upsert/conflict-handling call so re-publishes update existing rows instead of
erroring. Replace the plain insert used where frefResult is assigned with an
insert...onConflict (or upsert) that targets the (content_id, filepath) conflict
and updates fields like filehash, last_modified (and created if needed), and
keep the existing error check (frefResult.error) after the upsert call.
In `@packages/database/supabase/migrations/20260109035003_fileref.sql`:
- Line 62: The OWNER statement in the migration targets the wrong table
("FileReference") — do not edit this committed migration; instead create a new
migration that runs ALTER TABLE public."file_gc" OWNER TO "postgres"; or update
the schema generation source so subsequent migrations include the correct owner
for file_gc, and ensure the new migration references the table name file_gc (not
FileReference) to correct the owner assignment.
- Around line 90-102: Fix the trigger function after_delete_update_fref by: 1)
qualifying the GC table as public.file_gc instead of file_gc to avoid
search_path issues; 2) storing the file identifier used by storage (use
OLD.filehash rather than OLD.filepath, or map filepath→filehash before insert)
into public.file_gc so the garbage collector can locate assets; 3) change the
return to RETURN OLD (instead of RETURN NEW) for DELETE triggers (even though
AFTER triggers ignore it); and 4) apply these fixes in the proper
migration/schema (assets.sql or a new migration) rather than only editing this
generated file. Ensure references to public."FileReference" and
after_delete_update_fref are updated accordingly.
In `@packages/database/supabase/schemas/assets.sql`:
- Line 62: The ALTER TABLE OWNER statement is pointing to the wrong table:
change the ALTER TABLE public."FileReference" OWNER TO "postgres"; line so it
sets ownership for the file_gc table instead; locate the ALTER TABLE statement
in the schemas file (symbol: ALTER TABLE public."FileReference" OWNER TO
"postgres";) and update it to reference file_gc (symbol: file_gc) so the schema
assigns the correct owner.
- Around line 90-102: The trigger function after_delete_update_fref should be
fixed to reference the file_gc table with full schema qualification
(public.file_gc) and return OLD instead of NEW; update the INSERT to target
public.file_gc and change the function's final RETURN to RETURN OLD. Also
address the design mismatch: either change the stored GC key to filehash
(populate public.file_gc with OLD.filehash) or ensure the GC consumer resolves
filepath→filehash, and update the INSERT column/value accordingly to keep GC
consumers consistent with storage keys.
🧹 Nitpick comments (2)
apps/obsidian/src/utils/publishNode.ts (1)
6-6: Remove unused import.The
pllocale fromdate-fnsis imported but never used in this file.Proposed fix
-import { pl } from "date-fns/locale/pl";packages/database/supabase/migrations/20260109035003_fileref.sql (1)
107-110: Consider idempotent bucket creation.The
INSERT INTO storage.bucketswill fail if the assets bucket already exists (e.g., if migration is run multiple times in development). Consider usingON CONFLICT DO NOTHING:Proposed fix
INSERT INTO storage.buckets (id, name, public) VALUES -('assets', 'assets', true); +('assets', 'assets', true) +ON CONFLICT (id) DO NOTHING;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/obsidian/package.jsonapps/obsidian/src/utils/publishNode.tspackages/database/package.jsonpackages/database/src/dbTypes.tspackages/database/src/lib/files.tspackages/database/supabase/config.tomlpackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sqlpnpm-workspace.yaml
🧰 Additional context used
📓 Path-based instructions (4)
apps/obsidian/**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (apps/obsidian/AGENTS.md)
Prefer existing dependencies from package.json for the Obsidian plugin
Files:
apps/obsidian/package.jsonapps/obsidian/src/utils/publishNode.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Follow responsive design principles
Prefertypeoverinterfacefor TypeScript
Use explicit return types for functions in TypeScript
Avoidanytypes in TypeScript when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Prefer small, focused functions over inline code
Extract complex logic into well-named functions
Function names should describe their purpose clearly
Choose descriptive function names that make comments unnecessary
Break down complex operations into smaller, meaningful functions
Prefer early returns over nested conditionals for better readability
Prefer util functions for reusable logic and common operations
Add comments only when necessary; descriptive names should minimize the need for comments
Explain the why, not the what in comments, focusing on reasoning, trade-offs, and approaches
Document limitations, known bugs, or edge cases where behavior may not align with expectations
Files:
packages/database/src/lib/files.tsapps/obsidian/src/utils/publishNode.tspackages/database/src/dbTypes.ts
{packages/ui/**,apps/**}/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use platform-native UI components first with shadcn/ui as a fallback
Files:
apps/obsidian/src/utils/publishNode.ts
apps/obsidian/**/*.{md,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/obsidian/AGENTS.md)
apps/obsidian/**/*.{md,ts,tsx,js,jsx}: Utilize the icon anchor in embedded images to tweak spacing around icons for proper alignment with adjacent text
Icons should be surrounded by parenthesis in references (e.g., ( lucide-cog.svg > icon ))
Files:
apps/obsidian/src/utils/publishNode.ts
🧠 Learnings (18)
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/**/*.{ts,tsx,js,jsx,json} : Prefer existing dependencies from package.json for the Obsidian plugin
Applied to files:
apps/obsidian/package.jsonpackages/database/package.json
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/**/*.{md,ts,tsx,js,jsx} : Utilize the icon anchor in embedded images to tweak spacing around icons for proper alignment with adjacent text
Applied to files:
apps/obsidian/package.json
📚 Learning: 2025-09-11T23:54:41.861Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 431
File: packages/database/package.json:18-22
Timestamp: 2025-09-11T23:54:41.861Z
Learning: In the discourse-graph repository's packages/database/package.json, the typesVersions mapping for "./dbDotEnv" must retain the leading "./" prefix. Removing it (as suggested by standard TypeScript documentation) fails in practice in their specific build environment/tooling setup.
Applied to files:
apps/obsidian/package.jsonpackages/database/package.json
📚 Learning: 2026-01-15T03:03:44.167Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/roam/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:44.167Z
Learning: Applies to apps/roam/**/*.{js,ts,tsx,jsx} : Prefer existing dependencies from package.json
Applied to files:
apps/obsidian/package.jsonpackages/database/package.json
📚 Learning: 2026-01-15T03:03:47.971Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/website/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:47.971Z
Learning: Prefer existing dependencies from package.json
Applied to files:
apps/obsidian/package.json
📚 Learning: 2025-06-25T18:03:52.669Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Applied to files:
packages/database/src/lib/files.tspnpm-workspace.yamlpackages/database/package.jsonpackages/database/supabase/config.tomlpackages/database/src/dbTypes.ts
📚 Learning: 2026-01-14T22:16:15.442Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 698
File: apps/obsidian/src/utils/publishNode.ts:14-19
Timestamp: 2026-01-14T22:16:15.442Z
Learning: In apps/obsidian/src/utils/publishNode.ts, the function intentionally publishes to only the first group (data[0]?.group_id) from a user's group memberships. This single-group limitation is a deliberate design choice for the MVP phase, even though the system supports multi-group memberships via the group_membership table and the publishedToGroups frontmatter array.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/supabase/config.tomlpackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-22T23:50:23.771Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250513173724_content_concept_key.sql:37-60
Timestamp: 2025-05-22T23:50:23.771Z
Learning: For the discourse-graph project, database schema management focuses on the final state in the supabase/schemas directory, not on the individual migration files. When reviewing database changes, consider only the schema definitions in this directory, not potential duplications or conflicts across migration files.
Applied to files:
packages/database/supabase/config.tomlpackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:04:21.602Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250512142307_sync_table.sql:20-21
Timestamp: 2025-05-20T03:04:21.602Z
Learning: Database migration files (like the SQL files in packages/database/supabase/migrations/) should not be modified after they're created, even to fix minor issues like unused variables. Issues should be addressed in newer schema files instead to maintain migration history integrity.
Applied to files:
packages/database/supabase/config.tomlpackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:01:11.220Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504195841_remote_schema.sql:1-7
Timestamp: 2025-05-20T03:01:11.220Z
Learning: Supabase automatically creates necessary schemas like the "extensions" schema before migrations run, so there's no need to explicitly create these schemas in migration files.
Applied to files:
packages/database/supabase/config.tomlpackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-08-23T17:30:00.244Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-08-23T17:30:00.244Z
Learning: For PRs involving both `packages/database/supabase/schemas` and `packages/database/supabase/migrations` files, only report errors in the schema files to avoid duplication, as long as the schema and migration files are consistent.
Applied to files:
packages/database/supabase/config.toml
📚 Learning: 2025-05-20T03:02:36.027Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/schemas/extensions.sql:1-11
Timestamp: 2025-05-20T03:02:36.027Z
Learning: The necessary schemas for Supabase extensions (like 'extensions', 'graphql', and 'vault') are automatically created by Supabase before extensions are installed, so they don't need to be explicitly created in SQL migration files.
Applied to files:
packages/database/supabase/config.tomlpackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in this codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/config.tomlpackages/database/supabase/migrations/20260109035003_fileref.sql
📚 Learning: 2025-05-30T14:49:24.016Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 182
File: apps/website/app/utils/supabase/dbUtils.ts:22-28
Timestamp: 2025-05-30T14:49:24.016Z
Learning: In apps/website/app/utils/supabase/dbUtils.ts, expanding the KNOWN_EMBEDDINGS and DEFAULT_DIMENSIONS mappings to support additional embedding models requires corresponding database model changes (creating new embedding tables), which should be scoped as separate work from API route implementations.
Applied to files:
packages/database/supabase/config.tomlpackages/database/src/dbTypes.ts
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in the discourse-graph codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/config.tomlpackages/database/supabase/migrations/20260109035003_fileref.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, if you rename a table with ALTER TABLE ... RENAME TO, all properties including Row Level Security (RLS) settings remain on the table. If RLS was enabled before the rename, it will still be enabled afterward and does not need to be re-enabled via ALTER TABLE ... ENABLE ROW LEVEL SECURITY. Apply this guidance to any migration that uses RENAME TO in order to avoid unnecessary SQL for RLS.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sql
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: In the discourse-graph codebase, types.gen.ts contains automatically generated database function type definitions that may have reordered signatures between versions. This reordering is expected behavior from the code generation process and should not be flagged as an issue requiring fixes.
Applied to files:
packages/database/src/dbTypes.ts
🧬 Code graph analysis (2)
packages/database/src/lib/files.ts (1)
packages/database/src/lib/client.ts (1)
DGSupabaseClient(9-9)
apps/obsidian/src/utils/publishNode.ts (1)
packages/database/src/lib/files.ts (1)
addFile(5-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
- GitHub Check: validate
🔇 Additional comments (15)
packages/database/package.json (1)
48-48: LGTM -@supabase/storage-jsdependency follows existing patterns.The addition uses
catalog:reference, consistent with other@supabase/*packages in this file (lines 46-49). This enables the storage operations needed for file reference management.pnpm-workspace.yaml (1)
34-34: LGTM - Consistent Supabase package versioning.The
@supabase/storage-jsversion2.88.0matches the other Supabase packages in the catalog (@supabase/supabase-js,@supabase/functions-js,@supabase/auth-js), ensuring compatibility across the suite.packages/database/src/dbTypes.ts (3)
602-676: LGTM - Auto-generated types for new file reference entities.The generated types for
file_gcandFileReferencetables correctly reflect the database schema additions. TheFileReferencetable properly includes foreign key relationships toContentandSpacetables.
1322-1384: LGTM - View types correctly generated with nullable fields.The
my_file_referencesview type appropriately has all fields as nullable, which is the standard pattern for auto-generated view types in Supabase. The relationships mirror those of the underlyingFileReferencetable.
1664-1664: LGTM -file_existsfunction signature.The function type
{ Args: { hashvalue: string }; Returns: boolean }correctly represents the RPC for checking file existence by content hash, supporting the deduplication logic in the file upload flow.packages/database/supabase/config.toml (1)
58-58: LGTM - Schema path ordering looks correct.The
assets.sqlschema is positioned aftercontent.sql(which definesContenttable thatFileReferencereferences) and beforeconcept.sql. This ordering ensures dependencies are satisfied during schema application.apps/obsidian/package.json (1)
25-25: Reconsider this approval against the stated Obsidian coding guideline.The coding guideline requires preferring existing dependencies from package.json, but
mime-typesand@types/mime-typesare new dependencies being added. While Obsidian's API does not provide built-in MIME type detection (necessitating a library), the original approval did not address or justify this exception to the stated guideline. Either justify why this new dependency is necessary despite the constraint, or investigate whether an existing transitive dependency or alternative approach can provide the same functionality.Likely an incorrect or invalid review comment.
packages/database/src/lib/files.ts (2)
5-6: LGTM on function signature.Good use of named parameters with object destructuring per coding guidelines.
17-21: LGTM on hash computation.Correct SHA-256 implementation using Web Crypto API.
apps/obsidian/src/utils/publishNode.ts (1)
21-23: LGTM on context retrieval.Proper null checking and error handling for the Supabase context.
packages/database/supabase/migrations/20260109035003_fileref.sql (2)
104-105: Update trigger may queue files for GC prematurely.The UPDATE trigger fires on any column change, but the GC check uses
OLD.filepath. If a user updateslast_modifiedwithout changingfilepath, this could incorrectly evaluate the filepath count (though it should still find the row). More critically, iffilepathis updated, the old filepath gets queued for GC even though it may still be referenced by other content.
1-24: LGTM on FileReference table structure.The table design with composite PK, foreign keys with cascade, and appropriate indexes is well-structured.
packages/database/supabase/schemas/assets.sql (3)
112-115: INSERT policy allows any authenticated user to upload to assets bucket.The policy
storage_insert_assets_authenticatedonly checksbucket_id = 'assets'without verifying the user has rights to associate this file with any content. An authenticated user could potentially upload arbitrary files to the assets bucket.Consider adding a check that the user has already created a valid FileReference for this hash, or accept this as a trade-off for simpler upload flow (with orphan cleanup via GC).
1-8: LGTM on table structure and RLS policies.The FileReference table design and row-level security policies are well-structured with appropriate access controls.
Also applies to: 40-54
26-38: Inconsistent access check between view and RLS policy.The view
my_file_referencesusesmy_space_ids()(Line 36) while the RLS policy usesin_space()(Line 48). Verify these are semantically equivalent for your access control model.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/database/supabase/migrations/20260109035003_fileref.sql
Outdated
Show resolved
Hide resolved
maparent
added this to the
Improve Repository Management and Workflow Automation milestone
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/obsidian/src/utils/publishNode.ts`:
- Around line 110-117: The cleanup currently only runs when existingFiles.length
is truthy, so when a node has no embeds old FileReference rows are left behind;
change publish cleanup to always call
client.from("FileReference").delete().eq("content_id", contentId) and
conditionally add .notIn("filepath", [...new Set(existingFiles)]) only when
existingFiles.length > 0 so that if there are zero current attachments all
FileReference rows for that content_id are deleted; update the block that
references existingFiles and cleanupResult accordingly (keep the non-failing
error log behavior).
In `@packages/database/src/lib/files.ts`:
- Around line 1-2: The import upsertNodesToSupabaseAsContentWithEmbeddings at
the top of the file is unresolved and unused; remove the import statement
referencing "~/utils/upsertNodesAsContentWithEmbeddings" (or replace it with the
correct path only if the symbol is actually used elsewhere in this module) and
ensure there are no remaining references to
upsertNodesToSupabaseAsContentWithEmbeddings in this file (check functions in
this module that might have intended to call it and either wire the correct
import path or remove those calls).
- Around line 23-29: The file existence check using client.rpc("file_exists") is
non-atomic with the subsequent
client.storage.from(ASSETS_BUCKET_NAME).upload(...) call and can cause 409
conflicts under concurrent publishes; modify the upload invocation (where
uploadResult is assigned) to pass the upload option upsert: true so the storage
upload becomes idempotent and concurrent uploads overwrite rather than fail,
leaving the rest of the logic (lookForDup, exists handling, and throwing
uploadResult.error) unchanged.
♻️ Duplicate comments (1)
packages/database/supabase/schemas/assets.sql (1)
59-101: GC mismatch: column/trigger should use filehash, not filepath.The GC condition checks
filepathand the table column isfilepath, but storage keys arefilehash. This can leave orphaned blobs when different contents share a filename. Consider storingfilehashinfile_gcand checkingfr.filehash.🛠️ Proposed fix
-CREATE TABLE IF NOT EXISTS public.file_gc ( - filepath character varying NOT NULL PRIMARY KEY -); +CREATE TABLE IF NOT EXISTS public.file_gc ( + filehash character varying NOT NULL PRIMARY KEY +); ... - IF (SELECT count(content_id) FROM public."FileReference" AS fr WHERE fr.filepath=OLD.filepath) = 0 THEN + IF (SELECT count(content_id) FROM public."FileReference" AS fr WHERE fr.filehash=OLD.filehash) = 0 THEN INSERT INTO public.file_gc VALUES (OLD.filehash);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/obsidian/src/utils/publishNode.tspackages/database/src/lib/files.tspackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Follow responsive design principles
Prefertypeoverinterfacefor TypeScript
Use explicit return types for functions in TypeScript
Avoidanytypes in TypeScript when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Prefer small, focused functions over inline code
Extract complex logic into well-named functions
Function names should describe their purpose clearly
Choose descriptive function names that make comments unnecessary
Break down complex operations into smaller, meaningful functions
Prefer early returns over nested conditionals for better readability
Prefer util functions for reusable logic and common operations
Add comments only when necessary; descriptive names should minimize the need for comments
Explain the why, not the what in comments, focusing on reasoning, trade-offs, and approaches
Document limitations, known bugs, or edge cases where behavior may not align with expectations
Files:
apps/obsidian/src/utils/publishNode.tspackages/database/src/lib/files.ts
{packages/ui/**,apps/**}/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use platform-native UI components first with shadcn/ui as a fallback
Files:
apps/obsidian/src/utils/publishNode.ts
apps/obsidian/**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (apps/obsidian/AGENTS.md)
Prefer existing dependencies from package.json for the Obsidian plugin
Files:
apps/obsidian/src/utils/publishNode.ts
apps/obsidian/**/*.{md,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/obsidian/AGENTS.md)
apps/obsidian/**/*.{md,ts,tsx,js,jsx}: Utilize the icon anchor in embedded images to tweak spacing around icons for proper alignment with adjacent text
Icons should be surrounded by parenthesis in references (e.g., ( lucide-cog.svg > icon ))
Files:
apps/obsidian/src/utils/publishNode.ts
🧠 Learnings (19)
📚 Learning: 2026-01-14T22:16:15.442Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 698
File: apps/obsidian/src/utils/publishNode.ts:14-19
Timestamp: 2026-01-14T22:16:15.442Z
Learning: In apps/obsidian/src/utils/publishNode.ts, the function intentionally publishes to only the first group (data[0]?.group_id) from a user's group memberships. This single-group limitation is a deliberate design choice for the MVP phase, even though the system supports multi-group memberships via the group_membership table and the publishedToGroups frontmatter array.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/**/*.{md,ts,tsx,js,jsx} : Utilize the icon anchor in embedded images to tweak spacing around icons for proper alignment with adjacent text
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/**/*.{md,ts,tsx,js,jsx} : Icons should be surrounded by parenthesis in references (e.g., ( lucide-cog.svg > icon ))
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/Attachments/icons/obsidian-icon-*.svg : Add the prefix obsidian-icon- before the Obsidian icon name when naming icon files
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-11-23T23:53:43.094Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-23T23:53:43.094Z
Learning: In `apps/roam/src/utils/supabaseContext.ts`, the Roam URL normalization uses a targeted string replacement `url.replace("/?server-port=3333#/", "/#/")` rather than URL parsing to avoid impacting unforeseen URL patterns. This conservative approach is intentional to handle only the specific known case.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/src/lib/files.tspackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-07-08T14:48:38.048Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 272
File: packages/ui/src/lib/supabase/contextFunctions.ts:50-111
Timestamp: 2025-07-08T14:48:38.048Z
Learning: The Supabase client does not offer transaction support, so idempotent upserts with proper conflict resolution (using onConflict with ignoreDuplicates: false) are the preferred approach for multi-step database operations in packages/ui/src/lib/supabase/contextFunctions.ts. This pattern prevents orphaned records when retrying operations.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-10-18T18:58:16.100Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.100Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-05-20T03:04:21.602Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250512142307_sync_table.sql:20-21
Timestamp: 2025-05-20T03:04:21.602Z
Learning: Database migration files (like the SQL files in packages/database/supabase/migrations/) should not be modified after they're created, even to fix minor issues like unused variables. Issues should be addressed in newer schema files instead to maintain migration history integrity.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in this codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-22T23:50:23.771Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250513173724_content_concept_key.sql:37-60
Timestamp: 2025-05-22T23:50:23.771Z
Learning: For the discourse-graph project, database schema management focuses on the final state in the supabase/schemas directory, not on the individual migration files. When reviewing database changes, consider only the schema definitions in this directory, not potential duplications or conflicts across migration files.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:01:11.220Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504195841_remote_schema.sql:1-7
Timestamp: 2025-05-20T03:01:11.220Z
Learning: Supabase automatically creates necessary schemas like the "extensions" schema before migrations run, so there's no need to explicitly create these schemas in migration files.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:02:36.027Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/schemas/extensions.sql:1-11
Timestamp: 2025-05-20T03:02:36.027Z
Learning: The necessary schemas for Supabase extensions (like 'extensions', 'graphql', and 'vault') are automatically created by Supabase before extensions are installed, so they don't need to be explicitly created in SQL migration files.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in the discourse-graph codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-08-23T17:30:00.244Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-08-23T17:30:00.244Z
Learning: For PRs involving both `packages/database/supabase/schemas` and `packages/database/supabase/migrations` files, only report errors in the schema files to avoid duplication, as long as the schema and migration files are consistent.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: When reviewing SQL functions in the discourse-graph codebase, be mindful that suggesting additional explicit error handling may introduce unnecessary database queries. Functions that use appropriate ON CONFLICT clauses may already handle errors sufficiently without needing extra validation queries.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, if you rename a table with ALTER TABLE ... RENAME TO, all properties including Row Level Security (RLS) settings remain on the table. If RLS was enabled before the rename, it will still be enabled afterward and does not need to be re-enabled via ALTER TABLE ... ENABLE ROW LEVEL SECURITY. Apply this guidance to any migration that uses RENAME TO in order to avoid unnecessary SQL for RLS.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sql
📚 Learning: 2025-06-29T14:06:04.789Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 254
File: packages/database/supabase/schemas/space.sql:37-45
Timestamp: 2025-06-29T14:06:04.789Z
Learning: For PostgreSQL triggers, return value requirements differ between BEFORE and AFTER triggers. BEFORE triggers must return appropriate values (OLD for DELETE, NEW for INSERT/UPDATE) because they can affect the operation. AFTER triggers can return any value (NEW, OLD, or NULL) because the return value is ignored - the operation has already completed.
Applied to files:
packages/database/supabase/schemas/assets.sql
🧬 Code graph analysis (2)
apps/obsidian/src/utils/publishNode.ts (1)
packages/database/src/lib/files.ts (1)
addFile(6-57)
packages/database/src/lib/files.ts (1)
packages/database/src/lib/client.ts (1)
DGSupabaseClient(9-9)
🪛 GitHub Actions: CI
packages/database/src/lib/files.ts
[error] 1-1: Cannot find module '~/utils/upsertNodesAsContentWithEmbeddings' or its corresponding type declarations (src/lib/files.ts:1,62).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (6)
packages/database/src/lib/files.ts (1)
31-55: Insert + conflict-update flow looks solid.The duplicate-key handling keeps FileReference metadata fresh without failing re-publishes.
apps/obsidian/src/utils/publishNode.ts (2)
7-8: Attachment regexes now cover markdown and wikilink embeds.Good coverage for both
and![[path]].
82-109: Attachment resolution + upload pipeline is coherent.Resolved paths, binary reads, and
addFileintegration fit the publish flow well.packages/database/supabase/migrations/20260109035003_fileref.sql (1)
1-131: Skipping migration review in favor of schema files.Per project guidance, issues should be addressed in
supabase/schemas(not historical migrations). Based on learnings, focusing review on schema sources instead.packages/database/supabase/schemas/assets.sql (2)
1-55: Core FileReference + RLS setup looks consistent.Table structure, indexes, and policies align with the access model.
107-131: Storage bucket seed + policies look good.Policies align with the file_access/file_exists functions and the assets bucket.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/obsidian/src/utils/publishNode.ts`:
- Around line 82-109: Filter out markdown note links before uploading: when
iterating matches from MARKDOWN_FILE_LINK_RE and WIKILINK_RE, resolve the
attachment via plugin.app.metadataCache.getFirstLinkpathDest as before but skip
processing if the resolved attachment is a markdown/text file (e.g.,
attachment.path endsWith('.md') or mime.lookup(attachment.path) ===
'text/markdown' / startsWith('text/')). Only push non-markdown paths into
existingFiles and only call addFile for those non-text assets; leave
console.warn and continue for missing attachments.
- Around line 7-8: The current regex-based extraction using
MARKDOWN_FILE_LINK_RE and WIKILINK_RE in publishNode.ts fails for filenames with
parentheses and skips valid embeds; replace the regex parsing with Obsidian's
metadata cache: use plugin.app.metadataCache.getFileCache(file)?.embeds
(defaulting to empty array), iterate embeds and resolve each embed via
plugin.app.metadataCache.getFirstLinkpathDest(link, file.path), then process the
resolved attachment instead of relying on the regex matches; remove or disable
MARKDOWN_FILE_LINK_RE and WIKILINK_RE usage and ensure tests cover filenames
with parentheses.
♻️ Duplicate comments (1)
packages/database/supabase/schemas/assets.sql (1)
56-113: Prevent GC of shared hashes (use filehash consistently).Line 95 checks remaining references by
filepath, but GC entries are keyed by hashes (Line 96) and storage objects are hash-named. If a hash is referenced by multiple filepaths, deleting one path can enqueue GC and delete an asset still in use. Align the GC table and trigger logic to usefilehashconsistently (and rename the column to avoid ambiguity).✅ Suggested fix (schema-level)
--- a/packages/database/supabase/schemas/assets.sql +++ b/packages/database/supabase/schemas/assets.sql @@ --- We could pass the name to the edge function, but it's safer to accumulate paths in a table +-- We could pass the name to the edge function, but it's safer to accumulate hashes in a table CREATE TABLE IF NOT EXISTS public.file_gc ( - filepath character varying NOT NULL PRIMARY KEY + filehash character varying NOT NULL PRIMARY KEY ); @@ - IF (SELECT count(content_id) FROM public."FileReference" AS fr WHERE fr.filepath=OLD.filepath) = 0 THEN - INSERT INTO public.file_gc VALUES (OLD.filehash); + IF (SELECT count(content_id) FROM public."FileReference" AS fr WHERE fr.filehash = OLD.filehash) = 0 THEN + INSERT INTO public.file_gc (filehash) VALUES (OLD.filehash); @@ - IF NEW.filehash IS NOT NULL THEN - DELETE FROM public.file_gc WHERE filepath = NEW.filehash; + IF NEW.filehash IS NOT NULL THEN + DELETE FROM public.file_gc WHERE filehash = NEW.filehash; END IF; @@ - DELETE FROM public.file_gc WHERE filepath = NEW.filehash; + DELETE FROM public.file_gc WHERE filehash = NEW.filehash;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/obsidian/src/utils/publishNode.tspackages/database/src/lib/files.tspackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/database/src/lib/files.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Follow responsive design principles
Prefertypeoverinterfacefor TypeScript
Use explicit return types for functions in TypeScript
Avoidanytypes in TypeScript when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Prefer small, focused functions over inline code
Extract complex logic into well-named functions
Function names should describe their purpose clearly
Choose descriptive function names that make comments unnecessary
Break down complex operations into smaller, meaningful functions
Prefer early returns over nested conditionals for better readability
Prefer util functions for reusable logic and common operations
Add comments only when necessary; descriptive names should minimize the need for comments
Explain the why, not the what in comments, focusing on reasoning, trade-offs, and approaches
Document limitations, known bugs, or edge cases where behavior may not align with expectations
Files:
apps/obsidian/src/utils/publishNode.ts
{packages/ui/**,apps/**}/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use platform-native UI components first with shadcn/ui as a fallback
Files:
apps/obsidian/src/utils/publishNode.ts
apps/obsidian/**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (apps/obsidian/AGENTS.md)
Prefer existing dependencies from package.json for the Obsidian plugin
Files:
apps/obsidian/src/utils/publishNode.ts
apps/obsidian/**/*.{md,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/obsidian/AGENTS.md)
apps/obsidian/**/*.{md,ts,tsx,js,jsx}: Utilize the icon anchor in embedded images to tweak spacing around icons for proper alignment with adjacent text
Icons should be surrounded by parenthesis in references (e.g., ( lucide-cog.svg > icon ))
Files:
apps/obsidian/src/utils/publishNode.ts
🧠 Learnings (20)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
📚 Learning: 2026-01-14T22:16:15.442Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 698
File: apps/obsidian/src/utils/publishNode.ts:14-19
Timestamp: 2026-01-14T22:16:15.442Z
Learning: In apps/obsidian/src/utils/publishNode.ts, the function intentionally publishes to only the first group (data[0]?.group_id) from a user's group memberships. This single-group limitation is a deliberate design choice for the MVP phase, even though the system supports multi-group memberships via the group_membership table and the publishedToGroups frontmatter array.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/**/*.{md,ts,tsx,js,jsx} : Utilize the icon anchor in embedded images to tweak spacing around icons for proper alignment with adjacent text
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/**/*.{md,ts,tsx,js,jsx} : Icons should be surrounded by parenthesis in references (e.g., ( lucide-cog.svg > icon ))
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/Attachments/icons/obsidian-icon-*.svg : Add the prefix obsidian-icon- before the Obsidian icon name when naming icon files
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-11-23T23:53:43.094Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-23T23:53:43.094Z
Learning: In `apps/roam/src/utils/supabaseContext.ts`, the Roam URL normalization uses a targeted string replacement `url.replace("/?server-port=3333#/", "/#/")` rather than URL parsing to avoid impacting unforeseen URL patterns. This conservative approach is intentional to handle only the specific known case.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-10-18T18:58:16.100Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.100Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:04:21.602Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250512142307_sync_table.sql:20-21
Timestamp: 2025-05-20T03:04:21.602Z
Learning: Database migration files (like the SQL files in packages/database/supabase/migrations/) should not be modified after they're created, even to fix minor issues like unused variables. Issues should be addressed in newer schema files instead to maintain migration history integrity.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in this codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-22T23:50:23.771Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250513173724_content_concept_key.sql:37-60
Timestamp: 2025-05-22T23:50:23.771Z
Learning: For the discourse-graph project, database schema management focuses on the final state in the supabase/schemas directory, not on the individual migration files. When reviewing database changes, consider only the schema definitions in this directory, not potential duplications or conflicts across migration files.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in the discourse-graph codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:01:11.220Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504195841_remote_schema.sql:1-7
Timestamp: 2025-05-20T03:01:11.220Z
Learning: Supabase automatically creates necessary schemas like the "extensions" schema before migrations run, so there's no need to explicitly create these schemas in migration files.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:02:36.027Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/schemas/extensions.sql:1-11
Timestamp: 2025-05-20T03:02:36.027Z
Learning: The necessary schemas for Supabase extensions (like 'extensions', 'graphql', and 'vault') are automatically created by Supabase before extensions are installed, so they don't need to be explicitly created in SQL migration files.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, if you rename a table with ALTER TABLE ... RENAME TO, all properties including Row Level Security (RLS) settings remain on the table. If RLS was enabled before the rename, it will still be enabled afterward and does not need to be re-enabled via ALTER TABLE ... ENABLE ROW LEVEL SECURITY. Apply this guidance to any migration that uses RENAME TO in order to avoid unnecessary SQL for RLS.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sql
📚 Learning: 2025-08-23T17:30:00.244Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-08-23T17:30:00.244Z
Learning: For PRs involving both `packages/database/supabase/schemas` and `packages/database/supabase/migrations` files, only report errors in the schema files to avoid duplication, as long as the schema and migration files are consistent.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: When reviewing SQL functions in the discourse-graph codebase, be mindful that suggesting additional explicit error handling may introduce unnecessary database queries. Functions that use appropriate ON CONFLICT clauses may already handle errors sufficiently without needing extra validation queries.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-10-15T00:12:54.623Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 482
File: packages/database/supabase/schemas/access_token.sql:43-43
Timestamp: 2025-10-15T00:12:54.623Z
Learning: In PostgreSQL RLS policies, you cannot combine multiple operations like `FOR SELECT, UPDATE, DELETE` in a single CREATE POLICY statement. Use `FOR ALL` to cover all operations, or create separate policies for each operation type (FOR SELECT, FOR INSERT, FOR UPDATE, FOR DELETE).
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-06-29T14:06:04.789Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 254
File: packages/database/supabase/schemas/space.sql:37-45
Timestamp: 2025-06-29T14:06:04.789Z
Learning: For PostgreSQL triggers, return value requirements differ between BEFORE and AFTER triggers. BEFORE triggers must return appropriate values (OLD for DELETE, NEW for INSERT/UPDATE) because they can affect the operation. AFTER triggers can return any value (NEW, OLD, or NULL) because the return value is ignored - the operation has already completed.
Applied to files:
packages/database/supabase/schemas/assets.sql
🧬 Code graph analysis (1)
apps/obsidian/src/utils/publishNode.ts (1)
packages/database/src/lib/files.ts (1)
addFile(5-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (4)
apps/obsidian/src/utils/publishNode.ts (4)
3-5: Verifymime-typesdefault import compatibility.If
esModuleInteropisn’t enabled, the default import can beundefined. Consider switching to a namespace import or confirm the TS config.🛠️ Suggested change (if needed)
-import mime from "mime-types"; +import * as mime from "mime-types";
23-25: Good guard for missing Supabase context.Fail-fast here keeps publish behavior predictable.
45-53: Idempotent upserts look good — please verify SDK behavior.
ignoreDuplicatesis the right intent, but confirm the option is supported and behaves as expected for your supabase-js version.Also applies to: 65-73
110-120: Cleanup handling is solid.Nice to see cleanup runs even when there are zero attachments; this avoids stale references.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/obsidian/src/utils/publishNode.ts`:
- Around line 42-46: Remove the stray console.log in publishNode.ts that prints
idResponse.data.last_modified, lastModifiedDb.getTime(), and file.stat.mtime;
either delete this debug statement or replace it with a proper logger call at
debug/trace level (e.g., using the existing project logger) gated by
configuration, so it does not run in production during every publish. Target the
console.log inside the publishNode flow where idResponse, lastModifiedDb, and
file.stat.mtime are available.
In `@packages/database/src/lib/files.ts`:
- Around line 26-29: Replace the fragile string check on
uploadResult.error.message with a check of the error code: after calling
client.storage.from(ASSETS_BUCKET_NAME).upload(hashvalue, content, {contentType:
mimetype}) inspect uploadResult.error.code and treat code === "already_exists"
as the non-fatal case; for any other error code rethrow uploadResult.error. Keep
the existing upload call and variables (ASSETS_BUCKET_NAME, hashvalue, content,
mimetype) and only change the conditional from checking error.message to
checking uploadResult.error.code.
♻️ Duplicate comments (1)
packages/database/supabase/schemas/assets.sql (1)
95-101: GC trigger should check references byfilehash, notfilepath.If multiple paths share a hash (or a hash changes on update), counting by
filepathcan enqueue a hash that’s still referenced, leading to premature GC or orphaned blobs. Switch the reference check tofilehashto align with storage keys.🛠️ Suggested fix
- IF (SELECT count(content_id) FROM public."FileReference" AS fr WHERE fr.filepath=OLD.filepath) = 0 THEN + IF (SELECT count(*) FROM public."FileReference" AS fr WHERE fr.filehash = OLD.filehash) = 0 THEN INSERT INTO public.file_gc VALUES (OLD.filehash); -- TODO: Invocation with pg_net, following the pattern in -- https://supabase.com/docs/guides/functions/schedule-functions END IF;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/obsidian/src/utils/publishNode.tspackages/database/src/lib/files.tspackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Follow responsive design principles
Prefertypeoverinterfacefor TypeScript
Use explicit return types for functions in TypeScript
Avoidanytypes in TypeScript when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Prefer small, focused functions over inline code
Extract complex logic into well-named functions
Function names should describe their purpose clearly
Choose descriptive function names that make comments unnecessary
Break down complex operations into smaller, meaningful functions
Prefer early returns over nested conditionals for better readability
Prefer util functions for reusable logic and common operations
Add comments only when necessary; descriptive names should minimize the need for comments
Explain the why, not the what in comments, focusing on reasoning, trade-offs, and approaches
Document limitations, known bugs, or edge cases where behavior may not align with expectations
Files:
apps/obsidian/src/utils/publishNode.tspackages/database/src/lib/files.ts
{packages/ui/**,apps/**}/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use platform-native UI components first with shadcn/ui as a fallback
Files:
apps/obsidian/src/utils/publishNode.ts
apps/obsidian/**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (apps/obsidian/AGENTS.md)
Prefer existing dependencies from package.json for the Obsidian plugin
Files:
apps/obsidian/src/utils/publishNode.ts
apps/obsidian/**/*.{md,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/obsidian/AGENTS.md)
apps/obsidian/**/*.{md,ts,tsx,js,jsx}: Utilize the icon anchor in embedded images to tweak spacing around icons for proper alignment with adjacent text
Icons should be surrounded by parenthesis in references (e.g., ( lucide-cog.svg > icon ))
Files:
apps/obsidian/src/utils/publishNode.ts
🧠 Learnings (29)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
📚 Learning: 2026-01-14T22:16:15.442Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 698
File: apps/obsidian/src/utils/publishNode.ts:14-19
Timestamp: 2026-01-14T22:16:15.442Z
Learning: In apps/obsidian/src/utils/publishNode.ts, the function intentionally publishes to only the first group (data[0]?.group_id) from a user's group memberships. This single-group limitation is a deliberate design choice for the MVP phase, even though the system supports multi-group memberships via the group_membership table and the publishedToGroups frontmatter array.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/**/*.{md,ts,tsx,js,jsx} : Utilize the icon anchor in embedded images to tweak spacing around icons for proper alignment with adjacent text
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/**/*.{md,ts,tsx,js,jsx} : Icons should be surrounded by parenthesis in references (e.g., ( lucide-cog.svg > icon ))
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/Attachments/icons/obsidian-icon-*.svg : Add the prefix obsidian-icon- before the Obsidian icon name when naming icon files
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-11-23T23:53:43.094Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-23T23:53:43.094Z
Learning: In `apps/roam/src/utils/supabaseContext.ts`, the Roam URL normalization uses a targeted string replacement `url.replace("/?server-port=3333#/", "/#/")` rather than URL parsing to avoid impacting unforeseen URL patterns. This conservative approach is intentional to handle only the specific known case.
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-10-18T18:58:16.100Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.100Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.
Applied to files:
apps/obsidian/src/utils/publishNode.tspackages/database/src/lib/files.ts
📚 Learning: 2026-01-15T03:03:39.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: apps/obsidian/AGENTS.md:0-0
Timestamp: 2026-01-15T03:03:39.934Z
Learning: Applies to apps/obsidian/**/*.{ts,tsx,js,jsx,json} : Prefer existing dependencies from package.json for the Obsidian plugin
Applied to files:
apps/obsidian/src/utils/publishNode.ts
📚 Learning: 2025-07-08T14:48:38.048Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 272
File: packages/ui/src/lib/supabase/contextFunctions.ts:50-111
Timestamp: 2025-07-08T14:48:38.048Z
Learning: The Supabase client does not offer transaction support, so idempotent upserts with proper conflict resolution (using onConflict with ignoreDuplicates: false) are the preferred approach for multi-step database operations in packages/ui/src/lib/supabase/contextFunctions.ts. This pattern prevents orphaned records when retrying operations.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-08-31T17:37:17.173Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 400
File: packages/database/src/dbDotEnv.js:24-31
Timestamp: 2025-08-31T17:37:17.173Z
Learning: The "all" variant for SUPABASE_USE_DB should only be used in createEnv.mts and not in the dbDotEnv module. It was incorrectly added to dbDotEnv.d.ts but should be removed to maintain proper separation of concerns.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-07-21T14:22:20.752Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 291
File: packages/database/supabase/functions/create-space/index.ts:0-0
Timestamp: 2025-07-21T14:22:20.752Z
Learning: In the discourse-graph codebase, types.gen.ts is not accessible from Supabase edge functions, requiring duplication of types and utilities when needed in the edge function environment at packages/database/supabase/functions/.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-09-11T23:54:41.861Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 431
File: packages/database/package.json:18-22
Timestamp: 2025-09-11T23:54:41.861Z
Learning: In the discourse-graph repository's packages/database/package.json, the typesVersions mapping for "./dbDotEnv" must retain the leading "./" prefix. Removing it (as suggested by standard TypeScript documentation) fails in practice in their specific build environment/tooling setup.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-06-25T18:03:52.669Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-05-30T14:49:24.016Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 182
File: apps/website/app/utils/supabase/dbUtils.ts:22-28
Timestamp: 2025-05-30T14:49:24.016Z
Learning: In apps/website/app/utils/supabase/dbUtils.ts, expanding the KNOWN_EMBEDDINGS and DEFAULT_DIMENSIONS mappings to support additional embedding models requires corresponding database model changes (creating new embedding tables), which should be scoped as separate work from API route implementations.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: In the discourse-graph codebase, types.gen.ts contains automatically generated database function type definitions that may have reordered signatures between versions. This reordering is expected behavior from the code generation process and should not be flagged as an issue requiring fixes.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-09-01T19:09:07.052Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-09-01T19:09:07.052Z
Learning: User prefers to avoid making types explicit in each require statement due to anti-DRY concerns when working with module imports in TypeScript.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-08-23T17:30:00.244Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-08-23T17:30:00.244Z
Learning: For PRs involving both `packages/database/supabase/schemas` and `packages/database/supabase/migrations` files, only report errors in the schema files to avoid duplication, as long as the schema and migration files are consistent.
Applied to files:
packages/database/src/lib/files.tspackages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:04:21.602Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250512142307_sync_table.sql:20-21
Timestamp: 2025-05-20T03:04:21.602Z
Learning: Database migration files (like the SQL files in packages/database/supabase/migrations/) should not be modified after they're created, even to fix minor issues like unused variables. Issues should be addressed in newer schema files instead to maintain migration history integrity.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in this codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-22T23:50:23.771Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250513173724_content_concept_key.sql:37-60
Timestamp: 2025-05-22T23:50:23.771Z
Learning: For the discourse-graph project, database schema management focuses on the final state in the supabase/schemas directory, not on the individual migration files. When reviewing database changes, consider only the schema definitions in this directory, not potential duplications or conflicts across migration files.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in the discourse-graph codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-05-20T03:01:11.220Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504195841_remote_schema.sql:1-7
Timestamp: 2025-05-20T03:01:11.220Z
Learning: Supabase automatically creates necessary schemas like the "extensions" schema before migrations run, so there's no need to explicitly create these schemas in migration files.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: When reviewing SQL functions in the discourse-graph codebase, be mindful that suggesting additional explicit error handling may introduce unnecessary database queries. Functions that use appropriate ON CONFLICT clauses may already handle errors sufficiently without needing extra validation queries.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2025-10-15T00:12:54.623Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 482
File: packages/database/supabase/schemas/access_token.sql:43-43
Timestamp: 2025-10-15T00:12:54.623Z
Learning: In PostgreSQL RLS policies, you cannot combine multiple operations like `FOR SELECT, UPDATE, DELETE` in a single CREATE POLICY statement. Use `FOR ALL` to cover all operations, or create separate policies for each operation type (FOR SELECT, FOR INSERT, FOR UPDATE, FOR DELETE).
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sqlpackages/database/supabase/schemas/assets.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, if you rename a table with ALTER TABLE ... RENAME TO, all properties including Row Level Security (RLS) settings remain on the table. If RLS was enabled before the rename, it will still be enabled afterward and does not need to be re-enabled via ALTER TABLE ... ENABLE ROW LEVEL SECURITY. Apply this guidance to any migration that uses RENAME TO in order to avoid unnecessary SQL for RLS.
Applied to files:
packages/database/supabase/migrations/20260109035003_fileref.sql
📚 Learning: 2025-05-20T03:02:36.027Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/schemas/extensions.sql:1-11
Timestamp: 2025-05-20T03:02:36.027Z
Learning: The necessary schemas for Supabase extensions (like 'extensions', 'graphql', and 'vault') are automatically created by Supabase before extensions are installed, so they don't need to be explicitly created in SQL migration files.
Applied to files:
packages/database/supabase/schemas/assets.sql
📚 Learning: 2025-06-29T14:06:04.789Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 254
File: packages/database/supabase/schemas/space.sql:37-45
Timestamp: 2025-06-29T14:06:04.789Z
Learning: For PostgreSQL triggers, return value requirements differ between BEFORE and AFTER triggers. BEFORE triggers must return appropriate values (OLD for DELETE, NEW for INSERT/UPDATE) because they can affect the operation. AFTER triggers can return any value (NEW, OLD, or NULL) because the return value is ignored - the operation has already completed.
Applied to files:
packages/database/supabase/schemas/assets.sql
🧬 Code graph analysis (2)
apps/obsidian/src/utils/publishNode.ts (1)
packages/database/src/lib/files.ts (1)
addFile(5-57)
packages/database/src/lib/files.ts (1)
packages/database/src/lib/client.ts (1)
DGSupabaseClient(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (4)
packages/database/supabase/schemas/assets.sql (1)
1-55: FileReference schema + RLS setup looks consistent.PK/indexes and the visibility view/policies align well with the access model.
packages/database/supabase/migrations/20260109035003_fileref.sql (1)
1-145: No additional comments for the migration mirror.The schema file is the review source of truth for DB logic in this PR.
Based on learnings, schema files are the focus for DB correctness reviews.apps/obsidian/src/utils/publishNode.ts (2)
52-80: Access upsert flow looks good.The upsert + ignoreDuplicates handling keeps publish idempotent.
89-125: Attachment extraction and cleanup look solid.Using metadata cache embeds + text filtering and unconditional cleanup is a clean, robust flow.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/database/src/lib/files.ts`:
- Around line 25-30: The upload error check currently does a strict string
comparison of uploadResult.error.statusCode to "409" which can fail if
statusCode is numeric; update the logic inside the block that calls
client.storage.from(ASSETS_BUCKET_NAME).upload(hashvalue, content, {contentType:
mimetype}) so you coerce the statusCode to a number (e.g.,
Number((uploadResult.error as any).statusCode)) or otherwise normalize types
before comparing to 409, and only throw uploadResult.error when the normalized
statusCode !== 409; keep the rest of the uploadResult.error handling unchanged.
🧹 Nitpick comments (1)
packages/database/src/lib/files.ts (1)
43-56: Insert-then-update pattern is appropriate; consider removing redundantspace_idfrom update.The manual upsert pattern correctly handles the duplicate key scenario (code "23505"). However,
space_idin the update payload (line 47) is redundant since it's matching bycontent_idandfilepath, andspace_idshouldn't change for the same reference.Optional cleanup
if (frefResult.error.code === "23505") { const updateResult = await client.from("FileReference").update({ - /* eslint-disable `@typescript-eslint/naming-convention` */ - space_id: spaceId, last_modified: lastModified.toISOString(), - /* eslint-enable `@typescript-eslint/naming-convention` */ filehash: hashvalue, created: created.toISOString() }).eq("content_id", contentId).eq("filepath", fname);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/obsidian/src/utils/publishNode.tspackages/database/src/lib/files.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/obsidian/src/utils/publishNode.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Follow responsive design principles
Prefertypeoverinterfacefor TypeScript
Use explicit return types for functions in TypeScript
Avoidanytypes in TypeScript when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Prefer small, focused functions over inline code
Extract complex logic into well-named functions
Function names should describe their purpose clearly
Choose descriptive function names that make comments unnecessary
Break down complex operations into smaller, meaningful functions
Prefer early returns over nested conditionals for better readability
Prefer util functions for reusable logic and common operations
Add comments only when necessary; descriptive names should minimize the need for comments
Explain the why, not the what in comments, focusing on reasoning, trade-offs, and approaches
Document limitations, known bugs, or edge cases where behavior may not align with expectations
Files:
packages/database/src/lib/files.ts
🧠 Learnings (10)
📚 Learning: 2025-07-08T14:48:38.048Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 272
File: packages/ui/src/lib/supabase/contextFunctions.ts:50-111
Timestamp: 2025-07-08T14:48:38.048Z
Learning: The Supabase client does not offer transaction support, so idempotent upserts with proper conflict resolution (using onConflict with ignoreDuplicates: false) are the preferred approach for multi-step database operations in packages/ui/src/lib/supabase/contextFunctions.ts. This pattern prevents orphaned records when retrying operations.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-10-18T18:58:16.100Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.100Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-08-31T17:37:17.173Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 400
File: packages/database/src/dbDotEnv.js:24-31
Timestamp: 2025-08-31T17:37:17.173Z
Learning: The "all" variant for SUPABASE_USE_DB should only be used in createEnv.mts and not in the dbDotEnv module. It was incorrectly added to dbDotEnv.d.ts but should be removed to maintain proper separation of concerns.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-07-21T14:22:20.752Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 291
File: packages/database/supabase/functions/create-space/index.ts:0-0
Timestamp: 2025-07-21T14:22:20.752Z
Learning: In the discourse-graph codebase, types.gen.ts is not accessible from Supabase edge functions, requiring duplication of types and utilities when needed in the edge function environment at packages/database/supabase/functions/.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-09-11T23:54:41.861Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 431
File: packages/database/package.json:18-22
Timestamp: 2025-09-11T23:54:41.861Z
Learning: In the discourse-graph repository's packages/database/package.json, the typesVersions mapping for "./dbDotEnv" must retain the leading "./" prefix. Removing it (as suggested by standard TypeScript documentation) fails in practice in their specific build environment/tooling setup.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-06-25T18:03:52.669Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-05-30T14:49:24.016Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 182
File: apps/website/app/utils/supabase/dbUtils.ts:22-28
Timestamp: 2025-05-30T14:49:24.016Z
Learning: In apps/website/app/utils/supabase/dbUtils.ts, expanding the KNOWN_EMBEDDINGS and DEFAULT_DIMENSIONS mappings to support additional embedding models requires corresponding database model changes (creating new embedding tables), which should be scoped as separate work from API route implementations.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: In the discourse-graph codebase, types.gen.ts contains automatically generated database function type definitions that may have reordered signatures between versions. This reordering is expected behavior from the code generation process and should not be flagged as an issue requiring fixes.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-09-01T19:09:07.052Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-09-01T19:09:07.052Z
Learning: User prefers to avoid making types explicit in each require statement due to anti-DRY concerns when working with module imports in TypeScript.
Applied to files:
packages/database/src/lib/files.ts
📚 Learning: 2025-08-23T17:30:00.244Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-08-23T17:30:00.244Z
Learning: For PRs involving both `packages/database/supabase/schemas` and `packages/database/supabase/migrations` files, only report errors in the schema files to avoid duplication, as long as the schema and migration files are consistent.
Applied to files:
packages/database/src/lib/files.ts
🧬 Code graph analysis (1)
packages/database/src/lib/files.ts (1)
packages/database/src/lib/client.ts (1)
DGSupabaseClient(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (5)
packages/database/src/lib/files.ts (5)
1-4: LGTM!Clean import setup and constant naming follows guidelines (UPPERCASE for constants).
5-16: LGTM!Good use of named parameters via object destructuring for a function with many parameters, explicit return type, and arrow function syntax—all per coding guidelines.
17-21: LGTM!Hash computation is correct. The comment on line 17 appropriately documents the memory assumption.
22-24: LGTM!RPC call with proper error propagation.
31-41: LGTM!Insert operation is well-structured with proper field mapping.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
maparent
force-pushed
the
eng-1285-base-user-affordance-to-trigger-node-sharing
branch
from
d34f8dd to
a45aadf
Compare
maparent
force-pushed
the
eng-1180-f7-publish-non-text-assets-referenced-by-a-dg-node
branch
from
785e305 to
0cf87a3
Compare
maparent
force-pushed
the
eng-1180-f7-publish-non-text-assets-referenced-by-a-dg-node
branch
from
0cf87a3 to
d79feec
Compare
|
@CodeRabbit this was rebased and partly rewritten. Please review. |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/obsidian/src/utils/publishNode.ts`:
- Around line 31-57: The timestamp parsing currently always appends "Z" to
idResponse.data.last_modified, producing an invalid Date when the value already
contains a timezone; change the logic around creating lastModifiedDb so you
first inspect idResponse.data.last_modified (from the idResponse returned by
client.from("Content").select(...)), and only append "Z" if the string has no
timezone indicator (e.g., not ending with "Z" and not containing a ±HH:MM
offset); then construct lastModifiedDb = new Date(parsedString) and keep the
existing skip check that uses existingPublish, myGroup, and file.stat.mtime to
decide whether to return early. Ensure you update the block that defines
lastModifiedDb and the subsequent comparison so NaN is no longer possible.
In `@packages/database/supabase/schemas/assets.sql`:
- Around line 89-104: The trigger function after_delete_update_fref has two
issues: change the EXISTS/count check to use count(*) instead of
count(content_id) and compare file hashes (fr.filehash = OLD.filehash) so GC is
decided by hash references rather than filepaths; also update the cleanup DELETE
in the NEW.filehash branch to delete from public.file_gc by the filehash column
(e.g., WHERE filehash = NEW.filehash) instead of matching filepath. Ensure
references to the FileReference table and public.file_gc use the filehash column
consistently.
🧹 Nitpick comments (2)
apps/obsidian/src/utils/publishNode.ts (1)
62-87: Consider filtering text-like MIME types beyondtext/*.
mime.lookup()returnsapplication/jsonfor JSON files (and other text-like types) which won’t matchtext/*and would still be uploaded. If the intent is strictly “non-text assets,” consider usingmime.charset()to detect text encodings.♻️ Possible refinement
- const mimetype = mime.lookup(attachment.path) || "application/octet-stream"; - if (mimetype.startsWith("text/")) continue; + const mimetype = mime.lookup(attachment.path) || "application/octet-stream"; + const charset = mime.charset(mimetype); + if (mimetype.startsWith("text/") || charset) continue;packages/database/supabase/schemas/assets.sql (1)
55-65: Consider renamingfile_gc.filepathcolumn tofilehash.The
file_gctable column is namedfilepathbut stores hash values (see Line 95 whereOLD.filehashis inserted). This naming inconsistency could cause confusion during maintenance. Consider renaming tofilehashfor clarity.Suggested fix
CREATE TABLE IF NOT EXISTS public.file_gc ( - filepath character varying NOT NULL PRIMARY KEY + filehash character varying NOT NULL PRIMARY KEY );This would also require updating the DELETE statements in the trigger functions.
| const idResponse = await client | ||
| .from("Content") | ||
| .select("id,last_modified") | ||
| .eq("source_local_id", nodeId) | ||
| .eq("space_id", spaceId) | ||
| .eq("variant", "full") | ||
| .maybeSingle(); | ||
| if (idResponse.error || !idResponse.data) { | ||
| throw idResponse.error || new Error("no data while fetching node"); | ||
| } | ||
| const contentId = idResponse.data.id; | ||
| const lastModifiedDb = new Date(idResponse.data.last_modified + "Z"); | ||
| if ( | ||
| existingPublish.includes(myGroup) && | ||
| file.stat.mtime <= lastModifiedDb.getTime() | ||
| ) | ||
| return; // already published | ||
| const publishResponse = await client.from("ContentAccess").upsert( | ||
| { | ||
| /* eslint-disable @typescript-eslint/naming-convention */ | ||
| account_uid: myGroup, | ||
| source_local_id: nodeId, | ||
| space_id: spaceId, | ||
| /* eslint-enable @typescript-eslint/naming-convention */ | ||
| }, | ||
| { ignoreDuplicates: true }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/obsidian/src/utils/publishNode.ts | head -70Repository: DiscourseGraphs/discourse-graph
Length of output: 3145
🏁 Script executed:
fd . apps/obsidian --type f -name "*.sql" -o -name "*.ts" | xargs grep -l "last_modified" 2>/dev/null | head -5Repository: DiscourseGraphs/discourse-graph
Length of output: 305
🏁 Script executed:
rg "last_modified" apps/obsidian --type ts -B 2 -A 2Repository: DiscourseGraphs/discourse-graph
Length of output: 3888
🏁 Script executed:
node << 'EOF'
// Test JavaScript Date behavior with Z suffix
const testCases = [
"2024-01-15T10:30:45.123Z", // Already has Z
"2024-01-15T10:30:45.123ZZ", // Double Z (invalid)
"2024-01-15T10:30:45.123", // Without Z (without tz)
];
testCases.forEach(tc => {
const withZ = new Date(tc + "Z");
console.log(`Input: "${tc}" -> Parsed: ${withZ.toISOString()} (valid: ${!isNaN(withZ.getTime())})`);
});
EOFRepository: DiscourseGraphs/discourse-graph
Length of output: 722
🏁 Script executed:
rg "last_modified" apps/obsidian --type ts -B 5 -A 5 | grep -A 10 -B 10 "new Date"Repository: DiscourseGraphs/discourse-graph
Length of output: 5064
🏁 Script executed:
find apps/obsidian -type f \( -name "*.md" -o -name "*.json" -o -name "supabase.json" \) | xargs grep -l "timestamp" 2>/dev/nullRepository: DiscourseGraphs/discourse-graph
Length of output: 59
🏁 Script executed:
rg "timestamp" apps/obsidian --type ts --type json -B 2 -A 2 | head -40Repository: DiscourseGraphs/discourse-graph
Length of output: 393
Fix double-timezone appending that breaks the skip check.
On line 42, appending "Z" to a Supabase timestamp that already includes it (ISO 8601 format from .toISOString()) creates an invalid Date with NaN value. This causes the skip condition on lines 43–46 to always fail because NaN comparisons evaluate to false, defeating the optimization. Parse the raw value first and only append "Z" if no timezone is present.
🛠️ Suggested fix
- const lastModifiedDb = new Date(idResponse.data.last_modified + "Z");
+ const lastModifiedRaw = idResponse.data.last_modified;
+ const lastModifiedDb = new Date(
+ /Z|[+-]\d{2}:\d{2}$/.test(lastModifiedRaw)
+ ? lastModifiedRaw
+ : `${lastModifiedRaw}Z`,
+ );📝 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 idResponse = await client | |
| .from("Content") | |
| .select("id,last_modified") | |
| .eq("source_local_id", nodeId) | |
| .eq("space_id", spaceId) | |
| .eq("variant", "full") | |
| .maybeSingle(); | |
| if (idResponse.error || !idResponse.data) { | |
| throw idResponse.error || new Error("no data while fetching node"); | |
| } | |
| const contentId = idResponse.data.id; | |
| const lastModifiedDb = new Date(idResponse.data.last_modified + "Z"); | |
| if ( | |
| existingPublish.includes(myGroup) && | |
| file.stat.mtime <= lastModifiedDb.getTime() | |
| ) | |
| return; // already published | |
| const publishResponse = await client.from("ContentAccess").upsert( | |
| { | |
| /* eslint-disable @typescript-eslint/naming-convention */ | |
| account_uid: myGroup, | |
| source_local_id: nodeId, | |
| space_id: spaceId, | |
| /* eslint-enable @typescript-eslint/naming-convention */ | |
| }, | |
| { ignoreDuplicates: true }, | |
| ); | |
| const idResponse = await client | |
| .from("Content") | |
| .select("id,last_modified") | |
| .eq("source_local_id", nodeId) | |
| .eq("space_id", spaceId) | |
| .eq("variant", "full") | |
| .maybeSingle(); | |
| if (idResponse.error || !idResponse.data) { | |
| throw idResponse.error || new Error("no data while fetching node"); | |
| } | |
| const contentId = idResponse.data.id; | |
| const lastModifiedRaw = idResponse.data.last_modified; | |
| const lastModifiedDb = new Date( | |
| /Z|[+-]\d{2}:\d{2}$/.test(lastModifiedRaw) | |
| ? lastModifiedRaw | |
| : `${lastModifiedRaw}Z`, | |
| ); | |
| if ( | |
| existingPublish.includes(myGroup) && | |
| file.stat.mtime <= lastModifiedDb.getTime() | |
| ) | |
| return; // already published | |
| const publishResponse = await client.from("ContentAccess").upsert( | |
| { | |
| /* eslint-disable `@typescript-eslint/naming-convention` */ | |
| account_uid: myGroup, | |
| source_local_id: nodeId, | |
| space_id: spaceId, | |
| /* eslint-enable `@typescript-eslint/naming-convention` */ | |
| }, | |
| { ignoreDuplicates: true }, | |
| ); |
🤖 Prompt for AI Agents
In `@apps/obsidian/src/utils/publishNode.ts` around lines 31 - 57, The timestamp
parsing currently always appends "Z" to idResponse.data.last_modified, producing
an invalid Date when the value already contains a timezone; change the logic
around creating lastModifiedDb so you first inspect
idResponse.data.last_modified (from the idResponse returned by
client.from("Content").select(...)), and only append "Z" if the string has no
timezone indicator (e.g., not ending with "Z" and not containing a ±HH:MM
offset); then construct lastModifiedDb = new Date(parsedString) and keep the
existing skip check that uses existingPublish, myGroup, and file.stat.mtime to
decide whether to return early. Ensure you update the block that defines
lastModifiedDb and the subsequent comparison so NaN is no longer possible.
| CREATE OR REPLACE FUNCTION public.after_delete_update_fref() RETURNS TRIGGER | ||
| SET search_path = '' | ||
| SECURITY DEFINER | ||
| LANGUAGE plpgsql AS $$ | ||
| BEGIN | ||
| IF (SELECT count(content_id) FROM public."FileReference" AS fr WHERE fr.filepath=OLD.filepath) = 0 THEN | ||
| INSERT INTO public.file_gc VALUES (OLD.filehash); | ||
| -- TODO: Invocation with pg_net, following the pattern in | ||
| -- https://supabase.com/docs/guides/functions/schedule-functions | ||
| END IF; | ||
| IF NEW.filehash IS NOT NULL THEN | ||
| DELETE FROM public.file_gc WHERE filepath = NEW.filehash; | ||
| END IF; | ||
| RETURN OLD; | ||
| END; | ||
| $$; |
There was a problem hiding this comment.
Fix trigger function column reference and GC logic.
Two issues in after_delete_update_fref():
-
Line 94:
count(content_id)references a non-existent column.FileReferencehas nocontent_idcolumn—usecount(*)instead. -
Line 94: The condition checks
fr.filepath=OLD.filepath, but the GC decision should be based on whether thefilehashis still referenced by any other row, not the filepath. Multiple files with different filepaths could share the same hash.
Proposed fix
CREATE OR REPLACE FUNCTION public.after_delete_update_fref() RETURNS TRIGGER
SET search_path = ''
SECURITY DEFINER
LANGUAGE plpgsql AS $$
BEGIN
- IF (SELECT count(content_id) FROM public."FileReference" AS fr WHERE fr.filepath=OLD.filepath) = 0 THEN
+ IF (SELECT count(*) FROM public."FileReference" AS fr WHERE fr.filehash=OLD.filehash) = 0 THEN
INSERT INTO public.file_gc VALUES (OLD.filehash);
-- TODO: Invocation with pg_net, following the pattern in
-- https://supabase.com/docs/guides/functions/schedule-functions
END IF;
IF NEW.filehash IS NOT NULL THEN
- DELETE FROM public.file_gc WHERE filepath = NEW.filehash;
+ DELETE FROM public.file_gc WHERE filehash = NEW.filehash;
END IF;
RETURN OLD;
END;
$$;Note: The second diff line assumes the column rename suggested earlier. If keeping filepath as the column name, keep the current DELETE statement.
📝 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.
| CREATE OR REPLACE FUNCTION public.after_delete_update_fref() RETURNS TRIGGER | |
| SET search_path = '' | |
| SECURITY DEFINER | |
| LANGUAGE plpgsql AS $$ | |
| BEGIN | |
| IF (SELECT count(content_id) FROM public."FileReference" AS fr WHERE fr.filepath=OLD.filepath) = 0 THEN | |
| INSERT INTO public.file_gc VALUES (OLD.filehash); | |
| -- TODO: Invocation with pg_net, following the pattern in | |
| -- https://supabase.com/docs/guides/functions/schedule-functions | |
| END IF; | |
| IF NEW.filehash IS NOT NULL THEN | |
| DELETE FROM public.file_gc WHERE filepath = NEW.filehash; | |
| END IF; | |
| RETURN OLD; | |
| END; | |
| $$; | |
| CREATE OR REPLACE FUNCTION public.after_delete_update_fref() RETURNS TRIGGER | |
| SET search_path = '' | |
| SECURITY DEFINER | |
| LANGUAGE plpgsql AS $$ | |
| BEGIN | |
| IF (SELECT count(*) FROM public."FileReference" AS fr WHERE fr.filehash=OLD.filehash) = 0 THEN | |
| INSERT INTO public.file_gc VALUES (OLD.filehash); | |
| -- TODO: Invocation with pg_net, following the pattern in | |
| -- https://supabase.com/docs/guides/functions/schedule-functions | |
| END IF; | |
| IF NEW.filehash IS NOT NULL THEN | |
| DELETE FROM public.file_gc WHERE filehash = NEW.filehash; | |
| END IF; | |
| RETURN OLD; | |
| END; | |
| $$; |
🤖 Prompt for AI Agents
In `@packages/database/supabase/schemas/assets.sql` around lines 89 - 104, The
trigger function after_delete_update_fref has two issues: change the
EXISTS/count check to use count(*) instead of count(content_id) and compare file
hashes (fr.filehash = OLD.filehash) so GC is decided by hash references rather
than filepaths; also update the cleanup DELETE in the NEW.filehash branch to
delete from public.file_gc by the filehash column (e.g., WHERE filehash =
NEW.filehash) instead of matching filepath. Ensure references to the
FileReference table and public.file_gc use the filehash column consistently.
https://linear.app/discourse-graphs/issue/ENG-1180/f7-publish-non-text-assets-referenced-by-a-dg-node
Create a bucket for assets, indexed by content hash. Create a file reference table that associates files with content-local paths. Upload image assets on file publish.
https://www.loom.com/share/00a010ce3b4740cfbb3dff2febb7beb2
The loom video had a glitch: I would not re-publish a published page. Now I check if the file was modified since the sync, but I did not re-do the loom video.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.