[WEB-2706] chore: Switch to wa-sqlite #5859
Conversation
…into fix-local-cache-issues
- Wait till all the tables are created
WalkthroughThe changes in this pull request primarily involve the addition of a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 28
🧹 Outside diff range and nitpick comments (43)
web/core/local-db/utils/query-executor.ts (1)
4-8: Approve: Good addition of optional chainingThe use of optional chaining (
?.) onpersistence.db?.exec()is a good defensive programming practice. It prevents potential runtime errors ifpersistence.dbis undefined, which aligns well with the transition towa-sqlite-library.Consider adding a fallback or error handling for cases where
persistence.dbis undefined:if (!persistence.db) { throw new Error("Database is not initialized"); } const data = await persistence.db.exec({ // ... (rest of the options) });This would provide more explicit error handling and make debugging easier.
web/sentry.edge.config.ts (1)
Inconsistent Sentry initialization methods
Multiple Sentry configuration files use both
init({...})andSentry.init({...}). Please standardize the initialization method across all configuration files to ensure consistency.Affected files:
- live/src/core/config/sentry-config.ts
- space/sentry.client.config.ts
- space/sentry.edge.config.ts
- space/sentry.server.config.ts
- web/sentry.client.config.ts
- web/sentry.server.config.ts
🔗 Analysis chain
Line range hint
8-16: LGTM: Simplified Sentry initializationThe change to use
init({...})directly is consistent with the updated import statement. The configuration parameters remain unchanged, maintaining the existing setup.To ensure that all Sentry configurations are consistent across the project, let's verify other Sentry configuration files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in Sentry configuration files # Test: Search for Sentry configuration files and their content echo "Sentry configuration files:" fd -e ts -e js sentry echo "\nContent of Sentry configuration files:" fd -e ts -e js sentry --exec cat {}Length of output: 6371
web/sentry.client.config.ts (1)
PR Objective Not Addressed
The PR does not include any changes related to switching from
sqlite-wasamtowa-sqlite-library. Please ensure that all necessary changes are incorporated to achieve the intended objective.
- Remove references to
sqlite-wasam.- Add references to
wa-sqlite-library.🔗 Analysis chain
Line range hint
5-29: Overall changes look good, but PR objective not addressedThe changes in this file improve code clarity and align with modern JavaScript practices. However, these changes don't appear to be related to the stated PR objective of switching from
sqlite-wasamtowa-sqlite-library.To verify if the PR objective is addressed elsewhere, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for changes related to sqlite libraries # Test 1: Search for removed references to sqlite-wasam echo "Searching for removed references to sqlite-wasam:" git diff origin/master..HEAD | grep -E '^\-.*sqlite-wasam' # Test 2: Search for added references to wa-sqlite-library echo "Searching for added references to wa-sqlite-library:" git diff origin/master..HEAD | grep -E '^\+.*wa-sqlite-library' # Test 3: Search for any remaining references to sqlite-wasam echo "Searching for any remaining references to sqlite-wasam:" rg 'sqlite-wasam' # Test 4: Search for new references to wa-sqlite-library echo "Searching for new references to wa-sqlite-library:" rg 'wa-sqlite-library'Length of output: 1023
Script:
#!/bin/bash # Description: Retry searching for changes related to sqlite libraries using the correct base branch # Test 1: Get the current branch name echo "Getting current branch name:" git branch --show-current # Test 2: Search for removed references to sqlite-wasam using 'main' as the base branch echo "Searching for removed references to sqlite-wasam:" git diff main..HEAD | grep -E '^\-.*sqlite-wasam' # Test 3: Search for added references to wa-sqlite-library using 'main' as the base branch echo "Searching for added references to wa-sqlite-library:" git diff main..HEAD | grep -E '^\+.*wa-sqlite-library' # Test 4: Search for any remaining references to sqlite-wasam echo "Searching for any remaining references to sqlite-wasam:" rg 'sqlite-wasam' # Test 5: Search for new references to wa-sqlite-library echo "Searching for new references to wa-sqlite-library:" rg 'wa-sqlite-library'Length of output: 1098
Script:
#!/bin/bash # Description: Identify the default base branch and search for changes related to sqlite libraries # Test 1: Identify the default base branch from origin/HEAD echo "Identifying the default base branch:" DEFAULT_BRANCH=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@') if [ -z "$DEFAULT_BRANCH" ]; then echo "Failed to identify the default base branch. Please verify the repository configuration." exit 1 fi echo "Default base branch is: $DEFAULT_BRANCH" # Test 2: Search for removed references to sqlite-wasam using the identified default base branch echo "Searching for removed references to sqlite-wasam:" git diff origin/$DEFAULT_BRANCH..HEAD | grep -E '^\-.*sqlite-wasam' || echo "No removed references to sqlite-wasam found." # Test 3: Search for added references to wa-sqlite-library using the identified default base branch echo "Searching for added references to wa-sqlite-library:" git diff origin/$DEFAULT_BRANCH..HEAD | grep -E '^\+.*wa-sqlite-library' || echo "No added references to wa-sqlite-library found." # Test 4: Search for any remaining references to sqlite-wasam in the codebase echo "Searching for any remaining references to sqlite-wasam:" rg 'sqlite-wasam' || echo "No remaining references to sqlite-wasam found." # Test 5: Search for new references to wa-sqlite-library in the codebase echo "Searching for new references to wa-sqlite-library:" rg 'wa-sqlite-library' || echo "No new references to wa-sqlite-library found."Length of output: 1315
web/core/local-db/utils/tables.ts (3)
27-40: Approve asynchronous changes, but consider optimization.The transition to asynchronous operations is a good improvement. It ensures that each database operation completes before moving to the next, potentially improving reliability. However, executing these operations sequentially might not be the most efficient approach.
Consider using
Promise.all()to execute these operations in parallel, which could significantly improve performance, especially when dealing with multiple table creations.Here's a suggested optimization:
export const createTables = async () => { await persistence.db.exec("BEGIN TRANSACTION;"); const tableCreationPromises = [ persistence.db.exec(createTableSQLfromSchema("issues", issueSchema)), persistence.db.exec(createTableSQLfromSchema("issue_meta", issueMetaSchema)), persistence.db.exec(createTableSQLfromSchema("modules", moduleSchema)), persistence.db.exec(createTableSQLfromSchema("labels", labelSchema)), persistence.db.exec(createTableSQLfromSchema("states", stateSchema)), persistence.db.exec(createTableSQLfromSchema("cycles", cycleSchema)), persistence.db.exec(createTableSQLfromSchema("estimate_points", estimatePointSchema)), persistence.db.exec(createTableSQLfromSchema("members", memberSchema)), persistence.db.exec(createTableSQLfromSchema("options", optionsSchema)) ]; await Promise.all(tableCreationPromises); await persistence.db.exec("COMMIT;"); };This approach maintains the transaction while potentially improving performance by executing table creations in parallel.
27-28: Address the TODO comment for optimization.The TODO comment suggests using
Promise.all()or sending all statements at once. This is a valid optimization strategy that could significantly improve performance.Would you like assistance in implementing this optimization? I can provide a code example that uses
Promise.all()to execute all table creation statements in parallel within the transaction.
Inconsistent Async Patterns Found in Database Operations
The
persistence.db.execcalls in the following files lackasync/await, which may lead to unreliable database operations:
web/core/local-db/utils/load-issues.tsweb/core/local-db/utils/load-workspace.tsweb/core/local-db/utils/indexes.ts🔗 Analysis chain
Line range hint
1-40: Overall assessment of changes intables.tsThe modifications to the
createTablesfunction represent a positive step towards more reliable database operations by making the function asynchronous and usingawaitfor each operation. This ensures that each table creation completes before moving to the next, potentially reducing race conditions or incomplete operations.However, as noted in the TODO comment and our previous suggestions, there's room for optimization. Implementing parallel execution of table creations within the transaction could significantly improve performance, especially as the number of tables grows.
These changes align well with the PR objective of switching to
wa-sqlite, as they prepare the ground for more efficient and reliable database operations. Ensure that these changes are consistent with the new library's best practices and that proper error handling is implemented to catch any issues that might arise during table creation.To ensure consistency across the codebase, let's check if similar changes have been applied to other database-related files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar async patterns in other database-related files # Test: Search for files with similar database operations rg -g '!web/core/local-db/utils/tables.ts' -A 5 'persistence\.db\.exec'Length of output: 11395
web/core/components/issues/issue-layouts/roots/draft-issue-layout-root.tsx (2)
Line range hint
34-42: LGTM: Enhanced useSWR usage with loading stateThe update to destructure
isLoadingfrom theuseSWRhook is a good improvement for tracking the loading state. The revalidation options are appropriately set to prevent unnecessary refetches.Consider using an early return or null coalescing operator to simplify the condition:
const { isLoading } = useSWR( workspaceSlug && projectId && `DRAFT_ISSUES_${workspaceSlug}_${projectId}`, async () => { await issuesFilter?.fetchFilters(workspaceSlug, projectId); }, { revalidateIfStale: false, revalidateOnFocus: false } );This change would eliminate the need for the nested if statement and make the code more concise.
49-55: LGTM: Improved loading state handlingThe addition of the
LogoSpinnerduring the loading state enhances the user experience by providing visual feedback. The condition correctly checks bothisLoadingand the absence ofissueFilters.Consider extracting the loading spinner into a separate component for better reusability:
const LoadingSpinner = () => ( <div className="h-full w-full flex items-center justify-center"> <LogoSpinner /> </div> ); // Usage if (isLoading && !issueFilters) return <LoadingSpinner />;This would make the code more modular and easier to maintain if you need to use the same loading spinner elsewhere.
web/package.json (2)
7-7: LGTM: CPU profiling added to development script.The addition of
NEXT_CPU_PROF=1to thedevelopscript enables CPU profiling for the Next.js development server. This change aligns with the project's goals of improving performance monitoring and debugging capabilities.Consider updating the project's documentation to mention this new profiling feature and provide instructions on how to interpret the profiling results.
14-15: LGTM: New tracing scripts added.The addition of
dev:traceandview-tracescripts enhances the project's development and debugging capabilities:
dev:traceenables both TurboPack tracing and CPU profiling.view-tracestarts the internal Turbo trace server.These additions align with the project's focus on improving performance monitoring and debugging.
Consider updating the project's documentation to include information about these new scripts, their purposes, and how to use them effectively in the development workflow.
web/core/local-db/utils/load-issues.ts (6)
12-16: Improved error handling and transaction management.The changes enhance the robustness of the
addIssuefunction:
- The new condition prevents operations when the database is unavailable.
- The use of
awaitfor transaction operations ensures proper asynchronous handling.These improvements are well-implemented and align with best practices.
Consider wrapping the transaction in a try-catch block to handle potential errors and ensure the transaction is rolled back if an error occurs:
try { await persistence.db.exec("BEGIN TRANSACTION;"); stageIssueInserts(issue); await persistence.db.exec("COMMIT;"); } catch (error) { await persistence.db.exec("ROLLBACK;"); throw error; }
20-38: Enhanced bulk insert with improved error handling and performance monitoring.The changes to
addIssuesBulkfunction are well-implemented:
- The new condition prevents operations when the database is unavailable.
- The use of
awaitfor transaction operations ensures proper asynchronous handling.- The addition of performance timing is valuable for monitoring bulk insert operations.
These improvements enhance both the robustness and observability of the function.
Consider the following minor improvements:
- Wrap the transaction in a try-catch block for error handling and rollback:
try { await persistence.db.exec("BEGIN TRANSACTION;"); // ... existing code ... await persistence.db.exec("COMMIT;"); } catch (error) { await persistence.db.exec("ROLLBACK;"); throw error; }
- Use a more descriptive variable name for timing, e.g.,
bulkInsertDurationinstead of${insertEnd - insertStart}.
41-41: Improved error handling in deleteIssueFromLocal function.The addition of the
persistence.dbcheck enhances the robustness of the function by preventing operations when the database is unavailable.Consider the following improvements:
- Use
awaitfor transaction operations to ensure proper asynchronous handling:await persistence.db.exec("BEGIN TRANSACTION;"); await persistence.db.exec(deleteQuery); await persistence.db.exec(deleteMetaQuery); await persistence.db.exec("COMMIT;");
- Wrap the transaction in a try-catch block for error handling and rollback:
try { await persistence.db.exec("BEGIN TRANSACTION;"); await persistence.db.exec(deleteQuery); await persistence.db.exec(deleteMetaQuery); await persistence.db.exec("COMMIT;"); } catch (error) { await persistence.db.exec("ROLLBACK;"); throw error; }
53-53: Improved error handling in updateIssue function.The addition of the
persistence.dbcheck enhances the robustness of the function by preventing operations when the database is unavailable.Consider the following improvements:
- Wrap the delete and add operations in a single transaction for atomicity:
try { await persistence.db.exec("BEGIN TRANSACTION;"); await deleteIssueFromLocal(issue_id); await addIssue(issue); await persistence.db.exec("COMMIT;"); } catch (error) { await persistence.db.exec("ROLLBACK;"); throw error; }
- Update the TODO comment to reflect the current implementation or remove it if no longer relevant.
62-62: Improved error handling in syncDeletesToLocal function.The addition of the
persistence.dbcheck enhances the robustness of the function by preventing operations when the database is unavailable.Consider the following improvements:
- Use
Promise.allto handle multiple asynchronous delete operations concurrently:if (Array.isArray(response)) { await Promise.all(response.map(issue => deleteIssueFromLocal(issue))); }
- Add error handling for the API call to
issueService.getDeletedIssues:try { const response = await issueService.getDeletedIssues(workspaceId, projectId, queries); // ... existing code ... } catch (error) { console.error('Failed to fetch deleted issues:', error); // Handle the error appropriately }
Line range hint
1-124: Overall improvements in error handling and database operations.The changes in this file consistently enhance error handling and database operation management:
- Addition of
persistence.dbchecks across multiple functions improves robustness.- Enhanced transaction handling in some functions ensures better data integrity.
- Introduction of performance monitoring for bulk operations aids in observability.
These improvements align well with the PR objectives of transitioning to a new SQLite library and enhance the overall reliability of the local database operations.
To further improve the codebase:
- Consider implementing a transaction wrapper utility function to standardize error handling and rollback procedures across all database operations.
- Evaluate the possibility of using a database connection pool to manage multiple concurrent operations more efficiently.
- Implement a comprehensive error logging and reporting mechanism to track and analyze database-related issues in production.
web/core/local-db/utils/utils.ts (1)
17-18: LGTM: Improved error logging.The changes to the
logErrorfunction are good improvements:
- Using
console.erroris more appropriate for logging errors.- The Sentry usage is now consistent with the import statement change.
These updates enhance error logging clarity and maintain consistency in error handling.
Consider adding a comment explaining the purpose of the
logErrorfunction, especially if it's used throughout the codebase. This can help other developers understand its importance and usage.web/core/local-db/utils/load-workspace.ts (2)
Line range hint
107-107: Fix bug in collecting estimate pointsThere's a bug in how estimate points are being collected. The
concatmethod returns a new array but doesn't modify the original one.Replace the line with:
-objects.concat(estimate.points); +objects.push(...estimate.points);This change ensures that the estimate points are correctly added to the
objectsarray.
Line range hint
133-143: Review overall impact of removing transaction managementThe consistent removal of transaction management across all functions suggests a significant change in how data is handled. While this may be related to the switch to
wa-sqlite-library, it's crucial to ensure that data integrity is maintained.Consider the following recommendations:
- Document the rationale behind removing transaction management in the code or PR description.
- Implement a global error handling mechanism to manage failures during data loading.
- Consider implementing a rollback mechanism or data validation step after loading to ensure consistency.
- If
wa-sqlite-libraryprovides alternative transaction management, implement it consistently across all functions.To verify the new approach:
#!/bin/bash # Search for new error handling or transaction management patterns rg -i 'try|catch|error|transaction|rollback' web/core/local-db/This will help identify any new error handling or transaction management patterns introduced with the switch to
wa-sqlite-library.web/core/services/issue/issue.service.ts (1)
66-69: Consistent usage of imported Sentry function.The direct usage of
startSpanaligns well with the updated import statement. This change maintains the existing functionality while improving code consistency.For even better consistency, consider updating the variable name:
- const response = await startSpan({ name: "GET_ISSUES" }, async () => { + const issues = await startSpan({ name: "GET_ISSUES" }, async () => { const res = await persistence.getIssues(workspaceSlug, projectId, queries, config); return res; }); - return response as TIssuesResponse; + return issues as TIssuesResponse;This small change makes the variable name more descriptive of its content.
web/core/store/issue/helpers/issue-filter-helper.store.ts (2)
117-118: LGTM: Addition oflayouttoissueFiltersParamsThe inclusion of the
layoutproperty inissueFiltersParamsis appropriate and consistent with the method's purpose. This change allows for the layout type to be considered in issue filtering.For consistency with the rest of the method, consider using the nullish coalescing operator:
if (displayFilters?.layout != null) issueFiltersParams.layout = displayFilters.layout;This change would make the condition consistent with how other properties are handled in this method.
Line range hint
262-271: LGTM: NewgetShouldClearIssuesmethodThe addition of the
getShouldClearIssuesmethod is appropriate and follows the existing pattern set bygetShouldReFetchIssues. It correctly identifieslayoutchanges as a trigger for clearing issues.For consistency and future maintainability, consider the following suggestions:
Use a constant for the filter types:
const CLEAR_ISSUES_DISPLAY_FILTERS = ["layout"] as const;Use the constant in the method:
getShouldClearIssues = (displayFilters: IIssueDisplayFilterOptions) => { const displayFilterKeys = Object.keys(displayFilters); return CLEAR_ISSUES_DISPLAY_FILTERS.some((filter) => displayFilterKeys.includes(filter)); };These changes will make it easier to maintain and extend the list of filters that should trigger clearing issues in the future.
web/core/local-db/worker/db.ts (3)
26-26: Use a meaningful VFS name instead of 'hello'.The VFS is registered with the name
"hello", which might not be descriptive. Consider using a more meaningful name or making it configurable to better reflect its purpose.
37-38: Consider using a structured logging mechanism instead ofconsole.log.Using
console.logfor error logging is not recommended in production code. Consider using a logging library or framework for better log management and consistency.
66-66: Remove commented-outdebuggerstatement.Commented-out code can clutter the codebase and cause confusion. If the
debugger;statement is no longer needed, consider removing it to improve code cleanliness.web/core/local-db/worker/wa-sqlite/src/VFS.js (1)
36-38: Use 'override' instead of 'overload' in commentsIn JavaScript, the term 'override' is more appropriate than 'overload' when referring to methods in subclasses that replace methods in the base class. Please update the comment to enhance clarity.
Apply this diff to correct the comment:
/** - * Overload in subclasses to indicate which methods are asynchronous. + * Override in subclasses to indicate which methods are asynchronous. * @param {string} methodName * @returns {boolean} */web/core/store/issue/archived/filter.store.ts (1)
Line range hint
63-69: Simplify the check fordisplayFiltersingetIssueFiltersIn the
getIssueFiltersmethod, you useisEmpty(displayFilters)to check ifdisplayFiltersis empty or undefined. SinceisEmpty(undefined)returnstrue, andthis.filters[projectId]can beundefined, you can simplify the logic by directly checking ifdisplayFiltersis falsy.Apply this diff to simplify the
getIssueFiltersmethod:getIssueFilters(projectId: string) { - const displayFilters = this.filters[projectId] || undefined; - if (isEmpty(displayFilters)) return undefined; + const displayFilters = this.filters[projectId]; + if (!displayFilters) return undefined; const _filters: IIssueFilters = this.computedIssueFilters(displayFilters); return _filters; }web/core/store/issue/cycle/filter.store.ts (2)
34-34: Consider adding documentation forgetIssueFiltersmethodAdding a JSDoc comment to
getIssueFilters(cycleId: string): IIssueFilters | undefined;would improve code readability and maintain consistency with the rest of the interface methods.
Line range hint
82-154: RefactorupdateFiltersmethod for better maintainabilityThe
updateFiltersmethod contains a lengthy switch-case statement handling different filter types. To enhance readability and reduce complexity, consider extracting each case into separate methods.Apply the following refactor:
// In the `updateFilters` method, replace the switch-case with method calls updateFilters = async ( workspaceSlug: string, projectId: string, type: EIssueFilterType, filters: IIssueFilterOptions | IIssueDisplayFilterOptions | IIssueDisplayProperties | TIssueKanbanFilters, cycleId: string ) => { try { if (isEmpty(this.filters) || isEmpty(this.filters[cycleId]) || isEmpty(filters)) return; switch (type) { case EIssueFilterType.FILTERS: await this.updateIssueFilters(workspaceSlug, projectId, filters as IIssueFilterOptions, cycleId); break; case EIssueFilterType.DISPLAY_FILTERS: await this.updateDisplayFilters(workspaceSlug, projectId, filters as IIssueDisplayFilterOptions, cycleId); break; case EIssueFilterType.DISPLAY_PROPERTIES: await this.updateDisplayProperties(workspaceSlug, projectId, filters as IIssueDisplayProperties, cycleId); break; case EIssueFilterType.KANBAN_FILTERS: this.updateKanbanFilters(workspaceSlug, filters as TIssueKanbanFilters, cycleId); break; default: break; } } catch (error) { if (cycleId) this.fetchFilters(workspaceSlug, projectId, cycleId); throw error; } }; // Then, implement each case as a separate method private async updateIssueFilters( workspaceSlug: string, projectId: string, updatedFilters: IIssueFilterOptions, cycleId: string ) { // Logic for updating issue filters } private async updateDisplayFilters( workspaceSlug: string, projectId: string, updatedDisplayFilters: IIssueDisplayFilterOptions, cycleId: string ) { // Logic for updating display filters } private async updateDisplayProperties( workspaceSlug: string, projectId: string, updatedDisplayProperties: IIssueDisplayProperties, cycleId: string ) { // Logic for updating display properties } private updateKanbanFilters( workspaceSlug: string, updatedKanbanFilters: TIssueKanbanFilters, cycleId: string ) { // Logic for updating kanban filters }This refactor improves code modularity, making the
updateFiltersmethod cleaner and each update operation more focused.web/core/store/issue/module/filter.store.ts (2)
Line range hint
72-159: Consider refactoringupdateFiltersmethod for improved maintainabilityThe
updateFiltersmethod handles multiple filter types within a single large function, which increases complexity and can make the code harder to read, maintain, and test. Consider refactoring each case in theswitchstatement into separate, well-named helper methods. This will enhance modularity, readability, and make future enhancements or bug fixes more manageable.
Line range hint
60-70: Remove redundant try-catch block infetchFiltersmethodThe
try...catchblock in thefetchFiltersmethod rethrows the caught error without additional processing or logging. Since no specific error handling is performed, thetry...catchblock is unnecessary and can be removed to simplify the code.web/core/local-db/storage.sqlite.ts (4)
62-62: Update or remove misleading commentThe comment
// return if the window gets hiddenis no longer accurate since the check fordocument.hiddenhas been removed. Please update or remove the comment to reflect the current code behavior.Apply this diff to fix the issue:
- if (!rootStore.user.localDBEnabled) return false; // return if the window gets hidden + if (!rootStore.user.localDBEnabled) return false;
105-105: Remove unnecessary placeholder commentThe comment
// Your SQLite code here.appears to be a leftover placeholder. Consider removing it to clean up the code.Apply this diff to fix the issue:
- // Your SQLite code here.
126-130: Update or remove misleading commentThe comment
// return if the window gets hiddenis misleading because thedocument.hiddencheck has been commented out. Please update or remove the comment to reflect the current logic.Apply this diff to fix the issue:
if ( - // document.hidden || !rootStore.user.localDBEnabled ) - return false; // return if the window gets hidden + return false;
416-416: Correct typographical error in commentAdd a space between '5' and 'seconds' in the comment for clarity.
Apply this diff to fix the issue:
- // Check if it has been more than 5seconds + // Check if it has been more than 5 secondsweb/core/local-db/worker/wa-sqlite/src/OPFSCoopSyncVFS.js (3)
233-249: Consider handling unusedflagsparameter injAccessmethodIn the
jAccessmethod, theflagsparameter is currently unused. If certain access modes are unsupported, consider checkingflagsto handle them appropriately or document the method's limitations.
292-293: Verify necessity of usingsubarray()onpDataIn the
jReadmethod, you mention thatpDatais a Proxy of aUint8Array, and you usepData.subarray()to obtain a realUint8Array. Verify whether this is necessary, as usingsubarray()may create an unnecessary copy. IfpDataworks directly, you can simplify the code.
321-326: Verify necessity of usingsubarray()onpDatainjWriteSimilar to
jRead, in thejWritemethod, confirm whether it's necessary to usepData.subarray(). IfpDatacan be used directly withaccessHandle.write(), you can simplify the code by removing thesubarray()call.web/core/local-db/worker/wa-sqlite/src/types/index.d.ts (5)
3-3: Typographical Correction: Capitalize 'JavaScript' ProperlyOn line 3, the word 'Javascript' should be capitalized as 'JavaScript' to follow the correct spelling.
18-18: Consistency in Array Type NotationIn the type definition on line 18, consider using a consistent array type notation. Currently,
Array<number>is used alongsideUint8Array. For clarity and consistency, you might prefer to usenumber[]instead ofArray<number>.
546-546: Missing Semicolon in Method DeclarationOn line 546, the
libversion_numbermethod is missing a terminating semicolon. In TypeScript interfaces, method signatures should end with a semicolon.Apply this diff to add the missing semicolon:
- libversion_number(): number + libversion_number(): number;
604-606: Simplify Type Annotation forvalueParameterIn the
resultmethod on lines 604-606, thevalueparameter is typed as(SQLiteCompatibleType|number[])|null. Sincenumber[]is already included inSQLiteCompatibleTypeasArray<number>, this may be redundant. Consider simplifying the type annotation for clarity.Apply this diff to simplify the type annotation:
- result(context: number, value: (SQLiteCompatibleType|number[])|null): void; + result(context: number, value: SQLiteCompatibleType|null): void;
174-176: Consider Adding Error Handling in Example CodeIn the example code on lines 174-176, where
const db = await sqlite3.open_v2('myDB');is used, consider wrapping the database opening call in a try-catch block. This will help handle any potential errors during database connection and provide a better experience for developers using this code.Example:
try { const db = await sqlite3.open_v2('myDB'); // Proceed with database operations } catch (error) { console.error('Failed to open database:', error); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
web/core/local-db/worker/wa-sqlite/src/wa-sqlite.wasmis excluded by!**/*.wasmyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (34)
- packages/types/src/view-props.d.ts (1 hunks)
- web/core/components/issues/issue-layouts/roots/archived-issue-layout-root.tsx (3 hunks)
- web/core/components/issues/issue-layouts/roots/cycle-layout-root.tsx (4 hunks)
- web/core/components/issues/issue-layouts/roots/draft-issue-layout-root.tsx (3 hunks)
- web/core/components/issues/issue-layouts/roots/module-layout-root.tsx (3 hunks)
- web/core/components/issues/issue-layouts/roots/project-layout-root.tsx (3 hunks)
- web/core/components/issues/issue-layouts/roots/project-view-layout-root.tsx (1 hunks)
- web/core/local-db/storage.sqlite.ts (11 hunks)
- web/core/local-db/utils/indexes.ts (1 hunks)
- web/core/local-db/utils/load-issues.ts (3 hunks)
- web/core/local-db/utils/load-workspace.ts (6 hunks)
- web/core/local-db/utils/query-executor.ts (1 hunks)
- web/core/local-db/utils/query.utils.ts (2 hunks)
- web/core/local-db/utils/tables.ts (1 hunks)
- web/core/local-db/utils/utils.ts (2 hunks)
- web/core/local-db/worker/db.ts (1 hunks)
- web/core/local-db/worker/wa-sqlite/src/FacadeVFS.js (1 hunks)
- web/core/local-db/worker/wa-sqlite/src/OPFSCoopSyncVFS.js (1 hunks)
- web/core/local-db/worker/wa-sqlite/src/VFS.js (1 hunks)
- web/core/local-db/worker/wa-sqlite/src/sqlite-api.js (1 hunks)
- web/core/local-db/worker/wa-sqlite/src/sqlite-constants.js (1 hunks)
- web/core/local-db/worker/wa-sqlite/src/types/globals.d.ts (1 hunks)
- web/core/local-db/worker/wa-sqlite/src/types/index.d.ts (1 hunks)
- web/core/services/issue/issue.service.ts (2 hunks)
- web/core/store/issue/archived/filter.store.ts (1 hunks)
- web/core/store/issue/cycle/filter.store.ts (1 hunks)
- web/core/store/issue/draft/filter.store.ts (1 hunks)
- web/core/store/issue/helpers/issue-filter-helper.store.ts (2 hunks)
- web/core/store/issue/module/filter.store.ts (1 hunks)
- web/core/store/issue/project-views/filter.store.ts (2 hunks)
- web/core/store/issue/project/filter.store.ts (1 hunks)
- web/package.json (3 hunks)
- web/sentry.client.config.ts (2 hunks)
- web/sentry.edge.config.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/core/local-db/worker/wa-sqlite/src/sqlite-constants.js
🧰 Additional context used
🪛 Biome
web/core/local-db/storage.sqlite.ts
[error] 73-73: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
web/core/local-db/worker/wa-sqlite/src/FacadeVFS.js
[error] 16-18: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
web/core/local-db/worker/wa-sqlite/src/OPFSCoopSyncVFS.js
[error] 62-64: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 459-459: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 460-460: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
web/core/local-db/worker/wa-sqlite/src/sqlite-api.js
[error] 269-269: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 270-270: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 757-757: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 758-758: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 GitHub Check: Codacy Static Code Analysis
web/core/local-db/worker/wa-sqlite/src/OPFSCoopSyncVFS.js
[warning] 86-86: web/core/local-db/worker/wa-sqlite/src/OPFSCoopSyncVFS.js#L86
This rule identifies use of cryptographically weak random number generators.
[warning] 114-114: web/core/local-db/worker/wa-sqlite/src/OPFSCoopSyncVFS.js#L114
This rule identifies use of cryptographically weak random number generators.web/core/local-db/worker/wa-sqlite/src/sqlite-api.js
[warning] 77-77: web/core/local-db/worker/wa-sqlite/src/sqlite-api.js#L77
The numeric literal '0x100000000' will have at different value at runtime.
[warning] 177-177: web/core/local-db/worker/wa-sqlite/src/sqlite-api.js#L177
The numeric literal '0x7fffffff' will have at different value at runtime.
🪛 GitHub Check: lint-web
web/core/local-db/worker/wa-sqlite/src/types/globals.d.ts
[failure] 13-13:
Unexpected var, use let or const instead
[failure] 14-14:
Unexpected var, use let or const instead
[failure] 15-15:
Unexpected var, use let or const instead
[failure] 16-16:
Unexpected var, use let or const instead
[failure] 17-17:
Unexpected var, use let or const instead
[failure] 18-18:
Unexpected var, use let or const instead
[failure] 19-19:
Unexpected var, use let or const instead
[failure] 20-20:
Unexpected var, use let or const instead
[failure] 21-21:
Unexpected var, use let or const instead
🔇 Additional comments (63)
web/core/local-db/utils/query-executor.ts (1)
10-10: Verify: Change in returned data structureThe return statement has been simplified from (presumably)
return data.result.resultRows;toreturn data;. This changes the structure of the returned data.Please ensure that all consumers of this function are updated to handle the new data structure. Run the following script to find potential usage of this function:
Review the results to ensure all usages are compatible with the new return structure.
✅ Verification successful
Verified: Change in returned data structure
No issues found. All consumers are compatible with the new returned data structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of runQuery function and its return value # Search for imports of runQuery echo "Imports of runQuery:" rg -n "import.*runQuery.*from.*query-executor" --type ts # Search for usages of runQuery echo "\nUsages of runQuery:" rg -n "runQuery\(" --type ts # Search for potential usages of the old return structure echo "\nPotential usages of old return structure:" rg -n "\.result\.resultRows" --type tsLength of output: 1761
web/sentry.edge.config.ts (1)
6-6: LGTM: Improved import statementThe change to import only the
initfunction from "@sentry/nextjs" is a good practice. It simplifies the code and potentially reduces the bundle size by importing only what's necessary.web/sentry.client.config.ts (3)
5-5: LGTM: Improved import statementThe change to import specific functions (
replayIntegrationandinit) from@sentry/nextjsis a good practice. It improves code clarity and potentially reduces bundle size.
7-7: LGTM: Consistent initialization updateThe change from
Sentry.init({...})toinit({...})is consistent with the updated import statement. It simplifies the code while maintaining the same functionality.
Line range hint
25-29: LGTM: Consistent integration configuration updateThe change from
Sentry.replayIntegration({...})toreplayIntegration({...})is consistent with the updated import statement. It simplifies the code while maintaining the same functionality and configuration options.web/core/components/issues/issue-layouts/roots/archived-issue-layout-root.tsx (5)
7-7: LGTM: LogoSpinner import added.The import of
LogoSpinneris correctly placed and will be used for the new loading state UI.
Line range hint
20-29: LGTM: useSWR hook updated to handle loading state.The
useSWRhook has been updated to destructureisLoading, which allows for explicit handling of the loading state. The rest of the configuration remains unchanged, maintaining the existing functionality.
30-31: LGTM: Issue filters retrieval added.The
getIssueFiltersmethod is correctly used to retrieve issue filters based on theprojectId. This aligns with the new filtering capabilities mentioned in the summary.
34-39: LGTM: Loading state handling improved.The addition of a loading state check and the
LogoSpinnercomponent improves the user experience by providing visual feedback during data fetching. The condition correctly checks bothisLoadingand the absence ofissueFilters.
Line range hint
1-54: LGTM: Component structure maintained with improved loading handling.The overall structure of the
ArchivedIssueLayoutRootcomponent has been maintained while successfully integrating the new loading state handling and issue filters retrieval. The changes improve the component's responsiveness without disrupting its core functionality.web/core/local-db/utils/indexes.ts (1)
50-50: LGTM. Please verify the reasoning for this index change.The change from indexing the 'name' column to the 'key' column in the 'options' table looks good. This modification aligns with the PR objective of optimizing for 'wa-sqlite'. However, it would be beneficial to understand the specific reasoning behind this change.
Could you please provide more context on why the index was changed from 'name' to 'key' for the 'options' table? This will help ensure that the change aligns with the intended database usage patterns in the application.
To verify the impact of this change, you can run the following script to check for queries using the 'options' table:
This will help us understand how the 'options' table is being used and confirm that the new index on the 'key' column is appropriate.
✅ Verification successful
Index on 'key' column is appropriate and aligns with current query usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for queries using the 'options' table to verify the impact of the index change. # Test: Search for SQL queries involving the 'options' table rg -i 'from options' --type ts rg -i 'select.*from options' --type tsLength of output: 676
web/core/components/issues/issue-layouts/roots/draft-issue-layout-root.tsx (3)
5-5: LGTM: Import of LogoSpinnerThe addition of the
LogoSpinnerimport is appropriate for enhancing the loading state visualization.
Line range hint
1-70: Overall assessment: Improved component with enhanced loading state managementThe changes in this file successfully improve the loading state management and align with updates in other components. The introduction of
LogoSpinner, the updateduseSWRusage, and the new method for retrieving issue filters contribute to a better user experience and more consistent code architecture.While the specific library change mentioned in the PR objectives (switching from
sqlite-wasamtowa-sqlite-library) is not directly visible in this file, the modifications are in line with the overall goal of improving the application's functionality.
44-45: LGTM: Updated issue filter retrievalThe change to use
getIssueFiltersmethod is consistent with updates in other components and improves the overall architecture of the filtering system.To ensure consistency across the codebase, let's verify the usage of
getIssueFilters:✅ Verification successful
[/run_scripts]
[/run_scripts]
Consistent usage of
getIssueFiltersconfirmedThe usage of
getIssueFiltersis consistent across all relevant components and stores, ensuring architectural coherence.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of getIssueFilters method across components # Test: Search for getIssueFilters usage rg "getIssueFilters\(" --type ts --type tsxLength of output: 73
Script:
#!/bin/bash # Description: Verify consistent usage of getIssueFilters method across components # Test: Search for getIssueFilters usage in .ts and .tsx files rg "getIssueFilters\(" -g "*.ts" -g "*.tsx"Length of output: 4842
Script:
#!/bin/bash # Description: Verify consistent usage of getIssueFilters method across components # Test: Search for getIssueFilters usage in .ts and .tsx files rg "getIssueFilters\(" -g "*.ts" -g "*.tsx"Length of output: 4842
web/core/components/issues/issue-layouts/roots/project-view-layout-root.tsx (2)
61-61: Great improvement in loading state management!The updated loading condition
if (isLoading && !issueFilters)is a good enhancement. It ensures that the loading spinner is only displayed when the data is still being fetched andissueFiltersis not yet available. This will improve the user experience by preventing unnecessary loading states.
56-57: LGTM! Verify the behavior ofgetIssueFilters.The changes look good and align with the PR objectives. The use of
getIssueFiltersmethod suggests a more centralized approach to fetching filters, which is a positive change.To ensure the new method behaves as expected, please run the following verification:
web/package.json (3)
64-64: LGTM: Duplicaterechartsdependency removed.The
rechartsdependency is now correctly listed only once, resolving the previous duplication issue. The version remains unchanged at^2.12.7.
Line range hint
1-89: Overall assessment: Changes align with PR objectives and enhance development capabilities.The modifications to
package.jsonsuccessfully support the transition fromsqlite-wasamtowa-sqlite-libraryand introduce new development tools for profiling and tracing. These changes are consistent with the PR objectives and the AI-generated summary. The enhancements to scripts and dependencies will likely improve the development workflow and debugging capabilities.
42-42: LGTM: Dependencies updated for wa-sqlite transition.The removal of
@sqlite.org/sqlite-wasmand addition ofcomlinkalign with the PR objective of transitioning fromsqlite-wasamtowa-sqlite-library. These changes support the modifications inweb/core/local-db/storage.sqlite.tsfor worker communication.To ensure a smooth transition, please run the following verification script:
✅ Verification successful
Verified: Dependencies updated correctly.
- Removed
@sqlite.org/sqlite-wasm.- Added
comlinkand confirmed its usage across relevant files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of sqlite-wasm and addition of wa-sqlite # Test 1: Check if sqlite-wasm is no longer used echo "Checking for any remaining usage of sqlite-wasm:" rg -i "sqlite-wasm" # Test 2: Verify the addition of wa-sqlite echo "Verifying the addition of wa-sqlite:" rg -i "wa-sqlite" # Test 3: Check for Comlink usage echo "Checking for Comlink usage:" rg -i "comlink"Length of output: 127040
web/core/components/issues/issue-layouts/roots/module-layout-root.tsx (5)
8-8: LGTM: LogoSpinner import added.The import of
LogoSpinneris correctly placed and aligns with the goal of improving loading state visualization.
59-60: LGTM: Issue filters retrieval updated.The use of
getIssueFiltersmethod to retrieve issue filters is consistent with the changes mentioned in the PR summary. This approach centralizes filter management and improves code consistency.
63-68: LGTM: Loading state handling added.The addition of a loading state check and
LogoSpinnerrendering improves user experience by providing visual feedback during data fetching. The condition correctly checks both the loading state and the availability of issue filters.
70-70: LGTM: activeLayout assignment updated.The change in
activeLayoutassignment correctly reflects the updated data structure and filter retrieval method. This ensures consistency with the newgetIssueFiltersapproach.
Line range hint
47-58: LGTM: useSWR hook updated to include isLoading state.The explicit extraction of
isLoadingimproves code clarity. However, consider the implications of setting bothrevalidateIfStaleandrevalidateOnFocustofalse. This might affect data freshness. Ensure this aligns with the desired behavior for issue filter updates.To verify the impact of these SWR options, you can run the following script:
web/core/components/issues/issue-layouts/roots/project-layout-root.tsx (5)
9-9: LGTM: New import for LogoSpinnerThe import statement for
LogoSpinneris correctly added and follows the project's import style.
Line range hint
48-57: LGTM: Improved loading state handling in useSWR hookThe
isLoadingstate is now correctly destructured from theuseSWRhook's return value. This change enables better management of loading states throughout the component.
58-59: LGTM: Improved issue filter retrievalThe changes to retrieve issue filters and active layout are consistent with the new approach mentioned in the summary. This update improves code consistency across components and provides better abstraction of filter retrieval logic.
63-68: LGTM: Enhanced loading state handlingThe new conditional rendering block for the loading state improves user experience by providing visual feedback during data fetching. The condition correctly checks both the loading state and the availability of issue filters.
69-69: LGTM: Improved code readabilityThe addition of an empty line after the loading state block enhances code readability by clearly separating the loading state logic from the main component render.
web/core/components/issues/issue-layouts/roots/cycle-layout-root.tsx (5)
8-8: LGTM: LogoSpinner import addedThe addition of the
LogoSpinnerimport is appropriate and aligns with the goal of improving the loading state visualization.
80-86: LGTM: Added loading spinner for better UXThe addition of the
LogoSpinnercomponent when data is loading is an excellent improvement to the user experience. It provides clear visual feedback during data fetching operations.
Line range hint
1-114: Overall assessment: Well-implemented improvementsThe changes in this file effectively enhance the
CycleLayoutRootcomponent by improving loading state management and data fetching. The addition of theLogoSpinner, the use ofisLoadingfromuseSWR, and the updated issue filter retrieval method all contribute to a better user experience. These modifications align well with the PR objectives and are consistently implemented throughout the component.
66-67: LGTM: Improved issue filter retrievalThe use of
getIssueFiltersmethod is a good improvement over direct property access. It allows for more flexible and potentially context-aware filter retrieval.To ensure the new method is correctly implemented, let's verify its definition:
#!/bin/bash # Search for the definition of getIssueFilters method rg "getIssueFilters\s*\([^)]*\)\s*{" --type tsx --type ts
Line range hint
54-64: LGTM: Improved loading state handling with useSWRThe addition of
isLoadingfrom theuseSWRhook is a good practice for managing loading states. This change aligns well with the PR objectives of improving user experience during data fetching.Consider the implications of using
revalidateIfStale: false, revalidateOnFocus: false. While this can reduce unnecessary API calls, it might lead to stale data in some scenarios. Verify if this aligns with the desired behavior for cycle data:web/core/local-db/utils/load-issues.ts (1)
7-7: LGTM: New import for logging function.The addition of the
logimport from the localutilsmodule is appropriate for enhancing logging capabilities in this file.web/core/local-db/utils/utils.ts (1)
1-1: LGTM: Improved Sentry import.The change from a namespace import to a named import for Sentry's
captureExceptionfunction is a good practice. It simplifies usage and potentially reduces bundle size by importing only what's necessary.packages/types/src/view-props.d.ts (1)
80-81: LGTM: Addition of "layout" to TIssueParamsThe addition of "layout" to the TIssueParams type is consistent with the existing structure and aligns with the PR objective. This change allows for layout specification in issue parameters, which could be beneficial for the new wa-sqlite implementation.
To ensure consistency, please run the following script to check for any inconsistencies in the usage of "layout" across the codebase:
Please review the output to ensure that the usage of "layout" is consistent across the codebase.
✅ Verification successful
Verification Successful: "layout" is consistently used across the codebase
The addition of "layout" to the TIssueParams type is consistent and properly utilized throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of "layout" in issue-related types and interfaces # Test 1: Check for occurrences of "layout" in issue-related types and interfaces echo "Occurrences of 'layout' in issue-related types and interfaces:" rg -i 'layout.*:' packages/types/src/view-props.d.ts # Test 2: Check for usage of "layout" in issue-related functions or methods echo "Usage of 'layout' in issue-related functions or methods:" rg -i 'layout\s*[=:]\s*' --type ts,tsxLength of output: 487
Script:
#!/bin/bash # Description: Re-verify the usage of "layout" in issue-related types and interfaces with corrected file type flags # Test 1: Check for occurrences of "layout" in issue-related types and interfaces echo "Occurrences of 'layout' in issue-related types and interfaces:" rg -i 'layout.*:' packages/types/src/view-props.d.ts # Test 2: Check for usage of "layout" in issue-related functions or methods with corrected file types echo "Usage of 'layout' in issue-related functions or methods:" rg -i 'layout\s*[=:]\s*' --type ts --type tsxLength of output: 491
Script:
#!/bin/bash # Description: Re-verify the usage of "layout" in issue-related types and interfaces with corrected file patterns # Test 1: Check for occurrences of "layout" in issue-related types and interfaces echo "Occurrences of 'layout' in issue-related types and interfaces:" rg -i 'layout.*:' packages/types/src/view-props.d.ts # Test 2: Check for usage of "layout" in issue-related functions or methods with corrected file patterns echo "Usage of 'layout' in issue-related functions or methods:" rg -i 'layout\s*[=:]\s*' -g "*.ts" -g "*.tsx"Length of output: 32295
web/core/local-db/utils/load-workspace.ts (2)
62-66:⚠️ Potential issueFix schema mismatch and verify transaction handling
- The function is incorrectly using
labelSchemafor modules. This needs to be corrected to use the appropriate schema for modules.- Similar to
loadLabels, the transaction management code has been commented out. Ensure this aligns with thewa-sqlite-libraryusage.Please apply the following fix:
-stageInserts("modules", labelSchema, label); +stageInserts("modules", moduleSchema, module);Also, verify the
wa-sqlite-libraryusage:#!/bin/bash # Search for wa-sqlite-library import and transaction handling rg -i 'wa-sqlite-library.*transaction'
77-81: Consistent removal of transaction managementThe transaction management code has been consistently commented out across functions. This pattern suggests an intentional change related to the switch to
wa-sqlite-library.To ensure this is the correct approach:
- Confirm that
wa-sqlite-librarydoesn't require explicit transaction management for these operations.- If transactions are still needed, implement them using the new library's API.
Verify the new transaction handling approach:
#!/bin/bash # Search for any new transaction handling methods rg -i 'transaction|commit|rollback' web/core/local-db/web/core/local-db/utils/query.utils.ts (3)
9-9: LGTM: Addition oflayoutparameterThe addition of the
layoutparameter aligns with the PR objective of supporting different layouts, particularly the transition to wa-sqlite.
48-50: Verify the impact of the new conditional blockThe new conditional block modifies the
order_byproperty for the "spreadsheet" layout. Please ensure this change doesn't introduce unintended side effects in query results.Could you provide more context on the
SPECIAL_ORDER_BYobject? It's not defined in this file, and understanding its contents is crucial for assessing the impact of this change.Also, consider adding a comment explaining the rationale behind this modification to improve code maintainability.
Here's a suggested comment to add above the conditional block:
// Override the order_by property for spreadsheet layout to ensure consistent sorting
Line range hint
1-383: Summary of changes and their impactThe changes in this file primarily affect the
translateQueryParamsfunction by introducing support for different layouts, specifically the "spreadsheet" layout. This aligns with the PR objective of transitioning to wa-sqlite.Key points:
- The addition of the
layoutparameter enhances the flexibility of the function.- The new conditional block for "spreadsheet" layout modifies the
order_byproperty, which could impact query results.These changes seem appropriate, but it's important to ensure they don't introduce unintended side effects in other parts of the application that rely on this function. Consider adding unit tests to verify the behavior of
translateQueryParamswith different layout values.web/core/store/issue/project-views/filter.store.ts (3)
34-34: LGTM: New method signature is consistent and well-defined.The addition of
getIssueFiltersmethod to theIProjectViewIssuesFilterinterface is consistent with other filter interfaces and provides a clear way to retrieve issue filters for a specific view.
Line range hint
77-84: LGTM: Implementation ofgetIssueFiltersis correct and efficient.The
getIssueFiltersmethod is well-implemented. It correctly handles empty filters and usescomputedIssueFiltersfor processing. The implementation aligns with the interface definition and provides the expected functionality.
268-277: LGTM: Kanban filter handling is properly implemented.The addition of kanban filter handling in the
updateFiltersmethod is well-implemented. It correctly stores the filters in local storage, uses appropriate enums, and includes a check forcurrentUserIdto ensure it only applies to authenticated users.web/core/services/issue/issue.service.ts (1)
1-1: Improved import statement for Sentry.The change from a namespace import to a named import for
startSpanis a good practice. It enhances code clarity by explicitly showing which functionality is used and can potentially lead to better tree-shaking and smaller bundle sizes.web/core/store/issue/helpers/issue-filter-helper.store.ts (2)
16-16: LGTM: New import forEIssueLayoutTypesThe addition of
EIssueLayoutTypesto the imports is appropriate and aligns with the changes made in the file.
Line range hint
1-371: Overall assessment: Changes are well-implemented and enhance functionalityThe modifications to this file, including the new import, updates to
computedFilteredParams, and the addition ofgetShouldClearIssues, are well-implemented and consistent with the existing code structure. These changes enhance the issue filtering capabilities by incorporating layout considerations.The suggestions provided in the review comments are minor and aimed at improving consistency and maintainability. Overall, the changes effectively support the PR objective of transitioning to
wa-sqlite-libraryby accommodating new layout-related functionality.web/core/local-db/worker/wa-sqlite/src/types/globals.d.ts (1)
13-21: 'declare var' is appropriate in.d.tsfiles; linter warnings may be false positivesThe linter reports warnings about using
varand suggestsletorconst. In TypeScript declaration files (.d.ts), it's standard to usedeclare varwhen declaring global variables. These linter warnings may be false positives due to the context. Consider updating the linter configuration to correctly handle declaration files.🧰 Tools
🪛 GitHub Check: lint-web
[failure] 13-13:
Unexpected var, use let or const instead
[failure] 14-14:
Unexpected var, use let or const instead
[failure] 15-15:
Unexpected var, use let or const instead
[failure] 16-16:
Unexpected var, use let or const instead
[failure] 17-17:
Unexpected var, use let or const instead
[failure] 18-18:
Unexpected var, use let or const instead
[failure] 19-19:
Unexpected var, use let or const instead
[failure] 20-20:
Unexpected var, use let or const instead
[failure] 21-21:
Unexpected var, use let or const insteadweb/core/local-db/worker/wa-sqlite/src/VFS.js (1)
213-222: Confirm correct calculation of 'FILE_TYPE_MASK'Ensure that the
FILE_TYPE_MASKis correctly combining all file type constants using the bitwise OR operation. Verify that the initial value in thereducefunction produces the desired result.Run the following script to check the calculated value:
web/core/store/issue/draft/filter.store.ts (1)
36-36: LGTMThe
getIssueFiltersmethod is appropriately added to theIDraftIssuesFilterinterface, ensuring consistency and enhancing functionality.web/core/store/issue/archived/filter.store.ts (3)
36-36: Good addition ofgetIssueFiltersmethod to the interfaceAdding the
getIssueFiltersmethod enhances the interfaceIArchivedIssuesFilter, allowing consistent retrieval of issue filters based onprojectId.
Line range hint
154-192: Verify consistency betweendisplayFiltersandkanbanFiltersIn the
updateFiltersmethod under theDISPLAY_FILTERScase, changes todisplayFiltersmight impact the state ofkanbanFilters. Specifically, whengroup_byorsub_group_byare updated or set tonull, consider whether corresponding updates tokanbanFiltersare necessary to maintain consistent filtering behavior.Ensure that any changes to grouping in
displayFiltersare appropriately reflected inkanbanFiltersif they are related. This verification will help prevent any unintended discrepancies between display and kanban filters.
Line range hint
203-216: Assess the necessity ofcurrentUserIdwhen updatingkanbanFiltersIn the
KANBAN_FILTERScase, the update to local filters is conditional on the presence ofcurrentUserId:const currentUserId = this.rootIssueStore.currentUserId; if (currentUserId) this.handleIssuesLocalFilters.set(EIssuesStoreType.ARCHIVED, type, workspaceSlug, projectId, undefined, { kanban_filters: _filters.kanbanFilters, });Consider whether
currentUserIdis required for storingkanbanFilters. IfcurrentUserIdmight be undefined (e.g., for anonymous users or during initial application load), and you still need to updatekanbanFilters, you may want to remove this condition or handle the undefined case appropriately.web/core/store/issue/project/filter.store.ts (1)
34-34: Addition ofgetIssueFiltersMethod Is AppropriateThe method
getIssueFilters(projectId: string): IIssueFilters | undefined;is correctly added to theIProjectIssuesFilterinterface. It enhances the filter retrieval functionality and is consistent with existing interface methods.web/core/store/issue/module/filter.store.ts (1)
34-34: MethodgetIssueFilterscorrectly added to interfaceAdding the
getIssueFilters(moduleId: string): IIssueFilters | undefinedmethod to theIModuleIssuesFilterinterface ensures that all implementations of the interface explicitly define this method, enhancing type safety and consistency across the codebase.web/core/local-db/storage.sqlite.ts (2)
23-24: Evaluate the impact of reducingPAGE_SIZEandBATCH_SIZEThe constants
PAGE_SIZEandBATCH_SIZEhave been reduced from 1000 to 500. This change might affect performance due to an increased number of batches when syncing data. Ensure that this adjustment aligns with performance requirements and does not negatively impact data handling efficiency.
386-386: Verify handling of updatedgetOptionmethod signatureThe
getOptionmethod now accepts an optionalfallbackparameter of typesstring | boolean | number. Ensure that all invocations ofgetOptionhandle the different possible return types (string | boolean | number | undefined) to prevent type-related issues.web/core/local-db/worker/wa-sqlite/src/FacadeVFS.js (1)
506-508: Verify 64-bit integer handling indelegalizefunctionThe
delegalizefunction converts two 32-bit integers into a 64-bit integer using JavaScript'sNumbertype. SinceNumbercan safely represent integers up to2^53 - 1, there may be precision issues with very large integers.Ensure that the function handles all possible 64-bit values accurately. If necessary, consider using
BigIntfor precise 64-bit arithmetic.web/core/local-db/worker/wa-sqlite/src/OPFSCoopSyncVFS.js (2)
554-556: EnsurehandleLockReleaseris called correctly in#releaseAccessHandleIn the
#releaseAccessHandlemethod, there is a possibility thatfile.persistentFile.handleLockReleaserisnull. Ensure that you check for its existence before calling it to prevent potential runtime errors.
374-384: Handle potential absence ofaccessHandleinjFileSizeIn the
jFileSizemethod, ensure thataccessHandleis available before callingaccessHandle.getSize(). IfaccessHandleisnull, callinggetSize()will throw an error.web/core/local-db/worker/wa-sqlite/src/sqlite-api.js (1)
849-864: Handle retry logic cautiously to prevent infinite loopsIn the
retryfunction, ifModule.retryOpskeeps populating new operations due to persistent failures, it could lead to an infinite loop. Ensure there's a maximum retry limit or a condition to break out of the loop after a certain number of attempts.Consider adding a retry limit or a break condition to prevent infinite retries.
web/core/local-db/worker/wa-sqlite/src/types/index.d.ts (1)
1009-1015: Verify Constant Values for Index ConstraintsOn lines 1009-1015, the constants
SQLITE_INDEX_CONSTRAINT_LIKE,SQLITE_INDEX_CONSTRAINT_GLOB,SQLITE_INDEX_CONSTRAINT_REGEXP,SQLITE_INDEX_CONSTRAINT_NE,SQLITE_INDEX_CONSTRAINT_ISNOT,SQLITE_INDEX_CONSTRAINT_ISNOTNULL, andSQLITE_INDEX_CONSTRAINT_ISNULLare assigned specific integer values. Please verify these values against the official SQLite documentation to ensure they are correct and up-to-date.Run the following script to confirm the constant values:
✅ Verification successful
All Index Constraint Constants Verified Correctly
The
SQLITE_INDEX_CONSTRAINT_*constants on lines 1009-1015 match the official SQLite documentation values.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that SQLITE_INDEX_CONSTRAINT_* constants match official SQLite values. # Expected values from SQLite documentation. declare -A expected_values=( ["SQLITE_INDEX_CONSTRAINT_LIKE"]=65 ["SQLITE_INDEX_CONSTRAINT_GLOB"]=66 ["SQLITE_INDEX_CONSTRAINT_REGEXP"]=67 ["SQLITE_INDEX_CONSTRAINT_NE"]=68 ["SQLITE_INDEX_CONSTRAINT_ISNOT"]=69 ["SQLITE_INDEX_CONSTRAINT_ISNOTNULL"]=70 ["SQLITE_INDEX_CONSTRAINT_ISNULL"]=71 ) # Search the codebase for the constant definitions. for constant in "${!expected_values[@]}"; do value=$(grep -oP "${constant}: \K[0-9]+" web/core/local-db/worker/wa-sqlite/src/types/index.d.ts) if [ "$value" != "${expected_values[$constant]}" ]; then echo "Mismatch for $constant: expected ${expected_values[$constant]}, found $value" else echo "$constant is correct." fi doneLength of output: 2271
| | "issue_type" | ||
| | "layout"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more specific type for "layout"
While the addition of "layout" to TIssueParams is consistent with its presence in IIssueDisplayFilterOptions, consider using a more specific type instead of a string literal. This could improve type safety and maintainability.
Consider the following changes:
- For TIssueParams:
- | "layout";
+ | "layout" | EIssueLayoutTypes;- For IIssueDisplayFilterOptions:
- layout?: EIssueLayoutTypes;
+ layout?: TIssueLayouts;This change would ensure that only valid layout types are used and maintain consistency with the existing TIssueLayouts type.
Also applies to: 107-109
| // await persistence.db.exec("BEGIN TRANSACTION;"); | ||
| batch.forEach((label: any) => { | ||
| stageInserts("labels", labelSchema, label); | ||
| }); | ||
| await persistence.db.exec("COMMIT;"); | ||
| // await persistence.db.exec("COMMIT;"); |
There was a problem hiding this comment.
💡 Codebase verification
Potential Data Consistency Issue due to Removed Transaction Management
The transaction management code in load-workspace.ts has been commented out. However, the wa-sqlite-library does not appear to be in use, which means data consistency may be at risk.
- Action Required:
- Ensure that
wa-sqlite-libraryis properly integrated and handles transactions. - If not, reinstate transaction management or implement appropriate error handling mechanisms.
- Ensure that
🔗 Analysis chain
Verify the removal of transaction management
The transaction management code has been commented out. While this aligns with the switch to wa-sqlite-library, it's crucial to ensure data consistency is maintained.
Please confirm if this change is intentional and if wa-sqlite-library handles transactions differently. If not, consider one of the following alternatives:
- Implement transaction management using
wa-sqlite-library's API. - Implement error handling to ensure data consistency in case of failures.
To verify the wa-sqlite-library usage, run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for wa-sqlite-library import and usage
rg -i 'wa-sqlite-library'
Length of output: 127338
web/core/local-db/worker/db.ts
Outdated
| @@ -0,0 +1,77 @@ | |||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | |||
There was a problem hiding this comment.
Avoid disabling ESLint rules; prefer explicit typing over 'any'.
Disabling the @typescript-eslint/no-explicit-any rule can hide potential type safety issues. It's better to define explicit types for variables and function return types to leverage TypeScript's type-checking capabilities.
web/core/local-db/worker/db.ts
Outdated
| const mergeToObject = (columns: string[], row: any[]) => { | ||
| const obj: any = {}; | ||
| columns.forEach((column, index) => { | ||
| obj[column] = row[index]; | ||
| }); | ||
| return obj; | ||
| }; | ||
| export class DBClass { | ||
| instance: any = {}; | ||
| sqlite3: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define explicit types instead of using 'any'.
Using any bypasses TypeScript's type checking, potentially leading to runtime errors. Consider defining proper interfaces or types for instance, sqlite3, obj, and function parameters to ensure type safety and improve code readability.
| runQuery(sql: string) { | ||
| return this.instance.exec(sql); | ||
| } |
There was a problem hiding this comment.
Potential SQL injection vulnerability when executing raw SQL strings.
The runQuery method and this.instance.exec execute raw SQL strings without parameter binding. If any user input is concatenated into the sql string, this could expose the application to SQL injection attacks. It's recommended to use parameterized queries and bind parameters to prevent SQL injection.
Proposed Fix:
Modify the runQuery method to accept bind parameters and use the exec method with parameter binding.
- runQuery(sql: string) {
- return this.instance.exec(sql);
- }
+ runQuery(sql: string, bind?: any[]) {
+ return this.exec({ sql, bind });
+ }Ensure that all SQL execution uses parameterized queries with proper binding to enhance security.
Also applies to: 30-41
| return async function (db) { | ||
| verifyDatabase(db); | ||
| const result = await f(db); | ||
| databases.delete(db); | ||
| return check(fname, result, db); | ||
| }; |
There was a problem hiding this comment.
Ensure resources are properly released after successful operations
In sqlite3.close, the database handle is removed from the databases set before verifying the close operation's success. If f(db) fails, the database remains open, but it's no longer tracked, leading to potential resource leaks or inconsistent states.
Apply this diff to delete the database handle only after confirming a successful closure:
verifyDatabase(db);
const result = await f(db);
- databases.delete(db);
return check(fname, result, db);
+ databases.delete(db);📝 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.
| return async function (db) { | |
| verifyDatabase(db); | |
| const result = await f(db); | |
| databases.delete(db); | |
| return check(fname, result, db); | |
| }; | |
| return async function (db) { | |
| verifyDatabase(db); | |
| const result = await f(db); | |
| return check(fname, result, db); | |
| databases.delete(db); | |
| }; |
| const lo32 = sqlite3.column_int(stmt, iCol); | ||
| const hi32 = Module.getTempRet0(); |
There was a problem hiding this comment.
Wrap variable declarations inside switch cases to prevent scope leakage
Declaring variables directly within a case clause can cause them to be accessible in other cases due to JavaScript's function-level scoping in switch statements. Wrap the variable declarations in a block { ... } to restrict their scope to the intended case.
Apply this diff to encapsulate the variable declarations:
case SQLite.SQLITE_INTEGER:
+ {
const lo32 = sqlite3.column_int(stmt, iCol);
const hi32 = Module.getTempRet0();
return cvt32x2AsSafe(lo32, hi32);
+ }Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Biome
[error] 269-269: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 270-270: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
| // Combine the upper and lower 32-bit numbers. The complication is | ||
| // that lo32 is a signed integer which makes manipulating its bits | ||
| // a little tricky - the sign bit gets handled separately. | ||
| return hi32 * 0x100000000 + (lo32 & 0x7fffffff) - (lo32 & 0x80000000); |
There was a problem hiding this comment.
Use BigInt literals for large numeric values to prevent precision loss
The numeric literal 0x100000000 exceeds the Number.MAX_SAFE_INTEGER limit and may lead to precision loss in calculations. Consider using BigInt literals to ensure accurate arithmetic operations with large integers.
Apply this diff to use BigInt literals:
- return hi32 * 0x100000000 + (lo32 & 0x7fffffff) - (lo32 & 0x80000000);
+ return BigInt(hi32) * 0x100000000n + BigInt(lo32 & 0x7fffffff) - BigInt(lo32 & 0x80000000);Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 77-77: web/core/local-db/worker/wa-sqlite/src/sqlite-api.js#L77
The numeric literal '0x100000000' will have at different value at runtime.
| ): number|Promise<number>; | ||
|
|
||
| /** @see https://sqlite.org/c3ref/io_methods.html */ | ||
| xTruncate(fileId: number, iSizeLo: number, iSizeHi): number|Promise<number>; |
There was a problem hiding this comment.
Missing Type Annotation for iSizeHi Parameter
The parameter iSizeHi in the xTruncate method on line 65 lacks a type annotation. It should be specified as number to maintain type safety.
Apply this diff to fix the missing type annotation:
- xTruncate(fileId: number, iSizeLo: number, iSizeHi): number|Promise<number>;
+ xTruncate(fileId: number, iSizeLo: number, iSizeHi: number): number|Promise<number>;📝 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.
| xTruncate(fileId: number, iSizeLo: number, iSizeHi): number|Promise<number>; | |
| xTruncate(fileId: number, iSizeLo: number, iSizeHi: number): number|Promise<number>; |
| * @param handler | ||
| * @param userData | ||
| */ | ||
| progress_handler(db: number, nProgressOps: number, handler: (userData: any) => number|Promise<number>, userData); |
There was a problem hiding this comment.
Missing Type Annotations and Return Type in progress_handler Method
In the progress_handler method on line 591, the parameter userData lacks a type annotation, and the method's return type is not specified. For better type safety and clarity, please annotate the parameter and specify the return type.
Apply this diff to fix the type annotations:
- progress_handler(db: number, nProgressOps: number, handler: (userData: any) => number|Promise<number>, userData);
+ progress_handler(db: number, nProgressOps: number, handler: (userData: any) => number|Promise<number>, userData: any): 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.
| progress_handler(db: number, nProgressOps: number, handler: (userData: any) => number|Promise<number>, userData); | |
| progress_handler(db: number, nProgressOps: number, handler: (userData: any) => number|Promise<number>, userData: any): void; |
Fix clear OPFS on safari
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (7)
web/core/local-db/worker/db.ts (1)
75-75: Consider adding proper worker termination handling.When exposing the class with Comlink, it's important to handle worker termination properly to ensure resources are cleaned up.
+// Handle worker termination +if (typeof self !== 'undefined') { + self.addEventListener('unload', async () => { + const db = new DBClass(); + await db.close(); + }); +} Comlink.expose(DBClass);web/core/local-db/utils/load-issues.ts (5)
12-16: Consider adding error handling for database operations.While the changes look good, the transaction operations could benefit from try-catch blocks to handle potential database errors gracefully.
Consider wrapping the database operations in a try-catch block:
export const addIssue = async (issue: any) => { if (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return; + try { await persistence.db.exec("BEGIN TRANSACTION;"); stageIssueInserts(issue); await persistence.db.exec("COMMIT;"); + } catch (error) { + await persistence.db.exec("ROLLBACK;"); + throw error; + } };
Line range hint
41-50: Make transaction operations async/await consistent.Transaction operations should be async/await for consistency with other functions and to prevent potential race conditions.
Apply this change:
export const deleteIssueFromLocal = async (issue_id: any) => { if (!rootStore.user.localDBEnabled || !persistence.db) return; const deleteQuery = `delete from issues where id='${issue_id}'`; const deleteMetaQuery = `delete from issue_meta where issue_id='${issue_id}'`; - persistence.db.exec("BEGIN TRANSACTION;"); - persistence.db.exec(deleteQuery); - persistence.db.exec(deleteMetaQuery); - persistence.db.exec("COMMIT;"); + try { + await persistence.db.exec("BEGIN TRANSACTION;"); + await persistence.db.exec(deleteQuery); + await persistence.db.exec(deleteMetaQuery); + await persistence.db.exec("COMMIT;"); + } catch (error) { + await persistence.db.exec("ROLLBACK;"); + throw error; + } };
Line range hint
53-58: Wrap update operation in a transaction.The update operation should be wrapped in a transaction to ensure atomicity of the delete and insert operations.
Consider this implementation:
export const updateIssue = async (issue: TIssue & { is_local_update: number }) => { if (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return; - const issue_id = issue.id; - // delete the issue and its meta data - await deleteIssueFromLocal(issue_id); - addIssue(issue); + try { + await persistence.db.exec("BEGIN TRANSACTION;"); + await deleteIssueFromLocal(issue.id); + await addIssue(issue); + await persistence.db.exec("COMMIT;"); + } catch (error) { + await persistence.db.exec("ROLLBACK;"); + throw error; + } };
Line range hint
62-68: Fix async operation in map.Similar to the forEach issue earlier, using async/await inside map doesn't wait for the promises to resolve.
Replace the map with Promise.all:
if (Array.isArray(response)) { - response.map(async (issue) => deleteIssueFromLocal(issue)); + await Promise.all(response.map((issue) => deleteIssueFromLocal(issue))); }
Line range hint
71-103: Fix SQL injection vulnerability and use consistent parameterization.The main insert query is vulnerable to SQL injection through direct string interpolation. Also, there's inconsistent use of parameterized queries between the main insert and meta operations.
Consider using parameterized queries consistently:
- const columns = "'" + Object.keys(sanitizedIssue).join("','") + "'"; - const values = Object.values(sanitizedIssue) - .map((value) => { - if (value === null) { - return ""; - } - if (typeof value === "object") { - return `'${JSON.stringify(value).replace(/'/g, "''")}'`; - } - if (typeof value === "string") { - return `'${value.replace(/'/g, "''")}'`; - } - return value; - }) - .join(", "); - - const query = `INSERT OR REPLACE INTO issues (${columns}) VALUES (${values});`; - persistence.db.exec(query); + const columns = Object.keys(sanitizedIssue); + const placeholders = columns.map(() => '?').join(','); + const values = Object.values(sanitizedIssue).map(value => + value === null ? '' : + typeof value === 'object' ? JSON.stringify(value) : + value + ); + + await persistence.db.exec({ + sql: `INSERT OR REPLACE INTO issues (${columns.join(',')}) VALUES (${placeholders})`, + bind: values + });web/core/local-db/utils/utils.ts (1)
17-18: Consider adding context to error logging.The error logging implementation is good, but could be enhanced with additional context.
Consider adding more context to help with debugging:
-console.error(e); -captureException(e); +console.error('[SQLite Error]', e); +captureException(e, { tags: { source: 'sqlite' } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- web/core/local-db/storage.sqlite.ts (11 hunks)
- web/core/local-db/utils/load-issues.ts (5 hunks)
- web/core/local-db/utils/utils.ts (3 hunks)
- web/core/local-db/worker/db.ts (1 hunks)
- web/package.json (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/package.json
🧰 Additional context used
🔇 Additional comments (5)
web/core/local-db/utils/load-issues.ts (1)
7-7: LGTM! Good addition of logging utility.The addition of the log utility enhances observability for database operations.
web/core/local-db/utils/utils.ts (2)
1-1: LGTM! Improved import statement.The change to a named import is more efficient and follows modern JavaScript/TypeScript best practices.
Line range hint
1-168: Verify database migration implementation.Since this file is part of the database migration from
sqlite-wasamtowa-sqlite, we should verify the implementation across all related files.✅ Verification successful
Based on the shell script results, I can now provide a final verification response:
Database migration implementation is consistent and properly handled
The verification results show:
- No references to
sqlite-wasamwere found in the codebase, confirming complete migration- Error handling is consistently implemented across local-db:
- Proper error logging using
logErrorutility- Integration with Sentry for error tracking via
captureException- OPFS (Origin Private File System) implementation is correctly structured:
- Clean OPFS management through
clearOPFSutility- Proper file system access handling in
wa-sqliteimplementation- Consistent directory and file handle management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to sqlite-wasam echo "Checking for sqlite-wasam references..." rg -l "sqlite-wasam" # Check for consistent error handling pattern echo "Checking error handling consistency in local-db..." rg -C 3 "logError|captureException" web/core/local-db/ # Check for OPFS usage echo "Checking OPFS usage..." rg -C 3 "getDirectory|FileSystem" web/core/local-db/Length of output: 10075
web/core/local-db/storage.sqlite.ts (2)
Line range hint
384-393: Protect getOption from SQL injectionThe getOption method is vulnerable to SQL injection attacks.
[security]- getOption = async (key: string, fallback?: string | boolean | number) => { - try { - const options = await runQuery(`select * from options where key='${key}'`); - if (options.length) { - return options[0].value; - } - - return fallback; - } catch (e) { - return fallback; - } - }; + getOption = async (key: string, fallback?: string | boolean | number) => { + try { + // Use parameterized query to prevent SQL injection + const options = await this.safeQuery( + 'SELECT * FROM options WHERE key = ?', + [key] + ); + if (options.length) { + return options[0].value; + } + return fallback; + } catch (e) { + return fallback; + } + };
23-24: Verify the impact of reduced batch sizesThe PAGE_SIZE and BATCH_SIZE constants have been reduced from 1000 to 500. While smaller batches can improve memory usage, they might increase the number of database operations.
✅ Verification successful
Based on the search results, I can make a final assessment. The codebase shows consistent usage of batch sizes across different modules:
- In
web/core/local-db/utils/load-workspace.ts, all loading functions (loadLabels, loadModules, loadCycles, etc.) use a defaultbatchSize = 500- The only exception is in
web/core/local-db/utils/load-issues.tswhereaddIssuesBulkuses a smaller batch size of 100- The performance measurements in the code suggest that the codebase already has instrumentation to monitor the impact of these operations
The batch size reduction is aligned with existing patterns
The reduction of PAGE_SIZE and BATCH_SIZE from 1000 to 500 in
storage.sqlite.tsis actually consistent with the batch sizes used throughout the codebase, particularly in the workspace loading utilities. The smaller batch size of 500 appears to be a standard choice across the application, suggesting it's a proven value for balancing memory usage and performance.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any performance-related comments or issues in the codebase rg -i "performance|slow|batch.*size|page.*size" --type tsLength of output: 4962
| const mergeToObject = (columns: string[], row: any[]) => { | ||
| const obj: any = {}; | ||
| columns.forEach((column, index) => { | ||
| obj[column] = row[index]; | ||
| }); | ||
| return obj; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety of the mergeToObject helper function.
The function can be made more type-safe by using generics and proper typing.
-const mergeToObject = (columns: string[], row: any[]) => {
- const obj: any = {};
+const mergeToObject = <T extends Record<string, unknown>>(columns: string[], row: unknown[]): T => {
+ const obj = {} as T;
columns.forEach((column, index) => {
obj[column] = row[index];
});
return obj;
};📝 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 mergeToObject = (columns: string[], row: any[]) => { | |
| const obj: any = {}; | |
| columns.forEach((column, index) => { | |
| obj[column] = row[index]; | |
| }); | |
| return obj; | |
| }; | |
| const mergeToObject = <T extends Record<string, unknown>>(columns: string[], row: unknown[]): T => { | |
| const obj = {} as T; | |
| columns.forEach((column, index) => { | |
| obj[column] = row[index]; | |
| }); | |
| return obj; | |
| }; |
| batch.forEach(async (issue: any) => { | ||
| if (!issue.type_id) { | ||
| issue.type_id = ""; | ||
| } | ||
| stageIssueInserts(issue); | ||
| await stageIssueInserts(issue); | ||
| }); |
There was a problem hiding this comment.
Fix async operation in forEach loop.
Using async/await inside forEach doesn't wait for the promises to resolve. This could lead to the COMMIT being executed before all inserts are complete.
Replace the forEach with a for...of loop:
- batch.forEach(async (issue: any) => {
- if (!issue.type_id) {
- issue.type_id = "";
- }
- await stageIssueInserts(issue);
- });
+ for (const issue of batch) {
+ if (!issue.type_id) {
+ issue.type_id = "";
+ }
+ await stageIssueInserts(issue);
+ }📝 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.
| batch.forEach(async (issue: any) => { | |
| if (!issue.type_id) { | |
| issue.type_id = ""; | |
| } | |
| stageIssueInserts(issue); | |
| await stageIssueInserts(issue); | |
| }); | |
| for (const issue of batch) { | |
| if (!issue.type_id) { | |
| issue.type_id = ""; | |
| } | |
| await stageIssueInserts(issue); | |
| } |
web/core/local-db/utils/utils.ts
Outdated
| export const clearOPFS = async () => { | ||
| const storageManager = window.navigator.storage; | ||
| const fileSystemDirectoryHandle = await storageManager.getDirectory(); | ||
| const entries = await (fileSystemDirectoryHandle as any).entries(); | ||
| for await (const entry of entries) { | ||
| const [name] = entry; | ||
| try { | ||
| await fileSystemDirectoryHandle.removeEntry(name); | ||
| } catch (e) { | ||
| console.log("Error", e); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Several improvements needed for the clearOPFS function.
The function needs enhancement in several areas:
- Error handling should be more robust
- Types should be properly defined
- Function should indicate success/failure
- Documentation is missing
- Resource cleanup could be improved
Here's a suggested implementation:
+/**
+ * Clears all entries from the Origin Private File System.
+ * Used during database migration from sqlite-wasam to wa-sqlite.
+ * @returns Promise<boolean> - true if successful, false if any errors occurred
+ */
export const clearOPFS = async () => {
+ let hasErrors = false;
const storageManager = window.navigator.storage;
- const fileSystemDirectoryHandle = await storageManager.getDirectory();
+ const fileSystemDirectoryHandle = await storageManager.getDirectory() as FileSystemDirectoryHandle;
const entries = await (fileSystemDirectoryHandle as any).entries();
+
+ try {
for await (const entry of entries) {
const [name] = entry;
try {
await fileSystemDirectoryHandle.removeEntry(name);
} catch (e) {
- console.log("Error", e);
+ hasErrors = true;
+ logError(e); // Use existing error logging utility
}
}
+ } finally {
+ // Ensure iterator is closed in case of errors
+ if (entries.return) await entries.return();
+ }
+
+ return !hasErrors;
};📝 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 clearOPFS = async () => { | |
| const storageManager = window.navigator.storage; | |
| const fileSystemDirectoryHandle = await storageManager.getDirectory(); | |
| const entries = await (fileSystemDirectoryHandle as any).entries(); | |
| for await (const entry of entries) { | |
| const [name] = entry; | |
| try { | |
| await fileSystemDirectoryHandle.removeEntry(name); | |
| } catch (e) { | |
| console.log("Error", e); | |
| } | |
| } | |
| }; | |
| /** | |
| * Clears all entries from the Origin Private File System. | |
| * Used during database migration from sqlite-wasam to wa-sqlite. | |
| * @returns Promise<boolean> - true if successful, false if any errors occurred | |
| */ | |
| export const clearOPFS = async () => { | |
| let hasErrors = false; | |
| const storageManager = window.navigator.storage; | |
| const fileSystemDirectoryHandle = await storageManager.getDirectory() as FileSystemDirectoryHandle; | |
| const entries = await (fileSystemDirectoryHandle as any).entries(); | |
| try { | |
| for await (const entry of entries) { | |
| const [name] = entry; | |
| try { | |
| await fileSystemDirectoryHandle.removeEntry(name); | |
| } catch (e) { | |
| hasErrors = true; | |
| logError(e); // Use existing error logging utility | |
| } | |
| } | |
| } finally { | |
| // Ensure iterator is closed in case of errors | |
| if (entries.return) await entries.return(); | |
| } | |
| return !hasErrors; | |
| }; |
| getIsWriteInProgress = async (op: string) => { | ||
| const writeStartTime = await this.getOption(op, false); | ||
| if (writeStartTime) { | ||
| // Check if it has been more than 5seconds | ||
| const current = new Date(); | ||
| const start = new Date(writeStartTime); | ||
|
|
||
| if (current.getTime() - start.getTime() < 5000) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Extract timeout configuration and add SQL injection protection
The write operation timeout is hardcoded and the SQL queries are vulnerable to injection.
+ private static readonly WRITE_TIMEOUT_MS = 5000;
+
getIsWriteInProgress = async (op: string) => {
const writeStartTime = await this.getOption(op, false);
if (writeStartTime) {
- // Check if it has been more than 5seconds
const current = new Date();
const start = new Date(writeStartTime);
- if (current.getTime() - start.getTime() < 5000) {
+ if (current.getTime() - start.getTime() < Storage.WRITE_TIMEOUT_MS) {
return true;
}
return false;
}
return false;
};[security]
Consider using parameterized queries to prevent SQL injection. Example:
// Add a helper method for safe query execution
private async safeQuery(query: string, params: any[]) {
// This is a pseudo-implementation. The actual implementation
// depends on the wa-sqlite API for parameterized queries
return await this.db.exec(query, params);
}
// Use it in methods
deleteOption = async (key: string) => {
await this.safeQuery('DELETE FROM options WHERE key = ?', [key]);
};
web/core/local-db/storage.sqlite.ts
Outdated
| if (!rootStore.user.localDBEnabled) return; | ||
| const syncInProgress = await this.getIsWriteInProgress("sync_workspace"); | ||
| if (syncInProgress) { | ||
| log("Sync in progress, skipping"); | ||
| } | ||
| await startSpan({ name: "LOAD_WS", attributes: { slug: this.workspaceSlug } }, async () => { | ||
| this.setOption("sync_workspace", new Date().toUTCString()); | ||
| await loadWorkSpaceData(this.workspaceSlug); | ||
| this.deleteOption("sync_workspace"); |
There was a problem hiding this comment.
Add missing return statement and improve error handling
The sync operation needs a few improvements:
- Missing return statement after detecting ongoing sync
- No error handling for the sync operation
if (!rootStore.user.localDBEnabled) return;
const syncInProgress = await this.getIsWriteInProgress("sync_workspace");
if (syncInProgress) {
log("Sync in progress, skipping");
+ return;
}
- await startSpan({ name: "LOAD_WS", attributes: { slug: this.workspaceSlug } }, async () => {
- this.setOption("sync_workspace", new Date().toUTCString());
- await loadWorkSpaceData(this.workspaceSlug);
- this.deleteOption("sync_workspace");
- });
+ try {
+ await startSpan({ name: "LOAD_WS", attributes: { slug: this.workspaceSlug } }, async () => {
+ this.setOption("sync_workspace", new Date().toUTCString());
+ await loadWorkSpaceData(this.workspaceSlug);
+ this.deleteOption("sync_workspace");
+ });
+ } catch (error) {
+ this.deleteOption("sync_workspace"); // Ensure cleanup on error
+ throw error;
+ }📝 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 (!rootStore.user.localDBEnabled) return; | |
| const syncInProgress = await this.getIsWriteInProgress("sync_workspace"); | |
| if (syncInProgress) { | |
| log("Sync in progress, skipping"); | |
| } | |
| await startSpan({ name: "LOAD_WS", attributes: { slug: this.workspaceSlug } }, async () => { | |
| this.setOption("sync_workspace", new Date().toUTCString()); | |
| await loadWorkSpaceData(this.workspaceSlug); | |
| this.deleteOption("sync_workspace"); | |
| if (!rootStore.user.localDBEnabled) return; | |
| const syncInProgress = await this.getIsWriteInProgress("sync_workspace"); | |
| if (syncInProgress) { | |
| log("Sync in progress, skipping"); | |
| return; | |
| } | |
| try { | |
| await startSpan({ name: "LOAD_WS", attributes: { slug: this.workspaceSlug } }, async () => { | |
| this.setOption("sync_workspace", new Date().toUTCString()); | |
| await loadWorkSpaceData(this.workspaceSlug); | |
| this.deleteOption("sync_workspace"); | |
| }); | |
| } catch (error) { | |
| this.deleteOption("sync_workspace"); // Ensure cleanup on error | |
| throw error; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
web/core/local-db/utils/load-issues.ts (3)
Line range hint
42-51: Add missing awaits for transaction commands.Transaction commands should be awaited to ensure proper sequencing.
Apply this fix:
- persistence.db.exec("BEGIN TRANSACTION;"); - persistence.db.exec(deleteQuery); - persistence.db.exec(deleteMetaQuery); - persistence.db.exec("COMMIT;"); + await persistence.db.exec("BEGIN TRANSACTION;"); + await persistence.db.exec(deleteQuery); + await persistence.db.exec(deleteMetaQuery); + await persistence.db.exec("COMMIT;");
Line range hint
63-69: Fix async operation in map and improve typing.The current implementation has two issues:
- Using async callback in map doesn't wait for the promises to resolve
- The issue parameter lacks proper typing
Apply these fixes:
if (Array.isArray(response)) { - response.map(async (issue) => deleteIssueFromLocal(issue)); + await Promise.all(response.map((issue: string) => deleteIssueFromLocal(issue))); }
Based on the search results, I can now generate a final response as we have sufficient evidence of SQL query patterns in the codebase.
Security: SQL injection vulnerabilities found in multiple locations
The review comment's concern about SQL injection is valid and extends beyond the initially reviewed code. Found multiple instances of unsafe SQL query construction:
In
load-issues.ts:
INSERT OR REPLACE INTO issuesusing string concatenationDELETE from issue_meta where issue_id='${issue_id}'using template literalsdeleteMetaQueryusing string interpolationIn
load-workspace.ts:
INSERT OR REPLACE INTO ${table}using string concatenation and template literalsThe codebase shows inconsistent usage of SQL query patterns:
- Some queries use proper parameterization:
sql: "... values (?,?,?)", bind: [...]- Others use direct string concatenation or template literals
Recommendation:
- Use parameterized queries consistently across all SQL operations
- Replace string concatenation/template literals with parameterized queries
- Follow the pattern already used in issue_meta insertions:
persistence.db.exec({ sql: "INSERT OR REPLACE INTO table (col1, col2) VALUES (?, ?)", bind: [value1, value2] });🔗 Analysis chain
Line range hint
72-123: Security: Use parameterized queries to prevent SQL injection.The current implementation uses string concatenation for SQL queries which is vulnerable to SQL injection. While there is some escaping of single quotes, it's safer to use parameterized queries consistently, like you're already doing for issue_meta operations.
Let's verify if there are other instances of string concatenation in SQL queries:
Consider refactoring to use parameterized queries:
- const query = `INSERT OR REPLACE INTO issues (${columns}) VALUES (${values});`; - persistence.db.exec(query); + await persistence.db.exec({ + sql: "INSERT OR REPLACE INTO issues (${placeholders}) VALUES (${valueParams})", + bind: [/* array of values */] + });
Let me try a different approach to search for SQL query patterns in the codebase.
The previous regex might have been too specific. Let's broaden the search to find all SQL query patterns and exec calls to better understand the SQL usage patterns in the codebase. This will help us verify if there are other instances of potential SQL injection vulnerabilities and understand the common patterns used for database operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential SQL injection vulnerabilities rg -g '*.ts' "exec\(\s*[\`'\"].*\$\{.*\}" .Length of output: 45
Script:
#!/bin/bash # Search for SQL queries with string interpolation rg -g '*.ts' -A 2 -B 2 "INSERT.*INTO.*\$\{|DELETE.*FROM.*\$\{|UPDATE.*SET.*\$\{|SELECT.*FROM.*\$\{" . # Also search for direct exec calls rg -g '*.ts' -A 2 -B 2 "\.exec\(" . # Look for SQL-related files fd -e ts -e js sqlLength of output: 9907
web/core/local-db/utils/load-workspace.ts (3)
Line range hint
20-47: Consider implementing batch inserts for better performance.The current implementation performs individual INSERT statements for each record. For large datasets, this could be inefficient. Consider using batch inserts to reduce the number of database operations.
Here's a suggested optimization:
-const stageInserts = async (table: string, schema: Schema, data: any) => { +const stageInserts = async (table: string, schema: Schema, dataArray: any[]) => { const keys = Object.keys(schema); - // Pick only the keys that are in the schema - const filteredData = keys.reduce((acc: any, key) => { - if (data[key] || data[key] === 0) { - acc[key] = data[key]; - } - return acc; - }, {}); - const columns = "'" + Object.keys(filteredData).join("','") + "'"; - const values = Object.values(filteredData) - .map((value) => { + const columns = "'" + keys.join("','") + "'"; + + const valuesList = dataArray.map(data => { + const filteredData = keys.reduce((acc: any, key) => { + if (data[key] || data[key] === 0) { + acc[key] = data[key]; + } + return acc; + }, {}); + + return '(' + Object.values(filteredData).map(value => { if (value === null) { return ""; } if (typeof value === "object") { return `'${JSON.stringify(value).replace(/'/g, "''")}'`; } if (typeof value === "string") { return `'${value.replace(/'/g, "''")}'`; } return value; - }) - .join(", "); - const query = `INSERT OR REPLACE INTO ${table} (${columns}) VALUES (${values});`; + }).join(', ') + ')'; + }).join(',\n'); + + const query = `INSERT OR REPLACE INTO ${table} (${columns}) VALUES ${valuesList};`; await persistence.db.exec(query); };
Line range hint
108-109: Fix array concatenation in loadEstimatePoints.The
concatmethod returns a new array but doesn't modify the original array. This means no estimate points are being added to theobjectsarray.Apply this fix:
- objects.concat(estimate.points); + objects.push(...estimate.points);
Line range hint
136-146: Add error handling and transaction rollback.The function needs proper error handling to ensure database consistency. If any promise fails, the transaction should be rolled back.
Here's the suggested implementation:
export const loadWorkSpaceData = async (workspaceSlug: string) => { - persistence.db.exec("BEGIN TRANSACTION;"); - const promises = []; - promises.push(loadLabels(workspaceSlug)); - promises.push(loadModules(workspaceSlug)); - promises.push(loadCycles(workspaceSlug)); - promises.push(loadStates(workspaceSlug)); - promises.push(loadEstimatePoints(workspaceSlug)); - promises.push(loadMembers(workspaceSlug)); - await Promise.all(promises); - persistence.db.exec("COMMIT"); + try { + await persistence.db.exec("BEGIN TRANSACTION;"); + + const promises = [ + loadLabels(workspaceSlug), + loadModules(workspaceSlug), + loadCycles(workspaceSlug), + loadStates(workspaceSlug), + loadEstimatePoints(workspaceSlug), + loadMembers(workspaceSlug) + ]; + + await Promise.all(promises); + await persistence.db.exec("COMMIT;"); + } catch (error) { + await persistence.db.exec("ROLLBACK;"); + throw error; // Re-throw to allow caller to handle + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- web/core/local-db/utils/load-issues.ts (5 hunks)
- web/core/local-db/utils/load-workspace.ts (9 hunks)
- web/core/local-db/utils/utils.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/local-db/utils/utils.ts
🧰 Additional context used
🔇 Additional comments (2)
web/core/local-db/utils/load-issues.ts (2)
7-7: LGTM! Good addition of logging utility.The log utility will help with monitoring performance of database operations.
20-39: LGTM! Good improvements to bulk processing.The changes properly address the async/await issue from the past review and add helpful performance monitoring.
| if (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return; | ||
|
|
||
| persistence.db.exec("BEGIN TRANSACTION;"); | ||
| await persistence.db.exec("BEGIN TRANSACTION;"); | ||
| stageIssueInserts(issue); | ||
| persistence.db.exec("COMMIT;"); | ||
| await persistence.db.exec("COMMIT;"); |
There was a problem hiding this comment.
Add missing await for stageIssueInserts.
The transaction could commit before the insert is complete since stageIssueInserts is not awaited.
Apply this fix:
await persistence.db.exec("BEGIN TRANSACTION;");
- stageIssueInserts(issue);
+ await stageIssueInserts(issue);
await persistence.db.exec("COMMIT;");📝 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 (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return; | |
| persistence.db.exec("BEGIN TRANSACTION;"); | |
| await persistence.db.exec("BEGIN TRANSACTION;"); | |
| stageIssueInserts(issue); | |
| persistence.db.exec("COMMIT;"); | |
| await persistence.db.exec("COMMIT;"); | |
| if (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return; | |
| await persistence.db.exec("BEGIN TRANSACTION;"); | |
| await stageIssueInserts(issue); | |
| await persistence.db.exec("COMMIT;"); |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (6)
web/core/local-db/worker/db.ts (1)
45-70: Add transaction support and proper error handling in exec method.The
execmethod lacks transaction support and proper error handling for database operations.Consider implementing transaction support and error handling:
async exec(props: string | TQueryProps) { try { await this.instance.exec('BEGIN TRANSACTION'); try { const result = await this._executeQuery(props); await this.instance.exec('COMMIT'); return result; } catch (error) { await this.instance.exec('ROLLBACK'); throw error; } } catch (error) { throw new Error(`Database execution failed: ${error.message}`); } } private async _executeQuery(props: string | TQueryProps) { // Current exec implementation }This ensures:
- Atomic operations through transactions
- Proper rollback on failures
- Consistent error handling
web/core/local-db/utils/load-issues.ts (2)
Line range hint
41-49: Add error handling and make transaction operations async.The delete operation lacks proper error handling and doesn't await transaction operations.
Consider this safer implementation:
export const deleteIssueFromLocal = async (issue_id: any) => { if (!rootStore.user.localDBEnabled || !persistence.db) return; const deleteQuery = `delete from issues where id='${issue_id}'`; const deleteMetaQuery = `delete from issue_meta where issue_id='${issue_id}'`; - persistence.db.exec("BEGIN TRANSACTION;"); - persistence.db.exec(deleteQuery); - persistence.db.exec(deleteMetaQuery); - persistence.db.exec("COMMIT;"); + try { + await persistence.db.exec("BEGIN TRANSACTION;"); + await persistence.db.exec(deleteQuery); + await persistence.db.exec(deleteMetaQuery); + await persistence.db.exec("COMMIT;"); + } catch (error) { + await persistence.db.exec("ROLLBACK;"); + throw error; + } };
Line range hint
71-102: Critical: Use prepared statements to prevent SQL injection.The current implementation uses string concatenation for SQL query construction, which is vulnerable to SQL injection attacks. While this is local storage, it's still best practice to use prepared statements consistently.
Consider using prepared statements:
- const columns = "'" + Object.keys(sanitizedIssue).join("','") + "'"; - const values = Object.values(sanitizedIssue) - .map((value) => { - if (value === null) { - return ""; - } - if (typeof value === "object") { - return `'${JSON.stringify(value).replace(/'/g, "''")}'`; - } - if (typeof value === "string") { - return `'${value.replace(/'/g, "''")}'`; - } - return value; - }) - .join(", "); - const query = `INSERT OR REPLACE INTO issues (${columns}) VALUES (${values});`; - persistence.db.exec(query); + const columns = Object.keys(sanitizedIssue); + const placeholders = columns.map(() => '?').join(','); + const values = Object.values(sanitizedIssue).map(value => + value === null ? '' : + typeof value === 'object' ? JSON.stringify(value) : + value + ); + + await persistence.db.exec({ + sql: `INSERT OR REPLACE INTO issues (${columns.join(',')}) VALUES (${placeholders})`, + bind: values + });web/core/local-db/storage.sqlite.ts (3)
89-104: Enhance error handling in DB initializationWhile the worker initialization is more straightforward with Comlink, consider adding more specific error handling for different failure scenarios (e.g., worker creation failure, initialization failure).
const { DBClass } = await import("./worker/db"); -const MyWorker = Comlink.wrap<typeof DBClass>(new Worker(new URL("./worker/db.ts", import.meta.url))); +try { + const MyWorker = Comlink.wrap<typeof DBClass>(new Worker(new URL("./worker/db.ts", import.meta.url))); + this.workspaceSlug = workspaceSlug; + this.dbName = workspaceSlug; + const instance = await new MyWorker(); + await instance.init(workspaceSlug); + this.db = { + exec: instance.exec, + close: instance.close, + }; + this.status = "ready"; + await createTables(); + await this.setOption("DB_VERSION", DB_VERSION.toString()); + return true; +} catch (error) { + logError("Failed to initialize database worker:", error); + this.status = "error"; + return false; +}
Line range hint
384-394: Fix SQL injection vulnerability in getOptionThe
keyparameter is used directly in the SQL query without proper sanitization, which could lead to SQL injection attacks.- const options = await runQuery(`select * from options where key='${key}'`); + const options = await runQuery(`select * from options where key=?`, [key]);
411-424: Make write lock timeout configurableThe 5-second timeout for write operations is hard-coded. Consider making this configurable to accommodate different performance characteristics and requirements.
+const WRITE_LOCK_TIMEOUT_MS = 5000; getIsWriteInProgress = async (op: string) => { const writeStartTime = await this.getOption(op, false); if (writeStartTime) { const current = new Date(); const start = new Date(writeStartTime); - if (current.getTime() - start.getTime() < 5000) { + if (current.getTime() - start.getTime() < WRITE_LOCK_TIMEOUT_MS) { return true; } return false; } return false; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- web/core/local-db/storage.sqlite.ts (11 hunks)
- web/core/local-db/utils/load-issues.ts (5 hunks)
- web/core/local-db/utils/utils.ts (3 hunks)
- web/core/local-db/worker/db.ts (1 hunks)
- web/package.json (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/package.json
🧰 Additional context used
🔇 Additional comments (3)
web/core/local-db/utils/utils.ts (2)
1-1: LGTM: Improved error logging implementation.The changes improve the code by:
- Using named imports for better tree-shaking
- Using
console.errorwhich is more appropriate for error loggingAlso applies to: 17-18
Line range hint
1-168: Verify SQLite error handling compatibility with wa-sqlite.The
parseSQLite3Errorfunction and error handling inlogErrorassume specific error structures. Let's verify compatibility with the new SQLite library.✅ Verification successful
Let me analyze the SQLite error handling in the codebase to verify compatibility with wa-sqlite.
Let me analyze the error handling patterns found in both libraries to determine compatibility.
From the search results, I can see:
- Current SQLite3 error handling in utils.ts:
if (e?.result?.errorClass === "SQLite3Error") { e = parseSQLite3Error(e); } console.error(e); captureException(e);
- wa-sqlite error handling:
export class SQLiteError extends Error { constructor(message, code) { super(message); this.code = code; } }The error handling is compatible because:
- The current code checks for
SQLite3Errorclass name and parses it, while wa-sqlite usesSQLiteErrorclass with standard error code pattern- Both approaches ultimately result in proper error objects that can be caught and logged
- The
parseSQLite3Error()function simply stringifies the result, which is compatible with wa-sqlite's error handling that already provides formatted messagesThe error handling is compatible and requires no changes. The current error handling will work with both libraries since:
- The error check is loose (
e?.result?.errorClass)- The error parsing is generic (just stringifies the result)
- The error logging uses standard console.error and Sentry capture
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SQLite error handling patterns across the codebase echo "Checking SQLite error class references:" rg "SQLite3Error|sqlite3Error" -A 5 echo "\nChecking wa-sqlite error handling examples:" rg "wa-sqlite.*error" -iLength of output: 124251
Script:
#!/bin/bash # Search for wa-sqlite error handling patterns echo "Checking wa-sqlite error handling:" rg "wa-sqlite.*error|sqlite.*error" -A 3 web/core/local-db/worker/ # Search for SQLite error handling in the codebase echo "\nChecking SQLite error handling in local-db:" rg "SQLite.*Error|sqlite.*error" -A 3 web/core/local-db/Length of output: 249852
web/core/local-db/storage.sqlite.ts (1)
23-24: Verify the impact of reduced batch sizesThe
PAGE_SIZEandBATCH_SIZEconstants have been reduced from 1000 to 500. While this might improve memory usage, it could potentially impact performance by requiring more round trips.
web/core/local-db/worker/db.ts
Outdated
| export class DBClass { | ||
| instance: any = {}; | ||
| sqlite3: any; | ||
| async init(dbName: string) { | ||
| const m = await SQLiteESMFactory(); | ||
| this.sqlite3 = SQLite.Factory(m); | ||
| const vfs = await MyVFS.create("hello", m); | ||
| this.sqlite3.vfs_register(vfs, true); | ||
| const db = await this.sqlite3.open_v2(`${dbName}.sqlite3`); | ||
| this.instance.db = db; | ||
| this.instance.exec = async (sql: string) => { | ||
| const rows: any[] = []; | ||
| await this.sqlite3.exec(db, sql, (row: any[], columns: string[]) => { | ||
| rows.push(mergeToObject(columns, row)); | ||
| }); | ||
|
|
||
| return rows; | ||
| }; | ||
| return true; | ||
| } | ||
|
|
||
| runQuery(sql: string) { | ||
| return this.instance.exec(sql); | ||
| } | ||
|
|
||
| async exec(props: string | TQueryProps) { | ||
| let sql: string, bind: any[]; | ||
| if (typeof props === "string") { | ||
| sql = props; | ||
| } else { | ||
| ({ sql, bind } = props); | ||
| if (bind) { | ||
| for await (const stmt of this.sqlite3.statements(this.instance.db, sql)) { | ||
| bind.forEach((b, i) => { | ||
| this.sqlite3.bind(stmt, i + 1, b); | ||
| }); | ||
|
|
||
| const rows = []; | ||
|
|
||
| do { | ||
| const columns = await this.sqlite3.column_names(stmt); | ||
| const row = await this.sqlite3.row(stmt); | ||
| rows.push(mergeToObject(columns, row)); | ||
| } while ((await this.sqlite3.step(stmt)) === SQLite.SQLITE_ROW); | ||
|
|
||
| return rows; | ||
| } | ||
| } | ||
| } | ||
| return this.instance.exec(sql); | ||
| } | ||
| async close() { | ||
| await this.sqlite3.close(this.instance.db); | ||
| } | ||
| } | ||
| Comlink.expose(DBClass); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety of the DBClass implementation.
The class uses any types which reduces type safety and IDE support.
Consider defining proper interfaces and types:
interface SQLiteInstance {
db: Database;
exec: (sql: string) => Promise<unknown[]>;
}
interface Database {
close(): Promise<void>;
// Add other necessary database methods
}
interface SQLite3Interface {
vfs_register(vfs: unknown, makeDefault: boolean): void;
open_v2(filename: string): Promise<Database>;
exec(db: Database, sql: string, callback: (row: unknown[], columns: string[]) => void): Promise<void>;
close(db: Database): Promise<void>;
// Add other necessary SQLite3 methods
}
export class DBClass {
private instance: SQLiteInstance = {} as SQLiteInstance;
private sqlite3!: SQLite3Interface;
// Rest of the implementation
}
web/core/local-db/worker/db.ts
Outdated
| async init(dbName: string) { | ||
| const m = await SQLiteESMFactory(); | ||
| this.sqlite3 = SQLite.Factory(m); | ||
| const vfs = await MyVFS.create("hello", m); | ||
| this.sqlite3.vfs_register(vfs, true); | ||
| const db = await this.sqlite3.open_v2(`${dbName}.sqlite3`); | ||
| this.instance.db = db; | ||
| this.instance.exec = async (sql: string) => { | ||
| const rows: any[] = []; | ||
| await this.sqlite3.exec(db, sql, (row: any[], columns: string[]) => { | ||
| rows.push(mergeToObject(columns, row)); | ||
| }); | ||
|
|
||
| return rows; | ||
| }; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Add proper error handling and cleanup for database initialization.
The init method lacks error handling and cleanup in case of failures.
Consider implementing proper error handling:
async init(dbName: string) {
+ try {
const m = await SQLiteESMFactory();
this.sqlite3 = SQLite.Factory(m);
const vfs = await MyVFS.create("hello", m);
this.sqlite3.vfs_register(vfs, true);
const db = await this.sqlite3.open_v2(`${dbName}.sqlite3`);
this.instance.db = db;
this.instance.exec = async (sql: string) => {
const rows: any[] = [];
await this.sqlite3.exec(db, sql, (row: any[], columns: string[]) => {
rows.push(mergeToObject(columns, row));
});
return rows;
};
return true;
+ } catch (error) {
+ if (this.instance.db) {
+ await this.close();
+ }
+ throw new Error(`Failed to initialize database: ${error.message}`);
+ }
}Committable suggestion was skipped due to low confidence.
| const mergeToObject = (columns: string[], row: any[]) => { | ||
| const obj: any = {}; | ||
| columns.forEach((column, index) => { | ||
| obj[column] = row[index]; | ||
| }); | ||
| return obj; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety of the mergeToObject helper function.
The function can be improved with proper TypeScript types to ensure type safety.
Consider this typed implementation:
-const mergeToObject = (columns: string[], row: any[]) => {
- const obj: any = {};
+const mergeToObject = <T extends Record<string, unknown>>(columns: string[], row: unknown[]): T => {
+ const obj = {} as T;
columns.forEach((column, index) => {
obj[column] = row[index];
});
return obj;
};| export const addIssue = async (issue: any) => { | ||
| if (document.hidden || !rootStore.user.localDBEnabled) return; | ||
| if (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return; | ||
|
|
||
| persistence.db.exec("BEGIN TRANSACTION;"); | ||
| await persistence.db.exec("BEGIN TRANSACTION;"); | ||
| stageIssueInserts(issue); | ||
| persistence.db.exec("COMMIT;"); | ||
| await persistence.db.exec("COMMIT;"); | ||
| }; | ||
|
|
||
| export const addIssuesBulk = async (issues: any, batchSize = 100) => { | ||
| if (!rootStore.user.localDBEnabled) return; | ||
| if (!rootStore.user.localDBEnabled || !persistence.db) return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and add input validation.
Several functions use any type and lack input validation. This could lead to runtime errors if invalid data is passed.
Consider these improvements:
- Define proper types instead of using
any:
interface IssueInput {
id: string;
type_id?: string;
priority: keyof typeof PRIORITY_MAP;
// ... other required fields
}
export const addIssue = async (issue: IssueInput) => {
// Validate required fields
if (!issue.id) throw new Error('Issue ID is required');
// ... rest of the implementation
}- Add validation for required fields before database operations.
Also applies to: 71-71
| if (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return; | ||
|
|
||
| persistence.db.exec("BEGIN TRANSACTION;"); | ||
| await persistence.db.exec("BEGIN TRANSACTION;"); | ||
| stageIssueInserts(issue); | ||
| persistence.db.exec("COMMIT;"); | ||
| await persistence.db.exec("COMMIT;"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing transaction for single insert operation.
While the database existence check is good, wrapping a single insert operation in a transaction might be unnecessary overhead. Transactions are typically used for maintaining consistency across multiple operations.
- await persistence.db.exec("BEGIN TRANSACTION;");
stageIssueInserts(issue);
- await persistence.db.exec("COMMIT;");📝 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 (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return; | |
| persistence.db.exec("BEGIN TRANSACTION;"); | |
| await persistence.db.exec("BEGIN TRANSACTION;"); | |
| stageIssueInserts(issue); | |
| persistence.db.exec("COMMIT;"); | |
| await persistence.db.exec("COMMIT;"); | |
| if (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return; | |
| stageIssueInserts(issue); |
| if (!rootStore.user.localDBEnabled || !persistence.db) return; | ||
|
|
||
| const insertStart = performance.now(); | ||
| await persistence.db.exec("BEGIN TRANSACTION;"); | ||
|
|
||
| for (let i = 0; i < issues.length; i += batchSize) { | ||
| const batch = issues.slice(i, i + batchSize); | ||
|
|
||
| persistence.db.exec("BEGIN TRANSACTION;"); | ||
| batch.forEach((issue: any) => { | ||
| batch.forEach(async (issue: any) => { | ||
| if (!issue.type_id) { | ||
| issue.type_id = ""; | ||
| } | ||
| stageIssueInserts(issue); | ||
| await stageIssueInserts(issue); | ||
| }); | ||
| await persistence.db.exec("COMMIT;"); | ||
| } | ||
| await persistence.db.exec("COMMIT;"); | ||
|
|
||
| const insertEnd = performance.now(); | ||
| log("Inserted issues in ", `${insertEnd - insertStart}ms`); |
There was a problem hiding this comment.
Verify batch processing completion before commit.
The current implementation might commit the transaction before all batch operations complete due to async forEach. This could lead to partial commits or race conditions.
Consider this safer implementation:
- batch.forEach(async (issue: any) => {
- if (!issue.type_id) {
- issue.type_id = "";
- }
- await stageIssueInserts(issue);
- });
+ await Promise.all(batch.map(async (issue: any) => {
+ if (!issue.type_id) {
+ issue.type_id = "";
+ }
+ await stageIssueInserts(issue);
+ }));📝 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 (!rootStore.user.localDBEnabled || !persistence.db) return; | |
| const insertStart = performance.now(); | |
| await persistence.db.exec("BEGIN TRANSACTION;"); | |
| for (let i = 0; i < issues.length; i += batchSize) { | |
| const batch = issues.slice(i, i + batchSize); | |
| persistence.db.exec("BEGIN TRANSACTION;"); | |
| batch.forEach((issue: any) => { | |
| batch.forEach(async (issue: any) => { | |
| if (!issue.type_id) { | |
| issue.type_id = ""; | |
| } | |
| stageIssueInserts(issue); | |
| await stageIssueInserts(issue); | |
| }); | |
| await persistence.db.exec("COMMIT;"); | |
| } | |
| await persistence.db.exec("COMMIT;"); | |
| const insertEnd = performance.now(); | |
| log("Inserted issues in ", `${insertEnd - insertStart}ms`); | |
| if (!rootStore.user.localDBEnabled || !persistence.db) return; | |
| const insertStart = performance.now(); | |
| await persistence.db.exec("BEGIN TRANSACTION;"); | |
| for (let i = 0; i < issues.length; i += batchSize) { | |
| const batch = issues.slice(i, i + batchSize); | |
| await Promise.all(batch.map(async (issue: any) => { | |
| if (!issue.type_id) { | |
| issue.type_id = ""; | |
| } | |
| await stageIssueInserts(issue); | |
| })); | |
| } | |
| await persistence.db.exec("COMMIT;"); | |
| const insertEnd = performance.now(); | |
| log("Inserted issues in ", `${insertEnd - insertStart}ms`); |
web/core/local-db/utils/utils.ts
Outdated
| export const clearOPFS = async () => { | ||
| const storageManager = window.navigator.storage; | ||
| const fileSystemDirectoryHandle = await storageManager.getDirectory(); | ||
| const entries = await fileSystemDirectoryHandle.entries(); | ||
| for await (const entry of entries) { | ||
| const [name] = entry; | ||
| try { | ||
| await fileSystemDirectoryHandle.removeEntry(name); | ||
| } catch (e) { | ||
| console.log("Error", e); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the robustness and documentation of clearOPFS.
The function would benefit from the following improvements:
- Use the existing
logErrorutility for consistent error handling - Add documentation explaining its purpose and usage
- Return a success/failure status
Consider this improved implementation:
+/**
+ * Clears all entries from the Origin Private File System (OPFS).
+ * Used during SQLite library migration to clean up storage.
+ *
+ * @returns {Promise<boolean>} Returns true if cleanup was successful, false otherwise
+ */
export const clearOPFS = async () => {
const storageManager = window.navigator.storage;
+ try {
const fileSystemDirectoryHandle = await storageManager.getDirectory();
const entries = await fileSystemDirectoryHandle.entries();
for await (const entry of entries) {
const [name] = entry;
try {
await fileSystemDirectoryHandle.removeEntry(name);
} catch (e) {
- console.log("Error", e);
+ logError(e);
}
}
+ return true;
+ } catch (e) {
+ logError(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.
| export const clearOPFS = async () => { | |
| const storageManager = window.navigator.storage; | |
| const fileSystemDirectoryHandle = await storageManager.getDirectory(); | |
| const entries = await fileSystemDirectoryHandle.entries(); | |
| for await (const entry of entries) { | |
| const [name] = entry; | |
| try { | |
| await fileSystemDirectoryHandle.removeEntry(name); | |
| } catch (e) { | |
| console.log("Error", e); | |
| } | |
| } | |
| }; | |
| /** | |
| * Clears all entries from the Origin Private File System (OPFS). | |
| * Used during SQLite library migration to clean up storage. | |
| * | |
| * @returns {Promise<boolean>} Returns true if cleanup was successful, false otherwise | |
| */ | |
| export const clearOPFS = async () => { | |
| const storageManager = window.navigator.storage; | |
| try { | |
| const fileSystemDirectoryHandle = await storageManager.getDirectory(); | |
| const entries = await fileSystemDirectoryHandle.entries(); | |
| for await (const entry of entries) { | |
| const [name] = entry; | |
| try { | |
| await fileSystemDirectoryHandle.removeEntry(name); | |
| } catch (e) { | |
| logError(e); | |
| } | |
| } | |
| return true; | |
| } catch (e) { | |
| logError(e); | |
| return false; | |
| } | |
| }; |
| deleteOption = async (key: string) => { | ||
| await runQuery(` DELETE FROM options where key='${key}'`); | ||
| }; |
There was a problem hiding this comment.
Fix SQL injection vulnerability in deleteOption
Similar to getOption, the key parameter is used directly in the SQL query without proper sanitization.
- await runQuery(` DELETE FROM options where key='${key}'`);
+ await runQuery(`DELETE FROM options where key=?`, [key]);📝 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.
| deleteOption = async (key: string) => { | |
| await runQuery(` DELETE FROM options where key='${key}'`); | |
| }; | |
| deleteOption = async (key: string) => { | |
| await runQuery(`DELETE FROM options where key=?`, [key]); | |
| }; |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (6)
web/core/local-db/utils/load-issues.ts (3)
Line range hint
53-59: Add missing await for addIssue call.The addIssue call needs to be awaited to ensure the update completes properly.
Apply this fix:
await deleteIssueFromLocal(issue_id); - addIssue(issue); + await addIssue(issue);
Line range hint
62-69: Fix async operations in array map.The current implementation using map with async callbacks doesn't wait for all delete operations to complete.
Apply this fix:
if (Array.isArray(response)) { - response.map(async (issue) => deleteIssueFromLocal(issue)); + await Promise.all(response.map((issue) => deleteIssueFromLocal(issue))); }
Line range hint
71-122: Use parameterized queries consistently.The main insert query uses string concatenation which is vulnerable to SQL injection, while the meta inserts use proper parameterized queries. This inconsistency in query style also makes the code harder to maintain.
Consider refactoring to use parameterized queries consistently:
const stageIssueInserts = async (issue: any) => { const issue_id = issue.id; issue.priority_proxy = PRIORITY_MAP[issue.priority as keyof typeof PRIORITY_MAP]; const keys = Object.keys(issueSchema); const sanitizedIssue = keys.reduce((acc: any, key) => { if (issue[key] || issue[key] === 0) { acc[key] = issue[key]; } return acc; }, {}); const columns = Object.keys(sanitizedIssue); const placeholders = columns.map(() => '?').join(', '); const values = Object.values(sanitizedIssue).map(value => value === null ? '' : typeof value === 'object' ? JSON.stringify(value) : value ); await persistence.db.exec({ sql: `INSERT OR REPLACE INTO issues (${columns.join(', ')}) VALUES (${placeholders})`, bind: values }); // ... rest of the function }web/core/local-db/utils/query-constructor.ts (1)
Line range hint
155-159: Critical: SQL injection vulnerabilities detected.The code directly interpolates values into SQL queries without proper escaping or parameterization:
sql += ` INNER JOIN issue_meta ${field} ON i.id = ${field}.issue_id AND ${field}.key = '${field}' AND ${field}.value IN ('${value.split(",").join("','")}')This pattern appears throughout the file and poses significant security risks.
Recommendations:
- Use parameterized queries or prepared statements
- Implement proper input validation and sanitization
- Consider using an SQL query builder library that handles escaping
Example refactor approach:
// Define parameter placeholder const params: any[] = []; sql += ` INNER JOIN issue_meta ? ON i.id = ?.issue_id AND ?.key = ? AND ?.value IN (?)`; params.push(field, field, field, field, field, value.split(",")); // Use the params with wa-sqlite's prepared statement feature // Execute the query with params arrayWould you like me to help create a comprehensive fix for all SQL injection vulnerabilities in this file?
web/core/local-db/storage.sqlite.ts (2)
Line range hint
246-247: Improve error logging in getIssues method.Consider adding more context to the log messages to help with debugging.
-log(`Project ${projectId} is loading, falling back to server`); +log(`Project ${projectId} is ${currentProjectStatus}, falling back to server. Status: ${this.status}`);Also applies to: 281-282
Line range hint
402-420: Fix SQL injection vulnerabilities in option management methods.The option management methods are vulnerable to SQL injection attacks. Use parameterized queries instead of string concatenation.
Apply these fixes:
getOption = async (key: string, fallback?: string | boolean | number) => { try { - const options = await runQuery(`select * from options where key='${key}'`); + const options = await runQuery('select * from options where key=?', [key]); if (options.length) { return options[0].value; } return fallback; } catch (e) { return fallback; } }; setOption = async (key: string, value: string) => { - await runQuery(`insert or replace into options (key, value) values ('${key}', '${value}')`); + await runQuery('insert or replace into options (key, value) values (?, ?)', [key, value]); }; deleteOption = async (key: string) => { - await runQuery(` DELETE FROM options where key='${key}'`); + await runQuery('DELETE FROM options where key=?', [key]); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
- web/core/local-db/storage.sqlite.ts (12 hunks)
- web/core/local-db/utils/load-issues.ts (4 hunks)
- web/core/local-db/utils/load-workspace.ts (3 hunks)
- web/core/local-db/utils/query-constructor.ts (1 hunks)
- web/core/local-db/utils/tables.ts (1 hunks)
- web/core/local-db/utils/utils.ts (3 hunks)
- web/core/local-db/worker/db.ts (1 hunks)
- web/core/services/issue/issue.service.ts (4 hunks)
- web/package.json (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- web/core/local-db/utils/load-workspace.ts
- web/core/local-db/utils/tables.ts
- web/core/local-db/utils/utils.ts
- web/core/services/issue/issue.service.ts
- web/package.json
🔇 Additional comments (7)
web/core/local-db/utils/load-issues.ts (3)
13-15: Consider removing transaction for single operation.While the database existence check is good, wrapping a single insert operation in a transaction adds unnecessary overhead since SQLite operations are atomic by default.
19-38: LGTM! Great improvements to batch processing.The changes properly address previous concerns about async operations and add useful performance monitoring.
11-11: Improve type safety across the module.Several functions use
anytype which reduces type safety. Consider defining proper interfaces for the issue data structure and using them consistently across the module.Also applies to: 71-71
web/core/local-db/utils/query-constructor.ts (1)
28-28: Verify the impact of reversed default sort order.The change from
"created_at"to"-created_at"reverses the default sort order to show newest items first, which aligns with modern UI practices. However, we should verify that this change doesn't break any existing consumers.✅ Verification successful
Let me gather more information about the query construction and its usage to better understand the impact of the default sort order change.
Let me search for more specific usage patterns to understand how the order is being consumed.
Based on the comprehensive search results, I can now provide a final response about the change in default sort order.
The change to
-created_atdefault sort order is safe and aligns with existing patternsThe change from
created_atto-created_atas the default sort order is safe and consistent with the codebase's patterns for the following reasons:
- The
-created_atordering is already the default in numerous parts of the codebase:
- Used as default in project filters (
web/core/store/project/project_filter.store.ts)- Set as default in multiple model configurations (e.g., in
apiserver/plane/db/models/issue.py,workspace.py,module.py)- Used consistently in UI components and API endpoints
- The change maintains consistency with the existing UI practices where newest items are shown first
- No tests were found that explicitly depend on ascending order
- The order can still be overridden by consumers when needed through the
order_byparameter🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for direct usage of issueFilterQueryConstructor without explicit order_by parameter # to identify potentially affected code paths # Test: Look for function calls without order_by parameter rg "issueFilterQueryConstructor.*\)" -A 5 | grep -v "order_by"Length of output: 1595
Script:
#!/bin/bash # Let's examine the full query constructor implementation and its usage # First, let's see the full implementation of the query constructor cat web/core/local-db/utils/query-constructor.ts # Then, let's look for any tests or specs that might be relying on the default order rg -l "test|spec.*issueFilterQueryConstructor" # Finally, let's check for any UI components that might be directly consuming this rg "from.*local-db.*query-constructor" -A 2Length of output: 8047
Script:
#!/bin/bash # Let's look for direct imports and usage of the query constructor rg "issueFilterQueryConstructor" -B 2 -A 2 # Also check for any explicit order_by parameters being passed rg "order_by.*created_at" -B 2 -A 2Length of output: 38194
web/core/local-db/storage.sqlite.ts (3)
123-138: LGTM: Improved sync workspace implementation.The sync workspace implementation now includes proper error handling and sync progress checks.
23-24: Consider the performance impact of reduced page and batch sizes.The reduction of
PAGE_SIZEandBATCH_SIZEfrom 1000 and 200 to 500 respectively might affect sync performance. While smaller batches can reduce memory usage, they increase the number of required operations.Let's analyze the impact:
429-442: 🛠️ Refactor suggestionExtract timeout configuration in getIsWriteInProgress.
The timeout value should be configurable and not hardcoded.
+private static readonly WRITE_TIMEOUT_MS = 5000; + getIsWriteInProgress = async (op: string) => { const writeStartTime = await this.getOption(op, false); if (writeStartTime) { - // Check if it has been more than 5seconds const current = new Date(); const start = new Date(writeStartTime); - if (current.getTime() - start.getTime() < 5000) { + if (current.getTime() - start.getTime() < Storage.WRITE_TIMEOUT_MS) { return true; } return false; } return false; };Likely invalid or redundant comment.
| type TQueryProps = { | ||
| sql: string; | ||
| rowMode: string; | ||
| returnValue: string; | ||
| bind: any[]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety of TQueryProps interface.
The type definition can be improved in several ways:
- Remove unused properties
rowModeandreturnValue - Use more specific types for
bindparameter
Consider this improved type definition:
type TQueryProps = {
sql: string;
- rowMode: string;
- returnValue: string;
- bind: any[];
+ bind: (string | number | boolean | null)[];
};📝 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.
| type TQueryProps = { | |
| sql: string; | |
| rowMode: string; | |
| returnValue: string; | |
| bind: any[]; | |
| }; | |
| type TQueryProps = { | |
| sql: string; | |
| bind: (string | number | boolean | null)[]; | |
| }; |
| interface SQLiteInstance { | ||
| db: unknown; | ||
| exec: (sql: string) => Promise<unknown[]>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance SQLiteInstance interface with comprehensive type definitions.
The interface could benefit from more specific types and additional methods that might be needed for database operations.
Consider expanding the interface:
interface SQLiteInstance {
- db: unknown;
+ db: Database;
exec: (sql: string) => Promise<unknown[]>;
+ // Add other common database operations
+ prepare?: (sql: string) => Promise<Statement>;
+ get?: <T = unknown>(sql: string, params?: any[]) => Promise<T>;
+ all?: <T = unknown>(sql: string, params?: any[]) => Promise<T[]>;
}
+interface Database {
+ close(): Promise<void>;
+ prepare(sql: string): Promise<Statement>;
+}
+interface Statement {
+ bind(...params: any[]): Promise<void>;
+ get(): Promise<any>;
+ all(): Promise<any[]>;
+ finalize(): Promise<void>;
+}Committable suggestion was skipped due to low confidence.
| private sqlite3: any; | ||
| private tp: Promise<any> | null = null; | ||
| private tpResolver: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety of class properties.
The class properties use any types which reduces type safety.
Consider using more specific types:
- private sqlite3: any;
- private tp: Promise<any> | null = null;
- private tpResolver: any;
+ private sqlite3: SQLite3Interface;
+ private tp: Promise<void> | null = null;
+ private tpResolver: {
+ resolve: () => void;
+ reject: (error: Error) => void;
+ } | null = null;
+interface SQLite3Interface {
+ vfs_register(vfs: unknown, makeDefault: boolean): void;
+ open_v2(filename: string): Promise<Database>;
+ exec(db: Database, sql: string, callback: (row: unknown[], columns: string[]) => void): Promise<void>;
+ statements(db: Database, sql: string): AsyncIterableIterator<Statement>;
+ bind(stmt: Statement, index: number, value: any): void;
+ step(stmt: Statement): Promise<number>;
+ column_names(stmt: Statement): Promise<string[]>;
+ row(stmt: Statement): Promise<any[]>;
+ close(db: Database): Promise<void>;
+}Committable suggestion was skipped due to low confidence.
| for await (const stmt of this.sqlite3.statements(this.instance.db, sql)) { | ||
| bind.forEach((b, i) => { | ||
| this.sqlite3.bind(stmt, i + 1, b); | ||
| }); | ||
|
|
||
| const rows = []; | ||
|
|
||
| do { | ||
| const columns = await this.sqlite3.column_names(stmt); | ||
| const row = await this.sqlite3.row(stmt); | ||
| rows.push(mergeToObject(columns, row)); | ||
| } while ((await this.sqlite3.step(stmt)) === SQLite.SQLITE_ROW); | ||
|
|
||
| return rows; | ||
| } |
There was a problem hiding this comment.
Potential memory leak in statement handling.
The statements created by sqlite3.statements are not being properly finalized, which could lead to memory leaks.
Add proper statement cleanup:
for await (const stmt of this.sqlite3.statements(this.instance.db, sql)) {
+ try {
bind.forEach((b, i) => {
this.sqlite3.bind(stmt, i + 1, b);
});
const rows = [];
do {
const columns = await this.sqlite3.column_names(stmt);
const row = await this.sqlite3.row(stmt);
rows.push(mergeToObject(columns, row));
} while ((await this.sqlite3.step(stmt)) === SQLite.SQLITE_ROW);
+ await this.sqlite3.finalize(stmt);
return rows;
+ } catch (error) {
+ await this.sqlite3.finalize(stmt);
+ throw error;
+ }
}📝 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 await (const stmt of this.sqlite3.statements(this.instance.db, sql)) { | |
| bind.forEach((b, i) => { | |
| this.sqlite3.bind(stmt, i + 1, b); | |
| }); | |
| const rows = []; | |
| do { | |
| const columns = await this.sqlite3.column_names(stmt); | |
| const row = await this.sqlite3.row(stmt); | |
| rows.push(mergeToObject(columns, row)); | |
| } while ((await this.sqlite3.step(stmt)) === SQLite.SQLITE_ROW); | |
| return rows; | |
| } | |
| for await (const stmt of this.sqlite3.statements(this.instance.db, sql)) { | |
| try { | |
| bind.forEach((b, i) => { | |
| this.sqlite3.bind(stmt, i + 1, b); | |
| }); | |
| const rows = []; | |
| do { | |
| const columns = await this.sqlite3.column_names(stmt); | |
| const row = await this.sqlite3.row(stmt); | |
| rows.push(mergeToObject(columns, row)); | |
| } while ((await this.sqlite3.step(stmt)) === SQLite.SQLITE_ROW); | |
| await this.sqlite3.finalize(stmt); | |
| return rows; | |
| } catch (error) { | |
| await this.sqlite3.finalize(stmt); | |
| throw error; | |
| } | |
| } |
| async exec(props: string | TQueryProps) { | ||
| if (this.tp && props === "BEGIN;") { | ||
| await this.tp; | ||
| } | ||
| let sql: string, bind: any[]; | ||
| if (typeof props === "string") { | ||
| sql = props; | ||
| } else { | ||
| ({ sql, bind } = props); | ||
| if (bind) { | ||
| for await (const stmt of this.sqlite3.statements(this.instance.db, sql)) { | ||
| bind.forEach((b, i) => { | ||
| this.sqlite3.bind(stmt, i + 1, b); | ||
| }); | ||
|
|
||
| const rows = []; | ||
|
|
||
| do { | ||
| const columns = await this.sqlite3.column_names(stmt); | ||
| const row = await this.sqlite3.row(stmt); | ||
| rows.push(mergeToObject(columns, row)); | ||
| } while ((await this.sqlite3.step(stmt)) === SQLite.SQLITE_ROW); | ||
|
|
||
| return rows; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (sql === "BEGIN;") { | ||
| this.tp = new Promise((resolve, reject) => { | ||
| this.tpResolver = { resolve, reject }; | ||
| }); | ||
| } | ||
|
|
||
| if (sql === "COMMIT;" && this.tp) { | ||
| await this.instance.exec(sql); | ||
| this.tpResolver.resolve(); | ||
| this.tp = null; | ||
| return; | ||
| } | ||
| return await this.instance.exec(sql); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify transaction handling logic in exec method.
The exec method mixes transaction handling with query execution, making it harder to maintain and test. Consider separating these concerns.
Consider refactoring the method:
+ private async handleTransaction(sql: string) {
+ if (sql === "BEGIN;") {
+ this.tp = new Promise((resolve, reject) => {
+ this.tpResolver = { resolve, reject };
+ });
+ } else if (sql === "COMMIT;" && this.tp) {
+ await this.instance.exec(sql);
+ this.tpResolver.resolve();
+ this.tp = null;
+ return true;
+ }
+ return false;
+ }
async exec(props: string | TQueryProps) {
if (this.tp && props === "BEGIN;") {
await this.tp;
}
let sql: string, bind: any[];
if (typeof props === "string") {
sql = props;
} else {
({ sql, bind } = props);
}
+ const isTransactionHandled = await this.handleTransaction(sql);
+ if (isTransactionHandled) return;
+
if (bind) {
// ... rest of the bind handling code
}
- if (sql === "BEGIN;") {
- this.tp = new Promise((resolve, reject) => {
- this.tpResolver = { resolve, reject };
- });
- }
-
- if (sql === "COMMIT;" && this.tp) {
- await this.instance.exec(sql);
- this.tpResolver.resolve();
- this.tp = null;
- return;
- }
return await this.instance.exec(sql);
}Committable suggestion was skipped due to low confidence.
| if (!rootStore.user.localDBEnabled || !persistence.db) return; | ||
|
|
||
| const deleteQuery = `delete from issues where id='${issue_id}'`; | ||
| const deleteMetaQuery = `delete from issue_meta where issue_id='${issue_id}'`; | ||
|
|
||
| persistence.db.exec("BEGIN TRANSACTION;"); | ||
| persistence.db.exec("BEGIN;"); | ||
| persistence.db.exec(deleteQuery); | ||
| persistence.db.exec(deleteMetaQuery); | ||
| persistence.db.exec("COMMIT;"); | ||
| }; |
There was a problem hiding this comment.
Fix transaction handling and SQL injection vulnerability.
Two issues need attention:
- Transaction operations are not awaited, which could lead to race conditions
- SQL queries are vulnerable to SQL injection due to string concatenation
Apply these fixes:
- persistence.db.exec("BEGIN;");
- persistence.db.exec(deleteQuery);
- persistence.db.exec(deleteMetaQuery);
- persistence.db.exec("COMMIT;");
+ await persistence.db.exec("BEGIN;");
+ await persistence.db.exec({
+ sql: "DELETE FROM issues WHERE id = ?",
+ bind: [issue_id]
+ });
+ await persistence.db.exec({
+ sql: "DELETE FROM issue_meta WHERE issue_id = ?",
+ bind: [issue_id]
+ });
+ await persistence.db.exec("COMMIT;");📝 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 (!rootStore.user.localDBEnabled || !persistence.db) return; | |
| const deleteQuery = `delete from issues where id='${issue_id}'`; | |
| const deleteMetaQuery = `delete from issue_meta where issue_id='${issue_id}'`; | |
| persistence.db.exec("BEGIN TRANSACTION;"); | |
| persistence.db.exec("BEGIN;"); | |
| persistence.db.exec(deleteQuery); | |
| persistence.db.exec(deleteMetaQuery); | |
| persistence.db.exec("COMMIT;"); | |
| }; | |
| if (!rootStore.user.localDBEnabled || !persistence.db) return; | |
| await persistence.db.exec("BEGIN;"); | |
| await persistence.db.exec({ | |
| sql: "DELETE FROM issues WHERE id = ?", | |
| bind: [issue_id] | |
| }); | |
| await persistence.db.exec({ | |
| sql: "DELETE FROM issue_meta WHERE issue_id = ?", | |
| bind: [issue_id] | |
| }); | |
| await persistence.db.exec("COMMIT;"); | |
| }; |
| const { DBClass } = await import("./worker/db"); | ||
| const worker = new Worker(new URL("./worker/db.ts", import.meta.url)); | ||
| const MyWorker = Comlink.wrap<typeof DBClass>(worker); | ||
|
|
||
| const configResponse = await promiser("config-get", {}); | ||
| log("Running SQLite3 version", configResponse.result.version.libVersion); | ||
| // Add cleanup on window unload | ||
| window.addEventListener("unload", () => worker.terminate()); | ||
|
|
||
| this.workspaceSlug = workspaceSlug; | ||
| this.dbName = workspaceSlug; | ||
| const instance = await new MyWorker(); | ||
| await instance.init(workspaceSlug); | ||
|
|
||
| const openResponse = await promiser("open", { | ||
| filename: `file:${this.dbName}.sqlite3?vfs=opfs`, | ||
| }); | ||
| const { dbId } = openResponse; | ||
| this.db = { | ||
| dbId, | ||
| exec: async (val: any) => { | ||
| if (typeof val === "string") { | ||
| val = { sql: val }; | ||
| } | ||
| return promiser("exec", { dbId, ...val }); | ||
| }, | ||
| exec: instance.exec, | ||
| close: instance.close, | ||
| }; | ||
|
|
||
| // dump DB of db version is matching | ||
| const dbVersion = await this.getOption("DB_VERSION"); | ||
| if (dbVersion !== "" && parseInt(dbVersion) !== DB_VERSION) { | ||
| await this.clearStorage(); | ||
| this.reset(); | ||
| await this._initialize(workspaceSlug); | ||
| return false; | ||
| } | ||
|
|
||
| log( | ||
| "OPFS is available, created persisted database at", | ||
| openResponse.result.filename.replace(/^file:(.*?)\?vfs=opfs$/, "$1") | ||
| ); | ||
| this.status = "ready"; | ||
| // Your SQLite code here. | ||
| await createTables(); | ||
|
|
||
| await this.setOption("DB_VERSION", DB_VERSION.toString()); | ||
| } catch (err) { | ||
| logError(err); | ||
| throw err; | ||
| return true; | ||
| } catch (error) { | ||
| this.status = "error"; | ||
| throw new Error(`Failed to initialize database worker: ${error}`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance worker error handling and cleanup.
The worker initialization has been improved with proper cleanup, but consider adding:
- Timeout handling for worker initialization
- Retry mechanism for failed operations
- Error details in the thrown error
Consider this enhancement:
try {
const { DBClass } = await import("./worker/db");
const worker = new Worker(new URL("./worker/db.ts", import.meta.url));
+ const timeoutId = setTimeout(() => {
+ worker.terminate();
+ throw new Error('Worker initialization timed out');
+ }, 5000);
const MyWorker = Comlink.wrap<typeof DBClass>(worker);
// Add cleanup on window unload
window.addEventListener("unload", () => worker.terminate());
this.workspaceSlug = workspaceSlug;
this.dbName = workspaceSlug;
const instance = await new MyWorker();
+ clearTimeout(timeoutId);
await instance.init(workspaceSlug);
this.db = {
exec: instance.exec,
close: instance.close,
};
this.status = "ready";
await createTables();
await this.setOption("DB_VERSION", DB_VERSION.toString());
return true;
} catch (error) {
this.status = "error";
- throw new Error(`Failed to initialize database worker: ${error}`);
+ throw new Error(`Failed to initialize database worker: ${error.message}\n${error.stack}`);
}📝 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 { DBClass } = await import("./worker/db"); | |
| const worker = new Worker(new URL("./worker/db.ts", import.meta.url)); | |
| const MyWorker = Comlink.wrap<typeof DBClass>(worker); | |
| const configResponse = await promiser("config-get", {}); | |
| log("Running SQLite3 version", configResponse.result.version.libVersion); | |
| // Add cleanup on window unload | |
| window.addEventListener("unload", () => worker.terminate()); | |
| this.workspaceSlug = workspaceSlug; | |
| this.dbName = workspaceSlug; | |
| const instance = await new MyWorker(); | |
| await instance.init(workspaceSlug); | |
| const openResponse = await promiser("open", { | |
| filename: `file:${this.dbName}.sqlite3?vfs=opfs`, | |
| }); | |
| const { dbId } = openResponse; | |
| this.db = { | |
| dbId, | |
| exec: async (val: any) => { | |
| if (typeof val === "string") { | |
| val = { sql: val }; | |
| } | |
| return promiser("exec", { dbId, ...val }); | |
| }, | |
| exec: instance.exec, | |
| close: instance.close, | |
| }; | |
| // dump DB of db version is matching | |
| const dbVersion = await this.getOption("DB_VERSION"); | |
| if (dbVersion !== "" && parseInt(dbVersion) !== DB_VERSION) { | |
| await this.clearStorage(); | |
| this.reset(); | |
| await this._initialize(workspaceSlug); | |
| return false; | |
| } | |
| log( | |
| "OPFS is available, created persisted database at", | |
| openResponse.result.filename.replace(/^file:(.*?)\?vfs=opfs$/, "$1") | |
| ); | |
| this.status = "ready"; | |
| // Your SQLite code here. | |
| await createTables(); | |
| await this.setOption("DB_VERSION", DB_VERSION.toString()); | |
| } catch (err) { | |
| logError(err); | |
| throw err; | |
| return true; | |
| } catch (error) { | |
| this.status = "error"; | |
| throw new Error(`Failed to initialize database worker: ${error}`); | |
| const { DBClass } = await import("./worker/db"); | |
| const worker = new Worker(new URL("./worker/db.ts", import.meta.url)); | |
| const timeoutId = setTimeout(() => { | |
| worker.terminate(); | |
| throw new Error('Worker initialization timed out'); | |
| }, 5000); | |
| const MyWorker = Comlink.wrap<typeof DBClass>(worker); | |
| // Add cleanup on window unload | |
| window.addEventListener("unload", () => worker.terminate()); | |
| this.workspaceSlug = workspaceSlug; | |
| this.dbName = workspaceSlug; | |
| const instance = await new MyWorker(); | |
| clearTimeout(timeoutId); | |
| await instance.init(workspaceSlug); | |
| this.db = { | |
| exec: instance.exec, | |
| close: instance.close, | |
| }; | |
| this.status = "ready"; | |
| // Your SQLite code here. | |
| await createTables(); | |
| await this.setOption("DB_VERSION", DB_VERSION.toString()); | |
| return true; | |
| } catch (error) { | |
| this.status = "error"; | |
| throw new Error(`Failed to initialize database worker: ${error.message}\n${error.stack}`); |
sqlite-wasam requires coep and coop headers which come with lot of limitations, switch to wa-sqlite-library which doesn't need these headers set.
Summary by CodeRabbit
New Features
getIssueFiltersmethods across various filter interfaces to enhance issue filtering capabilities based on project, cycle, module, and view IDs.layoutoption to theTIssueParamstype for improved layout configuration in issue-related queries.Bug Fixes
Chores
package.jsonfor improved profiling and tracing capabilities.