ENG-1855 Add Roam shared-node import discovery#1214
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. 1 Skipped Deployment
|
sid597
left a comment
There was a problem hiding this comment.
Four short guideposts through the ENG-1855 data flow.
| const resources: ResourceAccess[] = []; | ||
| for (let from = 0; ; from += PAGE_SIZE) { | ||
| const response = await client | ||
| .from("ResourceAccess") |
There was a problem hiding this comment.
Review starts here: candidate identities come only from ResourceAccess rows granted to the user's groups. The later my_* reads only resolve those keys.
| const variants = contentByResource.get(resourceKey); | ||
| const direct = variants?.direct; | ||
| const full = variants?.full; | ||
| if ( |
There was a problem hiding this comment.
This is the ENG-1847 contract gate: only typed instances with both direct and typed full content reach the dialog.
|
|
||
| return [ | ||
| { | ||
| alreadyImported: importedSourceRids.has(sourceNodeRid), |
There was a problem hiding this comment.
Imported status joins on the stable RID, not title or local ID, so identical local IDs in different source spaces remain distinct.
| <HTMLTable striped style={{ width: "100%" }}> | ||
| <thead> | ||
| <tr> | ||
| <th>Source app</th> |
There was a problem hiding this comment.
This stays intentionally read-only: these columns match ENG-1855's pre-import metadata, while materialization remains in ENG-1859.
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.
|
|
||
| return ( | ||
| <Dialog | ||
| autoFocus={false} |
There was a problem hiding this comment.
Roam's command palette dispatches commands on Enter keyup. Without autoFocus={false} that keyup lands as a click on the freshly-focused close button and dismisses the dialog immediately. Same props as AdvancedSearchDialog.
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:
|
Replaces the hand-rolled paging loop with the existing @repo/database/lib/pagination helper, matching how syncDgNodesToSupabase already reads ResourceAccess.
There was a problem hiding this comment.
Sorry for the delay in review.
I'm going to approve this so we can unblock the stacked PRs, but there are 2 big concerns:
- We shouldn't pass the entire
fullcontent to the client for all possible nodes. That should be deferred to only when a user decides to import. Let's make sure we either fix that here or create a follow up ticket to fix it. (link) - The timestamp isn't normalized to UTC so it will provide different values to different clients. (link)
The RID comment is the next strongest to look at.
The rest are strong recommendations.
| spaceId: number; | ||
| }): string => `${spaceId}:${sourceLocalId}`; | ||
|
|
||
| const getLatestTimestamp = (timestamps: (string | null)[]): string | null => { |
There was a problem hiding this comment.
Concept.last_modified and Content.last_modified are PostgreSQL timestamp without time zone columns. The application writes UTC values, but PostgREST returns them without a Z or offset, so Date.parse(timestamp) interprets them in the browser’s local timezone. This definitely shifts the displayed time and can also affect latest-selection/sorting around DST transitions.
Could we normalize these values here before any comparison and have getLatestTimestamp return a canonical toISOString() value? That keeps the PR-local fix small and also makes the existing dialog formatter safe. The test fixtures should use the production shape, such as 2026-06-14T15:00:00, rather than including Z.
Out of scope, but this is something that should be looked at repo wide: ENG-2034: Standardize UTC timestamp handling across the repository
There was a problem hiding this comment.
Addressed: production-shaped timestamps get Z before parsing and getLatestTimestamp returns canonical toISOString() values.
|
I made full optional in ENG-2016, which should make this easier. |
|
And yes UTC! |
|
For me UTC is a blocker, because it means inserting invalid data in the database. |
|
ENG-1855 — addressed in this PR ENG-2019 — follow-up in #1216 |
|
@mdroidian @maparent addressed some comments others will be addressed in next pr |
maparent
left a comment
There was a problem hiding this comment.
One tiny needed change, two comments. Focused on the backend.
| currentSpaceId: number; | ||
| }): Promise<SharedNodeRows> => { | ||
| const resources = (await getGroupSharedResources(client)).filter( | ||
| (resource) => resource.space_id !== currentSpaceId, |
There was a problem hiding this comment.
nit: this should be pushed to Db request. Not vital.
| client | ||
| .from("my_concepts") | ||
| .select( | ||
| "is_schema, last_modified, schema_id, source_local_id, space_id", |
There was a problem hiding this comment.
I'd expect metadata and author_id here, but I guess they can both come later.
| "is_schema, last_modified, schema_id, source_local_id, space_id", | ||
| ) | ||
| .eq("space_id", spaceId) | ||
| .eq("is_schema", false) |
There was a problem hiding this comment.
Add .eq("arity", 0) to avoid relations.
https://www.loom.com/share/7210cfc4b3984f2ab927e7092dfab9c1
Summary
Adds a read-only Roam discovery surface for group-visible shared nodes.
ResourceAccess, so discovery reflects shared persistence rather than direct source-app access or every resource the user can read.my_spaces,my_concepts, andmy_contents.directand typedfullcontent before a resource is shown.discourse-graph.importedFrom.sourceNodeRidpages as imported.DG: Discover shared nodeswith Blueprint loading, retry, empty, search, reload, and status states.autoFocus={false}/enforceFocus={false}(same asAdvancedSearchDialog): Roam's command palette runs commands on Enter keyup, and without this the keyup clicks the autofocused close button and instantly dismisses the dialog.Imported status
Nothing writes
importedFromyet, so every node shows Available for now. This PR defines the prop path it reads (discourse-graph.importedFrom.sourceNodeRid); the import action (ENG-1859) writes that metadata when it materializes nodes.How to read this diff
This is 766 added lines because Roam had no discovery surface yet; it is four bounded pieces, not a broad rewrite.
The command registry is the only existing production file changed. There are no changes to shared packages, persistence schemas, migrations, sync behavior, or existing import code.
Suggested review order: start at
getGroupSharedResources, follow the rows throughbuildDiscoveredSharedNodes, then scan the table and tests.Review guide
discoverSharedNodes.ts: group-backed candidate selection and contract filtering.DiscoverSharedNodesDialog.tsx: read-only presentation of the ticket's source metadata.discoverSharedNodes.test.ts: resource identity, contract completeness, imported RID matching, current-space exclusion, and sorting.registerCommandPaletteCommands.ts: sync-gated command entry only.Scope
This PR intentionally stops at discovery. It does not materialize or refresh Roam nodes, import relations or assets, change sync behavior, or modify the database schema.
Validation
pnpm --filter roam test- 10 files / 53 tests passedpnpm --filter roam check-typespnpm --filter roam build- built with 0 errorspnpm --filter roam lint- 0 errors; existing warnings unchangedgit diff --check