[TypeScript] Make db.runQuery generic to make it easy to type DB query results
#4247
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged No assets were unchanged |
joel-jeremy
commented
Jan 28, 2025
| // TODO: In the next phase, we will make this function generic | ||
| // and pass the type of the return type to `runQuery`. | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| return runQuery(sql, params, true) as any[]; |
There was a problem hiding this comment.
This will be made generic on the the next PRs so that we can control the PR size
joel-jeremy
commented
Jan 28, 2025
| // TODO: In the next phase, we will make this function generic | ||
| // and pass the type of the return type to `runQuery`. | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| return arr.length === 0 ? null : (arr[0] as any); |
There was a problem hiding this comment.
This will be made generic on the the next PRs so that we can control the PR size
joel-jeremy
commented
Jan 28, 2025
| // TODO: In the next phase, we will make this function generic | ||
| // and pass the type of the return type to `runQuery`. | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| return arr.length === 0 ? null : (arr[0] as any); |
There was a problem hiding this comment.
This will be made generic on the the next PRs so that we can control the PR size
joel-jeremy
commented
Jan 28, 2025
| // TODO: In the next phase, we will make this function generic | ||
| // and pass the type of the return type to `runQuery`. | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| return rows[0] as any; |
There was a problem hiding this comment.
This will be made generic on the the next PRs so that we can control the PR size
joel-jeremy
changed the title
WalkthroughThe pull request introduces comprehensive type safety improvements across multiple files in the Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/loot-core/src/server/tools/app.ts (1)
Line range hint
1-1: Consider removing@ts-strict-ignoreto align with type safety goals.Given that this PR aims to improve type safety, particularly around database operations, we should remove this ignore directive and properly type the code. This would help catch potential type-related issues early.
Consider:
- Removing the
@ts-strict-ignoredirective- Adding proper type annotations for database query results
- Utilizing the new generic
runQueryfunctionality introduced in this PR
🧹 Nitpick comments (5)
packages/loot-core/src/server/db/types/index.ts (3)
100-102: Consider removing or documenting unused properties.There are properties marked as "Unused in the codebase". Consider:
- Removing them if they're truly unused
- Adding documentation explaining why they're kept
Also applies to: 172-176
8-315: Consider enhancing type documentation.While the types are well-structured, they could benefit from additional documentation:
- Add JSDoc comments for complex types
- Document the purpose of boolean flags (1 | 0)
- Explain the relationship between view types and their base types
Example enhancement:
+/** Represents a bank account in the system */ export type DbAccount = { + /** Unique identifier for the account */ id: string; name: string; + /** Whether the account is excluded from budget calculations (1: yes, 0: no) */ offbudget: 1 | 0; // ... rest of the properties };
276-277: Consider consolidating view types.The types
DbViewTransactionInternalAliveandDbViewTransactionare identical. Consider:
- Using a single type
- Adding documentation explaining why multiple type aliases exist
packages/loot-core/src/server/sync/migrate.test.ts (1)
82-86: Consider selecting specific columns instead of using wildcard.Since the test only checks
lengthandparent_id, consider making the query more explicit:- const transactions = db.runQuery<db.DbTransaction>( - 'SELECT * FROM transactions', + const transactions = db.runQuery<Pick<db.DbTransaction, 'parent_id'>>( + 'SELECT parent_id FROM transactions', [], true, );This change would:
- Make the query more performant by only fetching needed columns
- Better align the runtime data shape with the TypeScript type
- Make the test's intentions clearer
packages/loot-core/src/server/tools/app.ts (1)
Line range hint
13-98: Consider breaking down the large method into smaller, focused functions.The
tools/fix-split-transactionsmethod handles multiple distinct operations. Consider refactoring it into smaller, focused functions for better maintainability and testability:+ async function fixBlankPayees() { + const blankPayeeRows = await db.all(` + SELECT t.*, p.payee AS parentPayee FROM v_transactions_internal t + LEFT JOIN v_transactions_internal p ON t.parent_id = p.id + WHERE t.is_child = 1 AND t.payee IS NULL AND p.payee IS NOT NULL + `); + + await runMutator(async () => { + const updated = blankPayeeRows.map(row => ({ + id: row.id, + payee: row.parentPayee, + })); + await batchUpdateTransactions({ updated }); + }); + + return blankPayeeRows.length; + } app.method('tools/fix-split-transactions', async () => { - // Current implementation + const numBlankPayees = await fixBlankPayees(); + const numCleared = await fixClearedFlags(); + // ... similar refactoring for other operations return { numBlankPayees, numCleared, // ... rest of the stats }; });Additionally, consider:
- Adding error handling for database operations
- Adding logging for better observability
- Adding transaction boundaries for related operations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4247.mdis excluded by!**/*.md
📒 Files selected for processing (11)
packages/loot-core/src/server/budget/base.ts(2 hunks)packages/loot-core/src/server/budget/statements.ts(2 hunks)packages/loot-core/src/server/budget/template-notes.test.ts(2 hunks)packages/loot-core/src/server/db/index.ts(19 hunks)packages/loot-core/src/server/db/mappings.ts(1 hunks)packages/loot-core/src/server/db/types.d.ts(0 hunks)packages/loot-core/src/server/db/types/index.ts(1 hunks)packages/loot-core/src/server/main.ts(5 hunks)packages/loot-core/src/server/sync/index.ts(1 hunks)packages/loot-core/src/server/sync/migrate.test.ts(1 hunks)packages/loot-core/src/server/tools/app.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/loot-core/src/server/db/types.d.ts
👮 Files not reviewed due to content moderation or server errors (4)
- packages/loot-core/src/server/budget/base.ts
- packages/loot-core/src/server/db/index.ts
- packages/loot-core/src/server/sync/index.ts
- packages/loot-core/src/server/main.ts
🔇 Additional comments (6)
packages/loot-core/src/server/budget/statements.ts (1)
1-2: LGTM! Type-safe implementation.The changes correctly implement type safety by:
- Importing the new
DbScheduletype- Using it as the return type for
getActiveSchedulesAlso applies to: 44-47
packages/loot-core/src/server/db/mappings.ts (1)
25-30: LGTM! Enhanced type safety with const assertions.The addition of
as constassertions improves type safety by:
- Making the tuple types more specific (exact types instead of general
string)- Ensuring immutability of the mapped arrays
packages/loot-core/src/server/budget/template-notes.test.ts (1)
22-22: LGTM! Tests updated to use new type definitions.The mock functions have been correctly updated to use the new
DbScheduletype, maintaining consistency with the type system changes.Also applies to: 247-266
packages/loot-core/src/server/db/types/index.ts (1)
1-5: LGTM! Clear documentation of type purpose.The comment clearly distinguishes between database schema types and AQL query framework types.
packages/loot-core/src/server/sync/migrate.test.ts (2)
82-86: LGTM! Type parameter improves type safety.The addition of
db.DbTransactiontype parameter aligns with the PR's objective to enhance type safety in database queries.
Line range hint
1-1: Consider removing@ts-strict-ignoreas part of type safety improvements.Since this PR focuses on enhancing type safety through generic queries, it would be beneficial to address the issues preventing strict TypeScript checking in this file.
Let's identify what's preventing strict mode:
joel-jeremy
changed the title
db.runQuery generic to make it easy to type DB query results
This was referenced Jan 28, 2025
matt-fidd
reviewed
Feb 12, 2025
lelemm
added a commit
to lagoom-br/actual
that referenced
this pull request
Feb 20, 2025
* 🐛 Fix Initializing the connection to the local database hang (actualbudget#4375) * fix initializing to the local db * release notes * Add percentage adjustments to schedule templates (actualbudget#4098) (actualbudget#4257) * add percentage adjustments to schedule templates * update release note * remove unecessary parentheses * Update packages/loot-core/src/server/budget/goalsSchedule.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * PR comments addressed * Linting fixes * Updated error handling, added tests * Linting fixes --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> * Custom mapping and import settings for bank sync providers (actualbudget#4253) * barebones UI * add saving and prefs * add last sync functionality * use mapping for synced transactions * note * jest -u * Update VRT * Coderabbit Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * add new fields * rename migration, newer in master * lint * coderabbit * update snapshots * GoCardless handlers fallback and notes * expose new fields from SimpleFIN * update tests * update instructions on GoCardless handlers * lint * feedback * Update VRT --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> * Add fallback value for payeename: 'undefined' - CBC Bank (actualbudget#4384) * Add fallback value for payename: 'undefined' * docs: add release note * Add fallback value for payename: 'undefined' (for negative amounts) * [TypeScript] Convert test page models to TS (actualbudget#4218) * Dummy commit * Delete js snapshots * Move extended expect and test to fixtures * Fix wrong commit * Update VRT * Dummy commit to run GH actions * Convert test page models to TS * Release notes * Fix typecheck errors * New page models to TS * Fix typecheck error * Fix page name * Put awaits on getTableTotals --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * rename two migrations to realign with convention (actualbudget#4343) * Updating sync server package name to @actual-app/sync-server (actualbudget#4370) * updating sync server to have a consistent package name * release notes * Add today button on mobile budget page (actualbudget#4380) * feat: today button on mobile budget page Jumps to the current month * add release note * cleaner onCurrentMonth * Update VRT * use SvgCalendar from icons/v2 Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk> * Update VRT --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk> * Development mode for sync server (React Fast Refresh on port 5006) (actualbudget#4372) * devmode for sync server * removed pluggy from this version * md * code review * changed how open browser * missed this * linter * trigger actions * 🐛 Fix: Error rate limit at user directory page (actualbudget#4397) * Error rate limit * md * 🐛 Fix new proxy middleware dependency missing on prod build (actualbudget#4400) * fix new proxy middleware not installed on prod build * release notes * Remove deprecated imports for several components (actualbudget#4385) * don't unbudget goals * lint * Fixes actualbudget#4069 : Ignore CSV inOutMode during OFX imports (actualbudget#4382) * Ignore inOutMode during OFX imports * Add release notes --------- Co-authored-by: youngcw <calebyoung94@gmail.com> * fix tooltip translation (actualbudget#4402) * [TypeScript] Make `db.runQuery` generic to make it easy to type DB query results (actualbudget#4247) * Make runQuery generic to make it easy to type DB query results. * Release notes * typo * update mapping data for existing synced transactions and always show direction dropdown (actualbudget#4403) * update sync mapping data for existing transactions on sync * show payment direction dropdown regardless of sample data * note * ignore changes in raw_synced_data * Fix top-level types of `send` function (actualbudget#4145) * Add release notes * Fix types of `send` function * Fix `send` types in a number of places * PR feedback * Foundations for the budget automations UI (actualbudget#4308) * Foundations for the budget automations UI * Coderabbit * Fix react-hooks/exhaustive-deps error on useSelected.tsx (actualbudget#4258) * Fix react-hooks/exhaustive-deps error on useSelected.tsx * Release notes * Fix react-hooks/exhaustive-deps error on usePayees.ts (actualbudget#4260) * Fix react-hooks/exhaustive-deps error on usePayees.ts * Rename * Release notes * Fix react-hooks/exhaustive-deps error on useAccounts.ts (actualbudget#4262) * Fix react-hooks/exhaustive-deps error on useAccounts.ts * Release notes * Fix react-hooks/exhaustive-deps error on Titlebar.tsx (actualbudget#4273) * Fix react-hooks/exhaustive-deps error on Titlebar.tsx * Release notes * [WIP] BANKS_WITH_LIMITED_HISTORY constant update (actualbudget#4388) * Fix react-hooks/exhaustive-deps error on useProperFocus.tsx (actualbudget#4259) * Fix react-hooks/exhaustive-deps error on useProperFocus.tsx * Remove comment in eslint config * Release notes * Fix react-hooks/exhaustive-deps error on TransactionsTable.jsx (actualbudget#4268) * Fix react-hooks/exhaustive-deps error on TransactionsTable.jsx * Release notes * Fix lint * Fix react-hooks/exhaustive-deps error on table.tsx (actualbudget#4274) * Fix react-hooks/exhaustive-deps error on table.tsx * Release notes * Fix react-hooks/exhaustive-deps error on useCategories.ts (actualbudget#4261) * Fix react-hooks/exhaustive-deps error on useCategories.ts * Release notes * 👷 Typescript: Improving typing of asyncStorage (global-store.json) (actualbudget#4369) * typing globalprefs * adding release notes * unneeded partial * removing prop that crept in * 📚 Translation batch #1 (actualbudget#4408) * Translation batch * md * Update packages/desktop-client/src/components/settings/Export.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix code review from coderabbit * code review --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Fix currencyToAmount incorrectly converting input (actualbudget#4383) * fix: ensure currencyToAmount works regardless of the configured number format * chore: linting * docs: add release note * test: ensure correct amount is entered for debit when adding split transactions * chore: rename variable thousandsSep to thousandsSeparator Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> * chore: rename variable decimalSep to decimalSeparator Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> * chore: rename decimalSep and thousandsSep variables to decimalSeparator and thousandsSeparator --------- Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> * useDisplayPayee hook to unify payee names in mobile and desktop. (actualbudget#4213) * useDisplayPayee hook to unify payee logic in mobile and desktop * Release notes * Fix typecheck errors * Fix test * Update test * Revert (No payee) color * Fix tests * VRT * Fix category transactions * Fix lint and VRT * Update VRT * Translate --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Extract transaction related server handlers from `main.ts` to `server/transactions/app.ts` (actualbudget#4221) * Move transaction related handlers to server/transactions folder and use the new convention * Fix lint and typecheck * Release notes * Update handler names * Move get-earliest-transaction * Update release notes * Fix tests * Fix types * Fix lint * Update snapshot * Remove duplicate parse-file.ts * Fix lint * 🐛 Fix `On budget` / `Off budget` underline with translated languages (actualbudget#4417) * Fix `On budget` / `Off budget` underline * md * ajuste para o merge --------- Co-authored-by: Michael Clark <5285928+MikesGlitch@users.noreply.github.com> Co-authored-by: Matt Farrell <10377148+MattFaz@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Martin Michotte <55855805+MMichotte@users.noreply.github.com> Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> Co-authored-by: Adam Stück <adam@adast.dk> Co-authored-by: Alberto Cortina Eduarte <albertocortina96@gmail.com> Co-authored-by: Gabriel J. Michael <gabriel.j.michael@gmail.com> Co-authored-by: Julian Dominguez-Schatz <julian.dominguezschatz@gmail.com> Co-authored-by: Michał Gołąb <23549913+michalgolab@users.noreply.github.com> Co-authored-by: Antoine Taillard <an.taillard@gmail.com>
This was referenced Feb 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is to make the
runQueryfunction generic to make it easy to type DB query results and propagate the proper types from the bottom up. Right now therunQueryreturnsanywhich means we lose type safety on the callers of this function unless we cast the result to the proper type.Note: We have 2
runQuerys in the codebase. One is for AQL (server/aql folder) and the other is for direct DB access (server/db folder). This PR is for the DB.The next phase for this is to make the higher level functions e.g.
first,all,firstSync, etc, generic and all of their callers.db.runQuerydb.firstdb.firstSyncdb.alldb.selectdb.selectWithSchemadb.selectFirstWithSchema