ENG-2175 Backfill format on existing node type schema rows - #1346
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Notes on the non-obvious decisions, one per thread.
| import { type DiscourseNode } from "./getDiscourseNodes"; | ||
|
|
||
| export const SCHEMA_FORMAT_PROBE_SELECT = | ||
| "source_local_id, format:literal_content->>format"; |
There was a problem hiding this comment.
PostgREST reads an absent jsonb key and a JSON null the same way: both come back as null through ->>. The Roam producers never write a null format, so both mean "backfill", and we partition client-side instead of filtering with .is(...). supabase-js types the projection string; the declared row type widens it to string | null, which is what actually arrives.
This mirrors eng-2155's core_title helper but stays in apps/roam: Obsidian schema rows carry source_data.format from the start, so only Roam needs it. Obsidian's template_content probe pushes the null test into the query instead; here we need the "already set" count, so we fetch all own-space schema rows (tens) and split.
| return { | ||
| nodeTypeIdsToBackfill: intersection(missingFormatIds, localTypeIds), | ||
| withFormatCount, | ||
| orphanedCount: difference(missingFormatIds, localTypeIds).size, |
There was a problem hiding this comment.
Orphans are probed rows that no longer match a configured node type. We report the count and never guess a format. They can persist: cleanupOrphanedNodes deletes rows whose Roam block is gone, but a type removed from the DG config keeps its page and its row. The core_title backfill accepted the same permanence for renamed pages.
An empty format counts as already set. The producer wrote the key (format defaults to ""), so there is nothing to recompute and the backfill converges.
| if (sharedNodeTypeIds.has(nodeType.type)) { | ||
| if ( | ||
| sharedNodeTypeIds.has(nodeType.type) || | ||
| backfillNodeTypeIds.has(nodeType.type) |
There was a problem hiding this comment.
A second force-include beside sharedNodeTypeIds instead of smuggling the backfill through it; they mean different things. The set is an intersection with ids already in my_concepts, so the backfill can only rewrite existing rows. It cannot publish a schema the user never synced, including in shared-content-only mode.
| }); | ||
| }; | ||
|
|
||
| const probeSchemaFormatBackfill = async ({ |
There was a problem hiding this comment.
Runs only on isInitialSync, so the second cycle probes nothing and the report goes silent. Paged through getAllPages like the other my_concepts reads in this file; the expected scale is node types per space, so one page in practice. A probe failure fails the cycle like any other phase; initialSync stays true, so the next attempt probes again.
| }); | ||
| }; | ||
|
|
||
| const reportSchemaFormatBackfill = ({ |
There was a problem hiding this comment.
Deliberately independent of showToast, same as the core_title report on #1332: it announces a one-time data migration, not cycle status. skipped keeps the property name from the core_title posthog event; the toast calls the same number "already had one".
| node_uid: string, | ||
| ): { | ||
| author_uid: string; | ||
| author_local_id: string; |
There was a problem hiding this comment.
Required by the backfill, not opportunistic. concept_local_input has no author_uid field, so jsonb_populate_record dropped it and every sync-produced concept carried a NULL author_id. upsert_concepts sets author_id unconditionally on conflict, so re-upserting publish-origin schema rows (which do have an author) would blank them, and dbNodeSchemaToCrossApp then throws "Missing author". Identical to a493612 on eng-2155; whichever merges second sees a no-op conflict.
page_uid below is the same dead-key class but inert: nothing maps to it and nothing breaks because of it. Removing it belongs with the literal_content shape unification.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![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: 0e90eb4607
ℹ️ 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".
| .from("my_concepts") | ||
| .select(SCHEMA_FORMAT_PROBE_SELECT) |
There was a problem hiding this comment.
When a space has more than 1,000 non-relation schema rows, this unpaginated select is truncated by the repository's Supabase max_rows = 1000 setting (packages/database/supabase/config.toml:18). Because the query includes schemas that already have a format and initialSync is then cleared, rows beyond that first page are never added to nodeTypeIdsToBackfill; subsequent sessions will generally fetch the same first page and leave the remaining legacy rows unchanged. Use the existing getAllPages helper with a stable ordering, as the other potentially large my_concepts probes in this file do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Switched to getAllPages with .order("id") in 29d605a, matching the other my_concepts reads in this file. Expected scale is node types per space, so one page in practice, but you're right that a truncation at max_rows = 1000 would have been silent and would never converge.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (backfilled === 0 && orphaned === 0) return; | ||
| const messages = [ | ||
| `Backfilled format for ${backfilled} node type${backfilled === 1 ? "" : "s"}.`, | ||
| `${skipped} already had one.`, | ||
| ]; | ||
| if (orphaned > 0) { | ||
| messages.push(`${orphaned} no longer match a node type in this graph.`); | ||
| } |
There was a problem hiding this comment.
🟡 Toast reports "0 node types" backfilled
When no formatless rows match a local node type but some are orphaned, the guard still shows the toast, and its message is hardcoded to lead with Backfilled format for 0 node type(s). before the orphan warning. Users see a confusing zero-count line.
Prompt for agents
In reportSchemaFormatBackfill in apps/roam/src/utils/syncDgNodesToSupabase.ts, the early-return guard only suppresses the toast when both backfilled and orphaned are 0. When backfilled is 0 but orphaned > 0, the message array still unconditionally includes the line 'Backfilled format for 0 node types.' which is misleading. Consider only including the 'Backfilled format for N node types.' line when backfilled > 0, and only including the 'N already had one.' line when relevant, so the toast reads sensibly in the orphaned-only case.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Known shape, kept deliberately: it mirrors reportCoreTitleBackfill on #1332, which reads the same way in the orphaned-only case. The PR body tracks a joint fix for the two reports (recurring orphan toast, message composition) so they don't fork.
1 participant
Schema rows synced before #1316 never get
literal_content.format: publish skips schemas that already exist, and the periodic sync only re-selects types edited since the last watermark. This backfills them through the sync producer. Stacked on #1316; only the last commit is this PR.How it works:
is_schema = true,is_relation = false) for a missingformat.discourseNodeSchemaToLocalConceptrecomputes the complete row.Decisions, annotated inline on the diff:
upsert_conceptsreplacesliteral_contentwholesale, and the publish converter writes a different shape.getNodeExtraDatanow returnsauthor_local_idinstead ofauthor_uid. Without it this backfill would blankauthor_idon publish-origin rows. Same change as a493612 on eng-2155.Known gaps, shared with the core_title backfill on #1332: the orphan toast repeats on each load, and the posthog event has no space attribution. Worth one joint fix later rather than forking the two reports now.