[ENG-1861] Add Roam refresh-all for imported nodes - #1286
Conversation
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).