ENG-2019: Extract shared cross-space node discovery into packages/database#1216
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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:
|
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Reviewer orientation — the four places to read, in order.
| }; | ||
| }; | ||
|
|
||
| export const listGroupSharedNodes = async ({ |
There was a problem hiding this comment.
Entry point. Everything below this file's exports started as Roam's ENG-1855 code moved over; the deltas beyond a pure move are the optional groupIds (caller-supplied to avoid a duplicate getAvailableGroupIds query — Obsidian passes it, Roam doesn't) and the #1214 review items applied here: full fetched as a summary only, .neq current-space filter + full-PK ordering on ResourceAccess, Enums<"Platform">, and UTC/RID/arity preserved from the 1855 fixes.
| ); | ||
| }; | ||
|
|
||
| export const buildSharedNodeCandidates = ({ |
There was a problem hiding this comment.
Pure builder = the shared semantics both apps now use: contract gate (instance concept with schema_id + direct title; full is optional per ENG-2016 and only contributes last_modified), exact ResourceAccess identity match, current-space exclusion, newest-first sort. This is the semantics reconciliation — Obsidian previously had none of these gates. The current-space and variant checks are intentionally duplicated here even though the queries also filter them: the exported pure builder is the contract and shouldn't depend on callers pre-filtering.
| concepts, | ||
| contents, | ||
| currentSpaceId, | ||
| export const toDiscoveredSharedNodes = ({ |
There was a problem hiding this comment.
Roam swap: behavior-identical output. What stays app-side is the imported-RID lookup (roamAlphaAPI block props) and this mapper to the existing DiscoveredSharedNode shape (sourceSpaceId was always the space URI — the candidate now carries it directly).
| .neq("space_id", currentSpaceId); | ||
|
|
||
| if (error) { | ||
| const candidates = await listGroupSharedNodes({ |
There was a problem hiding this comment.
Obsidian swap — the PR's one behavior change lands here (see description). Same PublishedNode shape out. The old + "Z" conversions are gone on purpose: the shared module now returns canonical toISOString() values for created/lastModified, so appending another Z would produce NaN. Two hardening deltas vs the old code: null-metadata guard (old code threw on metadata: null) and re-wrapping PostgrestError so the modal Notice shows a real message.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Moved-to map for the deleted code — each comment marks where it landed in packages/database/src/lib/sharedNodes.ts and whether it changed.
| spaces: SharedSpace[]; | ||
| }; | ||
|
|
||
| const getResourceKey = ({ |
There was a problem hiding this comment.
→ sharedNodes.ts. getResourceKey and chunk are verbatim copies. getLatestTimestamp carries the 1855 UTC fix: it normalizes Z-less PostgREST timestamps and returns canonical toISOString() values (created goes through the same normalization). SharedContent gains author_id, created, metadata (Obsidian consumer needs them) and drops content_type (only the old full-gate used it).
| ); | ||
| }; | ||
|
|
||
| export const buildDiscoveredSharedNodes = ({ |
There was a problem hiding this comment.
→ sharedNodes.ts as buildSharedNodeCandidates. Sort/identity logic unchanged; the gate changed per ENG-2016 + #1214 review: full is optional and only its last_modified is considered (no full-text requirement). Also modified: drops importedSourceRids/alreadyImported (now mapped app-side, L29 of this file) and outputs the neutral candidate shape (rid, platform, lastModified, + spaceUri/created/authorId/directMetadata pass-throughs).
| ); | ||
| }; | ||
|
|
||
| const getGroupSharedResources = async ( |
There was a problem hiding this comment.
→ sharedNodes.ts getGroupSharedResources. Deltas vs the 1855 code: optional groupIds param (callers that already fetched group ids — the Obsidian modal — skip the internal getAvailableGroupIds refetch), the current-space exclusion pushed into the request as .neq("space_id", currentSpaceId) (maparent's #1214 nit), and ordering extended to (space_id, source_local_id, account_uid) — the full ResourceAccess PK — so offset pagination has a unique total order.
| values.slice(index * size, (index + 1) * size), | ||
| ); | ||
|
|
||
| const getSharedNodeRows = async ({ |
There was a problem hiding this comment.
→ sharedNodes.ts getSharedNodeRows. The single my_contents read is split per mdroidian's #1214 concern: direct rows carry the listing fields (title + author_id, created, metadata), full is a summary select (last_modified, source_local_id, space_id) — no full text leaves the DB at discovery. Also threads groupIds through and keeps the 1855 arity = 0 filter on concepts.
|
|
||
| // Query my_contents (RLS applied); exclude current space. Get both variants so we can use | ||
| // the latest last_modified per node and prefer "direct" for text (title). | ||
| const { data, error } = await client |
There was a problem hiding this comment.
Not moved — replaced. This query's semantics (any cross-space row, RLS-only, unpaginated) are superseded by the shared module's gated listing; the variant-grouping/direct-preference logic it did by hand is what buildSharedNodeCandidates does behind the gate.
|
ENG-1855 — addressed in this PR ENG-2019 — follow-up in #1216 |
…port-discovery-v2' into eng-2019-extract-shared-cross-space-node-discovery-into
1 participant
Note
Stacked on #1214 (ENG-1855). Base is
eng-1855-add-roam-shared-node-import-discovery-v2, so this diff shows only the extraction. Merge after #1214.Closes ENG-2019.
What
One shared module —
@repo/database/lib/sharedNodes— now lists group-visible shared nodes cross-space for both apps:discoverSharedNodes.ts:getGroupSharedResources,getSharedNodeRows,buildDiscoveredSharedNodes,chunk,getLatestTimestamppackages/database/src/lib/sharedNodes.ts(listGroupSharedNodes+ purebuildSharedNodeCandidates)getPublishedNodesForGroups: inlinemy_contentsquerylistGroupSharedNodes, maps to the existingPublishedNodeshapeRoam keeps its
DiscoveredSharedNodetype,discoverSharedNodesentry point, and app-side imported-RID detection (roamAlphaAPI); it maps candidates through a small exportedtoDiscoveredSharedNodes. Already-imported detection stays app-side in both apps by design.The one behavior change (Obsidian)
Obsidian's import listing previously returned any cross-space
my_contentsrow (RLS-trust only — itsgroupIdsparameter was dead code — no pagination, no contract check). It now uses the shared semantics extracted from Roam:schema_idset) and adirectcontent are listed.fullis optional (ENG-2016) and only itslast_modifiedis read at discovery — full text is never fetched until import.ResourceAccess∩getAvailableGroupIds(RLS stays as defense-in-depth). Whether to move to RLS-trust instead is deliberately out of scope.getAllPages+ 100-id chunked reads (the old query silently capped at the PostgREST row limit).modifiedAtnow takes the latest timestamp across the concept and both content variants (was: content rows only).Deviations from a pure code move
my_contentsselect gainsauthor_id, created, metadata— fields the Obsidian consumer needs.metadata !== nullguard in the Obsidian mapper: the old code crashed on adirectrow with null metadata (typeof null === "object"passes, then.filePaththrows); Roam-origin rows make that case likely now.Error(the shared module throws rawPostgrestError, which the modal's Notice would render as[object Object]).listGroupSharedNodestakes optionalgroupIdsso the Obsidian modal (which already fetches them for its empty-groups notice) doesn't trigger a secondgetAvailableGroupIdsquery. Roam omits it.Infra
packages/databasegets a minimal vitest setup mirroringpackages/content-model; script istest:unitbecause cucumber ownstest. Roam's pure-builder tests moved over (9 tests); Roam keeps 2 mapper tests.turbo.json+ CI gain atest:unitstep so the moved tests stay enforced (they previously ran via Roam's CI-filtered suite).pnpm-lock.yamlis a 3-line hand edit reusing the catalog's existing vitest resolution — a regenerated lockfile re-resolved unrelated packages and broke Obsidian's typecheck, so re-resolution was avoided entirely.Out of scope, unchanged on purpose: the 4 other
chunkcopies,importPreview.ts(preview-time schema checks are a different concern), and any RLS-vs-explicit gating redesign.Review feedback from #1214, resolved here
my_contentsread is split —directrows carry the listing fields;fullis a summary select (last_modified, source_local_id, space_idonly)."Roam" | "Obsidian"unions replaced withEnums<"Platform">(local alias, matchingsupabaseContext.tsusage)..neq("space_id", currentSpaceId)on theResourceAccessquery.buildSharedNodeCandidatesintentionally keeps its own current-space and variant checks — the exported pure builder is the semantic contract and shouldn't depend on callers pre-filtering; the query-side filters are transport optimizations.ResourceAccesspages order by(space_id, source_local_id, account_uid)— the full primary key, so offset pagination can't duplicate/drop tied rows.arity = 0filter, UTC normalization (getLatestTimestampreturns canonicaltoISOString();creatednormalized the same way, so the Obsidian mapper drops its+ "Z"), and RID-shaped source-local IDs preserved viaisRid.Deferred, on purpose:
metadata/author_idon the concepts select (no consumer needs them yet — maparent agreed "can come later") and slimming the candidate to RID-derived fields (Obsidian needs rawsourceLocalId+ numericspaceId, which aren't derivable from the RID; computingridcentrally keeps theisRidpreservation in one place — replied on #1214).Verification
tsc --noEmitclean inpackages/database,apps/roam,apps/obsidian; eslint/prettier clean on all touched files (importNodes.ts keeps its 21 pre-existing warnings, count unchanged).turbo run test:unitverified locally.SUPABASE_USE_DB=local; dist staged atdg-test-eng-2019for manual verification.