remove sync on startup#84
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis pull request refactors wallet data fetching and application startup logic. The Wallet component no longer pre-fetches all data before caching; instead, it updates the UI directly and conditionally saves the cache. The app startup flow transitions from a blocking synchronous sync operation to background parallel execution, allowing the main app to initialize immediately while offerbook sync occurs asynchronously. Additionally, unused Tailwind CSS utilities are removed from the style output. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as app.js
participant Wallet as Wallet Component
participant Taker as window.api.taker
participant UI as Main UI
rect rgba(100, 150, 200, 0.5)
Note over App,UI: Old Flow (Blocking Sync)
User->>App: Initialize App
App->>Taker: performLaunchSync()
Taker->>Taker: Sync offerbook
Taker-->>App: Sync complete
App->>UI: startMainApp()
UI->>Wallet: Mount & Initialize
Wallet->>Taker: fetchBalance/Transactions/Utxos
Taker-->>Wallet: Data
Wallet->>Wallet: Cache data
Wallet->>UI: Update UI
UI-->>User: App ready
end
rect rgba(150, 200, 100, 0.5)
Note over App,UI: New Flow (Parallel Background Sync)
User->>App: Initialize App
App->>UI: startMainApp()
par Parallel
App->>Taker: startBackgroundOfferbookSync()
Taker->>Taker: Async sync offerbook
Taker-->>Taker: Poll getSyncStatus()
and
UI->>Wallet: Mount & Initialize
Wallet->>Taker: Conditional sync()
Wallet->>Taker: updateBalance/Transactions/Utxos
Taker-->>Wallet: Data
Wallet->>Wallet: Conditional cache
Wallet->>UI: Update UI
UI-->>User: App ready immediately
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/wallet/Wallet.js`:
- Line 364: The code currently calls saveWalletToCache(balance, transactions,
utxos) whenever balance exists, which can mark incomplete data (missing or
fallback transactions/utxos) as fresh cache; change the guard so you only call
saveWalletToCache when all required pieces are present and valid (e.g., balance
is defined AND transactions is an array (or has expected length) AND utxos is
defined/array), and propagate the same stricter check to the other occurrence
that uses saveWalletToCache (the similar call at the second occurrence). Use the
function/variable names balance, transactions, utxos, and saveWalletToCache to
locate and update both places.
- Around line 515-520: The startup sync block should handle non-thrown failure
responses and avoid overlapping with the background offerbook sync: call
window.api.taker.sync(), inspect its return value and treat { success: false,
error } as a failure (log via console.error('⚠️ Initial wallet sync failed:',
error) including the error object), and do not proceed to fetch/cache data when
sync failed; also guard this startup sync with the same global sync flag used by
the background offerbook sync (check/set the shared sync state used by the main
sync handler and background offerbook sync before calling
window.api.taker.sync() to prevent concurrent syncs), and ensure the flag is
cleared in all failure and success paths so subsequent background syncs can run.
In `@src/js/app.js`:
- Around line 330-346: The poll currently resolves for both failure and success
but always prints "Background offerbook sync complete"; update the logic around
window.api.taker.getSyncStatus(syncId) and the Promise resolution so you only
call console.log('✅ Background offerbook sync complete') when status.success ===
true and syncStatus === 'completed'; otherwise call console.error with context
(e.g., console.error('⚠️ Offerbook sync failed:', status) or the caught error)
and ensure the catch block uses console.error('⚠️ Sync poll error:', err)
instead of console.warn; reference the async poll using setInterval, the
variables status/syncStatus, and the final console.log to make the conditional
logging change.
- Around line 187-188: The call to startMainApp() is currently unawaited and can
produce unhandled rejections from async failures (e.g.,
SwapStateManager.getActiveSwap()); update the places where startMainApp() is
invoked (the blocks that also call startBackgroundOfferbookSync()) to await
startMainApp() (these call sites are already inside async contexts so use
await), while leaving startBackgroundOfferbookSync() invoked without awaiting so
the background sync remains non-blocking; ensure any errors from startMainApp()
propagate or are caught where appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4fe1bd57-ebee-430b-b799-3176acfa5b50
📒 Files selected for processing (3)
src/components/wallet/Wallet.jssrc/js/app.jssrc/styles/output.css
💤 Files with no reviewable changes (1)
- src/styles/output.css
| updateUtxos(false), | ||
| ]); | ||
|
|
||
| if (balance) saveWalletToCache(balance, transactions, utxos); |
There was a problem hiding this comment.
Avoid marking partial wallet data as a fresh cache entry.
Saving when only balance exists can cache missing or fallback transactions/utxos as fresh for 15 minutes, hiding a failed transaction/UTXO refresh behind the cache.
🐛 Proposed fix
- if (balance) saveWalletToCache(balance, transactions, utxos);
+ if (balance && Array.isArray(transactions) && Array.isArray(utxos)) {
+ saveWalletToCache(balance, transactions, utxos);
+ }- if (balance) saveWalletToCache(balance, transactions, utxos);
+ if (balance && Array.isArray(transactions) && Array.isArray(utxos)) {
+ saveWalletToCache(balance, transactions, utxos);
+ }Also applies to: 526-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/wallet/Wallet.js` at line 364, The code currently calls
saveWalletToCache(balance, transactions, utxos) whenever balance exists, which
can mark incomplete data (missing or fallback transactions/utxos) as fresh
cache; change the guard so you only call saveWalletToCache when all required
pieces are present and valid (e.g., balance is defined AND transactions is an
array (or has expected length) AND utxos is defined/array), and propagate the
same stricter check to the other occurrence that uses saveWalletToCache (the
similar call at the second occurrence). Use the function/variable names balance,
transactions, utxos, and saveWalletToCache to locate and update both places.
Comment on lines
+515
to
+520
| console.log('🔄 Syncing and fetching fresh data...'); | ||
| try { | ||
| await window.api.taker.sync(); | ||
| } catch (syncErr) { | ||
| console.warn('⚠️ Initial wallet sync failed, proceeding anyway:', syncErr.message); | ||
| } |
There was a problem hiding this comment.
Validate sync failure responses and avoid overlapping startup syncs.
window.api.taker.sync() can return { success: false, error } without throwing, so this path proceeds to fetch/cache data after a failed sync. It can also overlap with the new background offerbook sync because the main sync handler does not check global sync state.
🐛 Proposed fix
console.log('🔄 Syncing and fetching fresh data...');
try {
- await window.api.taker.sync();
+ const syncState = await window.api.taker.getCurrentSyncState?.();
+ if (!syncState?.isRunning) {
+ await syncWalletState();
+ } else {
+ console.log('⏳ Sync already running, skipping initial wallet sync');
+ }
} catch (syncErr) {
- console.warn('⚠️ Initial wallet sync failed, proceeding anyway:', syncErr.message);
+ console.error('⚠️ Initial wallet sync failed, proceeding anyway:', syncErr);
}As per coding guidelines, errors should be logged with context using console.error('⚠️ Context:', error).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/wallet/Wallet.js` around lines 515 - 520, The startup sync
block should handle non-thrown failure responses and avoid overlapping with the
background offerbook sync: call window.api.taker.sync(), inspect its return
value and treat { success: false, error } as a failure (log via
console.error('⚠️ Initial wallet sync failed:', error) including the error
object), and do not proceed to fetch/cache data when sync failed; also guard
this startup sync with the same global sync flag used by the background
offerbook sync (check/set the shared sync state used by the main sync handler
and background offerbook sync before calling window.api.taker.sync() to prevent
concurrent syncs), and ensure the flag is cleared in all failure and success
paths so subsequent background syncs can run.
Comment on lines
+187
to
+188
| startMainApp(); | ||
| startBackgroundOfferbookSync(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure
git ls-files | grep -E '\.(js|ts|tsx)$' | head -20Repository: citadel-tech/taker-app
Length of output: 603
🏁 Script executed:
# Check the file size before reading
wc -l src/js/app.jsRepository: citadel-tech/taker-app
Length of output: 83
🏁 Script executed:
# Read the specific lines mentioned in the review (187-188 and 287-288) with context
sed -n '180,195p' src/js/app.js
echo "---"
sed -n '280,295p' src/js/app.jsRepository: citadel-tech/taker-app
Length of output: 1129
🏁 Script executed:
# Find the startMainApp function definition
rg -n "^(async\s+)?function\s+startMainApp|const\s+startMainApp\s*=" src/js/app.jsRepository: citadel-tech/taker-app
Length of output: 101
🏁 Script executed:
# Search for startMainApp calls to understand the context better
rg -n "startMainApp\(" src/js/app.jsRepository: citadel-tech/taker-app
Length of output: 207
🏁 Script executed:
# Check the startBackgroundOfferbookSync function
rg -n "startBackgroundOfferbookSync" src/js/app.js | head -10Repository: citadel-tech/taker-app
Length of output: 207
🏁 Script executed:
# Read the startMainApp function definition and implementation
sed -n '353,380p' src/js/app.jsRepository: citadel-tech/taker-app
Length of output: 1205
🏁 Script executed:
# Read the startBackgroundOfferbookSync function
sed -n '322,350p' src/js/app.jsRepository: citadel-tech/taker-app
Length of output: 1083
🏁 Script executed:
# Check all context around all startMainApp calls to see if any are awaited
sed -n '150,160p' src/js/app.js
echo "---"
sed -n '204,214p' src/js/app.jsRepository: citadel-tech/taker-app
Length of output: 589
🏁 Script executed:
# Check if there's error handling for unhandled rejections
rg -n "unhandledrejection|catch|\.then" src/js/app.js | head -20Repository: citadel-tech/taker-app
Length of output: 342
Await startMainApp() before starting background sync.
startMainApp() is async and will reject if SwapStateManager.getActiveSwap() fails or throws, but these calls don't await it, leaving unhandled rejections. Lines 187-188 and 287-288 are in async contexts where await startMainApp() can be used. Keep startBackgroundOfferbookSync() non-blocking by not awaiting it.
Proposed fix
- startMainApp();
- startBackgroundOfferbookSync();
+ await startMainApp();
+ startBackgroundOfferbookSync();- startMainApp();
- startBackgroundOfferbookSync();
+ await startMainApp();
+ startBackgroundOfferbookSync();Per the **/*.{js,ts,tsx} guideline, use async/await for asynchronous operations.
📝 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.
Suggested change
| startMainApp(); | |
| startBackgroundOfferbookSync(); | |
| await startMainApp(); | |
| startBackgroundOfferbookSync(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/app.js` around lines 187 - 188, The call to startMainApp() is
currently unawaited and can produce unhandled rejections from async failures
(e.g., SwapStateManager.getActiveSwap()); update the places where startMainApp()
is invoked (the blocks that also call startBackgroundOfferbookSync()) to await
startMainApp() (these call sites are already inside async contexts so use
await), while leaving startBackgroundOfferbookSync() invoked without awaiting so
the background sync remains non-blocking; ensure any errors from startMainApp()
propagate or are caught where appropriate.
Comment on lines
+330
to
+346
| await new Promise((resolve) => { | ||
| const poll = setInterval(async () => { | ||
| try { | ||
| const status = await window.api.taker.getSyncStatus(syncId); | ||
| const syncStatus = (status.sync || {}).status || 'syncing'; | ||
| if (!status.success || syncStatus === 'completed' || syncStatus === 'failed') { | ||
| clearInterval(poll); | ||
| resolve(); | ||
| } | ||
| }, 1000); | ||
| }); | ||
| } else { | ||
| if (statusLabel) { | ||
| statusLabel.textContent = | ||
| syncResult.error || 'Unable to start offerbook sync'; | ||
| } | ||
| if (progressBar) progressBar.style.width = '100%'; | ||
| } | ||
| } catch (err) { | ||
| console.warn('⚠️ Sync poll error:', err.message); | ||
| clearInterval(poll); | ||
| resolve(); | ||
| } | ||
| }, 1000); | ||
| }); | ||
| console.log('✅ Background offerbook sync complete'); |
There was a problem hiding this comment.
Do not log failed offerbook syncs as complete.
The polling loop resolves on status.success === false and on syncStatus === 'failed', but line 346 still logs Background offerbook sync complete. That masks failed or missing syncs as success.
🐛 Proposed fix
- await new Promise((resolve) => {
+ const finalSyncStatus = await new Promise((resolve) => {
const poll = setInterval(async () => {
try {
const status = await window.api.taker.getSyncStatus(syncId);
const syncStatus = (status.sync || {}).status || 'syncing';
- if (!status.success || syncStatus === 'completed' || syncStatus === 'failed') {
+ if (!status.success) {
clearInterval(poll);
- resolve();
+ resolve('missing');
+ return;
+ }
+ if (syncStatus === 'completed' || syncStatus === 'failed') {
+ clearInterval(poll);
+ resolve(syncStatus);
}
} catch (err) {
- console.warn('⚠️ Sync poll error:', err.message);
+ console.error('⚠️ Background offerbook sync poll error:', err);
clearInterval(poll);
- resolve();
+ resolve('error');
}
}, 1000);
});
+ if (finalSyncStatus !== 'completed') {
+ console.error('⚠️ Background offerbook sync did not complete:', finalSyncStatus);
+ return;
+ }
console.log('✅ Background offerbook sync complete');As per coding guidelines, errors should be logged with context using console.error('⚠️ Context:', error).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/app.js` around lines 330 - 346, The poll currently resolves for both
failure and success but always prints "Background offerbook sync complete";
update the logic around window.api.taker.getSyncStatus(syncId) and the Promise
resolution so you only call console.log('✅ Background offerbook sync complete')
when status.success === true and syncStatus === 'completed'; otherwise call
console.error with context (e.g., console.error('⚠️ Offerbook sync failed:',
status) or the caught error) and ensure the catch block uses console.error('⚠️
Sync poll error:', err) instead of console.warn; reference the async poll using
setInterval, the variables status/syncStatus, and the final console.log to make
the conditional logging change.
This was referenced Apr 23, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Style