[WEB-2868] fix: Handle Hyper Mode toggling better. #6214
Conversation
- Handle edge cases in sync workspace
WalkthroughThe pull request introduces modifications to storage management and error handling across multiple files in the local database and user store modules. The changes primarily focus on enhancing the Changes
Sequence DiagramsequenceDiagram
participant User
participant UserStore
participant Persistence
participant StorageManager
participant BrowserStorage
User->>UserStore: signOut()
UserStore->>Persistence: clearStorage(true)
Persistence->>StorageManager: clearOPFS(force)
StorageManager->>BrowserStorage: Remove entries
Note over StorageManager: Check browser type
Note over StorageManager: Conditional forced removal
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/core/local-db/utils/utils.ts (1)
168-205: Add error handling for root.values() iteration.The function handles errors well for individual operations but doesn't catch errors that might occur during the iteration of root.values().
export const clearOPFS = async (force = false) => { const storageManager = window.navigator.storage; const root = await storageManager.getDirectory(); if (force && isChrome()) { await (root as any).remove({ recursive: true }); return; } - // ts-ignore - for await (const entry of root.values()) { + try { + for await (const entry of root.values()) { if (entry.kind === "directory" && entry.name.startsWith(".ahp-")) { // ... existing code ... } else { root.removeEntry(entry.name); } } + } catch (error) { + console.error("Error during storage cleanup:", error); + } };web/core/local-db/storage.sqlite.ts (1)
62-68: Consider enhancing error handling and return value.The current error handling is quite generic. Consider these improvements:
- Return a boolean to indicate success/failure
- Log specific error types for better debugging
- Consider throwing specific errors for critical failures that should interrupt the calling flow
- clearStorage = async (force = false) => { + clearStorage = async (force = false): Promise<boolean> => { try { await this.db?.close(); await clearOPFS(force); this.reset(); + return true; } catch (e) { - console.error("Error clearing sqlite sync storage", e); + const error = e instanceof Error ? e : new Error('Unknown error'); + console.error("Error clearing sqlite sync storage:", { + error: error.message, + type: error.name, + stack: error.stack + }); + return false; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/core/local-db/storage.sqlite.ts(1 hunks)web/core/local-db/utils/load-workspace.ts(5 hunks)web/core/local-db/utils/utils.ts(1 hunks)web/core/store/user/index.ts(1 hunks)
🔇 Additional comments (4)
web/core/store/user/index.ts (1)
234-234: LGTM! Forcing storage clear during sign out is appropriate.
The change ensures complete cleanup of storage during sign out.
web/core/local-db/utils/load-workspace.ts (1)
112-112: LGTM! Good defensive programming with null checks.
The added null checks before mapping over database results prevent potential runtime errors. This pattern is consistently applied across all sync functions:
syncLabelssyncModulessyncCyclessyncStatessyncMembers
Also applies to: 143-143, 170-170, 197-197, 224-224
web/core/local-db/storage.sqlite.ts (2)
64-64: LGTM! Good use of optional chaining.
The addition of optional chaining (?.) for db.close() improves safety by preventing runtime errors when db is null, which can happen after calling reset().
62-65: Verify the usage and implementation of force parameter.
The new force parameter affects storage clearing behavior. Let's verify its usage and implementation:
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about both the usage and implementation of the force parameter.
Force parameter implementation is correct and used appropriately
The force parameter is properly implemented and used in specific scenarios:
- Used with
trueduring sign-out operations inweb/core/store/user/index.ts - Default
falsevalue used in regular workspace switching insettings.store.ts - Implementation in
clearOPFSshows it performs a complete recursive removal of OPFS storage in Chrome browsers when force is true, while doing selective cleanup otherwise
The implementation aligns with typical storage management patterns where forced cleanup is reserved for complete user session termination.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of clearStorage with force parameter and implementation of clearOPFS
# Check for clearStorage calls with force parameter
echo "Checking clearStorage calls:"
rg -A 2 "clearStorage\("
# Check implementation of clearOPFS
echo "Checking clearOPFS implementation:"
rg -A 10 "clearOPFS\s*=\s*async"
Length of output: 1369
| export const isChrome = () => { | ||
| const userAgent = navigator.userAgent; | ||
| const isChrome = userAgent.includes("Chrome") && !userAgent.includes("Edg") && !userAgent.includes("OPR"); | ||
| return userAgent.includes("Chrome") && !userAgent.includes("Edg") && !userAgent.includes("OPR"); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using feature detection instead of user agent sniffing.
Browser detection through user agent strings is unreliable as they can be spoofed. Consider using feature detection instead.
-export const isChrome = () => {
- const userAgent = navigator.userAgent;
- return userAgent.includes("Chrome") && !userAgent.includes("Edg") && !userAgent.includes("OPR");
-};
+export const isChrome = () => {
+ // Feature detection for Chrome-specific features
+ return !!(window.chrome && window.chrome.webstore === undefined && window.chrome.runtime);
+};📝 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.
| export const isChrome = () => { | |
| const userAgent = navigator.userAgent; | |
| const isChrome = userAgent.includes("Chrome") && !userAgent.includes("Edg") && !userAgent.includes("OPR"); | |
| return userAgent.includes("Chrome") && !userAgent.includes("Edg") && !userAgent.includes("OPR"); | |
| }; | |
| export const isChrome = () => { | |
| // Feature detection for Chrome-specific features | |
| return !!(window.chrome && window.chrome.webstore === undefined && window.chrome.runtime); | |
| }; |
- Handle edge cases in sync workspace
Summary by CodeRabbit
New Features
Bug Fixes
Chores