[ENG-1861] Add Roam refresh-all for imported nodes - #1286
Conversation
…erts Add an 'Enable node sharing' feature flag and gate the Publish tab, 'DG: Share current node' command, and page-title Publish button on it instead of the suggestive-mode overlay flag. Publishing now converts selected nodes to full CrossAppNodes (direct title + full markdown) and upserts schema, concept, and contents before granting group access; nodes whose upsert fails are excluded from grants.
…ting publish timestamps upsert_concepts returns -1 for unique violations and -2 for other errors; only -1 was treated as a failure, so a -2 node kept its ResourceAccess grant. Node timestamps now follow the sync query's fallback chain (create -> edit -> page-edit) instead of falling back to Date.now(), which would have persisted publish time as last_modified whenever :page/edit-time was absent. failedSyncedUids is renamed to failedUpsertUids and the publish toast counts only failed selected nodes.
… without sync The sync loop now also starts when only node sharing is enabled. In that mode it scopes node upserts to shared nodes and uploads their content without generating embeddings; users, shared full-content refresh, concept conversion, and orphan cleanup run as before. With the sync flag on, behavior is unchanged.
Shared-nodes-only cycles previously completed the same "embedding" sync task, advancing its watermark without producing embeddings. Enabling the sync flag later would then skip the initial embedding backfill for nodes whose content was already uploaded. Keeping the two modes on separate sync_info rows leaves the embedding watermark untouched until full sync actually runs, and stops the two modes from postponing each other's cycles in mixed-flag spaces.
A failed schema upsert leaves its concept absent, so dependent nodes insert with a null schema_id and shared-node discovery filters them out. Treat those nodes as failed too: no ResourceAccess grant, counted in the failure toast, and relations touching them are withheld.
ENG-2022 made node-sharing features sync-independent; the import and publish entry points gate on Enable node sharing, so the refresh button for imported nodes must too.
Add a command palette command that refreshes every imported node with stored source identity by reusing the single-node refresh path without force, so up-to-date imports are skipped, and report refreshed, skipped, and failed counts in a toast.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| renderDiscoverSharedNodesDialog({}); | ||
| }; | ||
|
|
||
| const refreshAllImportedNodesFromCommand = async () => { |
There was a problem hiding this comment.
🟡 New command handler omits the required explicit return type
The new batch-refresh command handler is declared without an explicit return type (const refreshAllImportedNodesFromCommand = async () => { at apps/roam/src/utils/registerCommandPaletteCommands.ts:353), which the repository style guide requires for all functions.
Impact: Code style diverges from the repository's mandated TypeScript conventions.
Rule source: AGENTS.md TypeScript Guidelines
AGENTS.md states under TypeScript Guidelines: "Use explicit return types for functions". The other new functions added in this PR (getImportedNodeUids in apps/roam/src/utils/importedSourceIdentity.ts:77 and refreshAllImportedNodes in apps/roam/src/utils/refreshAllImportedNodes.ts:10-11) do declare them, so this one is inconsistent as well.
| const refreshAllImportedNodesFromCommand = async () => { | |
| const refreshAllImportedNodesFromCommand = async (): Promise<void> => { |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fcf370564
ℹ️ 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".
| if (isNodeSharingEnabled()) { | ||
| void addCommand( | ||
| "DG: Refresh all imported nodes", | ||
| () => void refreshAllImportedNodesFromCommand(), |
There was a problem hiding this comment.
If this command is invoked twice quickly—or overlaps with the page-title refresh button—both asynchronous runs can refresh the same imported page concurrently because the callback is fire-and-forget and there is no shared in-flight guard. Both calls can observe the old source identity, and updateImportedPage can then have each call snapshot the same previous children, append a fresh copy of the source markdown, and delete only that shared snapshot, leaving duplicate imported content. Serialize refreshes per page or reject additional batch invocations until the active run completes.
Useful? React with 👍 / 👎.
Full sync selected nodes by title format and edit time only, so an imported page whose title matched a local node format was upserted to Supabase as this space's own content, embeddings, and concepts. Filter out pages with stored importedFrom identity before the mode split so both initial and incremental sync skip them; shared-nodes-only mode was already safe via the shared source-local-id filter. getImportedNodeUids now returns a Set for membership checks.
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:
|
1 participant
Adds a
DG: Refresh all imported nodescommand palette command (gated on node sharing, like the other sharing entry points) that refreshes every imported node with stored source identity and reports the outcome in a toast:N refreshed, N skipped, N failed.Each node goes through the ENG-1860 single-node refresh path, so one failure doesn't abort the batch. To let the batch tally the three outcomes,
refreshImportedNodenow returnsstatus: "refreshed" | "skipped" | "failed"instead of a booleansuccess, and takes aforceoption. The batch passesforce: falseso up-to-date imports are skipped instead of rewritten — that's where the skipped count comes from. The title button keepsforce: true(default): an explicit click on one node still overwrites local edits even when the source is unchanged, per ENG-1860's behavior.Nodes are fetched one at a time via
getSharedNodeByRidrather than one bulk list because the ticket asks to reuse the single-node refresh path, and imported-node counts are small. Failure reporting is intentionally counts-only (each failure is still logged viainternalError); actionable per-node reporting is ENG-1877.The second commit fixes an adjacent violation of "imported nodes must not be synced": full sync selects nodes by title format + edit time only, so an imported page whose title matches a local node format was uploaded to Supabase as this space's own content, embeddings, and concepts — and import/refresh touching those pages re-triggered it. The fix filters pages with stored
importedFromidentity out of the sync's node selection before the mode split, covering initial and incremental sync in both modes (shared-nodes-only mode was already safe via the shared source-local-id filter). Not covered here: the full-content path keys off explicit grants, which an imported page only enters via the publish title button that today also renders on imported pages — that publish-side exclusion is a separate follow-up.Stacked on #1279 (ENG-1860).