Skip to content

[ENG-1890] Add Publish tab to Roam query-results share dialog#1133

Open
sid597 wants to merge 6 commits into
mainfrom
eng-1890-add-publish-tab-to-roam-query-results-share-dialog
Open

[ENG-1890] Add Publish tab to Roam query-results share dialog#1133
sid597 wants to merge 6 commits into
mainfrom
eng-1890-add-publish-tab-to-roam-query-results-share-dialog

Conversation

@sid597

@sid597 sid597 commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

https://www.loom.com/share/d7cd9e76c02f46c6ad13fcda24088b90

Adds a "Publish" tab to the Roam Share Data / query-results dialog (Export.tsx), gated on isSyncEnabled(), that grants a sharing group access to the selected query-result discourse nodes via the existing SpaceAccess/ResourceAccess group-targeted model.

  • Promote getMyGroups/getAvailableGroupIds/MyGroup from Obsidian's importNodes util to shared @repo/database/lib/groups; Obsidian's file becomes a behaviour-preserving re-export shim.
  • New apps/roam/src/utils/publishNodesToGroups.ts: grants access only to nodes already synced (present in my_concepts) and reports the rest as not-yet-synced; upserts SpaceAccess (partial) + ResourceAccess for each node and its schema concept.
  • PublishPanel with multi-group picker, non-discourse-result filter, and a success/skipped/failed count toast.

Open in Devin Review

Adds a "Publish" tab to the Roam Share Data / query-results dialog
(Export.tsx), gated on isSyncEnabled(), that grants a sharing group
access to the selected query-result discourse nodes via the existing
SpaceAccess/ResourceAccess group-targeted model.

- Promote getMyGroups/getAvailableGroupIds/MyGroup from Obsidian's
  importNodes util to shared @repo/database/lib/groups; Obsidian's file
  becomes a behaviour-preserving re-export shim.
- New apps/roam/src/utils/publishNodesToGroups.ts: grants access only to
  nodes already synced (present in my_concepts) and reports the rest as
  not-yet-synced; upserts SpaceAccess (partial) + ResourceAccess for each
  node and its schema concept.
- PublishPanel with multi-group picker, non-discourse-result filter, and
  a success/skipped/failed count toast.
@linear-code

linear-code Bot commented Jun 19, 2026

Copy link
Copy Markdown

ENG-1890

@vercel

vercel Bot commented Jun 19, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
discourse-graph Skipped Skipped Jun 26, 2026 2:06pm

Request Review

@supabase

supabase Bot commented Jun 19, 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 ↗︎.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 4 potential issues.

View 1 additional finding in Devin Review.

Open in Devin Review

Comment thread apps/roam/src/components/Export.tsx Outdated
Comment thread apps/roam/src/utils/publishNodesToGroups.ts
Comment thread apps/roam/src/utils/publishNodesToGroups.ts
Comment thread apps/roam/src/utils/publishNodesToGroups.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 75ec5a3916

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

Comment thread apps/roam/src/utils/publishNodesToGroups.ts
@graphite-app

graphite-app Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

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:

  • What single problem this PR solves
  • Why the files/changes are coupled

@sid597 sid597 requested review from maparent and mdroidian and removed request for maparent and mdroidian June 23, 2026 18:14
@@ -0,0 +1,49 @@
import type { DGSupabaseClient } from "./client";

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 is a shared-home move for existing Obsidian group helpers, not new group behavior. Roam now needs the same getMyGroups / getAvailableGroupIds logic for publishing, and importing from apps/obsidian would create an app-
to-app dependency.

);
};

const getResultPublishNodes = (result: Result): PublishNode[] => {

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.

Roam query results can return wrapper pages whose UID is not itself a discourse node. In that case, the visible row points at the real DG page through :block/refs, so publish resolves referenced user-backed DG nodes before granting access.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@sid597 Can you show an example of this where we wouldn't be expecting to export the uid of the queried row?

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.

Concrete repro from my local ENG-1890 Roam test graph: test-1-graph, page 06-23-2026, query card roamjs-query-page-yRauPWaLM.

For the visible row [[[[QUE]] - cat]], queryBuilder.runQuery("yRauPWaLM") returned the row UID DXRV7Vijg, and queryBuilder.isDiscourseNode("DXRV7Vijg") was false.

But the DG page referenced by that row was [[QUE]] - cat with UID QgKNKlSx-, and queryBuilder.isDiscourseNode("QgKNKlSx-") was true.

So this is not changing normal export behavior for the queried row UID. It is publish-specific: when the query result row is a wrapper/ref page, publishing the row UID would skip it as non-DG/not-synced, while resolving :block/refs publishes the DG node the visible row points at.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you are saying that the query is returning a block that references a single discourse node, then we should not try to "extract" said discourse node and publish it. I'm not sure why we would try to include such behavior.

For example, if the query returned [[[[QUE]] - cat]] some other text, then queryBuilder.runQuery("yRauPWaLM") would return false and that would be correct behavior.

So this is not changing normal export behavior for the queried row UID.

As described, this would be changing expected export behavior.

Could you elaborate on the specific use case? Why would we want to "extract" a discourse node from a query row? When would this be the desired behavior?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Discussed in 1:1, behavior to be removed.


const getResultPublishNodes = (result: Result): PublishNode[] => {
const directNode = findDiscourseNode({ uid: result.uid });
if (directNode && directNode.backedBy === "user")

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.

We still require backedBy === "user" here because sync only uploads user-backed discourse node types; default Page/Block matches should not become publishable.

@sid597 sid597 requested review from maparent and mdroidian June 23, 2026 18:26
Comment thread apps/roam/src/components/Export.tsx Outdated

const { publishableNodes, nonDiscourseCount } = useMemo(() => {
if (!syncEnabled)
return { publishableNodes: [] as PublishNode[], nonDiscourseCount: 0 };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Avoid asserting when possible. If not possible, make a comment explaining why we must resort to asserting.

);
};

const getResultPublishNodes = (result: Result): PublishNode[] => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@sid597 Can you show an example of this where we wouldn't be expecting to export the uid of the queried row?

@maparent maparent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

@maparent maparent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved, but then saw missing functionality: When a node is already shared to a group, the button for that group should be already checked.

* [ENG-1851] Add Roam single-node share command (capture-half)

Add a 'DG: Share current node' command-palette action, gated on
isSyncEnabled(). It deterministically captures the current page's
discourse node (page uid = source-local id), validates it via
findDiscourseNode, and opens the existing Share Data dialog with
exactly one node.

The Publish-tab handoff (initialPanel: "publish") is deferred to
ENG-1890, which owns Export.tsx's initialPanel union and the tab;
finishing it is a one-line change to the exportRender call here.

Mirrors Obsidian's publish-discourse-node command and Roam's
exportCurrentPage.

* [ENG-1851] Open single-node share on Publish tab

* [ENG-1851] Add page title publish button

* [ENG-1851] Align title publish actions

* [ENG-1851] Gate publish action availability

* [ENG-1851] Inline title action layout styles

* [ENG-1851] Address title publish review nits
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.

3 participants