[ENG-1369] Sync non text assets automatically

[trangdoan982]

---

[linear]

ENG-1369 Make sure sync also handle publishing non-text assets when necessary

[supabase]

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 Review found 2 potential issues.

View 9 additional findings in Devin Review.

[devin-ai-integration]

Devin Review found 2 new potential issues.

View 13 additional findings in Devin Review.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 17 additional findings in Devin Review.

[devin-ai-integration]

🔴 syncPublishedNodesAssets unintentionally publishes nodes to the current user's group

syncPublishedNodesAssets filters nodes by checking if publishedToGroups has any entries (line 574-577), then calls publishNode for each. However, publishNode always publishes to the current user's group (myGroup at publishNode.ts:79), not to the groups the node was originally published to.

Detailed Explanation

If a node was published to group A but the current user's group is group B (e.g., the user changed groups, or publishedToGroups was set by a different mechanism), then:

1. syncPublishedNodesAssets sees publishedToGroups = ["groupA"] → length > 0 → passes filter
2. publishNode fetches myGroup = "groupB" from group_membership
3. existingPublish.includes(myGroup) → ["groupA"].includes("groupB") → false
4. skipPublishAccess becomes false
5. The full publish flow runs: SpaceAccess upsert, ResourceAccess upsert, schema publish — all for group B
6. At line 189-195, frontmatter is updated: publishedToGroups = ["groupA", "groupB"]

This means a routine background sync silently publishes the node to a group the user never explicitly chose to publish to. The intent of syncPublishedNodesAssets is only to ensure non-text assets are in storage, not to expand the set of groups a node is published to.

Impact: Nodes could be unintentionally shared with groups the user didn't explicitly publish to, which is a data exposure concern.

Prompt for agents

The syncPublishedNodesAssets function should only sync non-text assets for nodes that are already published to the current user's group, rather than calling the full publishNode function which may publish to new groups as a side effect. There are two approaches to fix this:

1. (Preferred) Extract the file sync and cleanup logic from publishNode into a separate function (e.g., syncNodeAssets) that only handles uploading non-text attachments and cleaning up stale FileReferences, without any publish access or frontmatter modification logic. Then call this new function from syncPublishedNodesAssets instead of publishNode.

2. (Simpler) In syncPublishedNodesAssets, after fetching the current user's group, filter the published nodes to only include those whose publishedToGroups already contains the current user's group. This prevents publishing to new groups but still reuses publishNode.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[maparent]

That is a real issue. Not urgent for holehouse, but I'm for fixing it now, it's not that much trouble.

[maparent]

I would that we looked at file modified dates; many would not be worth republishing. But that's an optimization, so it could be left for another task (in which case let's define it.)
(OTH I know doing this right will never be high priority which is why I hate checking in code with an obvious optimization.)
I'll correct the point made by Devin, that's real, and you're busy elsewhere.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 23 additional findings in Devin Review.

[devin-ai-integration]

🟡 Early return on FileReference query error silently succeeds, leaving DB and frontmatter in inconsistent state

When the FileReference query at line 164-168 fails, the function returns at line 171 without throwing. This silently skips both the cleanup (lines 200-211) and, critically, the frontmatter update at lines 213-219.

Root cause and impact

On first-time publish (commonGroups.length === 0), the access-publishing block (lines 121-159) runs successfully—granting SpaceAccess, ResourceAccess, and publishing the schema. Then the FileReference query fails, and the function returns void (no error).

The caller in apps/obsidian/src/utils/registerCommands.ts:246-253 treats a resolved promise as success:

publishNode({ plugin, file, frontmatter })
  .then(() => { new Notice("Published"); })
  .catch((error: Error) => { new Notice(error.message); });

So the user sees "Published", but:

1. publishedToGroups in the local frontmatter is never updated to include the new group.
2. Non-text assets are not uploaded.
3. Because frontmatter lacks publishedToGroups, the automatic asset sync in syncPublishedNodesAssets (apps/obsidian/src/utils/syncDgNodesToSupabase.ts:573-577) will filter this node out, so assets will never be synced automatically in subsequent syncs either.

The node is accessible in the DB (access granted) but its assets are permanently missing until the user manually retries the publish command.

Impact: On a transient DB error during the optimization query, the user gets a false success notification and the node's assets are never uploaded.

Prompt for agents

In apps/obsidian/src/utils/publishNode.ts lines 169-171, instead of returning early when the FileReference query errors, either throw the error (so the caller can report it to the user) or fall through to skip the optimization and proceed with the unconditional asset upload and frontmatter update. One approach: when the query fails, set existingReferencesByPath to an empty object (so all assets are re-uploaded) and continue execution rather than returning. This way the cleanup and frontmatter update on lines 200-219 still execute.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[maparent]

I'm happy with what you did, but made changes on my own, please review my changes and approve before this gets added!