Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@trangdoan982 After I broke the recursion, I was not satisfied because some code was run twice. I have refactored to avoid this, ready for review. |
| try { | ||
| await syncPublishedNodesAssets(plugin, changedNodes); | ||
| } catch (error) { | ||
| // console.error( | ||
| // `Failed to sync published node assets for ${node.file.path}:`, | ||
| // error, | ||
| // ); | ||
| } |
There was a problem hiding this comment.
Error is silently swallowed with no logging or user notification. The commented-out error message also references node which doesn't exist in this scope (should be referencing individual nodes from the loop in syncPublishedNodesAssets). This means:
- If asset syncing fails for published nodes, users won't be notified
- Debugging will be difficult as failures are hidden
- If the commented code is ever uncommented, it will throw a ReferenceError
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
}| try { | |
| await syncPublishedNodesAssets(plugin, changedNodes); | |
| } catch (error) { | |
| // console.error( | |
| // `Failed to sync published node assets for ${node.file.path}:`, | |
| // error, | |
| // ); | |
| } | |
| try { | |
| await syncPublishedNodesAssets(plugin, changedNodes); | |
| } catch (error) { | |
| console.error( | |
| 'Failed to sync published node assets:', | |
| error, | |
| ); | |
| } |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
We are indeed swallowing errors silently in the plugin. @trangdoan982 Do you think this one is worth a Notice?
There was a problem hiding this comment.
yeah maybe we show them a Notice and ask them to retry?
There was a problem hiding this comment.
Added a notice. Not sure we should ask them to retry, unless we have a way to detect multiple failures?
3dce23c to
801c1f4
Compare
| try { | ||
| await syncPublishedNodesAssets(plugin, changedNodes); | ||
| } catch (error) { | ||
| // console.error( | ||
| // `Failed to sync published node assets for ${node.file.path}:`, | ||
| // error, | ||
| // ); | ||
| } |
There was a problem hiding this comment.
yeah maybe we show them a Notice and ask them to retry?
https://linear.app/discourse-graphs/issue/ENG-1634/re-sync-nodes-if-needed-at-the-time-the-users-publish-the-node