ENG-2155 Migrate existing database titles to undecorated core_title - #1332
Conversation
…e-titles-into-core_title-on-publish' into eng-2155-migrate-existing-database-titles-to-undecorated-core_title
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Annotations on the non-obvious decisions in this backfill.
| @@ -0,0 +1,23 @@ | |||
| export const CORE_TITLE_PROBE_SELECT = | |||
| "source_local_id, core_title:literal_content->>core_title"; | |||
There was a problem hiding this comment.
literal_content is jsonb NOT NULL DEFAULT '{}', so the column is never null but the core_title key is absent on every row published before #1317/#1318. PostgREST can't tell an absent key from a JSON null. Both come back as null, and both mean the same thing here, so we partition client-side instead of filtering with .is(...).
|
|
||
| export type CoreTitleProbeRow = { | ||
| source_local_id: string | null; | ||
| core_title: string | null; |
There was a problem hiding this comment.
supabase-js types core_title:literal_content->>core_title as string, not string | null. That's wrong at runtime. Declaring the row type with string | null widens the response safely, so there's no cast and no guard, and the null branch stays honest.
| core_title: string | null; | ||
| }; | ||
|
|
||
| export const partitionByCoreTitle = ( |
There was a problem hiding this comment.
Shared because both apps need the identical definition of "this row is missing a core title". Obsidian has no test runner, so this is the only place that behavior is covered by a test. It describes a PostgREST projection, not cross-app data, which is why it doesn't extend crossAppContracts.ts.
| missingCoreTitleIds.has(node.source_local_id), | ||
| ), | ||
| withCoreTitleCount, | ||
| orphanedIds: [...difference(missingCoreTitleIds, localIds)], |
There was a problem hiding this comment.
orphanedCount is missing minus allNodes: rows whose source_local_id is not a discourse node in this graph any more, because the page was deleted or renamed so its title no longer matches its type's format. We report them, we never guess a title for them. cleanupOrphanedNodes later in the sync deletes only rows whose block is gone, so a renamed page's row stays and keeps being counted here. Only the count is kept: nothing reads the ids, and Obsidian stores a number too.
| } | ||
| }; | ||
|
|
||
| const reportCoreTitleBackfill = ({ |
There was a problem hiding this comment.
The probe only runs on isInitialSync, so the second sync finds nothing and this goes silent. The toast is deliberately independent of showToast. That flag gates sync failure toasts, and a one-time migration result should still show if the initial sync got retried by the timer.
| sharedSourceLocalIds.has(node.source_local_id), | ||
| ) | ||
| : changedNodeInstances; | ||
| const nodesToBackfillCoreTitle = ( |
There was a problem hiding this comment.
Backfill nodes are merged into the concept batch only. They never reach upsertNodesToSupabaseAsContentWithEmbeddings, so a backfill costs no re-embedding. When sync is off we only backfill nodes that are actually shared, matching what nodeInstancesToSync does one line up.
| context: activeContext, | ||
| }), | ||
| }); | ||
| if (coreTitleBackfill !== null) { |
There was a problem hiding this comment.
Reported after the concept upsert resolves. upsertConceptBatches throws on any batch error or negative row id, so reaching this line means the rows really were written (a throw leaves the earlier rows committed and skips this report; that is the pre-existing per-row semantics of upsert_concepts, not changed here). backfilled is counted after the sharedNodesOnlySync filter and skipped before it, so with sync off the non-shared rows missing a core title are in neither number; deferred carries them so the toast does not read as a complete pass. One thing to flag: pushing a row back through this producer also recomputes the concept name, which for block-backed types is node_title + " " + text. That is inherent to recomputing locally instead of patching one key.
| supabaseClient | ||
| .from("my_concepts") | ||
| .select("source_local_id") | ||
| .select(CORE_TITLE_PROBE_SELECT) |
There was a problem hiding this comment.
Same trick as the Roam side. This query already pages every own-space instance row for missingConcepts, so adding the projected key costs zero extra requests. It projects the key rather than literal_content, unlike the older template_content probe below, which pulls the whole column and re-filters in JS. That one is worth a follow-up ticket, but it's out of scope here.
| finalChangeTypes.length === 0 && | ||
| !missingConcepts?.has(node.nodeInstanceId) | ||
| !missingConcepts?.has(node.nodeInstanceId) && | ||
| !coreTitleProbe?.missingCoreTitleIds.has(node.nodeInstanceId) |
There was a problem hiding this comment.
Same widening the missingConcepts mechanism already uses. Forced-in nodes carry changeTypes: [], and getVariantsToCreate turns that into no content and no embedding work, so only the concept gets re-upserted. Imported nodes still continue above this, so their rows stay for the source space to backfill.
| fullSync: true, | ||
| }); | ||
|
|
||
| if (coreTitleBackfill !== null) { |
There was a problem hiding this comment.
The Notice fires after convertDgToSupabaseConcepts, which throws on upsert error, so the counts describe rows that were actually written. It stays silent when nothing was backfilled and nothing was orphaned, which is every run after the first.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1acd957d8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| core_title: extractContentFromTitle( | ||
| nodeTypesById[nodeData.nodeTypeId]?.format ?? "", | ||
| nodeData.file.basename, | ||
| ), |
There was a problem hiding this comment.
{content} capture instead of the first placeholder
When an Obsidian node type uses an allowed format with another placeholder before {content} (for example, {Source} - {content}), getDiscourseNodeFormatExpression creates one capture per placeholder but extractContentFromTitle always returns capture 1. This therefore stores the source value as core_title, corrupting the backfill and all subsequent concept upserts for that format; resolve the capture index for {content} as the Roam implementation does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed on the defect; it is in #1318's producer (apps/obsidian/src/utils/extractContentFromTitle.ts), which this PR's diff range includes but does not change. Added to the description's known-gaps list alongside the two other #1317/#1318 findings: all three store a non-null but wrong core_title, and this migration treats any non-null value as already set, so those rows need a value-aware pass once the extractor is fixed.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
getNodeExtraData returned the Roam creator uid under author_uid, a key the concept_local_input type does not have, so every instance pushed by the sync lost its author_id on upsert; re-pushing every pre-core_title row would have blanked the authors publish had set. The key is now author_local_id, which _local_concept_to_db_concept resolves against PlatformAccount. The Roam toast now states how many rows wait for sync to be enabled instead of implying the pass was complete, names the leftover rows as no longer matching a discourse node, and carries the explicit 5s timeout its siblings use; the backfill result holds the leftover count rather than ids nobody reads. Obsidian's asset pass runs only for nodes with a detected file change, so backfill-only nodes cost no file-reference round-trips.
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
| const last_modified = new Date(last_modified_t).toISOString(); | ||
| return { | ||
| author_uid, | ||
| author_local_id: author_uid, |
There was a problem hiding this comment.
The key this object is spread into is concept_local_input.author_local_id; author_uid was silently dropped by jsonb_populate_record, so every instance the Roam sync pushed had author_id = NULL and a re-push of a published row blanked the author publish had set. With the backfill re-pushing every pre-core_title row in one pass this stopped being a slow leak, hence the fix here. _local_concept_to_db_concept only looks the account up (no creation), so rows whose author has no PlatformAccount stay NULL as before; the datalog variable keeps its name.
| await syncPublishedNodesAssets(plugin, changedNodeInstances); | ||
| await syncPublishedNodesAssets( | ||
| plugin, | ||
| changedNodeInstances.filter((node) => node.changeTypes.length > 0), |
There was a problem hiding this comment.
syncPublishedNodeAssets reads and rewrites FileReference rows per published node unconditionally, so without this filter every published backfill-only node cost two round-trips on the first full sync after this ships. Nodes forced in with changeTypes: [] (core-title backfill, and the pre-existing missing-concept case) have unchanged files, so their asset references are already right.
| `Backfilled core title for ${backfilled} node${backfilled === 1 ? "" : "s"}.`, | ||
| `${skipped} already had one.`, | ||
| ]; | ||
| if (deferred > 0) { |
There was a problem hiding this comment.
With sync off only shared nodes are pushed, so backfilled is the post-filter count while skipped comes from the whole probe. deferred is the rows that were neither, so the toast does not claim a completed pass; they are picked up by the next initial sync once sync is on.
1 participant
eng-2155.mp4
Rows published before #1317 and #1318 have no
core_titleinliteral_content, so importers fall back to the foreign decorated title. This backfills them client-side, per app, over each app's own space. It has to be client-side:core_titleis{content}extracted with the node type's format, and the database never stores the format (crossAppNodeSchemaToDbConceptpersiststemplate/template_contentonly), so the undecoration can only be computed where the graph or vault is.Each app already pages every own-space instance concept during a full sync to work out which concepts went missing. This extends that same query to also project
literal_content->>core_titleand partitions the rows in JS: rows with a null value get forced into the batch that the #1317/#1318 producers recompute from the local graph or vault. No extra requests and noliteral_contentpayload, just the one key. The partition is client-side because the same paged result computesmissingConcepts; filtering it server-side would break that consumer.Sync core_title backfillPostHog event.cleanupOrphanedNodesremoves only rows whose block is gone, so a renamed page's row stays and keeps being reported.One producer fix rides along because the backfill made it urgent:
getNodeExtraDatareturned the Roam creator uid asauthor_uid, a keyconcept_local_inputdoes not have, so every instance the Roam sync pushed landed withauthor_id = NULL, and a later re-push of a row that publish had given an author blanked it. Re-pushing every pre-core_titlerow through that producer would have done that to the whole published corpus at once. The key is nowauthor_local_id, which_local_concept_to_db_conceptresolves againstPlatformAccount; rows whose author has no account row stay NULL as before. This also means ordinary Roam syncs now setauthor_idwhere they did not.Not rewritten: stored decorated titles, relations, assets. Pushing a Roam row back through the sync producer also recomputes the concept
name, which for block-backed node types isnode_title + " " + text. That's inherent to recomputing locally rather than patching one key.upsertConceptBatcheskeeps its existing behaviour of throwing on any negative row id, so one row failure aborts the sync with the earlier rows committed; that predates this PR (a first-ever sync has the same shape) and is not changed here.orderConceptsByDependency's "dependencies not in batch" warning fires whenever a node's type was not itself edited since the last sync, so it also fires on this pass; pre-existing noise, not a correctness issue (upsert_conceptsresolvesschema_idfrom the database).Three known gaps, all from open bot findings on #1317/#1318 and all producing a non-null but wrong
core_title: an empty{content}capture, unescaped regex metacharacters in a node format, and Obsidian'sextractContentFromTitlereturning capture 1 rather than the{content}capture (so{Source} - {content}stores the source). This migration treats any non-null value as already set, so those rows will need a separate value-aware pass once those bugs are fixed. The serialized keycore_titleis a bare literal on the producer and probe sides; a shared constant is a follow-up once #1317/#1318 and the readers on #1330/#1331 land, since they sit on different bases.partitionByCoreTitleand the probe select live inpackages/database/src/libbecause both apps need the identical definition of "missing a core title", and because Obsidian has no test runner, so the shared test is the only coverage over logic Obsidian runs. It's a PostgREST projection shape, not cross-app data, so it doesn't extendcrossAppContracts.ts. supabase-js types the projection asstring; it's null at runtime for rows without the key, so the row type declaresstring | null.Stacked on #1317; the diff range also includes #1318's 7-line commit. This branch predates main's switch from
aritytois_relationonmy_conceptsnode queries, so the two extended selects keeparityto match this tree and need the switch when #1317 rebases onto main.Demo note: the Roam half of the video shows the two rows before and after the first sync after extension load (the rows are read back over REST); the 5 s toast fires during the recorder's extension-load phase, so it is not on camera, and the Obsidian half shows the equivalent notice. While recording, the dev graph's first sync on this branch ended with a "Discourse node embeddings sync failed" toast after the concept rows were written. What I could establish afterwards: every targeted row had
core_titlewritten, the local Postgres log (log_min_messages = warning) shows noupsert_conceptsrow-failure warning and no statement timeout in that window, and everything after the concept upsert increateOrUpdateDiscourseEmbeddingis wrapped (cleanupOrphanedNodes,endSyncTask) or non-throwing (the report). So the throw is client-side and I could not reproduce it offline; the browser console linecreateOrUpdateDiscourseEmbedding: Process failed:from a live run is what pins it. Worth a look before merging.