Skip to content

ENG-2296 Simplify the Roam import dialog and import feedback - #1479

Draft
sid597 wants to merge 3 commits into
mainfrom
eng-2296-simplify-the-roam-import-dialog-and-import-feedback
Draft

sid597 wants to merge 3 commits into
mainfrom
eng-2296-simplify-the-roam-import-dialog-and-import-feedback

Conversation

@sid597

@sid597 sid597 commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

https://entire.io/gh/DiscourseGraphs/discourse-graph/trails/22

Reviewer brief

  • Review focus: already imported nodes are now hidden, as in Obsidian's import modal. To pull a newer version of an imported node, use the refresh button on its page or DG: Refresh all imported nodes.
  • Review focus: two decisions the ticket left open, made with sid during implementation:
    • The result lists every imported node as a link, not only nodes with warnings. Imported rows leave the table, so the result is the only place to reach them.
    • A click opens the page and closes the dialog. Shift-click opens it in the right sidebar and keeps the dialog open.
  • Review focus: if the relations import fails after the nodes import, the node results still show, only failed nodes stay selected, and a toast says the relations weren't imported.
  • Risk or follow-up: not yet exercised in Roam. The Loom is pending.
  • Risk or follow-up: the sortable header copies the relations table's header markup. Sharing one component would change the relations settings panel, so that's left for a follow-up ticket.
  • Risk or follow-up: the last commit (4c60e7db, the relations-failure message and body scrolling) came out of the second full review and hasn't had its own review pass.

Verification

On 4c60e7db:

  • pnpm install --frozen-lockfile, then pnpm ci:validate: passed (check-types 8/8, test:unit 6/6).
  • npx turbo run build --filter=roam: passed.
  • eslint and prettier on the changed files: passed. The only eslint warnings are pre-existing ones on untouched lines of registerCommandPaletteCommands.ts.

Loom video

Pending.

Scope check

  • Ran $scope-check against ENG-2296 and the final diff.
  • Scope beyond Done When: a relations-import failure now gets its own message, and relations import runs after the node results and selection are set.
  • Required now: hiding imported nodes made the old order leave hidden rows selected, with no result, whenever the relations import failed.
  • Anyone affected or consulted: No.
  • Decision: Not documented.

Standards check

  • Ran $dg-pr-adherence-check against the final diff and PR metadata.

Local delegated full review

  • Ran a comprehensive review of the entire final diff in a subagent with a fresh context. Use $dg-delegated-full-review when no other full-review workflow is available.

Hide already imported nodes and drop the source app, source ID, and
status columns. Sort by source space, title, or modified, newest first
by default. Rename the dialog and command to Import shared nodes, give
the dialog a fixed height, and show import progress and the result in
one area that links each imported node by its Roam title.
Set the node results and failed-node selection before the relations
import, so a relations failure no longer leaves hidden rows selected
without a result list. Stop searching the source app and source ID,
which the table no longer shows.
Report a relations failure after a successful node import as its own
message, so it no longer reads as a node import failure next to a
success summary. Let the dialog body scroll when a short window can't
fit the search, result, and table.
@vercel

vercel Bot commented Sep 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
discourse-graph Skipped Skipped Sep 24, 2026 5:45pm UTC

Request Review

@supabase

supabase Bot commented Sep 24, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@linear-code

linear-code Bot commented Sep 24, 2026

Copy link
Copy Markdown

ENG-2296

</tr>
);

const SortableHeader = ({

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header copies renderSortableHeader in DiscourseRelationConfigPanel.tsx, the ticket's example. The click order differs on purpose. The ticket asks for descending on the first click and ascending on the next, so this table has no unsorted state. Sharing one header means changing the relations settings panel, so it's left for a follow-up. The relations header's font-[inherit] text-[inherit] aren't in Roam's global Tailwind, so leaving them out doesn't change how this header looks.

void loadNodes();
}, [loadNodes]);

const availableNodes = useMemo(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imported nodes are hidden here, as in Obsidian's import modal. A newer version of an imported node still comes in through the refresh button on its page or DG: Refresh all imported nodes.

sendEmail: false,
});
}
await importSharedRelations(client, spaceId, [...importedRids]).catch(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relations import now runs after the node results and the failed-only selection are set. If it fails, the node results still show, and this toast says the relations weren't imported. The stale importedRids argument predates this PR and is harmless: discoverSharedRelations reads the imported RIDs from the graph again.

style={{ width: "min(68rem, calc(100vw - 2rem))" }}
style={{
width: "min(68rem, calc(100vw - 2rem))",
height: "min(48rem, calc(100vh - 4rem))",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The height is inline, next to the existing width, because apps/roam doesn't compile Tailwind and Roam's global stylesheet has no min() height utility.

This branch was previously deployed

1 inactive deployment
Preview — 4c60e7db Deployed Sep 24, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant