ENG-2019: Extract shared cross-space node discovery into packages/database#1216
ENG-2019: Extract shared cross-space node discovery into packages/database#1216sid597 wants to merge 21 commits into
Conversation
Roam's command palette runs commands on Enter keyup, which landed as a click on the dialog's autofocused close button and dismissed it instantly. Disable autoFocus/enforceFocus like AdvancedSearchDialog and restore outside-click close. Also from review: use Tailwind classes instead of inline styles, drop the unused ValidSharedSpace.id field, and unexport the file-internal getImportedSourceRids.
Replaces the hand-rolled paging loop with the existing @repo/database/lib/pagination helper, matching how syncDgNodesToSupabase already reads ResourceAccess.
|
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.
There was a problem hiding this comment.
getPublishedNodesForGroups is now redundant. It looks like the only thing it is doing is adding filePath, which doesn't warrant an entire wrapped function.
There was a problem hiding this comment.
We can either make that change here or create/point to a linear ticket where that change will be made. (the latter is preferred with the current size of this PR.)
There was a problem hiding this comment.
![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.
There was a problem hiding this comment.
It looks like a few specific functions weren't replaced 1:1. Could you walk through those decisions? For example:
- text
- modifiedAt
There was a problem hiding this comment.
text (title)
- Old: const text = direct?.text ?? latest.text ?? ""; — preferred the direct row, but fell back to the latest-modified row's text (which can be the full variant, i.e. the entire markdown body), then to "".
- New: title: direct.text, and the node is dropped if there's no direct row with string text (sharedNodes.ts:183).
- Why: the contract gate. A node with no direct content isn't a valid shared node, so instead of silently putting the full-markdown body (or an empty string) into the title, it's filtered out. The old ?? latest.text could surface a node's whole body as its title.
modifiedAt
- Old: max last_modified across the content rows only, + "Z" → epoch, 0 if null.
- New: getLatestTimestamp([concept.last_modified, direct.last_modified, full.last_modified]), canonical toISOString(), mapped to epoch; falls back to created if all are null (612e651).
- Why: (a) it now also folds in the concept timestamp — a node's type/schema metadata can change without a content edit, and discovery should reflect the newer of the two; (b) created fallback instead of 0, so a node with only a created date still sorts/displays sensibly.
|
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
…ed-cross-space-node-discovery-into
| .neq("space_id", currentSpaceId); | ||
|
|
||
| if (error) { | ||
| const candidates = await listGroupSharedNodes({ |
There was a problem hiding this comment.
getPublishedNodesForGroups is now redundant. It looks like the only thing it is doing is adding filePath, which doesn't warrant an entire wrapped function.
|
|
||
| // 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.
It looks like a few specific functions weren't replaced 1:1. Could you walk through those decisions? For example:
- text
- modifiedAt
| client: DGSupabaseClient; | ||
| currentSpaceId: number; | ||
| groupIds?: string[]; | ||
| }): Promise<SharedNodeCandidate[]> => { |
There was a problem hiding this comment.
Are we planning on re-using a CrossApp contract for the return type?
There was a problem hiding this comment.
CrossApp contract is for the setters and getters converters, we do have the setters converter for getters we will do it in ENG-1856 so, this will be handled in that pr.
SharedNode is a lightweight listing of what a user sees for the nodes that are in shared space.
|
|
||
| let rid: string; | ||
| try { | ||
| rid = isRid(concept.source_local_id) |
There was a problem hiding this comment.
When would source_local_id be rid?
There was a problem hiding this comment.
tbh I am not sure this is being carried over from past and for our existing usecases for roam and obsidian we are not using rid, and then case when we will be using it is in future. So I think we should remove this altogether, what do you guys think? @mdroidian @maparent
The future usecase is re-sharing (a node imported from one app, re-published under its origin RID via the converter)
…ng, created fallback
2 participants
eng-2019-upload.mp4
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.