-
Notifications
You must be signed in to change notification settings - Fork 4
ENG-1634 Re-sync nodes if needed at the time the users publish the node #951
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
d8578b2
3fec3af
801c1f4
6f9b189
9ca0b9c
4027e1f
f9b90f3
660ccb3
c1440af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,7 @@ | ||||||||||||||||||||||||||||||||||
| /* eslint-disable @typescript-eslint/naming-convention */ | ||||||||||||||||||||||||||||||||||
| import { FrontMatterCache, Notice, TFile } from "obsidian"; | ||||||||||||||||||||||||||||||||||
| import { Notice, TFile } from "obsidian"; | ||||||||||||||||||||||||||||||||||
| import { addFile } from "@repo/database/lib/files"; | ||||||||||||||||||||||||||||||||||
| import mime from "mime-types"; | ||||||||||||||||||||||||||||||||||
| import { ensureNodeInstanceId } from "~/utils/nodeInstanceId"; | ||||||||||||||||||||||||||||||||||
| import type { DGSupabaseClient } from "@repo/database/lib/client"; | ||||||||||||||||||||||||||||||||||
| import type { Json } from "@repo/database/dbTypes"; | ||||||||||||||||||||||||||||||||||
|
|
@@ -9,7 +11,7 @@ import { | |||||||||||||||||||||||||||||||||
| type SupabaseContext, | ||||||||||||||||||||||||||||||||||
| } from "./supabaseContext"; | ||||||||||||||||||||||||||||||||||
| import { default as DiscourseGraphPlugin } from "~/index"; | ||||||||||||||||||||||||||||||||||
| import { publishNode, ensurePublishedRelationsAccuracy } from "./publishNode"; | ||||||||||||||||||||||||||||||||||
| import { ensurePublishedRelationsAccuracy } from "./publishNode"; | ||||||||||||||||||||||||||||||||||
| import { upsertNodesToSupabaseAsContentWithEmbeddings } from "./upsertNodesAsContentWithEmbeddings"; | ||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||
| orderConceptsByDependency, | ||||||||||||||||||||||||||||||||||
|
|
@@ -579,6 +581,92 @@ const convertDgToSupabaseConcepts = async ({ | |||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| export const syncPublishedNodeAssets = async ({ | ||||||||||||||||||||||||||||||||||
| plugin, | ||||||||||||||||||||||||||||||||||
| client, | ||||||||||||||||||||||||||||||||||
| nodeId, | ||||||||||||||||||||||||||||||||||
| spaceId, | ||||||||||||||||||||||||||||||||||
| file, | ||||||||||||||||||||||||||||||||||
| attachments, | ||||||||||||||||||||||||||||||||||
| }: { | ||||||||||||||||||||||||||||||||||
| plugin: DiscourseGraphPlugin; | ||||||||||||||||||||||||||||||||||
| client: DGSupabaseClient; | ||||||||||||||||||||||||||||||||||
| nodeId: string; | ||||||||||||||||||||||||||||||||||
| spaceId: number; | ||||||||||||||||||||||||||||||||||
| file: TFile; | ||||||||||||||||||||||||||||||||||
| attachments?: TFile[]; | ||||||||||||||||||||||||||||||||||
| }) => { | ||||||||||||||||||||||||||||||||||
|
maparent marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||
| if (attachments === undefined) { | ||||||||||||||||||||||||||||||||||
| const embeds = plugin.app.metadataCache.getFileCache(file)?.embeds ?? []; | ||||||||||||||||||||||||||||||||||
| attachments = embeds | ||||||||||||||||||||||||||||||||||
| .map(({ link }) => { | ||||||||||||||||||||||||||||||||||
| const attachment = plugin.app.metadataCache.getFirstLinkpathDest( | ||||||||||||||||||||||||||||||||||
| link, | ||||||||||||||||||||||||||||||||||
| file.path, | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| return attachment; | ||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||
| .filter((a) => !!a); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| // Always sync non-text assets when node is published to this group | ||||||||||||||||||||||||||||||||||
| const existingFiles: string[] = []; | ||||||||||||||||||||||||||||||||||
| const existingReferencesReq = await client | ||||||||||||||||||||||||||||||||||
| .from("my_file_references") | ||||||||||||||||||||||||||||||||||
| .select("*") | ||||||||||||||||||||||||||||||||||
| .eq("space_id", spaceId) | ||||||||||||||||||||||||||||||||||
| .eq("source_local_id", nodeId); | ||||||||||||||||||||||||||||||||||
| if (existingReferencesReq.error) { | ||||||||||||||||||||||||||||||||||
| console.error(existingReferencesReq.error); | ||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| const existingReferencesByPath = Object.fromEntries( | ||||||||||||||||||||||||||||||||||
| existingReferencesReq.data.map((ref) => [ref.filepath, ref]), | ||||||||||||||||||||||||||||||||||
| ) as Record<string, (typeof existingReferencesReq.data)[0]>; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| for (const attachment of attachments) { | ||||||||||||||||||||||||||||||||||
| const mimetype = mime.lookup(attachment.path) || "application/octet-stream"; | ||||||||||||||||||||||||||||||||||
| if (mimetype.startsWith("text/")) continue; | ||||||||||||||||||||||||||||||||||
| // Do not use standard upload for large files | ||||||||||||||||||||||||||||||||||
| if (attachment.stat.size >= 6 * 1024 * 1024) { | ||||||||||||||||||||||||||||||||||
| new Notice( | ||||||||||||||||||||||||||||||||||
| `Asset file ${attachment.path} is larger than 6Mb and will not be uploaded`, | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| existingFiles.push(attachment.path); | ||||||||||||||||||||||||||||||||||
| const existingRef = existingReferencesByPath[attachment.path]; | ||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||
| !existingRef || | ||||||||||||||||||||||||||||||||||
| new Date(existingRef.last_modified + "Z").valueOf() < | ||||||||||||||||||||||||||||||||||
| attachment.stat.mtime | ||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||
| const content = await plugin.app.vault.readBinary(attachment); | ||||||||||||||||||||||||||||||||||
| await addFile({ | ||||||||||||||||||||||||||||||||||
| client, | ||||||||||||||||||||||||||||||||||
| spaceId, | ||||||||||||||||||||||||||||||||||
| sourceLocalId: nodeId, | ||||||||||||||||||||||||||||||||||
| fname: attachment.path, | ||||||||||||||||||||||||||||||||||
| mimetype, | ||||||||||||||||||||||||||||||||||
| created: new Date(attachment.stat.ctime), | ||||||||||||||||||||||||||||||||||
| lastModified: new Date(attachment.stat.mtime), | ||||||||||||||||||||||||||||||||||
| content, | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| let cleanupCommand = client | ||||||||||||||||||||||||||||||||||
| .from("FileReference") | ||||||||||||||||||||||||||||||||||
| .delete() | ||||||||||||||||||||||||||||||||||
| .eq("space_id", spaceId) | ||||||||||||||||||||||||||||||||||
| .eq("source_local_id", nodeId); | ||||||||||||||||||||||||||||||||||
| if (existingFiles.length) | ||||||||||||||||||||||||||||||||||
| cleanupCommand = cleanupCommand.notIn("filepath", [ | ||||||||||||||||||||||||||||||||||
| ...new Set(existingFiles), | ||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||
| const cleanupResult = await cleanupCommand; | ||||||||||||||||||||||||||||||||||
| // do not fail on cleanup | ||||||||||||||||||||||||||||||||||
| if (cleanupResult.error) console.error(cleanupResult.error); | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * For nodes that are already published, ensure non-text assets are pushed to | ||||||||||||||||||||||||||||||||||
| * storage. Called after content sync so new embeds (e.g. images) get uploaded. | ||||||||||||||||||||||||||||||||||
|
|
@@ -587,17 +675,26 @@ const syncPublishedNodesAssets = async ( | |||||||||||||||||||||||||||||||||
| plugin: DiscourseGraphPlugin, | ||||||||||||||||||||||||||||||||||
| nodes: ObsidianDiscourseNodeData[], | ||||||||||||||||||||||||||||||||||
| ): Promise<void> => { | ||||||||||||||||||||||||||||||||||
| const context = await getSupabaseContext(plugin); | ||||||||||||||||||||||||||||||||||
| if (!context) throw new Error("Cannot get context"); | ||||||||||||||||||||||||||||||||||
| const spaceId = context.spaceId; | ||||||||||||||||||||||||||||||||||
| const client = await getLoggedInClient(plugin); | ||||||||||||||||||||||||||||||||||
| if (!client) throw new Error("Cannot get client"); | ||||||||||||||||||||||||||||||||||
| const published = nodes.filter( | ||||||||||||||||||||||||||||||||||
| (n) => | ||||||||||||||||||||||||||||||||||
| ((n.frontmatter.publishedToGroups as string[] | undefined)?.length ?? 0) > | ||||||||||||||||||||||||||||||||||
| 0, | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| for (const node of published) { | ||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||
| await publishNode({ | ||||||||||||||||||||||||||||||||||
| const nodeId = node.frontmatter.nodeInstanceId as string | undefined; | ||||||||||||||||||||||||||||||||||
| if (!nodeId) throw new Error("Please sync the node first"); | ||||||||||||||||||||||||||||||||||
| await syncPublishedNodeAssets({ | ||||||||||||||||||||||||||||||||||
| plugin, | ||||||||||||||||||||||||||||||||||
| client, | ||||||||||||||||||||||||||||||||||
| nodeId, | ||||||||||||||||||||||||||||||||||
| spaceId, | ||||||||||||||||||||||||||||||||||
| file: node.file, | ||||||||||||||||||||||||||||||||||
| frontmatter: node.frontmatter as FrontMatterCache, | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||
| console.error( | ||||||||||||||||||||||||||||||||||
|
|
@@ -651,7 +748,14 @@ const syncChangedNodesToSupabase = async ({ | |||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // When file changes affect an already-published node, ensure new non-text | ||||||||||||||||||||||||||||||||||
| // assets (e.g. images) are pushed to storage. | ||||||||||||||||||||||||||||||||||
| await syncPublishedNodesAssets(plugin, changedNodes); | ||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||
| await syncPublishedNodesAssets(plugin, changedNodes); | ||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||
| // console.error( | ||||||||||||||||||||||||||||||||||
| // `Failed to sync published node assets`, | ||||||||||||||||||||||||||||||||||
| // error, | ||||||||||||||||||||||||||||||||||
| // ); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+751
to
+756
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error is silently swallowed with no logging or user notification. The commented-out error message also references
Fix: Either properly log the error or remove the try-catch to let errors propagate: try {
await syncPublishedNodesAssets(plugin, changedNodes);
} catch (error) {
console.error('Failed to sync published node assets:', error);
// Optionally show a Notice to the user
}
Suggested change
Spotted by Graphite
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are indeed swallowing errors silently in the plugin. @trangdoan982 Do you think this one is worth a Notice?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah maybe we show them a Notice and ask them to retry?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a notice. Not sure we should ask them to retry, unless we have a way to detect multiple failures?
maparent marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.