[WEB-2001] feat: Fix local cache issues r3 #5714
Conversation
- Add IS null where condition in query utils
Fix none filter
WalkthroughThe pull request introduces multiple modifications across several files, primarily focusing on enhancing SQL query handling, indexing capabilities, and local database caching management. Key changes include the addition of a new index field, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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.
Caution
Inline review comments failed to post
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
web/core/local-db/utils/load-workspace.ts (1)
30-33: Approve changes with a minor suggestion for improvementThe modifications to escape single quotes in both JSON stringified objects and regular strings are excellent. They effectively prevent SQL injection and syntax errors when inserting data into the local database, which aligns with the PR objective of fixing local cache issues.
Consider extracting the quote escaping logic into a separate function for better maintainability and reusability. For example:
const escapeSingleQuotes = (value: string) => value.replace(/'/g, "''"); // Then use it like this: if (typeof value === "object") { return `'${escapeSingleQuotes(JSON.stringify(value))}'`; } if (typeof value === "string") { return `'${escapeSingleQuotes(value)}'`; }This refactoring would make the code more DRY and easier to maintain if you need to modify the escaping logic in the future.
🛑 Comments failed to post (2)
web/core/local-db/utils/utils.ts (2)
19-21:
⚠️ Potential issuePotential issue with using
asyncfunctions insideforEachloopUsing
asyncfunctions within aforEachloop can lead to unexpected behavior becauseforEachdoes not await asynchronous operations. This means that theawaitcalls inside the loop may not complete before the function exits, leading to potential issues with data consistency.Consider replacing the
forEachloop with afor...ofloop to correctly handle asynchronous operations:- issueIds.forEach(async (issueId) => { + for (const issueId of issueIds) { const dbIssue = await persistence.getIssue(issueId); const issue = rootStore.issue.issues.getIssueById(issueId); if (issue) { // JSON.parse(JSON.stringify(issue)) is used to remove the mobx observables const issuePartial = pick({ ...dbIssue, ...JSON.parse(JSON.stringify(issue)) }, [ // ...properties "description_html", ]); updateIssue({ ...issuePartial, is_local_update: 1 }); } - }); + }📝 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.for (const issueId of issueIds) { const dbIssue = await persistence.getIssue(issueId); const issue = rootStore.issue.issues.getIssueById(issueId); if (issue) { // JSON.parse(JSON.stringify(issue)) is used to remove the mobx observables const issuePartial = pick({ ...dbIssue, ...JSON.parse(JSON.stringify(issue)) }, [ // ...properties "description_html", ]); updateIssue({ ...issuePartial, is_local_update: 1 }); } }
24-25: 🛠️ Refactor suggestion
Improve performance by avoiding unnecessary serialization
Using
JSON.parse(JSON.stringify(issue))to remove MobX observables can be inefficient and may impact performance, especially with large objects.Consider using MobX's
toJSutility to create a plain JavaScript object more efficiently:- // JSON.parse(JSON.stringify(issue)) is used to remove the mobx observables - const issuePartial = pick({ ...dbIssue, ...JSON.parse(JSON.stringify(issue)) }, [ + import { toJS } from "mobx"; + const issuePartial = pick({ ...dbIssue, ...toJS(issue) }, [ // ...properties "description_html", ]);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
web/app/profile/page.tsx (1)
Line range hint
1-458: Suggestions for code quality improvementsWhile the overall code structure is good, here are a few suggestions to further improve code quality and maintainability:
Consider extracting the
handleDeletefunction into a separate utility function or custom hook. This would improve readability of the main component.For consistency, consider moving all inline styles to className-based styling. This would make the styling more maintainable and easier to update.
The form submission logic in
onSubmitfunction could potentially be moved to a custom hook to separate concerns and make the component more focused on rendering.Consider adding more descriptive comments for complex logic sections, especially around form validation rules.
These are minor suggestions and the current implementation is already of good quality. Implementing these could further enhance the maintainability and readability of the code.
web/core/store/user/settings.store.ts (4)
Line range hint
83-86: Provide detailed error information in catch blockIn the catch block, the error is being caught but only a generic warning is logged. Including the error details can help with debugging.
Update the
console.warnto include the error details:} catch (e) { - console.warn("error while toggling local DB"); + console.warn("Error while toggling local DB:", e); runInAction(() => { this.canUseLocalDB = currentLocalDBValue; }); }
Line range hint
66-70: Update 'canUseLocalDB' after successful localStorage updateIt might be better to update
this.canUseLocalDBonly after confirming thatsetValueIntoLocalStoragewas successful. This ensures the observable state remains consistent with the actual local storage value.Apply this diff to update
this.canUseLocalDBafter successfully updating localStorage:try { - runInAction(() => { - this.canUseLocalDB = !currentLocalDBValue; - }); const transactionResult = setValueIntoLocalStorage(LOCAL_DB_ENABLED, !currentLocalDBValue); if (!transactionResult) { throw new Error("error while toggling local DB"); } + runInAction(() => { + this.canUseLocalDB = !currentLocalDBValue; + });
Line range hint
62-87: Add JSDoc comments to 'toggleLocalDB' method for better documentationAdding JSDoc comments to the
toggleLocalDBmethod will improve code readability and maintainability by providing context and usage information.Consider adding a JSDoc comment like:
/** * Toggles the local database usage. * @param workspaceSlug - The slug of the workspace. * @param projectId - The ID of the project. */
Line range hint
62-87: Add unit tests for 'toggleLocalDB' methodConsider adding unit tests for the
toggleLocalDBmethod to ensure its correctness, especially given its asynchronous operations and multiple conditional branches.Do you want me to help in drafting unit tests for this method or open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- web/app/profile/page.tsx (1 hunks)
- web/core/components/workspace/sidebar/help-section.tsx (2 hunks)
- web/core/store/user/settings.store.ts (3 hunks)
🔇 Additional comments (3)
web/app/profile/page.tsx (3)
9-10: LGTM: Import statements have been optimized.The consolidation of import statements and removal of unused imports (such as
useUserSettings) improves code readability and maintains consistency with the removal of local cache functionality.Also applies to: 19-19, 21-21
Line range hint
1-458: Clarification needed: Removal of local cache functionalityThe local cache functionality, including the
useUserSettingshook and the associated UI elements, has been completely removed from the component. While this aligns with the PR objective of fixing local cache issues, it's a significant change that might impact users who relied on this feature.Could you please provide more context on:
- The specific issues that led to the decision to remove the local cache feature entirely?
- How this change affects the overall user experience and application performance?
- Are there any plans to reintroduce a modified version of this feature in the future?
This information will help in understanding the full impact of these changes and ensure that we're not inadvertently removing a valuable feature without proper consideration.
Line range hint
1-458: LGTM: Core functionality of ProfileSettingsPage remains intactThe main functionality of the profile settings page, including handling user profile data updates, form validation, and submission logic, has been preserved. The removal of the local cache feature doesn't appear to have negatively impacted these core features.
The component still effectively manages:
- User avatar and cover image updates
- Personal information (name, email, role, etc.)
- User preferences (timezone)
- Account deactivation option
The code structure and logic for these features remain well-organized and functional.
| @@ -59,7 +59,7 @@ export class UserSettingsStore implements IUserSettingsStore { | |||
| this.userService = new UserService(); | |||
| } | |||
|
|
|||
| toggleLocalDB = async () => { | |||
| toggleLocalDB = async (workspaceSlug: string | undefined, projectId: string | undefined) => { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update method signature to use optional parameters
Ensure that the implementation matches the interface update by using the optional parameter syntax in the method definition as well.
Apply this diff to update the method signature:
-toggleLocalDB = async (workspaceSlug: string | undefined, projectId: string | undefined) => {
+toggleLocalDB = async (workspaceSlug?: string, projectId?: string) => {📝 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.
| toggleLocalDB = async (workspaceSlug: string | undefined, projectId: string | undefined) => { | |
| toggleLocalDB = async (workspaceSlug?: string, projectId?: string) => { |
| if (currentLocalDBValue) { | ||
| await persistence.clearStorage(); | ||
| } else if (workspaceSlug) { | ||
| await persistence.initialize(workspaceSlug); | ||
| persistence.syncWorkspace(); | ||
| projectId && persistence.syncIssues(projectId); |
There was a problem hiding this comment.
Await asynchronous operations to ensure proper execution
If persistence.syncWorkspace() and persistence.syncIssues(projectId) are asynchronous functions, you should await them to ensure they complete before proceeding.
Apply this diff to await the asynchronous calls:
- persistence.syncWorkspace();
- projectId && persistence.syncIssues(projectId);
+ await persistence.syncWorkspace();
+ if (projectId) {
+ await persistence.syncIssues(projectId);
+ }📝 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 (currentLocalDBValue) { | |
| await persistence.clearStorage(); | |
| } else if (workspaceSlug) { | |
| await persistence.initialize(workspaceSlug); | |
| persistence.syncWorkspace(); | |
| projectId && persistence.syncIssues(projectId); | |
| if (currentLocalDBValue) { | |
| await persistence.clearStorage(); | |
| } else if (workspaceSlug) { | |
| await persistence.initialize(workspaceSlug); | |
| await persistence.syncWorkspace(); | |
| if (projectId) { | |
| await persistence.syncIssues(projectId); | |
| } |
Handle cases when 'workspaceSlug' is undefined during initialization
When enabling the local database (currentLocalDBValue is false), if workspaceSlug is not provided, the initialization process is skipped without any notification or error. This might lead to unexpected behavior.
Consider adding validation to ensure that workspaceSlug is provided when enabling the local database, or handle the case appropriately.
Apply this diff to add a warning when workspaceSlug is undefined:
} else if (workspaceSlug) {
await persistence.initialize(workspaceSlug);
- persistence.syncWorkspace();
- projectId && persistence.syncIssues(projectId);
+ await persistence.syncWorkspace();
+ if (projectId) {
+ await persistence.syncIssues(projectId);
+ }
+ } else {
+ console.warn("Workspace slug is required to initialize local database");
}📝 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 (currentLocalDBValue) { | |
| await persistence.clearStorage(); | |
| } else if (workspaceSlug) { | |
| await persistence.initialize(workspaceSlug); | |
| persistence.syncWorkspace(); | |
| projectId && persistence.syncIssues(projectId); | |
| if (currentLocalDBValue) { | |
| await persistence.clearStorage(); | |
| } else if (workspaceSlug) { | |
| await persistence.initialize(workspaceSlug); | |
| await persistence.syncWorkspace(); | |
| if (projectId) { | |
| await persistence.syncIssues(projectId); | |
| } | |
| } else { | |
| console.warn("Workspace slug is required to initialize local database"); | |
| } |
| @@ -22,7 +22,7 @@ | |||
| canUseLocalDB: boolean; | |||
| // actions | |||
| fetchCurrentUserSettings: () => Promise<IUserSettings | undefined>; | |||
| toggleLocalDB: () => Promise<void>; | |||
| toggleLocalDB: (workspaceSlug: string | undefined, projectId: string | undefined) => Promise<void>; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use optional parameter syntax for cleaner code
In TypeScript, it's more idiomatic to use the optional parameter syntax ? for parameters that may be undefined. This enhances readability and conciseness.
Apply this diff to update the method signature:
-toggleLocalDB: (workspaceSlug: string | undefined, projectId: string | undefined) => Promise<void>;
+toggleLocalDB: (workspaceSlug?: string, projectId?: string) => Promise<void>;📝 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.
| toggleLocalDB: (workspaceSlug: string | undefined, projectId: string | undefined) => Promise<void>; | |
| toggleLocalDB: (workspaceSlug?: string, projectId?: string) => Promise<void>; |
| }} | ||
| className="flex w-full items-center justify-between text-xs hover:bg-custom-background-80" | ||
| > | ||
| <span className="racking-tight">Local Cache</span> |
There was a problem hiding this comment.
Typo in className 'racking-tight'
The className racking-tight appears to be a typo. It should likely be tracking-tight, which adjusts letter spacing.
Apply this diff to fix the typo:
-<span className="racking-tight">Local Cache</span>
+<span className="tracking-tight">Local Cache</span>📝 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.
| <span className="racking-tight">Local Cache</span> | |
| <span className="tracking-tight">Local Cache</span> |
| <span className="racking-tight">Local Cache</span> | ||
| <ToggleSwitch | ||
| value={canUseLocalDB} | ||
| onChange={() => toggleLocalDB(workspaceSlug?.toString(), projectId?.toString())} |
There was a problem hiding this comment.
Handle potential 'undefined' values when calling .toString()
When using optional chaining with .toString(), as in workspaceSlug?.toString(), if workspaceSlug is undefined, calling .toString() on it will result in a TypeError because undefined does not have a toString() method. Consider providing default values or handling undefined cases to prevent potential runtime errors.
Apply this diff to safely handle undefined values:
-onChange={() => toggleLocalDB(workspaceSlug?.toString(), projectId?.toString())}
+onChange={() => toggleLocalDB(workspaceSlug ? workspaceSlug.toString() : '', projectId ? projectId.toString() : '')}Alternatively, ensure that workspaceSlug and projectId are defined before calling .toString().
📝 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.
| onChange={() => toggleLocalDB(workspaceSlug?.toString(), projectId?.toString())} | |
| onChange={() => toggleLocalDB(workspaceSlug ? workspaceSlug.toString() : '', projectId ? projectId.toString() : '')} |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/core/local-db/storage.sqlite.ts (1 hunks)
🔇 Additional comments (2)
web/core/local-db/storage.sqlite.ts (2)
57-57: LGTM: Resetting storage state after clearing.The addition of
this.reset()after attempting to clear the storage is a good practice. It ensures that theStorageinstance is reset to its initial state, maintaining consistency between the actual storage state and the instance's internal state.
57-57: Verify the implementation of thereset()method.The
reset()method is called here, but its implementation is not visible in the provided code. Please ensure that thereset()method properly reinitializes all necessary properties of theStorageclass.
| const fileSystemDirectoryHandle = await storageManager.getDirectory(); | ||
| //@ts-expect-error , clear local issue cache | ||
| await fileSystemDirectoryHandle.remove({ recursive: true }); | ||
| this.reset(); | ||
| } catch (e) { | ||
| console.error("Error clearing sqlite sync storage", e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling in clearStorage.
The current implementation logs the error but doesn't propagate it. Consider either:
- Throwing the error after logging it to allow proper error handling by the caller.
- Returning a boolean indicating success or failure.
This would provide better visibility into storage clearing failures.
Here's a suggested implementation:
clearStorage = async () => {
try {
const storageManager = window.navigator.storage;
const fileSystemDirectoryHandle = await storageManager.getDirectory();
//@ts-expect-error , clear local issue cache
await fileSystemDirectoryHandle.remove({ recursive: true });
this.reset();
+ return true;
} catch (e) {
console.error("Error clearing sqlite sync storage", e);
+ return false;
}
};📝 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.
| const fileSystemDirectoryHandle = await storageManager.getDirectory(); | |
| //@ts-expect-error , clear local issue cache | |
| await fileSystemDirectoryHandle.remove({ recursive: true }); | |
| this.reset(); | |
| } catch (e) { | |
| console.error("Error clearing sqlite sync storage", e); | |
| } | |
| const fileSystemDirectoryHandle = await storageManager.getDirectory(); | |
| //@ts-expect-error , clear local issue cache | |
| await fileSystemDirectoryHandle.remove({ recursive: true }); | |
| this.reset(); | |
| return true; | |
| } catch (e) { | |
| console.error("Error clearing sqlite sync storage", e); | |
| return false; | |
| } |
Summary by CodeRabbit
New Features
sequence_id, improving data retrieval efficiency.issue_typein query parameters, allowing for more refined data queries.Bug Fixes
Chores