[ENG-1859] Add Roam import action for selected shared nodes - #1269
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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:
|
|
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.
Guidepost comments on the non-obvious decisions (details in the PR description).
| if ( | ||
| importedPageUid && | ||
| storedIdentity && | ||
| isImportUpToDate({ |
There was a problem hiding this comment.
The skip decision lives here (not in the import loop) so the find-imported-node lookup happens once and all source-identity logic stays in this module — the ENG-1860 refresh flow inherits the same semantics. Predicate: stored sourceModifiedAt >= source lastModified, both canonical UTC written by this module; missing/unparseable stored identity falls through to update, never to skip.
| let storedIdentity: ImportedSourceIdentity | undefined; | ||
| try { | ||
| importedPageUid = await findImportedNodeUidBySourceRid(sharedNode.rid); | ||
| storedIdentity = importedPageUid |
There was a problem hiding this comment.
find-imported-node moved ahead of fetch-content so an up-to-date node skips without fetching the heavy full payload. No caller observes the old order (ENG-1858 is unreleased on this stack) and all existing stage tests pass unchanged.
| } | ||
| : { sharedNode, status: "failed", message: result.error.message }, | ||
| ); | ||
| } catch (error) { |
There was a problem hiding this comment.
materializeSharedNode is designed to return failures rather than throw, but a few sync roamjs queries sit outside its try blocks — this catch is what guarantees the ticket's "partial failures do not abort the whole selected import" even if one materialization throws.
| ), | ||
| ); | ||
| const failedImports = results.filter(isFailedSharedNodeImport); | ||
| setSelectedRids( |
There was a problem hiding this comment.
Selection is not fully cleared after a run: failed nodes stay selected so a retry after fixing the reported problem (e.g. renaming a colliding page) is one click; successful ones are deselected.
| autoFocus={false} | ||
| canEscapeKeyClose | ||
| canOutsideClickClose | ||
| canEscapeKeyClose={!importing} |
There was a problem hiding this comment.
All close paths (escape, outside click, X, Close button) are disabled while an import runs because the underlying Roam writes can't be cancelled — closing would just hide the progress and the failure report.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| const toggleAllVisibleSelected = (): void => { | ||
| setSelectedRids((previous) => { | ||
| const next = new Set(previous); | ||
| if (allVisibleSelected) | ||
| importableVisibleRids.forEach((rid) => next.delete(rid)); | ||
| else importableVisibleRids.forEach((rid) => next.add(rid)); | ||
| return next; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Race condition: allVisibleSelected is a stale closure captured from render time, but the state updater function receives the current state. If the checkbox is toggled rapidly or state updates between renders, the toggle logic will use outdated allVisibleSelected values, causing incorrect selection behavior.
const toggleAllVisibleSelected = (): void => {
setSelectedRids((previous) => {
const next = new Set(previous);
// Compute allSelected based on current state, not captured render value
const allSelected = importableVisibleRids.length > 0 &&
importableVisibleRids.every((rid) => previous.has(rid));
if (allSelected)
importableVisibleRids.forEach((rid) => next.delete(rid));
else importableVisibleRids.forEach((rid) => next.add(rid));
return next;
});
};The fix computes the selection state inside the updater function based on the actual previous state rather than the captured allVisibleSelected value.
| const toggleAllVisibleSelected = (): void => { | |
| setSelectedRids((previous) => { | |
| const next = new Set(previous); | |
| if (allVisibleSelected) | |
| importableVisibleRids.forEach((rid) => next.delete(rid)); | |
| else importableVisibleRids.forEach((rid) => next.add(rid)); | |
| return next; | |
| }); | |
| }; | |
| const toggleAllVisibleSelected = (): void => { | |
| setSelectedRids((previous) => { | |
| const next = new Set(previous); | |
| const allSelected = | |
| importableVisibleRids.length > 0 && | |
| importableVisibleRids.every((rid) => previous.has(rid)); | |
| if (allSelected) | |
| importableVisibleRids.forEach((rid) => next.delete(rid)); | |
| else importableVisibleRids.forEach((rid) => next.add(rid)); | |
| return next; | |
| }); | |
| }; | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
…ng the shared node
| client: DGSupabaseClient; | ||
| currentSpaceId: number; | ||
| }): Promise<DiscoveredSharedNode[]> => { | ||
| }): Promise<{ |
There was a problem hiding this comment.
Discovery now returns its two sources separately instead of pre-joining them into the DiscoveredSharedNode shape this file had since #1214. That shape was a field-renamed copy of SharedNode plus a baked-in alreadyImported flag — and wiring up import meant mapping it back into a SharedNode while fabricating the fields the renamed copy didn't carry (created, directMetadata). The dialog is the shape's only consumer, so the parallel type is gone: rows stay SharedNode end to end (list → selection → importSharedNodes), and "already imported" is derived from importedSourceRids at render.
| const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { | ||
| const [nodes, setNodes] = useState<DiscoveredSharedNode[]>([]); | ||
| const [nodes, setNodes] = useState<SharedNode[]>([]); | ||
| const [importedRids, setImportedRids] = useState<Set<string>>(new Set()); |
There was a problem hiding this comment.
Single source of truth for imported-ness. Rows don't carry an alreadyImported flag anymore — the badge derives from importedRids.has(node.rid) at render, and a successful import just adds rids to this set (previously this meant walking the nodes array and patching a stored flag on each matching row).
| node.sourceNodeId, | ||
| ].some((value) => value?.toLocaleLowerCase().includes(normalizedSearch)), | ||
| node.sourceLocalId, | ||
| ].some((value) => value.toLocaleLowerCase().includes(normalizedSearch)), |
There was a problem hiding this comment.
No ?. here on purpose: all five searched fields are non-optional strings on SharedNode.
| @@ -1,48 +0,0 @@ | |||
| import { describe, expect, it } from "vitest"; | |||
| import { toDiscoveredSharedNodes } from "~/utils/discoverSharedNodes"; | |||
There was a problem hiding this comment.
Deleted with its subject: this file only tested the SharedNode → DiscoveredSharedNode field renaming, which no longer exists. The replacement join is importedRids.has(node.rid) at render — nothing left to unit-test.
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Second set of inline notes from the pre-PR pass: decisions a reviewer would otherwise have to ask about (staged-result shape, timestamp provenance, sequential loop, Obsidian-only gate).
| type MaterializationSuccess = SourceIdentity & { | ||
| success: true; | ||
| action: "created" | "updated"; | ||
| action: "created" | "updated" | "skipped"; |
There was a problem hiding this comment.
skipped extends the staged-result shape ENG-1858 established (identity echo + success/action or failure/stage). Consumers report per-item outcomes from action/stage; importSharedNodes collapses created/updated to a single "imported" status because the dialog doesn't distinguish them.
| export const getErrorMessage = (error: unknown): string => | ||
| error instanceof Error ? error.message : String(error); | ||
|
|
||
| const isImportUpToDate = ({ |
There was a problem hiding this comment.
Timestamp provenance for the Date.parse here: both sides are canonical ISO-with-Z strings — sourceModifiedAt comes from SharedNode.lastModified, which packages/database normalizes (normalizeUtcTimestamp → toISOString()), and storedModifiedAt is that same value echoed back from the block props we wrote at import time. So this is not naive DB-timestamp parsing; the Z-less-PostgREST hazard is handled a layer down. The NaN guard covers hand-edited or legacy props (block props are user-editable), in which case we fall through to a normal re-import.
| onProgress: (current: number, total: number) => void; | ||
| }): Promise<SharedNodeImportItem[]> => { | ||
| const items: SharedNodeImportItem[] = []; | ||
| for (const sharedNode of sharedNodes) { |
There was a problem hiding this comment.
Sequential on purpose, not an overlooked batch: each iteration is a full materialization (skip check → content fetch → page write) with per-item staged failure reporting and the onProgress callback driving the dialog's progress label. Batching the content fetches would front-load every node's full markdown (the payload discovery deliberately doesn't fetch) and lose per-item progress. Selections are user-sized (a handful), not data-scale.
| new Date(modifiedAt).toLocaleString(); | ||
|
|
||
| const SharedNodeRow = ({ node }: { node: DiscoveredSharedNode }) => ( | ||
| const isImportableSharedNode = (node: SharedNode): boolean => |
There was a problem hiding this comment.
Obsidian-only by MVP scope: Obsidian is currently the only producer of importable full content, so its rows are the only ones with something to materialize. Roam-origin rows still show in the list (visibility of what's shared) but aren't selectable. When another producer lands, this predicate is the one place to widen.
1 participant
eng-1859.mp4
Implements ENG-1859: wires the shared-node discovery dialog (ENG-1855) to the materializer (ENG-1858) so a Roam user can select visible Obsidian-origin nodes and import them, with imported / skipped / failed counts and per-node failure messages.
Stacked on
eng-1858-materialize-obsidian-origin-markdown-into-roam-v3— only the[ENG-1859]commits are new here.What's in the diff
DiscoverSharedNodesDialog.tsx— checkbox column (enabled only for Obsidian-origin rows, since the materializer only supports those), select-all header checkbox over visible importable rows, primaryImport selected (N)button that readsImporting X of N…while running, and a results Callout with the three counts plus per-node failure messages. Failures are also reported to PostHog viainternalError. Inherits the existingisSyncEnabled()gate on the dialog command.importSharedNodes.ts(new) — sequential loop over the selectedSharedNodes; each item is try/caught so partial failures never abort the batch (done-when Releases storage url rewrite #3); returns per-nodeimported | skipped | failed(+message)items. Mirrors Obsidian'simportSelectedNodesshape (onProgress(current, total), sequential, counts).materializeSharedNode.ts— gains the"skipped"action this ticket requires: when the existing imported page's storedsourceModifiedAtis at least as recent as the source's, it returns without fetching content or touching the page.discoverSharedNodes.ts—DiscoveredSharedNodegains one flat key,sourceSpaceDbId(the numeric Space id), plus atoSharedNodemapper that rebuilds theSharedNodethe materializer takes; the listing shape from ENG-1855/ENG-2019 is otherwise unchanged.Decisions a reviewer would ask about
sourceSpaceDbIdandtoSharedNode? The materializer's content query filters by the numericspace_id+source_local_id— and the RID can't replace them without an extra Space lookup per node (the URI→id resolution thatgetSpaceNameIdFromRiddoes for RID-only flows like refresh). Discovery already holds the numeric id, so it's passed through as one flat key, consistent with the existing flattened view model.toSharedNoderebuilds theSharedNodeinput at import time;created/directMetadataare null-filled there because the materializer never reads them (verified against every use) — if a future flow starts reading them, they need to be carried through instead."skipped"slots into the established staged-result shape ({ success: true, action }).sourceModifiedAt >=sourcelastModified, compared viaDate.parse— both values are canonical UTC written/normalized by this same module. Missing or unparseable stored identity falls through to update (never skips on bad data).fullpayload. No caller observes the old order — ENG-1858 is unreleased on this same stack, and all its tests still pass unchanged.getErrorMessageis now exported frommaterializeSharedNode.tsand reused byimportSharedNodes.tsinstead of adding a third private copy of the same helper.Known deferral
fullcontent in a per-node query (inside the materializer). Batching the content fetch for a multi-select import would require the materializer to accept prefetched content; payloads are heavy and the skip path avoids the fetch entirely for up-to-date nodes, so this is deferred rather than reshaped here.Out of scope (per ticket)
Refresh after import (ENG-1860), relations and assets, advanced title-collision UX (collisions fail with an actionable message naming the page to rename/remove).
Testing
eslint --max-warnings 0,prettier --check,tsc --noEmit --skipLibCheckall pass.apps/roam/src/utils, including new coverage: skip-when-unchanged (no content fetch, no writes), update-when-changed, update-when-stored-identity-invalid, per-node outcomes + progress reporting, and continue-after-throw.