ENG-298 Repeat calls to upsertNodesToSupabaseAsContent at regular itervals. #504
Conversation
Allows different steps to succeed independently; Backoff time in case of errors. Cancel on unload.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a public sync control API (setSyncActivity) to the Roam extension, exposes it on Changes
Sequence Diagram(s)sequenceDiagram
participant Ext as Roam Extension
participant API as queryBuilder (setSyncActivity)
participant Sync as Sync Loop
participant DB as Supabase
Ext->>API: setSyncActivity(true)
activate API
API->>API: doSync = true, schedule activeTimeout
API->>Sync: start/resume loop
deactivate API
loop while doSync
Sync->>Sync: createOrUpdateDiscourseEmbedding(showToast?)
alt Success
Sync->>DB: upsert nodes/embeddings/users
DB-->>Sync: OK
Sync->>Sync: reset error counter
else Transient error
Sync->>Sync: increment error counter, backoff, maybe showToast
Sync->>Sync: retry later
else Fatal error
Sync->>API: setSyncActivity(false)
Note right of Sync: throw FatalError, stop loop
end
end
Ext->>API: (on unload) setSyncActivity(false)
activate API
API->>API: doSync = false, clear timeouts
API->>Sync: stop loop
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/roam/src/utils/cleanupOrphanedNodes.ts (1)
362-398: Return value doesn’t reflect inner failures; consider propagating step outcomesHelpers log errors and return void/arrays, so this function returns true even if deletions fail. If the caller uses this boolean to drive backoff, it will under-report failures.
Recommend: have deleteNodesFromSupabase and get* helpers return booleans (or counts) and aggregate them here (return false on any failure). Optionally align with the pattern used elsewhere (boolean | number for partial success). I can draft a follow-up patch if you want.
apps/roam/src/index.ts (1)
145-146: Expose setSyncActivity on the public APIGood addition; enables pausing/resuming sync externally. Consider documenting this in your extension’s public API notes to avoid accidental misuse.
apps/roam/src/utils/syncDgNodesToSupabase.ts (2)
518-523: Release the sync task when the client/context is unavailableIf supabaseClient is null, you return early without calling endSyncTask, potentially holding the task until timeout. End the task as failed before returning.
- const supabaseClient = await getLoggedInClient(); - if (!supabaseClient) return; + const supabaseClient = await getLoggedInClient(); + if (!supabaseClient) { + await endSyncTask(worker, "failed", showToast); + return; + } const context = await getSupabaseContext(); if (!context) { await endSyncTask(worker, "failed"); // not worth retrying throw new FatalError("No Supabase context found."); }
501-507: Propagate showToast to scheduled retries/postponements (optional)Scheduled calls drop the showToast arg, changing UX after the first run. If you want consistent behavior, pass it through:
- activeTimeout = setTimeout( - createOrUpdateDiscourseEmbedding, - nextUpdateTime.valueOf() - Date.now() + 100, - ); + activeTimeout = setTimeout( + () => void createOrUpdateDiscourseEmbedding(showToast), + nextUpdateTime.valueOf() - Date.now() + 100, + );- activeTimeout = setTimeout(createOrUpdateDiscourseEmbedding, timeout); + activeTimeout = setTimeout(() => void createOrUpdateDiscourseEmbedding(showToast), timeout);- activeTimeout = setTimeout(createOrUpdateDiscourseEmbedding, 100, true); + activeTimeout = setTimeout(() => void createOrUpdateDiscourseEmbedding(true), 100);Also applies to: 565-566, 582-583
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/index.ts(3 hunks)apps/roam/src/utils/cleanupOrphanedNodes.ts(2 hunks)apps/roam/src/utils/syncDgNodesToSupabase.ts(10 hunks)
🔇 Additional comments (2)
apps/roam/src/index.ts (2)
30-33: Import and public surface LGTYImporting initializeSupabaseSync and setSyncActivity here is appropriate given the new sync loop. No issues.
159-159: Stop the sync loop on unloadCalling setSyncActivity(false) on unload is correct and prevents stray timers. Nice.
|
@CodeRabbit review latest changes |
|
@maparent: I'll review the latest changes in this PR. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/roam/src/utils/cleanupOrphanedNodes.ts (2)
4-85: Inconsistent error handling masks failures.Lines 21 and 83 correctly return
nullon error, but lines 47 and 70 return empty arrays on error. The caller at line 374 checks fornullto detect failures, so errors at lines 47 and 70 will be silently treated as "no results" rather than triggering the failure path.Apply this diff to return
nullconsistently:if (conceptResponse.error) { console.error( "Failed to get concepts from Supabase:", conceptResponse.error, ); - return []; + return null; }if (blockContentResponse.error) { console.error( "Failed to get block content from Supabase:", blockContentResponse.error, ); - return []; + return null; }
364-407: The review comment is valid but could be more precise about the root cause.The identified inconsistency is genuine:
deleteNodesFromSupabasereturns a boolean, allowingcleanupOrphanedNodesto track failures viasuccess &&=(line 380), whiledeleteNodeSchemasFromSupabasereturns a numeric count. Critically, this function cannot distinguish between "0 items deleted (success)" and "error occurred" (both return 0), and more importantly, the caller ignores the return value entirely at line 396-400. This means schema cleanup failures silently bypass success tracking, while node cleanup failures correctly propagate via thesuccessvariable.The function returns
trueeven if orphaned schema deletion fails, which is inconsistent with how node deletion failures are handled.Suggested fix: Either track the schema deletion result (
success &&= (deleted > 0 && orphanedSchemaUids.length > 0)or similar), or document that schema cleanup failures are intentionally non-blocking.
🧹 Nitpick comments (1)
apps/roam/src/utils/syncDgNodesToSupabase.ts (1)
495-507: Consider improving error message clarity.Line 497 throws a generic error when
nextUpdateTimeis undefined. This can occur whenproposeSyncTaskencounters transient issues (e.g., network errors). While the catch block will handle it, a more descriptive message would aid debugging.if (nextUpdateTime === undefined) { - throw new Error("Can't obtain sync task"); + throw new Error("proposeSyncTask failed to return a valid timestamp"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/index.ts(3 hunks)apps/roam/src/utils/cleanupOrphanedNodes.ts(11 hunks)apps/roam/src/utils/syncDgNodesToSupabase.ts(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-18T18:58:16.089Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.089Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.
Applied to files:
apps/roam/src/utils/syncDgNodesToSupabase.ts
🔇 Additional comments (19)
apps/roam/src/index.ts (4)
30-33: LGTM – imports are correct.The new imports for
initializeSupabaseSyncandsetSyncActivityare properly sourced and used later in the file.
143-144: Good practice – prefer ts-expect-error over ts-ignore.This change ensures the suppression fails if the type error is ever resolved, preventing stale directives.
145-145: LGTM – public API exposure is appropriate.Exposing
setSyncActivityon the extension's queryBuilder API allows external control of the sync process, which aligns with the PR's goal of providing sync control.
159-161: Proper cleanup on unload.Calling
setSyncActivity(false)ensures the sync loop is stopped and any pending timeout is cleared when the extension unloads. This prevents resource leaks and orphaned timers.apps/roam/src/utils/cleanupOrphanedNodes.ts (2)
87-126: LGTM – consistent null returns on error.The function properly returns
nullon all error paths, making failures easy to detect.
148-197: LGTM – proper boolean success/failure reporting.All error paths return
false, and the success path returnstrue. The boolean return allows the caller to track success state.apps/roam/src/utils/syncDgNodesToSupabase.ts (13)
23-24: LGTM – import updates are correct.The imports properly source types and client utilities from the database package.
33-33: LGTM – DEFAULT_TIME now a Date object.This change aligns with the updated timestamp types (
lastUpdateTimeandnextUpdateTimeare nowDateobjects) and simplifies usage at line 509.
35-41: LGTM – optional Date types are appropriate.Making
lastUpdateTimeandnextUpdateTimeoptional reflects their conditional presence in different response scenarios.
43-101: Well-implemented boolean return and optional toasts.The function now properly returns
booleanon all paths (including the early exits at lines 50 and 54), and theshowToastparameter provides flexible feedback control. The implementation addresses previous concerns about the return type contract.
103-175: LGTM – proper timestamp handling with Date objects.The function correctly distinguishes between "wait until" (future
nextUpdateTime) and "proceed with" (pastlastUpdateTime) scenarios.
318-382: LGTM – proper partial success semantics.The return values correctly distinguish complete success (
true), partial success (count asnumber), and complete failure (false), which aligns with the design goal of avoiding backoff on partial progress. Based on learnings.
384-413: LGTM – consistent partial success handling.The function follows the same pattern as
addMissingEmbeddings, properly distinguishing complete, partial, and failed outcomes. Based on learnings.
445-458: LGTM – boolean success indicator.The function properly returns
truewhen the error is null andfalseotherwise.
460-468: LGTM – proper sync control infrastructure.The constants, state variables, and
FatalErrorclass provide a solid foundation for the scheduled sync with backoff mechanism.
470-482: LGTM – proper sync toggle implementation.The function correctly manages the
doSyncflag andactiveTimeout. The guard at line 475 prevents spawning duplicate timers, and the 100ms initial delay provides a quick start when activating.
508-546: LGTM – proper error handling and partial success semantics.Line 509 correctly uses
toISOString()for precise timestamp formatting. The distinction betweenFatalError(unrecoverable) and regularError(retryable) is well-designed. Lines 535 and 537 properly treat numeric results (partial success) as non-failures, aligning with the design goal of continuing progress even when some items fail. Based on learnings.
547-575: LGTM – robust error handling with exponential backoff.The catch block distinguishes fatal from retryable errors. The exponential backoff (multiplier: 2^numFailures) provides reasonable intervals (10m, 20m, 40m, 80m before stopping), and the checks at lines 567-571 prevent orphaned timers.
577-592: LGTM – conditional initialization based on space existence.The initialization properly checks for space registration before enabling sync. The initial 100ms delay and
showToast: trueprovide immediate user feedback on first run.
| ); | ||
| } | ||
| if (supabaseSchemaUids === null) return false; | ||
| if (supabaseSchemaUids.length === 0) return true; |
There was a problem hiding this comment.
Early return masks previous failures.
Line 393 returns true unconditionally when there are no schemas to clean up, but this occurs after node cleanup operations that may have failed. If success is already false from lines 374-386, this early return will incorrectly report success.
Apply this diff to respect prior failures:
- if (supabaseSchemaUids.length === 0) return true;
+ if (supabaseSchemaUids.length === 0) return success;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (supabaseSchemaUids.length === 0) return true; | |
| if (supabaseSchemaUids.length === 0) return success; |
🤖 Prompt for AI Agents
In apps/roam/src/utils/cleanupOrphanedNodes.ts around line 393, the function
currently does "if (supabaseSchemaUids.length === 0) return true;" which masks
any prior failures recorded in the `success` flag; change this early return to
return the existing `success` variable instead (or compute and return
`success`), so that if earlier cleanup steps set `success = false` those
failures are propagated; ensure `success` is in scope and returned when there
are no schemas to process.
maparent
changed the title
|
Ok, decided to count partial success as failure for now; I wrote about the possibility of recording a partial success eventually, but I think that is an optimization, less urgent, and scope creep. |
|
@CodeRabbit review last commit. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7423 @maparent: I've reviewed the last commit. I found a critical logic error in how partial success is being evaluated, along with some good refactoring decisions. 🚨 Critical IssueFile: The logic for counting partial success as failure is inverted: success &&= partial !== true;This currently does:
Should be: success &&= partial === true;This would correctly:
✅ Good Changes
RecommendationFix the logic error on line ~522 before merging. |
|
@CodeRabbit good catch on the logic error. |
|
Tip For best results, initiate chat on the files or code changes. @maparent: You're welcome! Glad I could help spot that before it became a harder-to-debug issue. The inverted logic would have caused the opposite behavior of what you intended. Once you fix line ~522 to 🧠 Learnings used |
|
@sid597 could you review this one as well |
|
leaving this as a draft, as it contains the dependency sequence refactor, which I think is still useful. |
Allows different steps to succeed independently;
Backoff time in case of errors.
Cancel on unload.
Opportunistic base-case fixing of ENG-975.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes