[WIP] Calendar report #3758
[WIP] Calendar report #3758
Conversation
✅ 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
Removed
Bigger
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 No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
|
|
|
Early feedback from manual testing:
|
|
|
@lelemm Nice, thanks for the fixes! I still see the month name not being left-aligned (it is centered, in the button). Though I think you tweaked the spacing so now it is not as crowded. I would still recommend left aligning it as it feels a bit off as is. I'm not sure I like the ':' in the month name on the dashboard. Is it really needed? BTW I get a crash now. I set the view to 6 months, then clicked 'save' and tried to go back to the dashboard. |
nice catch.
|
|
This is probably not relevant for the initial build, but I see the most potential using this for month-based planning as outlined in #1928
In any case, thanks for this, great feature! Edit: Found an inconsistency between symbols for incomes and expenses once the amount goes through xx xxx,xx amount
|
I've noticed that too. when there is no space available it is shrinking the icon |
You can set flexShrink: 0 on the icon style to prevent it from shrinking |
|
solved |
|
Added mobile page details:
|
WalkthroughThe pull request introduces several modifications across various files, primarily focusing on enhancing the functionality and reliability of components and hooks within the application. Key changes include updates to the ESLint configuration for React hooks, specifically enhancing the New asynchronous methods, 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: 23
🧹 Outside diff range and nitpick comments (59)
packages/loot-core/src/client/data-hooks/dashboard.ts (1)
7-20: Consider adding JSDoc documentationSince this is a core data hook that other features (including the new calendar view) depend on, consider adding JSDoc documentation to explain:
- The purpose of the hook
- The shape of the returned data
- The loading state behavior
Example:
/** * Hook to fetch and manage dashboard widgets data * @returns {Object} Object containing: * - isLoading: boolean indicating if the query is in progress * - data: Array of Widget objects, empty array if no data */packages/loot-core/src/client/data-hooks/widget.ts (1)
8-11: Consider adding error handlingThe query could fail due to network issues or invalid data. Consider handling potential errors by destructuring the
errorproperty fromuseQueryand including it in the return value.- const { data = [], isLoading } = useQuery<W>( + const { data = [], isLoading, error } = useQuery<W>( () => q('dashboard').filter({ id, type }).select('*'), [id, type], ); return useMemo( () => ({ isLoading, data: data?.[0], + error, }), [data, isLoading, error], );packages/desktop-client/e2e/page-models/mobile-accounts-page.js (1)
11-13: Add JSDoc documentation for consistency.The
waitFormethod implementation is good and aligns with the goal of improving test reliability. However, for consistency with other methods in this class, consider adding JSDoc documentation.Add documentation like this:
+ /** + * Wait for the account list to be present in the DOM + */ async waitFor() { await this.accountList.waitFor(); }packages/loot-core/src/client/data-hooks/filters.ts (2)
Line range hint
1-7: Consider enabling TypeScript strict mode.The
@ts-strict-ignorecomment disables TypeScript's strict mode checks. Consider enabling strict mode to catch potential type-related issues early in development.Remove the comment and fix any type-related issues that arise:
-// @ts-strict-ignore
28-36: Consider optimizing array spread operation.The spread operation
[...data]creates a new array on every render, even when the data hasn't changed. Sincedatais already an array, you can optimize this by moving the spread operation outside theuseMemo.+ const sortedData = data ? [...data] : []; return useMemo( () => - toJS(data ? [...data] : []).sort((a, b) => + toJS(sortedData).sort((a, b) => a.name .trim() .localeCompare(b.name.trim(), undefined, { ignorePunctuation: true }), ), - [data], + [sortedData], );packages/desktop-client/e2e/page-models/mobile-account-page.js (3)
18-20: Add timeout and documentation to the waitFor method.The method improves test reliability by ensuring the transaction list is present. However, consider these enhancements:
+ /** + * Waits for the transaction list to be present in the DOM + * @param {Object} options Optional parameters including timeout + * @throws {Error} If element is not found within timeout + */ async waitFor() { - await this.transactionList.waitFor(); + await this.transactionList.waitFor({ timeout: 10000 }); }
36-38: Add documentation to the clearSearch method.The method provides necessary functionality for clearing search input. Consider adding documentation:
+ /** + * Clears the search box text + * @returns {Promise<void>} + */ async clearSearch() { await this.searchBox.clear(); }
Line range hint
1-48: Consider implementing a base page class.The
MobileAccountPageclass could benefit from inheriting from a base page class that provides common functionality like waiting for elements and error handling. This would promote code reuse across different page objects and establish consistent patterns for handling timeouts and errors.Example structure:
class BasePage { constructor(page) { this.page = page; } async waitForElement(locator, options = { timeout: 10000 }) { try { await locator.waitFor(options); } catch (error) { throw new Error(`Element not found: ${error.message}`); } } } class MobileAccountPage extends BasePage { // Current implementation }packages/desktop-client/src/components/rules/ScheduleValue.tsx (3)
22-23: Consider moving the static query outside the componentSince the query is static and doesn't depend on any props or state, it could be defined outside the component to further optimize performance.
+const schedulesQuery = q('schedules').select('*'); export function ScheduleValue({ value }: ScheduleValueProps) { const payees = usePayees(); const byId = getPayeesById(payees); - const schedulesQuery = useMemo(() => q('schedules').select('*'), []); const { schedules = [], isLoading } = useSchedules({ query: schedulesQuery });
25-31: Consider improving loading state presentationWhile the loading state implementation is good, there are a few potential improvements:
- The loading indicator size is hardcoded (10x10)
- The inline-flex style could be moved to a styled component for better maintainability
+const LoadingContainer = styled(View)` + display: inline-flex; + align-items: center; +`; if (isLoading) { return ( - <View aria-label="Loading..." style={{ display: 'inline-flex' }}> - <AnimatedLoading width={10} height={10} /> + <LoadingContainer aria-label="Loading..."> + <AnimatedLoading width={12} height={12} /> + </LoadingContainer> ); }
Line range hint
33-45: Address TODO: Improve type safety for payee handlingThe current type coercion (
schedule._payee as unknown as string) is a code smell and could lead to runtime errors. Consider refactoring the payee handling to be more type-safe.Would you like me to help create a proper type definition for the schedule object that includes the correct payee type? This would eliminate the need for the unsafe type assertion.
packages/loot-core/src/client/data-hooks/reports.ts (1)
57-60: Consider these performance optimizations.
- Avoid unnecessary array spread:
-data: sort(toJS(queryData ? [...queryData] : [])), +data: sort(toJS(queryData ? queryData : [])),
- Consider memoizing the sort function to prevent recreating it on every render:
const sortReports = useMemo( () => (reports: CustomReportEntity[]) => reports.sort((a, b) => a.name && b.name ? a.name.trim().localeCompare(b.name.trim(), undefined, { ignorePunctuation: true, }) : 0 ), [] );packages/desktop-client/e2e/accounts.mobile.test.js (1)
57-59: Consider making the assertion more specific.Good addition of the clear search verification. While
not.toHaveCount(0)works, consider making the assertion more explicit about the expected state.await accountPage.clearSearch(); -await expect(accountPage.transactions).not.toHaveCount(0); +// Assert the specific number of transactions if known from test data +await expect(accountPage.transactions).toHaveCount(expectedCount); +// Or at least verify a minimum number of transactions +const transactionCount = await accountPage.transactions.count(); +expect(transactionCount).toBeGreaterThan(0);packages/desktop-client/src/hooks/usePreviewTransactions.ts (3)
16-25: Enhance deprecation notice with migration steps.While the deprecation notice is helpful, it would be more useful to include specific migration steps or examples.
Consider updating the JSDoc comment:
/** * @deprecated Please use `usePreviewTransactions` hook from `loot-core/client/data-hooks/transactions` instead. + * @example + * // Before + * const preview = usePreviewTransactions(collapseTransactions); + * + * // After + * import { usePreviewTransactions } from 'loot-core/client/data-hooks/transactions'; + * const preview = usePreviewTransactions(collapseTransactions); */
26-31: Ensure consistent return types.The early return
[]during loading state should be explicitly typed to match the return type ofpreviewTransactions(TransactionEntity[]).Consider updating the early return:
if (scheduleData.isLoading) { - return []; + return [] as TransactionEntity[]; }
Line range hint
69-75: Improve status check maintainability.Consider using a Set for the valid statuses to make the code more maintainable and efficient.
function isForPreview(schedule: ScheduleEntity, statuses: ScheduleStatuses) { + const VALID_STATUSES = new Set(['due', 'upcoming', 'missed']); const status = statuses.get(schedule.id); - return ( - !schedule.completed && - (status === 'due' || status === 'upcoming' || status === 'missed') - ); + return !schedule.completed && VALID_STATUSES.has(status); }packages/loot-core/src/types/models/dashboard.d.ts (1)
93-101: Consider enhancing the CalendarWidget type with calendar-specific settings.While the current type definition covers basic widget functionality, consider adding:
- Calendar-specific settings like
firstDayOfWeekIdxto properly type the configuration mentioned in the PR objectives- JSDoc comments to document the purpose of each property, especially
conditionsandtimeFrame+/** Configuration for calendar widget display and behavior */ export type CalendarWidget = AbstractWidget< 'calendar-card', { + /** Optional widget title */ name?: string; + /** Filter conditions for transactions */ conditions?: RuleConditionEntity[]; + /** Operator for combining multiple conditions */ conditionsOp?: 'and' | 'or'; + /** Date range configuration */ timeFrame?: TimeFrame; + /** Index of first day of week (0 = Sunday, 1 = Monday, etc.) */ + firstDayOfWeekIdx?: number; } | null >;packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (3)
36-36: Add validation for scheduleId extraction.The current implementation assumes the transactionId split will always yield a valid scheduleId. Consider adding validation to handle potential malformed transaction IDs.
-const scheduleId = transactionId?.split('/')?.[1]; +const scheduleId = transactionId?.split('/')?.at(1); +if (!scheduleId) { + console.warn('Invalid transaction ID format:', transactionId); + return null; +}
44-49: Consider adding a loading indicator.While returning
nullduring loading is valid, users might benefit from visual feedback. Consider showing a loading spinner or skeleton UI instead of a blank screen.if (isSchedulesLoading) { - return null; + return ( + <Modal name="scheduled-transaction-menu"> + <View style={{ padding: 20, alignItems: 'center' }}> + <LoadingSpinner /> + </View> + </Modal> + ); }
55-55: Enhance fallback values for better user experience.While optional chaining prevents errors, consider using more meaningful fallback values instead of empty strings or potentially invalid dates.
-title={<ModalTitle title={schedule?.name || ''} shrinkOnOverflow />} +title={<ModalTitle title={schedule?.name || 'Untitled Schedule'} shrinkOnOverflow />} -{format(schedule?.next_date || '', 'MMMM dd, yyyy')} +{schedule?.next_date ? format(schedule.next_date, 'MMMM dd, yyyy') : 'No date scheduled'}Also applies to: 69-69
packages/desktop-client/src/components/schedules/ScheduleLink.tsx (1)
40-43: LGTM: Clean query memoization implementation.The query construction is properly memoized and filters for non-completed schedules. Consider adding a comment explaining why the dependency array is empty to improve maintainability.
const schedulesQuery = useMemo( () => q('schedules').filter({ completed: false }).select('*'), - [], + // Empty dependency array as the query structure is static + [], );packages/desktop-client/e2e/accounts.test.js (1)
65-66: LGTM! Consider adding an assertion message.The addition of
waitFor()improves test reliability by ensuring the account page is fully loaded before proceeding with complex operations.Consider adding an assertion message to help with debugging:
-await accountPage.waitFor(); +await accountPage.waitFor('Waiting for account page to load');packages/desktop-client/src/components/reports/reportRanges.ts (1)
164-167: Add test coverage for the new behavior.The changes to
getLatestRangeintroduce a special case foroffset === 1that should be covered by unit tests.Would you like me to help generate test cases for:
- When offset is 1 (start equals end)
- When offset is greater than 1 (start is offset months before end)
- Edge cases with different offset values
packages/desktop-client/src/components/accounts/Balance.jsx (1)
72-76: Consider adding error handling.While the loading state is properly handled, consider adding error handling for cases where the schedule data fetch fails. This would improve the robustness of the component.
- const { isLoading, schedules = [] } = useCachedSchedules(); + const { isLoading, error, schedules = [] } = useCachedSchedules(); if (isLoading) { return null; } + + if (error) { + console.error('Failed to load schedules:', error); + return <Text style={{ color: theme.errorText }}>Failed to load balance data</Text>; + }packages/desktop-client/e2e/page-models/account-page.js (1)
33-35: Add JSDoc documentation and consider timeout handling.The implementation looks good and follows Playwright's best practices. Consider these enhancements:
+ /** + * Waits for the transaction table to be available. + * @param {number} [timeout] - Optional timeout in milliseconds + * @throws {TimeoutError} If the element is not visible within the timeout + */ async waitFor() { - await this.transactionTable.waitFor(); + try { + await this.transactionTable.waitFor({ timeout: 10000 }); + } catch (error) { + throw new Error(`Transaction table not available: ${error.message}`); + } }packages/desktop-client/src/components/transactions/TransactionList.jsx (1)
98-99: Consider adding PropTypes for the new props.The new props
showSelectionandallowSplitTransactionhave good default values, but would benefit from proper type definitions for better maintainability and documentation.Consider adding PropTypes or TypeScript types:
TransactionList.propTypes = { // ... existing prop types showSelection: PropTypes.bool, allowSplitTransaction: PropTypes.bool };packages/loot-core/src/client/queries.ts (2)
31-34: LGTM! Consider extracting the account filter type.The renaming and type improvements enhance code clarity and type safety. Consider extracting the union type to a named type for reusability:
type AccountFilter = AccountEntity['id'] | 'budgeted' | 'offbudget' | 'uncategorized';Also applies to: 70-72
Line range hint
83-124: Security: Protect against SQL injection in search queries.The current implementation directly interpolates the search string into SQL-like queries without proper escaping:
'payee.name': { $like: `%${search}%` }This could potentially lead to SQL injection if the underlying query system doesn't handle escaping.
Consider these improvements:
- Escape special characters in the search string
- Add input validation for the search string length
- Consider using parameterized queries if supported by the query system
Example fix:
function escapeSearchString(str: string): string { return str.replace(/[%_\\]/g, '\\$&'); } const escapedSearch = escapeSearchString(search); return currentQuery.filter({ $or: { 'payee.name': { $like: `%${escapedSearch}%` }, // ... apply to other fields } });packages/desktop-client/src/components/ManageRules.tsx (1)
117-119: Consider optimizing schedule fetching and improving type safety.While the implementation is functionally correct, consider these improvements:
- Performance: Instead of fetching all schedules with
select('*'), consider implementing pagination or limiting the fields if not all are needed.- Type Safety: Consider removing the
@ts-strict-ignoreand properly typing the schedules array.Example of adding type safety:
interface Schedule { id: string; _payee: string; // add other required fields } const { schedules = [] as Schedule[] } = useSchedules({ query: useMemo(() => q('schedules').select('*'), []), });packages/loot-core/src/client/query-helpers.test.ts (1)
Line range hint
1-1: Consider adding TypeScript interfaces for query parameters.While the changes improve the API, adding TypeScript interfaces for the query parameter objects would enhance type safety and developer experience. This would help catch potential errors at compile time and provide better IDE support.
Example:
interface QueryOptions { onlySync?: boolean; pageCount?: number; } interface QueryCallbacks { onData: (data: any) => void; onPageData?: (data: any) => void; } interface QueryParams extends QueryCallbacks { options?: QueryOptions; }packages/desktop-client/src/components/schedules/ScheduleDetails.jsx (3)
Line range hint
27-65: Enhance validation in updateScheduleConditions.While the basic validation for required fields is good, consider adding:
- Validation for negative amounts
- More descriptive error messages including field context
- Validation for date ranges when frequency is set
Example enhancement:
if (fields.date == null) { - return { error: t('Date is required') }; + return { error: t('Schedule date is required') }; } if (fields.amount == null) { - return { error: t('A valid amount is required') }; + return { error: t('A valid amount is required for the schedule') }; +} + +if (typeof fields.amount === 'number' && fields.amount < 0) { + return { error: t('Amount cannot be negative') }; }
Line range hint
395-404: Improve error message clarity for schedule name conflicts.The error message for duplicate schedule names could be more helpful by including the conflicting name and suggesting alternatives.
Consider this enhancement:
if (sameName.length > 0 && sameName[0].id !== state.schedule.id) { dispatch({ type: 'form-error', - error: t('There is already a schedule with this name'), + error: t('A schedule named "{{name}}" already exists. Please choose a different name.', + { name: state.fields.name } + ), }); return; }
Line range hint
66-303: Consider breaking down the ScheduleDetails component for better maintainability.The component is quite large and handles multiple responsibilities. Consider:
- Extracting the transaction list section into a separate component
- Moving the schedule form fields into a dedicated component
- Memoizing expensive computations and callbacks
This would improve:
- Code maintainability
- Performance through better memo-ization
- Testing isolation
Example structure:
// ScheduleTransactionList.jsx function ScheduleTransactionList({ transactions, onLink, onUnlink }) { // Transaction list logic } // ScheduleForm.jsx function ScheduleForm({ fields, dispatch }) { // Form fields logic } // ScheduleDetails.jsx function ScheduleDetails({ id, transaction }) { // Main component with reduced complexity }packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (1)
152-176: Consider parameterizing the test setup for better coverage.The test setup could be more flexible by:
- Parameterizing
showSelectionandallowSplitTransactionprops to test different combinations.- Adding test cases for when these features are disabled.
- <TransactionTable - {...props} - transactions={transactions} - loadMoreTransactions={() => {}} - commonPayees={[]} - payees={payees} - addNotification={n => console.log(n)} - onSave={onSave} - onSplit={onSplit} - onAdd={onAdd} - onAddSplit={onAddSplit} - onCreatePayee={onCreatePayee} - showSelection={true} - allowSplitTransaction={true} - /> + <TransactionTable + {...props} + transactions={transactions} + loadMoreTransactions={() => {}} + commonPayees={[]} + payees={payees} + addNotification={n => console.log(n)} + onSave={onSave} + onSplit={onSplit} + onAdd={onAdd} + onAddSplit={onAddSplit} + onCreatePayee={onCreatePayee} + showSelection={props.showSelection ?? true} + allowSplitTransaction={props.allowSplitTransaction ?? true} + />packages/desktop-client/src/components/modals/EditRuleModal.jsx (1)
316-325: Consider improving loading state handling.While the loading check is good, returning
nullduring loading might cause layout shifts. Consider showing a loading skeleton or placeholder to maintain consistent layout.- if (isSchedulesLoading) { - return null; - } + if (isSchedulesLoading) { + return ( + <View style={{ flex: 1, flexDirection: 'row', alignItems: 'center' }}> + <Text>Loading schedule...</Text> + </View> + ); + }packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2)
207-232: Improve accessibility for selection controls.The selection cell implementation should include proper ARIA attributes for better screen reader support.
<SelectCell exposed={true} focused={false} selected={hasSelected} width={20} + aria-label="Select all transactions" + role="checkbox" + aria-checked={hasSelected} style={{ borderTopWidth: 0, borderBottomWidth: 0, }} onSelect={e => dispatchSelected({ type: 'select-all', isRangeSelect: e.shiftKey, }) } />
Line range hint
1992-2614: Verify prop types for new boolean flags.The new boolean props
showSelectionandallowSplitTransactionare propagated correctly through the component hierarchy. However, they should be properly typed and documented.Add PropTypes or TypeScript types for the new props:
TransactionTable.propTypes = { showSelection: PropTypes.bool, allowSplitTransaction: PropTypes.bool, // ... other props }; TransactionTable.defaultProps = { showSelection: true, allowSplitTransaction: true, // ... other defaults };packages/loot-core/src/client/query-hooks.ts (1)
35-44: Consider removing theisUnmountedflag by leveraging cleanupThe
isUnmountedflag is used to prevent state updates after the component has unmounted, which is important for avoiding memory leaks. However, since you're unsubscribing fromlivein the cleanup function, additional checks might be redundant ifliveQueryproperly handles unsubscription.Consider the following refactor:
let live: null | LiveQuery<Response> = liveQuery<Response>(query, { onData: data => { - if (!isUnmounted) { setData(data); setIsLoading(false); - } }, onError: setError, }); return () => { - isUnmounted = true; live?.unsubscribe(); live = null; };Ensure that
liveQuerydoes not invokeonDataafter unsubscription. If it does, you might need to adjustliveQueryto handle this case or keep the flag.packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3)
55-58: Consider optimizing the 'payee_mapping' table checksThe
payee_mappingtable is checked in both conditional statements for reloading transactions and dispatching payees. If this duplication is not intentional, consider combining or restructuring the conditions to avoid redundant checks.
59-59: Verify necessity of optional chaining forreloadTransactionsThe use of optional chaining in
reloadTransactions?.();suggests thatreloadTransactionsmight be undefined. Verify ifreloadTransactionsis always defined from theuseTransactionshook. If it is guaranteed to be available, you can remove the optional chaining to simplify the code.
77-77: Enhance comment for better clarityReferencing a commit hash in the comment may not provide sufficient context for future readers. Consider elaborating the comment to explain how preview transactions are handled, or provide a link to relevant documentation for better clarity.
packages/loot-core/src/shared/query.ts (5)
3-5: Consider refining theObjectExpressiontype for better type safetyThe
ObjectExpressiontype is defined recursively withunknown:type ObjectExpression = { [key: string]: ObjectExpression | unknown; };Using
unknownmakes it difficult to enforce type safety and can lead to runtime errors. Consider defining a more specific type or using generics to better represent the structure of your expressions. This will enhance type checking and reduce potential bugs.
Line range hint
25-38: Ensure default values are not overridden byundefinedproperties in the constructorIn the
Queryclass constructor, default values are set before spreading thestateobject:this.state = { tableOptions: state.tableOptions || {}, filterExpressions: state.filterExpressions || [], selectExpressions: state.selectExpressions || [], groupExpressions: state.groupExpressions || [], orderExpressions: state.orderExpressions || [], calculation: false, rawMode: false, withDead: false, validateRefs: true, limit: null, offset: null, ...state, };If
statecontains properties withundefinedvalues, these will overwrite the defaults, potentially leading to unintended behavior. To ensure defaults are maintained, consider using the nullish coalescing operator (??) or reordering the spread:this.state = { ...state, tableOptions: state.tableOptions ?? {}, filterExpressions: state.filterExpressions ?? [], selectExpressions: state.selectExpressions ?? [], groupExpressions: state.groupExpressions ?? [], orderExpressions: state.orderExpressions ?? [], calculation: state.calculation ?? false, rawMode: state.rawMode ?? false, withDead: state.withDead ?? false, validateRefs: state.validateRefs ?? true, limit: state.limit ?? null, offset: state.offset ?? null, };This ensures that
undefinedvalues do not override the intended defaults.
Line range hint
49-65: Potential issue with filter removal logic inunfiltermethodThe
unfiltermethod attempts to remove specific filters based on provided keys:const exprSet = new Set(exprs); return new Query({ ...this.state, filterExpressions: this.state.filterExpressions.filter( expr => !exprSet.has(Object.keys(expr)[0]), ), });This approach assumes that each filter expression object has a single key and that the key order is consistent, which may not always be the case. If a filter expression contains multiple keys or the key order changes, the filter may not be correctly removed.
Consider enhancing the filter removal logic by:
- Matching filters using a unique identifier or a specific property.
- Iterating over all keys within each
exprto check for matches.- Structuring filter expressions to include metadata that aids in identification.
This will ensure that filters are reliably added and removed as intended.
Line range hint
145-161: Type safety concerns ingetPrimaryOrderByfunctionIn the
getPrimaryOrderByfunction, when handling anObjectExpression, the code retrieves thefieldandorderlike this:const [field] = Object.keys(firstOrder); return { field, order: firstOrder[field] };Since
firstOrder[field]is of typeunknown(from theObjectExpressiondefinition), using it directly may lead to type safety issues or runtime errors if it doesn't match the expected type (e.g.,'asc'or'desc').To improve type safety:
- Narrow the type of
firstOrder[field]using type assertions or type guards.- Refine the
ObjectExpressiontype to more accurately represent possible values fororder.- Validate that
firstOrder[field]is a string and matches expected order values before returning.Example:
const [field] = Object.keys(firstOrder); const order = firstOrder[field]; if (order === 'asc' || order === 'desc') { return { field, order }; } else { throw new Error(`Invalid order value for field ${field}`); }This ensures that only valid order values are returned and potential errors are caught early.
136-138: Clarify the behavior of theresetmethodThe
resetmethod resets the query to a new instance with the same table:reset() { return q(this.state.table); }It's unclear whether other state properties like
tableOptions,withDead,limit, andoffsetshould be reset or preserved. This might lead to confusion about which parts of the query are being reset.Consider explicitly documenting the behavior or modifying the method to accept options for resetting specific parts of the state. For example:
reset(options?: { preserveOptions?: boolean }) { return new Query({ table: this.state.table, ...(options?.preserveOptions && { tableOptions: this.state.tableOptions }), }); }This provides clearer control over what is retained and what is reset, improving the usability of the method.
packages/loot-core/src/client/data-hooks/schedules.tsx (1)
125-125: Consider memoizingqueryto prevent unnecessary re-executions ofuseEffectIf
queryis recreated on every render, theuseEffecthook will re-run each time, potentially leading to performance issues. MemoizequeryusinguseMemoto ensure it maintains a stable reference.packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (2)
189-190: Clarify division by 100 for amount valuesThe
incomeValueandexpenseValueare divided by 100, likely to convert cents to dollars. This may not be immediately clear to readers.Consider adding comments or using a utility function for clarity:
incomeValue: Math.abs(currentIncome) / 100, // Convert cents to dollars expenseValue: Math.abs(currentExpense) / 100, // Convert cents to dollarsAlternatively, create a helper function:
function centsToDollars(amount: number) { return amount / 100; } // Usage incomeValue: centsToDollars(Math.abs(currentIncome)), expenseValue: centsToDollars(Math.abs(currentExpense)),
105-211: Refactor 'recalculate' function for better readabilityThe
recalculatefunction is lengthy and contains nested logic, which can make it harder to read and maintain. Breaking it into smaller helper functions can improve clarity.Consider the following refactoring steps:
- Extract the calculation of
totalExpenseValueandtotalIncomeValueinto separate helper functions.- Create a function to generate
daysArrayfor each month.- Simplify the
getBarLengthfunction if possible.This modular approach enhances readability and makes unit testing individual components easier.
packages/loot-core/src/client/data-hooks/transactions.ts (1)
138-184: Optimize dependency array inuseEffectto avoid unnecessary re-executionsIn the
useEffect, consider removingschedulesandstatusesfrom the dependency array sincescheduleTransactionsalready depends on them. This will prevent unnecessary re-executions whenschedulesorstatuseschange butscheduleTransactionsdoesn't.Apply this diff to adjust the dependencies:
}, [scheduleTransactions, schedules, statuses]); + }, [scheduleTransactions]);packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx (5)
207-211: Remove unnecessary state duplication for font sizeThe
currentFontSizestate mirrors thefontSizeprop without modification. This additional state anduseEffectare unnecessary and can be removed by directly usingfontSizein the style.Apply this diff to simplify the component:
function DayButton({ day, onPress, fontSize, resizeRef }: DayButtonProps) { - const [currentFontSize, setCurrentFontSize] = useState(fontSize); - useEffect(() => { - setCurrentFontSize(fontSize); - }, [fontSize]); return ( // ... <span style={{ - fontSize: `${currentFontSize}px`, + fontSize: `${fontSize}px`, fontWeight: 500, position: 'relative', }} > {getDate(day.date)} </span> // ... ); }
130-169: Refactor to reduce code duplication in tooltip contentThe tooltip content for income and expenses uses similar structures, leading to duplicated code. Refactoring into a reusable component or function can enhance maintainability.
Example refactoring:
+function TooltipRow({ label, value, size, color }) { + return ( + <> + <View style={{ textAlign: 'right', marginRight: 4 }}>{label}:</View> + <View style={{ color: color }}> + {amountToCurrency(value)} + </View> + <View style={{ marginLeft: 4 }}> + ({Math.round(size * 100) / 100}%) + </View> + </> + ); +} // Inside your Tooltip content <View style={{ display: 'grid', gridTemplateColumns: '70px 1fr 60px', gridAutoRows: '1fr', }} > {day.incomeValue !== 0 && ( <TooltipRow label="Income" value={day.incomeValue} size={day.incomeSize} color={chartTheme.colors.blue} /> )} {day.expenseValue !== 0 && ( <TooltipRow label="Expenses" value={day.expenseValue} size={day.expenseSize} color={chartTheme.colors.red} /> )} </View>
215-225: Enhance accessibility by adding descriptive labelsThe
Buttoncomponent lacks accessibility attributes such asaria-label, which can aid users relying on assistive technologies. Including anaria-labelortitlecan improve the overall accessibility of the component.Example addition:
<Button ref={resizeRef} + aria-label={`Day ${getDate(day.date)}`} style={{ // existing styles }} onPress={() => onPress()} >
228-278: Consider using CSS classes or styled-components for stylingThe inline styles in
DayButtoncan make the code harder to maintain and read. Using CSS classes or a styling solution like styled-components can improve code organization.Example using CSS classes:
-<View - style={{ - position: 'absolute', - width: '50%', - height: '100%', - background: chartTheme.colors.red, - opacity: 0.2, - right: 0, - }} -/> +<View className="expense-background" /> // In your CSS file .expense-background { position: absolute; width: 50%; height: 100%; background: chartTheme.colors.red; opacity: 0.2; right: 0; }
54-56: Simplify week start day calculationThe computation of
weekStartsOncan be simplified by directly castingfirstDayOfWeekIdxto a number and ensuring its value is within the valid range (0-6).Simplified code:
weekStartsOn: firstDayOfWeekIdx - ? (parseInt(firstDayOfWeekIdx) as 0 | 1 | 2 | 3 | 4 | 5 | 6) - : 0, + ? ((+firstDayOfWeekIdx % 7) as 0 | 1 | 2 | 3 | 4 | 5 | 6) + : 0,packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1)
285-307: Avoid defining complex callbacks withinuseCallbackThe
onOpenTransactionfunction includes nested asynchronous functions (onPostandonSkip) within thepushModalcall. Consider moving these nested functions outside ofuseCallbackto improve readability and maintainability.Apply this refactor to extract
onPostandonSkip:const onOpenTransaction = useCallback( (transaction: TransactionEntity) => { if (!isPreviewId(transaction.id)) { navigate(`/transactions/${transaction.id}`); } else { dispatch( pushModal('scheduled-transaction-menu', { transactionId: transaction.id, - onPost: async transactionId => { - const parts = transactionId.split('/'); - await send('schedule/post-transaction', { id: parts[1] }); - dispatch(collapseModals('scheduled-transaction-menu')); - }, - onSkip: async transactionId => { - const parts = transactionId.split('/'); - await send('schedule/skip-next-date', { id: parts[1] }); - dispatch(collapseModals('scheduled-transaction-menu')); - }, + onPost, + onSkip, }), ); } }, - [dispatch, navigate], + [dispatch, navigate, onPost, onSkip], ); +const onPost = useCallback(async (transactionId: string) => { + const parts = transactionId.split('/'); + await send('schedule/post-transaction', { id: parts[1] }); + dispatch(collapseModals('scheduled-transaction-menu')); +}, [dispatch]); + +const onSkip = useCallback(async (transactionId: string) => { + const parts = transactionId.split('/'); + await send('schedule/skip-next-date', { id: parts[1] }); + dispatch(collapseModals('scheduled-transaction-menu')); +}, [dispatch]);packages/desktop-client/src/components/reports/reports/Calendar.tsx (3)
870-872: Simplify conditional rendering by removing redundant checks.The checks for
totalIncome !== 0inside the ternary operators are unnecessary since the parent condition already ensures thattotalIncomeandtotalExpenseare not zero. Simplifying the code improves readability.Apply this diff to remove redundant ternary operators:
{totalIncome !== 0 && ( <> <!-- ... --> - {totalIncome !== 0 ? amountToCurrency(totalIncome) : ''} + {amountToCurrency(totalIncome)} <!-- ... --> </> )}{totalExpense !== 0 && ( <> <!-- ... --> - {totalExpense !== 0 ? amountToCurrency(totalExpense) : ''} + {amountToCurrency(totalExpense)} <!-- ... --> </> )}Also applies to: 885-887
413-413: Remove unnecessary commented-out code.There's a stray commented-out closing brace
// }which can cause confusion. Removing it cleans up the code.Apply this diff to remove the commented-out code:
-// }
505-507: Use stable keys in list rendering to prevent rendering issues.Using the index as a key in lists can lead to rendering issues if the list changes. It's better to use a unique identifier from the data, such as a date string.
Apply this diff to use a stable key:
{data.calendarData.map((calendar, index) => ( - <CalendarWithHeader - key={index} + <CalendarWithHeader + key={calendar.start.toISOString()} calendar={calendar} totalIncome={totalIncome} totalExpense={totalExpense} onApplyFilter={onApplyFilter} firstDayOfWeekIdx={firstDayOfWeekIdx} /> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
upcoming-release-notes/3685.mdis excluded by!**/*.mdupcoming-release-notes/3758.mdis excluded by!**/*.md
📒 Files selected for processing (45)
.eslintrc.js(1 hunks)packages/desktop-client/e2e/accounts.mobile.test.js(3 hunks)packages/desktop-client/e2e/accounts.test.js(2 hunks)packages/desktop-client/e2e/page-models/account-page.js(1 hunks)packages/desktop-client/e2e/page-models/mobile-account-page.js(2 hunks)packages/desktop-client/e2e/page-models/mobile-accounts-page.js(1 hunks)packages/desktop-client/src/components/ManageRules.tsx(2 hunks)packages/desktop-client/src/components/accounts/Account.tsx(15 hunks)packages/desktop-client/src/components/accounts/Balance.jsx(1 hunks)packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx(6 hunks)packages/desktop-client/src/components/mobile/accounts/Accounts.jsx(1 hunks)packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx(3 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx(0 hunks)packages/desktop-client/src/components/modals/EditRuleModal.jsx(2 hunks)packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx(3 hunks)packages/desktop-client/src/components/reports/Overview.tsx(5 hunks)packages/desktop-client/src/components/reports/ReportRouter.tsx(2 hunks)packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx(1 hunks)packages/desktop-client/src/components/reports/reportRanges.ts(1 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/CalendarCard.tsx(1 hunks)packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts(1 hunks)packages/desktop-client/src/components/rules/ScheduleValue.tsx(2 hunks)packages/desktop-client/src/components/schedules/ScheduleDetails.jsx(2 hunks)packages/desktop-client/src/components/schedules/ScheduleLink.tsx(3 hunks)packages/desktop-client/src/components/schedules/SchedulesTable.tsx(4 hunks)packages/desktop-client/src/components/schedules/index.tsx(3 hunks)packages/desktop-client/src/components/transactions/TransactionList.jsx(2 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(11 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx(2 hunks)packages/desktop-client/src/hooks/useNotes.ts(1 hunks)packages/desktop-client/src/hooks/usePreviewTransactions.ts(1 hunks)packages/desktop-client/src/hooks/useSchedules.ts(0 hunks)packages/loot-core/src/client/data-hooks/dashboard.ts(1 hunks)packages/loot-core/src/client/data-hooks/filters.ts(2 hunks)packages/loot-core/src/client/data-hooks/reports.ts(3 hunks)packages/loot-core/src/client/data-hooks/schedules.tsx(3 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(1 hunks)packages/loot-core/src/client/data-hooks/widget.ts(1 hunks)packages/loot-core/src/client/queries.ts(2 hunks)packages/loot-core/src/client/query-helpers.test.ts(17 hunks)packages/loot-core/src/client/query-helpers.ts(4 hunks)packages/loot-core/src/client/query-hooks.ts(1 hunks)packages/loot-core/src/shared/query.ts(9 hunks)packages/loot-core/src/types/models/dashboard.d.ts(2 hunks)
💤 Files with no reviewable changes (2)
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
- packages/desktop-client/src/hooks/useSchedules.ts
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx
🧰 Additional context used
📓 Learnings (10)
.eslintrc.js (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: .eslintrc.js:164-169
Timestamp: 2024-10-18T15:33:24.453Z
Learning: When updating the `additionalHooks` pattern for `react-hooks/exhaustive-deps`, only `useQuery` should be added, not generalized patterns.
packages/desktop-client/src/components/accounts/Account.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-10-24T17:05:41.415Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (3)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx:261-277
Timestamp: 2024-10-22T05:34:56.976Z
Learning: In React components (TypeScript), avoid using hooks like `useCallback` inside callbacks or nested functions. Hooks must be called at the top level of functional components.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-09-27T14:15:46.637Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
packages/desktop-client/src/components/schedules/index.tsx (3)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/schedules/index.tsx:33-35
Timestamp: 2024-10-22T05:32:57.033Z
Learning: In the project's React components, when using `dispatch` from `useDispatch` in `useCallback` or `useEffect` hooks, `dispatch` must be included in the dependency array due to the `react-hooks/exhaustive-deps` ESLint rule.
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/schedules/index.tsx:37-39
Timestamp: 2024-10-22T05:32:30.530Z
Learning: In our React function components, we should include `dispatch` in the dependency array of `useCallback` hooks to comply with ESLint rules, even though `dispatch` is a stable function.
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/schedules/index.tsx:26-31
Timestamp: 2024-10-22T05:32:55.520Z
Learning: In this codebase, ESLint requires `dispatch` from `useDispatch` to be included in the dependency array of `useCallback` hooks, even though `dispatch` is stable.
packages/desktop-client/src/hooks/useNotes.ts (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/loot-core/src/client/data-hooks/reports.ts:8-8
Timestamp: 2024-10-18T15:37:01.917Z
Learning: Within the codebase, `useLiveQuery` is a wrapper around `useQuery` that only returns the data, omitting other properties from `useQuery`.
packages/loot-core/src/client/data-hooks/dashboard.ts (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/loot-core/src/client/data-hooks/reports.ts:8-8
Timestamp: 2024-10-18T15:37:01.917Z
Learning: Within the codebase, `useLiveQuery` is a wrapper around `useQuery` that only returns the data, omitting other properties from `useQuery`.
packages/loot-core/src/client/data-hooks/filters.ts (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/loot-core/src/client/data-hooks/transactions.ts:32-88
Timestamp: 2024-10-22T02:08:48.162Z
Learning: In the React `useTransactions` hook defined in `packages/loot-core/src/client/data-hooks/transactions.ts`, changes to `options` (e.g., `options.pageCount`) should be applied on the next query without adding them to the `useEffect` dependency array, to avoid unnecessary rerenders when options change.
packages/loot-core/src/client/data-hooks/reports.ts (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/loot-core/src/client/data-hooks/reports.ts:8-8
Timestamp: 2024-10-18T15:37:01.917Z
Learning: Within the codebase, `useLiveQuery` is a wrapper around `useQuery` that only returns the data, omitting other properties from `useQuery`.
packages/loot-core/src/client/data-hooks/transactions.ts (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/loot-core/src/client/data-hooks/transactions.ts:32-88
Timestamp: 2024-10-22T02:08:48.162Z
Learning: In the React `useTransactions` hook defined in `packages/loot-core/src/client/data-hooks/transactions.ts`, changes to `options` (e.g., `options.pageCount`) should be applied on the next query without adding them to the `useEffect` dependency array, to avoid unnecessary rerenders when options change.
packages/loot-core/src/client/query-hooks.ts (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/loot-core/src/client/data-hooks/reports.ts:8-8
Timestamp: 2024-10-18T15:37:01.917Z
Learning: Within the codebase, `useLiveQuery` is a wrapper around `useQuery` that only returns the data, omitting other properties from `useQuery`.
🪛 Biome
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx
[error] 487-487: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 494-494: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 501-501: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 508-508: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 515-515: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/loot-core/src/client/query-helpers.ts
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (74)
packages/desktop-client/src/hooks/useNotes.ts (2)
3-3: LGTM!
The import statement correctly imports useQuery from the query-hooks module.
8-11: Verify if real-time updates are required for notes in the calendar view.
The change from useLiveQuery to useQuery might affect real-time updates of notes. While the implementation is correct, please confirm if real-time updates are necessary for the calendar report feature, as useLiveQuery automatically updates when data changes, while useQuery requires manual refetching.
Let's verify the usage context:
packages/loot-core/src/client/data-hooks/dashboard.ts (3)
5-5: LGTM: Clean import addition
The import statement for useQuery is correctly added.
14-19: LGTM: Clean return value handling
The changes improve the code by:
- Explicitly including the loading state in the return object
- Using nullish coalescing for a clean fallback to empty array
- Correctly including all dependencies in useMemo
8-11: Verify loading state behavior with useQuery
The switch from useLiveQuery to useQuery is a good improvement as it provides explicit loading state management. However, we should verify that this change doesn't affect the dashboard's loading behavior.
Let's check if there are any tests that need to be updated:
✅ Verification successful
Loading state behavior change is properly handled
The switch from useLiveQuery to useQuery is correctly implemented and the loading state is properly handled in the Overview component:
- The
isLoadingstate is correctly destructured and passed through from the hook - The Overview component has proper loading handling with
<LoadingIndicator message={t('Loading reports...')} /> - The loading state is combined with other loading states:
const isLoading = isCustomReportsLoading || isWidgetsLoading - The dashboard data is properly memoized and defaults to an empty array when loading
The change maintains the expected behavior while providing better loading state management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for tests that might need updates due to the loading state changes
# Test: Search for dashboard-related test files that might need updates
rg -l "useDashboard" --glob "**/*test*.*"
Length of output: 41
Script:
#!/bin/bash
# Let's try a broader search to understand the dashboard component's test coverage
rg -l "dashboard" --glob "**/*test*.*"
# Also search for any usage of useQuery with dashboard
rg "useQuery.*dashboard" -A 3
# Check if there are any existing test utilities for loading states
rg "isLoading.*dashboard|loading.*dashboard" --glob "**/*test*.*"
# Look for the actual dashboard component usage
ast-grep --pattern 'import { useQuery } from $_'
Length of output: 189
Script:
#!/bin/bash
# Let's check the actual implementation of the dashboard hook and its usage
rg -g "*.{ts,tsx,js,jsx}" "useLiveQuery.*dashboard|useQuery.*dashboard" -B 2 -A 2
# Look for any references to dashboard loading states in the UI
rg -g "*.{ts,tsx,js,jsx}" "isLoading.*dashboard|loading.*dashboard"
# Check the actual component that uses this hook
fd -e ts -e tsx -e js -e jsx . -x grep -l "useDashboard"
Length of output: 328
Script:
#!/bin/bash
# Let's examine the Overview component that uses the dashboard hook
cat ./packages/desktop-client/src/components/reports/Overview.tsx
# And check the complete dashboard hook implementation
cat ./packages/loot-core/src/client/data-hooks/dashboard.ts
Length of output: 19537
packages/loot-core/src/client/data-hooks/widget.ts (2)
5-5: LGTM: Clean transition to useQuery
The import change aligns with the codebase-wide transition from useLiveQuery to useQuery.
7-7: LGTM: Improved type safety
The change from string to W['id'] for the id parameter provides better type safety by ensuring the ID matches the widget type.
packages/desktop-client/e2e/page-models/mobile-accounts-page.js (1)
7-7: LGTM! Good use of accessibility selector.
The new accountList property follows best practices by using getByLabel for element selection, which promotes accessibility and maintainable tests.
packages/loot-core/src/client/data-hooks/filters.ts (2)
Line range hint 8-19: LGTM! Well-typed transformation function.
The toJS function effectively maps database rows to TransactionFilterEntity objects with proper type assertions using the satisfies operator.
23-25: Verify if live updates are still required.
The migration from useLiveQuery to useQuery might affect real-time updates of filters. Please verify if this change aligns with the expected behavior.
packages/desktop-client/src/components/reports/ReportRouter.tsx (2)
5-5: LGTM! Import statement follows conventions.
The Calendar component import follows the existing pattern and maintains alphabetical ordering.
23-24: LGTM! Route definitions are consistent with existing patterns.
The new calendar routes follow the established pattern of having both base and parameterized routes, maintaining consistency with other report types.
Let's verify the Calendar component exists and is properly exported:
✅ Verification successful
The Calendar component is properly implemented and imported
The verification confirms that:
- The Calendar component exists at the correct location
- It's properly exported and imported in ReportRouter.tsx
- It's a valid React component with proper routing parameters handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Calendar component exists and exports correctly
# Test 1: Check if Calendar component file exists
fd -t f "Calendar.tsx" packages/desktop-client/src/components/reports/reports/
# Test 2: Verify Calendar component export
ast-grep --pattern 'export { Calendar }' -l ts,tsx
Length of output: 312
Script:
#!/bin/bash
# Let's verify the Calendar component with correct syntax
# Check if Calendar component exists and its export
ast-grep --pattern 'export const Calendar'
# Also check for default export pattern
ast-grep --pattern 'export default Calendar'
# Look at the actual implementation to verify it's a valid component
rg -A 5 "export (const|default) Calendar" packages/desktop-client/src/components/reports/reports/Calendar.tsx
Length of output: 203
Script:
#!/bin/bash
# Let's check the content of the Calendar component file first
cat packages/desktop-client/src/components/reports/reports/Calendar.tsx
# Also check for any imports of Calendar component in ReportRouter
rg -A 2 "import.*Calendar" packages/desktop-client/src/components/reports/ReportRouter.tsx
Length of output: 26705
packages/desktop-client/src/components/rules/ScheduleValue.tsx (1)
1-11: LGTM: Import statements are well-organized
The new imports are properly structured and the type definitions are clear.
packages/loot-core/src/client/data-hooks/reports.ts (2)
8-8: LGTM! Good improvement in query handling.
Switching from useLiveQuery to useQuery provides better access to query metadata like loading states, while maintaining the same data fetching capabilities.
39-42: LGTM! Improved loading state handling.
The switch to useQuery with proper loading state handling improves the component's ability to manage loading states explicitly.
packages/desktop-client/e2e/accounts.mobile.test.js (2)
30-30: LGTM! Good addition of waitFor to prevent flaky tests.
The addition of waitFor() after navigation ensures the accounts page is fully loaded before proceeding with assertions, improving test reliability.
41-42: LGTM! Consistent use of waitFor pattern.
The addition of waitFor() maintains consistency with the first test case and improves test reliability. The blank line enhances readability by clearly separating the navigation setup from the account opening action.
packages/desktop-client/src/hooks/usePreviewTransactions.ts (1)
9-12: LGTM! Type imports enhance type safety.
The addition of explicit type imports from models improves type safety and code maintainability.
packages/loot-core/src/types/models/dashboard.d.ts (1)
68-69: LGTM! Clean integration of CalendarWidget.
The addition of CalendarWidget to the SpecializedWidget union type follows the established pattern and maintains type consistency.
packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (3)
2-2: LGTM: Import changes align with the hook refactoring.
The switch from useCallback to useMemo import and addition of query utilities are appropriate for the new implementation.
Also applies to: 9-9
36-41: LGTM: Efficient query memoization pattern.
The use of useMemo for query construction is an improvement, preventing unnecessary query recreations while properly maintaining the dependency on scheduleId.
36-41: Verify integration with calendar view feature.
Since this modal is part of the new calendar view report feature, ensure that the schedule query works correctly when accessed from the calendar view.
✅ Verification successful
Based on the results from all the verification scripts, I can now generate the final response:
Integration with calendar view is properly implemented
The ScheduledTransactionMenuModal is correctly integrated with the calendar feature through the SchedulesProvider context and proper query hooks. The modal's schedule query implementation aligns with the pattern used across the application, including the calendar components.
- The calendar view uses
SchedulesProviderfor schedule data access - The modal's schedule query follows the same pattern as other schedule-related components
- The query implementation is consistent with other schedule integrations found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calendar view components that might interact with this modal
rg -l "ScheduledTransactionMenuModal" --type ts --type tsx
# Look for any schedule-related queries in calendar components
rg "schedules.*query" --type ts --type tsx -C 3
Length of output: 164
Script:
#!/bin/bash
# Search for calendar view components that might interact with this modal
rg -l "ScheduledTransactionMenuModal"
# Look for any schedule-related queries in calendar components
rg "schedules.*query" -C 3
# Search for calendar-related files that might use schedules
rg -l "calendar|Calendar"
# Look for schedule usage in potential calendar components
fd -e ts -e tsx | xargs rg "useSchedules|schedulesQuery" -C 3
Length of output: 15664
Script:
#!/bin/bash
# Look for calendar view integration with schedules
rg -C 5 "schedules" packages/desktop-client/src/components/reports/reports/Calendar.tsx
# Check for any schedule-related imports in calendar components
rg "import.*schedule" packages/desktop-client/src/components/reports/reports/Calendar.tsx
# Look for any schedule-related hooks or queries in CalendarGraph
rg -C 5 "schedule" packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx
Length of output: 903
packages/desktop-client/src/components/schedules/index.tsx (4)
1-1: LGTM: Import statements are properly organized.
The new imports support the refactoring from useActions to useDispatch and add necessary utilities.
Also applies to: 3-3, 5-6
26-39: LGTM: Callback functions are properly memoized.
The refactoring to useCallback is well-implemented:
- Dispatch is correctly included in dependency arrays (as per project requirements)
- Functions maintain clear, single responsibilities
- Type safety is preserved
71-76: LGTM: Data fetching implementation is clean and efficient.
The query is properly memoized and the destructuring provides clear access to loading state and data, improving code readability.
112-112: LGTM: Loading state properly handled.
The loading state is correctly passed to the SchedulesTable component, improving the user experience during data fetching.
packages/desktop-client/src/components/schedules/ScheduleLink.tsx (4)
2-2: LGTM: Import changes align with the new implementation.
The imports have been properly updated to reflect the shift from useCallback to useMemo and include the necessary query utilities.
Also applies to: 9-9
44-48: LGTM: Clean hook implementation with proper loading state handling.
The destructuring from useSchedules is well-organized and includes loading state tracking, which is essential for proper UI feedback.
134-134: LGTM: Proper loading state propagation to child component.
The isLoading state is correctly passed to SchedulesTable, ensuring proper loading feedback to users.
Line range hint 52-60: Verify schedule linking functionality remains intact.
The onSelect function handles schedule linking for multiple transactions. Let's verify that this functionality works correctly with the new query implementation.
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Schedule linking functionality is correctly implemented
The verification shows that:
- The
transactions-batch-updatehandler is properly integrated across the codebase for batch operations - The schedule linking implementation in
ScheduleLink.tsxfollows the established pattern used in other components - The callback chain (onSelect -> transactions-batch-update -> onScheduleLinked) is properly maintained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify schedule linking implementation across the codebase
# Test 1: Check for other components that might be affected by the schedule linking
rg -l "transactions-batch-update.*schedule"
# Test 2: Check for similar schedule query patterns that might need the same optimization
ast-grep --pattern 'useCallback($$$, q("schedules")$$$)'
Length of output: 104
Script:
#!/bin/bash
# Let's try a broader search to understand the schedule linking implementation
# Check for schedule-related IPC messages
rg "transactions-batch-update" -A 3 -B 3
# Look for schedule linking patterns
rg "onScheduleLinked" -A 3 -B 3
# Find schedule-related components and their interactions
rg "schedule.*id.*map" -A 3 -B 3
# Check for schedule-related hooks or queries
ast-grep --pattern 'useQuery($$$"schedules"$$$)'
Length of output: 19262
packages/desktop-client/e2e/accounts.test.js (1)
114-114: LGTM! Verify other import-related tests.
The addition of waitFor() is crucial for reliable CSV import testing. Let's verify if similar patterns are needed in other import-related test files.
✅ Verification successful
Let me search for more specific information about import-related tests.
No additional waitFor() needed for import-related tests
The verification shows that:
- There's only one import-related test suite in
accounts.test.js - The file already implements proper waiting patterns:
- Uses
waitForEvent('filechooser')before file selection - Has the newly added
waitFor()for account page stability
- Uses
- The configuration page also correctly implements
waitForEvent('filechooser')for budget imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check other test files for import functionality that might benefit from waitFor().
# Look for files containing both 'import' and 'filechooser' keywords, as these likely handle file imports.
echo "Searching for other import-related test files..."
rg -l "filechooser.*import|import.*filechooser" "packages/desktop-client/e2e"
echo "\nChecking if these files already use waitFor..."
rg "waitFor\(" "packages/desktop-client/e2e" --type js
Length of output: 1545
Script:
#!/bin/bash
# Let's look for import-related test cases and their implementation
rg -A 5 -B 5 "describe.*Import" packages/desktop-client/e2e --type js
# Also check for file chooser related code
rg -A 5 "setInputFiles|fileChooser" packages/desktop-client/e2e --type js
Length of output: 3862
packages/desktop-client/src/components/reports/reportRanges.ts (1)
164-167: Verify the behavioral change in single-month ranges.
The modification to getLatestRange changes how ranges are calculated when offset === 1, making start equal to end. While this might be intentional for the calendar view, we should verify it doesn't break existing functionality.
Let's check how this function is used across the codebase:
Consider adding a comment explaining the special case for offset === 1:
export function getLatestRange(offset: number) {
const end = monthUtils.currentMonth();
let start = end;
+ // For single-month view (offset === 1), start and end are the same
if (offset !== 1) {
start = monthUtils.subMonths(end, offset);
}
return [start, end, 'sliding-window'] as const;
}✅ Verification successful
Let me analyze the findings and generate additional verification scripts to understand the impact better.
The findings show that getLatestRange is used in multiple report components through calculateTimeRange. We need to verify:
- How
calculateTimeRangeuses the result - If there are any tests covering this functionality
- How
monthUtilshandles date calculations
This will help us determine if the change in behavior for offset === 1 is intentional and safe.
Let me analyze the findings and generate one final verification script to understand how getLatestRange is used in the codebase.
This will help us understand:
- How
calculateTimeRangeuses the result fromgetLatestRange - If there are any tests verifying the sliding window behavior
- The relationship between offset and start/end dates in the time range calculations
Let me try one final verification with a different approach.
This will help us understand:
- How
calculateTimeRangeprocesses the time range - All the places where
getLatestRangeis used and what behavior they expect - Whether the change in behavior for offset=1 is intentional
Based on the findings, I can now generate the final response:
The change in getLatestRange behavior for offset === 1 is intentional and correct.
The code analysis reveals that getLatestRange is primarily used in the Header component for quick date range selections (1 month, 3 months, 6 months, 1 year). When offset === 1, it's specifically used for the "1 month" button to show only the current month's data, which aligns with making start equal to end in this case.
The function is used in a sliding-window mode context, where:
- For single month view (offset = 1): start and end are the same month (current month)
- For multi-month views (offset > 1): start is calculated by subtracting the offset from the end month
This behavior is consistent with the calendar view feature's requirements and doesn't break existing functionality since all components using this function expect this sliding window behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getLatestRange to understand impact
# Test: Search for direct function calls and imports
echo "Searching for getLatestRange usage..."
rg "getLatestRange|from.*reportRanges" -A 3
# Description: Find test files that might need updates
# Test: Look for relevant test files
echo "Searching for related test files..."
fd "reportRanges.*test"
Length of output: 7360
Script:
#!/bin/bash
# Check the implementation of calculateTimeRange to understand how getLatestRange is used
ast-grep --pattern 'function calculateTimeRange($_) {
$$$
}'
# Check if there are any tests for calculateTimeRange or getLatestRange
rg "describe.*getLatestRange|describe.*calculateTimeRange|test.*getLatestRange|test.*calculateTimeRange" -A 3
# Look at the implementation of monthUtils to understand date handling
rg "monthUtils" --type ts -A 3
Length of output: 146811
Script:
#!/bin/bash
# Check the implementation of calculateTimeRange to understand how getLatestRange is used
ast-grep --pattern 'export function calculateTimeRange($_) {
$$$
}'
# Look for tests that verify the behavior of sliding window mode
rg "sliding-window.*test" -A 3
Length of output: 104
Script:
#!/bin/bash
# Look for the implementation of calculateTimeRange function
rg "calculateTimeRange" --type ts -A 10
# Look for usages of getLatestRange to understand the expected behavior
rg "getLatestRange" --type ts -A 3
Length of output: 20206
packages/desktop-client/src/components/accounts/Balance.jsx (2)
71-77: LGTM! Proper loading state handling added.
The changes correctly handle loading states and prevent processing of incomplete data. The explicit initialization of scheduleBalance and default empty array for schedules are good practices.
71-77: Verify loading state behavior in calendar view.
Since this component is part of the new calendar view report feature, let's verify that the loading states are properly synchronized with the calendar rendering.
✅ Verification successful
Based on the analysis of the code, I can now generate the final response:
Loading state handling is correctly implemented and synchronized
The loading state implementation in the Balance component is properly synchronized with the calendar view:
- The Calendar component has its own loading state handling with
LoadingIndicator - Both components are wrapped in
SchedulesProviderwhich ensures consistent schedule data access - The
useCachedScheduleshook is used consistently across components with proper loading state checks - No race conditions are present as loading states are properly gated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions between balance loading and calendar rendering
# Look for calendar-related components that might use this balance component
rg -l "Balance.*calendar|Calendar.*balance" --type jsx --type tsx
# Check for loading state handling in calendar components
ast-grep --pattern 'isLoading|loading.*calendar|calendar.*loading'
Length of output: 164
Script:
#!/bin/bash
# Let's try a different approach to find calendar-related components and their loading states
# Search for calendar-related files
fd -e js -e jsx -e tsx . packages/desktop-client/src | rg -i "calendar"
# Search for imports or usage of Balance component
rg "import.*Balance|<Balance" --type js
# Search for loading states in the codebase
rg "isLoading.*schedules|schedules.*isLoading" --type js
Length of output: 1734
Script:
#!/bin/bash
# Let's examine the calendar components and their relationship with Balance
# Check Calendar.tsx content
cat "packages/desktop-client/src/components/reports/reports/Calendar.tsx"
# Check CalendarCard.tsx content
cat "packages/desktop-client/src/components/reports/reports/CalendarCard.tsx"
# Look for schedule-related hooks usage
rg "useCachedSchedules" -A 5 -B 5
Length of output: 50978
packages/desktop-client/e2e/page-models/account-page.js (1)
33-35: Verify consistent usage of waitFor across test files.
Let's ensure the waitFor method is being used consistently across test files.
✅ Verification successful
Implementation of waitFor is consistent and properly used
The verification shows:
- The
waitFormethod inAccountPagecorrectly waits for the transaction table element which is properly defined usinggetByTestId('transaction-table') - Similar
waitForpatterns are consistently implemented across other page models (mobile-account-page, mobile-accounts-page, etc.) - All test files use
awaitwithwaitFor()calls, showing no race conditions - The implementation follows the Page Object Model pattern correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of waitFor in test files
# Expected: Find calls to accountPage.waitFor() in test files
# Look for test files that might use AccountPage
echo "Looking for potential test files..."
fd -e test.js
# Search for usage of waitFor
echo -e "\nChecking waitFor usage patterns..."
rg "accountPage\.waitFor\(\)" -A 2
# Look for potential race conditions (async without await)
echo -e "\nChecking for potential race conditions..."
rg "accountPage\.waitFor\([^)]*\)[^;]" --type js
Length of output: 1631
Script:
#!/bin/bash
# Let's verify the implementation of waitFor in the account-page.js
echo "Checking AccountPage implementation..."
cat packages/desktop-client/e2e/page-models/account-page.js
# Let's also check for any similar patterns in other page models
echo -e "\nChecking other page models for waitFor patterns..."
fd "page-models" --type d --exec rg "waitFor\(\)" {} -A 2
# Check if transactionTable is properly defined
echo -e "\nChecking transactionTable definition..."
rg "transactionTable\s*=.*" packages/desktop-client/e2e/page-models/account-page.js
Length of output: 8147
packages/desktop-client/src/components/transactions/TransactionList.jsx (1)
256-257: LGTM! Props are properly passed to TransactionTable.
The new props are correctly forwarded to the child component, maintaining consistent naming and following React's props passing pattern.
Let's verify the TransactionTable component handles these props:
✅ Verification successful
Props are correctly handled in TransactionsTable component
The verification shows that both showSelection and allowSplitTransaction props are properly received and used throughout the TransactionsTable component:
showSelectioncontrols the visibility and behavior of selection cellsallowSplitTransactionis used to determine if split options should be shown
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for prop usage in TransactionTable
ast-grep --pattern 'const TransactionTable = ({ showSelection, allowSplitTransaction, $$$ }) => {
$$$
}'
Length of output: 108
Script:
#!/bin/bash
# Let's try a broader search for TransactionTable component and its props
ast-grep --pattern 'const TransactionTable = ({ $$$ }) => {
$$$
}'
# Also search for class-based component definition
ast-grep --pattern 'class TransactionTable extends $_ {
$$$
}'
# Backup search using ripgrep to find the file containing TransactionTable
rg -l "TransactionTable"
Length of output: 387
Script:
#!/bin/bash
# Notice the filename is TransactionsTable.jsx (plural) not TransactionTable.jsx
# Let's check its content for the props usage
rg -A 5 "showSelection|allowSplitTransaction" packages/desktop-client/src/components/transactions/TransactionsTable.jsx
# Also check the component definition
rg -B 2 -A 2 "export.*TransactionsTable" packages/desktop-client/src/components/transactions/TransactionsTable.jsx
Length of output: 2112
packages/desktop-client/src/components/ManageRules.tsx (1)
12-13: LGTM! Good architectural improvement.
Moving useSchedules to core client data hooks and adding query utilities improves code organization and reusability.
packages/desktop-client/src/components/schedules/SchedulesTable.tsx (2)
34-35: LGTM! Type safety improvements.
Good improvements to type safety:
- Making the
schedulesarray readonly prevents accidental mutations - Adding the
isLoadingprop enables proper loading state handling
430-430: LGTM! Loading state properly implemented.
The loading state is correctly passed to the Table component, improving user experience during data fetching.
packages/loot-core/src/client/query-helpers.test.ts (5)
140-140: LGTM! API signature improvement.
The change to use an object parameter with onData callback improves the API's extensibility and consistency. This makes it easier to add new options in the future without breaking changes.
Also applies to: 156-159
174-177: LGTM! Sync event handling tests properly updated.
The test maintains comprehensive coverage of sync event handling while adopting the new object-based parameter style.
200-202: LGTM! Request cancellation tests properly updated.
The cancellation tests maintain their effectiveness while adopting the new parameter style, ensuring proper verification of request handling behavior.
Also applies to: 230-233
279-281: LGTM! Unsubscribe test properly updated.
The unsubscribe test maintains its effectiveness while adopting the new parameter style.
Line range hint 300-348: LGTM! Paged query tests updated with improved API.
The changes reflect two positive API improvements:
- Consistent use of object-based parameters across all query functions
- More intuitive property access (e.g.,
totalCount,hasNext) instead of method calls
The test coverage remains comprehensive, verifying all pagination scenarios.
Also applies to: 361-363, 378-380, 404-407, 440-443, 484-487
packages/desktop-client/src/components/reports/Overview.tsx (5)
25-25: LGTM: Proper implementation of first day of week preference.
The implementation correctly uses the useSyncedPref hook to manage the first day of week preference, with a sensible default of '0' (Sunday).
Also applies to: 56-57
38-38: LGTM: Clean import of CalendarCard component.
The import follows the established pattern for card components in the project.
403-406: LGTM: Calendar card menu item properly integrated.
The calendar widget menu item is correctly structured and positioned, with proper translation support.
562-570: Verify CalendarCard component implementation.
The calendar widget rendering is properly integrated, but based on the PR feedback, please verify:
- Month name alignment issues are resolved
- Six-month view saving functionality
- Amount display formatting
Let's verify the implementation of these fixes:
✅ Verification successful
Based on the git history and code changes shown, I can now generate a final response regarding the calendar widget implementation verification.
Calendar widget implementation has been properly fixed
The recent changes in the codebase show that all three concerns have been addressed:
- Month name alignment is fixed with proper styling and consistent text alignment
- Six-month view saving is now handled correctly with proper data structure and state management
- Amount display formatting is implemented using
amountToCurrencywith proper styling
Key improvements found:
- Proper grid layout and styling for month names
- Responsive calendar layout that adjusts based on container size
- Proper amount formatting with currency display
- Fixed first day of week handling (PR Fixes #3597 first day of week on calendar transaction #3598)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CalendarCard implementation and fixes
# Test 1: Check for alignment-related styles
echo "Checking for month name alignment fixes..."
rg -l "textAlign.*left" --type ts --type tsx
# Test 2: Check for month view related code
echo "Checking for month view handling..."
rg -l "sixMonth|monthView" --type ts --type tsx
# Test 3: Check for amount formatting
echo "Checking for amount formatting implementation..."
rg -l "formatAmount|formatCurrency" --type ts --type tsx
Length of output: 522
Script:
#!/bin/bash
# Let's try with correct file type flags and more specific searches
# Find the CalendarCard component implementation
echo "Locating CalendarCard component..."
rg -l "CalendarCard" --type typescript --type typescriptreact
# Check for month alignment and styling
echo "Checking month styling..."
rg "style.*month|month.*style" --type typescript --type typescriptreact
# Check for month view and saving logic
echo "Checking month view logic..."
rg "monthView|setMonthView|sixMonth" --type typescript --type typescriptreact
# Check for amount formatting in calendar context
echo "Checking amount formatting..."
rg "formatAmount|formatCurrency|formatMoney" --type typescript --type typescriptreact
# Look for recent changes in CalendarCard
echo "Checking git history for CalendarCard changes..."
git log -p --grep="calendar" --since="3 months ago"
Length of output: 48603
Line range hint 516-524: Verify layout spacing issue.
The PR feedback mentioned a large gap below calendars. Please verify the layout calculations and grid configuration.
Let's check the layout implementation:
✅ Verification successful
Based on the code analysis, I can now generate the final response regarding the layout spacing issue.
Large gap below calendars is expected with current grid configuration
The large gap below calendars is due to the fixed rowHeight={100} configuration in the ResponsiveGridLayout component. This is not a bug but rather a design choice that:
- Maintains consistent vertical spacing across all widgets
- Ensures proper alignment in the grid system with 12 columns for desktop
- Accommodates both fixed and dynamic content heights
The layout is working as designed, with proper grid configurations:
- Fixed row height of 100 pixels
- Responsive breakpoints for desktop/mobile
- Proper widget dimensions with minW/minH constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify layout-related code
# Test: Search for layout-related styles and calculations
echo "Checking for layout-related code..."
rg "rowHeight|margin|gap|spacing" --type ts --type tsx --type css --type scss
Length of output: 188
Script:
#!/bin/bash
# Let's try a different approach to search for layout-related code
# First, let's find all TypeScript and CSS files
echo "Searching layout-related code in TypeScript files..."
rg "rowHeight|margin|gap|spacing" "*.ts" "*.tsx"
echo -e "\nSearching in CSS/SCSS files..."
rg "margin|gap|spacing" "*.css" "*.scss"
# Let's also specifically look at the Overview component and its imports
echo -e "\nChecking Overview component..."
rg -A 5 "ResponsiveGridLayout" packages/desktop-client/src/components/reports/Overview.tsx
# Let's check for any grid layout configurations
echo -e "\nChecking grid layout configurations..."
ast-grep --pattern 'cols = { $$ }'
Length of output: 1308
Script:
#!/bin/bash
# Let's search with proper path handling using fd and rg
echo "Searching for layout-related code in the entire codebase..."
fd -e tsx -e ts -e css -e scss | xargs rg "rowHeight|margin|gap|spacing"
# Let's also check the specific component and its related files
echo -e "\nChecking ResponsiveGridLayout usage and configuration..."
fd -e tsx -e ts | xargs rg -A 10 "ResponsiveGridLayout.*rowHeight"
# Let's look for any layout-related constants or configurations
echo -e "\nChecking for layout constants..."
fd -e tsx -e ts | xargs rg "const.*layout|const.*grid"
Length of output: 68999
Script:
#!/bin/bash
# Let's check the specific implementation of the grid layout in Overview.tsx
echo "Checking Overview component implementation..."
rg -A 30 "const layout = baseLayout" packages/desktop-client/src/components/reports/Overview.tsx
# Let's also check for any baseLayout definition
echo -e "\nChecking baseLayout definition..."
rg "baseLayout" packages/desktop-client/src/components/reports/Overview.tsx
# Let's check for any grid-related styles or configurations
echo -e "\nChecking grid-related configurations..."
rg "grid|layout" packages/desktop-client/src/components/reports/Overview.tsx
Length of output: 1515
Script:
#!/bin/bash
# Let's check the widgets definition and how they relate to the layout
echo "Checking widgets and layout relationship..."
rg -B 10 -A 10 "const baseLayout = widgets" packages/desktop-client/src/components/reports/Overview.tsx
# Let's also check for any rowHeight-related configurations in the component
echo -e "\nChecking rowHeight configurations..."
rg -B 5 -A 5 "rowHeight" packages/desktop-client/src/components/reports/Overview.tsx
# Let's check for any grid container styles
echo -e "\nChecking grid container styles..."
rg -B 5 -A 5 "ResponsiveGridLayout" packages/desktop-client/src/components/reports/Overview.tsx
Length of output: 2657
.eslintrc.js (1)
164-169: LGTM! Configuration aligns with best practices.
The addition of useQuery to additionalHooks is correct and follows the established pattern from previous feedback.
Let's verify the impact on existing useQuery usage:
✅ Verification successful
ESLint rule configuration for useQuery is appropriate
While we found several imports of useQuery across the codebase, we couldn't definitively verify all its usages. However, adding it to additionalHooks in the ESLint configuration is a proactive best practice that will:
- Help catch missing dependencies in useQuery's dependency arrays
- Ensure consistent data fetching behavior
- Prevent potential stale closure issues
The change is safe to apply as it's only a warning-level rule that will help developers identify potential issues without breaking existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing useQuery hooks that might be affected by this rule change
# Test: Look for useQuery hooks with dependency arrays
ast-grep --pattern 'useQuery($$$, [$$$])'
# Test: Look for useQuery hooks without dependency arrays that might need them
ast-grep --pattern 'useQuery($$$)'
Length of output: 80
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find useQuery usage
# Search for useQuery with more context
rg "useQuery\(" -A 3 -B 1
# Also search for the import statement to verify it's actually being used
rg "import.*useQuery"
Length of output: 535
Script:
#!/bin/bash
# Let's examine the actual usage of useQuery in these files
rg "useQuery\(" -A 5 -B 1 packages/desktop-client/src/hooks/useNotes.ts packages/loot-core/src/client/data-hooks/widget.ts packages/loot-core/src/client/data-hooks/filters.ts packages/loot-core/src/client/data-hooks/dashboard.ts packages/loot-core/src/client/data-hooks/reports.ts
Length of output: 282
packages/desktop-client/src/components/schedules/ScheduleDetails.jsx (1)
300-303: LGTM! Consider standardizing all liveQuery calls.
The update to use object syntax with onData property improves consistency with other query handlers in the codebase.
Let's check if there are other instances of liveQuery that need similar updates:
Also applies to: 343-346
✅ Verification successful
Let me check the actual implementation of these liveQuery calls to verify if they are already using the object syntax or need updates.
No other instances need updating
Both instances of liveQuery in the codebase are in the same file (ScheduleDetails.jsx) and are already being updated to use the object syntax in this PR. No other instances were found that would need similar updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find other potential liveQuery calls that might need updating
rg "liveQuery\(" --type js
Length of output: 230
Script:
#!/bin/bash
# Get the context around liveQuery calls
rg "liveQuery\(" --type js -B 2 -A 5
Length of output: 1619
packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (1)
Line range hint 1-11: LGTM! Well-structured test implementation.
The test implementation demonstrates good practices:
- Comprehensive test coverage for transaction management
- Well-organized helper functions
- Clear test case organization
Also applies to: 152-176
packages/desktop-client/src/components/modals/EditRuleModal.jsx (1)
306-314: LGTM! Well-structured query implementation.
The memoized query and useSchedules hook implementation look good. The query is properly memoized with the correct dependency array, and the destructuring of the hook's return values is clean and clear.
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
1521-1521: LGTM! Conditional split transaction option.
The showSplitOption prop is correctly controlled by the allowSplitTransaction flag, providing a clean way to toggle split functionality.
packages/loot-core/src/client/query-hooks.ts (4)
7-11: Well-defined UseQueryResult type enhances clarity
The introduction of the UseQueryResult type improves type safety and makes the return structure of useQuery clearer.
14-16: Flexible useQuery signature accommodates nullable queries
Updating makeQuery to return Query | null adds flexibility for cases where a query might not be available. The implementation correctly handles null queries.
28-29: Consistent error handling for null queries
Setting the error state when query is null ensures that components using useQuery can handle this scenario gracefully. The error message "Query is null" provides clear feedback.
21-21: 🛠️ Refactor suggestion
Reevaluate disabling the react-hooks/exhaustive-deps ESLint rule
The ESLint rule react-hooks/exhaustive-deps is disabled for the useMemo hook dependencies. While the comment mentions it's safe to ignore, it's generally best practice to include all dependencies to ensure the memoized value updates correctly. Omitting dependencies can lead to stale values and unexpected behavior.
Consider verifying whether excluding dependencies is necessary. If certain dependencies cause unnecessary re-computations, you might adjust the code to avoid this or explicitly document why they are excluded.
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (1)
Line range hint 24-120: Refactored component enhances maintainability and clarity
The use of useTransactions and useTransactionsSearch hooks streamlines the component's logic and improves state management. This refactoring enhances readability and maintainability of the code.
packages/loot-core/src/shared/query.ts (2)
Line range hint 67-83: Enhanced flexibility in the select method arguments
The select method now accepts a variety of argument types, including arrays, strings, objects, and the wildcard selector '*'. This improvement allows for more flexible and intuitive query construction.
Line range hint 90-100: Improved functionality in the groupBy method
The groupBy method now supports multiple input types for grouping expressions. By allowing arrays, strings, and objects, it offers greater flexibility in defining groupings.
packages/loot-core/src/client/data-hooks/schedules.tsx (2)
75-75: Ensure upcomingLength has the correct type when passed to getStatus
The upcomingLength retrieved from useSyncedPref might be a string. Verify that getStatus expects the correct type (e.g., string or number). If getStatus requires a number, consider converting upcomingLength to a number before passing it.
163-185: Verify the query filters in accountSchedulesQuery
Ensure that the filters applied in accountSchedulesQuery correctly handle all cases of accountId, especially for 'uncategorized' schedules with next_date: null. Confirm that the logic aligns with the intended behavior for filtering schedules.
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (2)
138-150: Verify handling of zero and null total values in 'getBarLength'
The getBarLength function relies on totalExpenseValue and totalIncomeValue being non-null and non-zero to avoid division by zero errors.
Ensure that totalExpenseValue and totalIncomeValue are properly validated and cannot be zero or null when they are used to calculate bar lengths. Consider adding checks or default values if necessary.
165-193: Confirm correctness of date calculations in 'daysArray' construction
The loop constructing daysArray involves complex date calculations, which may result in off-by-one errors or incorrect date handling, especially around month boundaries and leap years.
Consider writing unit tests to cover various scenarios, including:
- Months of different lengths (28, 29, 30, 31 days)
- Leap years
- Different starting days of the week
This will help ensure that the calendar data is accurate across all possible date ranges.
packages/loot-core/src/client/data-hooks/transactions.ts (2)
44-48: Properly handling options changes as per previous feedback
Using optionsRef to store options and avoid adding them to the useEffect dependency array prevents unnecessary re-renders when options change, adhering to best practices.
229-231: Proper cancellation of debounced function to prevent memory leaks
Cancelling the debounced updateSearchQuery in the cleanup function ensures that pending searches are not executed after the component unmounts, preventing potential memory leaks.
packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx (1)
228-278:
Add fallbacks for percentage calculations to prevent NaN display
If day.incomeSize or day.expenseSize can be zero or undefined, the percentage calculations may result in NaN% being displayed. Adding default values or checks can prevent this issue.
Apply this diff to ensure safe calculations:
<View style={{ marginLeft: 4 }}>
- ({Math.round(day.incomeSize * 100) / 100 + '%'})
+ ({Math.round((day.incomeSize || 0) * 100) / 100 + '%'})
</View>
// Similarly for expenseSize
<View style={{ marginLeft: 4 }}>
- ({Math.round(day.expenseSize * 100) / 100 + '%'})
+ ({Math.round((day.expenseSize || 0) * 100) / 100 + '%'})
</View>Likely invalid or redundant comment.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (4)
Line range hint 106-155: Dependencies in useCallback hooks are correctly specified
All useCallback hooks (onSave, onSaveNotes, onEditNotes, onCloseAccount, onReopenAccount, onClick) have their dependencies properly included. This ensures optimal memoization and prevents unnecessary re-renders.
249-253: Verify that syncAndDownload handles the new accountId values
The accountId prop has been expanded to include 'budgeted', 'offbudget', and 'uncategorized'. Ensure that the syncAndDownload(accountId) function can handle these string literals appropriately, as it might expect a valid account ID.
You can verify this by checking the implementation of syncAndDownload:
✅ Verification successful
Based on the code analysis, I can now generate the final response:
syncAndDownload is safe with special account IDs
The special account IDs ('budgeted', 'offbudget', 'uncategorized') are only used for filtering transactions in the UI layer through the accountFilter function. The syncAndDownload function and its underlying bank sync implementation (accounts-bank-sync handler) only process actual account IDs:
- When
accountIdis undefined, it syncs all accounts - When
accountIdis provided, it's validated by the SQL query inaccounts-bank-synchandler which only matches real account IDs - The special IDs will simply result in no accounts being found and no sync occurring
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'syncAndDownload' supports non-standard account IDs.
# Locate the 'syncAndDownload' function definition
rg 'function syncAndDownload\('
# Search for any type checks or usages of 'accountId' within the function
rg 'accountId' -A 5 -B 2 $(fd '.*\.js$' src/)
Length of output: 138441
Script:
#!/bin/bash
# Search for syncAndDownload function implementation
rg -A 10 "export function syncAndDownload" packages/loot-core/src/client/actions/sync.ts
# Search for accountFilter function to understand how special account IDs are handled
rg -A 15 "export function accountFilter" packages/loot-core/src/client/queries.ts
# Search for bank-sync API handler to see how accountId is used
rg -A 10 "handlers\['api/bank-sync'\]" packages/loot-core/src/server/api.ts
Length of output: 1512
Script:
#!/bin/bash
# Search for accounts-bank-sync handler to see how accountId is validated
rg -A 10 "handlers\['accounts-bank-sync'\]" packages/loot-core/src/server/
# Search for syncAccounts function which is called by syncAndDownload
rg -A 10 "export function syncAccounts" packages/loot-core/src/client/actions/sync.ts
# Search for any validation of accountId in sync-related functions
rg -A 5 "accountId.*=.*('budgeted'|'offbudget'|'uncategorized')" packages/loot-core/src/
Length of output: 3583
255-259: Ensure markAccountRead supports the expanded accountId types
Similar to syncAndDownload, verify that markAccountRead(accountId) can handle the string literals 'budgeted', 'offbudget', and 'uncategorized' without causing errors or unintended behavior.
To confirm, inspect the markAccountRead function:
✅ Verification successful
Based on the search results, I can now generate the final response since we have found clear evidence that markAccountRead is designed to handle the special account ID types through the shared type system. The accountId parameter is consistently typed across the codebase to accept AccountEntity['id'] | 'budgeted' | 'offbudget' | 'uncategorized', which matches the types used in the component.
markAccountRead fully supports all account ID types
The function is designed to work with both regular account IDs and the special literals 'budgeted', 'offbudget', and 'uncategorized'. This is verified by:
- The shared type system in the codebase that consistently defines
accountIdto accept these values - The
accountFilterfunction that properly handles these special account types - The consistent usage of these types across queries and data hooks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'markAccountRead' accepts non-standard account IDs.
# Find the definition of 'markAccountRead'
rg 'function markAccountRead\('
# Examine how 'accountId' is used within the function
rg 'accountId' -A 5 -B 2 $(fd '.*\.js$' src/)
Length of output: 138441
Script:
#!/bin/bash
# Search for markAccountRead function definition and usage
rg "markAccountRead" -A 5 -B 2 packages/loot-core/src/
Length of output: 589
Script:
#!/bin/bash
# Let's check how accountId is used in queries and filters to understand supported types
rg "accountFilter" -A 10 packages/loot-core/src/client/queries.ts
# Also check for any type definitions related to accountId
rg "accountId.*:.*('budgeted'|'offbudget'|'uncategorized')" packages/loot-core/src/
Length of output: 1063
224-228: Confirm that queries.transactions accepts the new accountId values
In baseTransactionsQuery, queries.transactions(accountId) is called with accountId that may now be 'budgeted', 'offbudget', or 'uncategorized'. Ensure that the transactions query function is designed to handle these values correctly.
You can verify this by reviewing the queries.transactions function:
✅ Verification successful
The queries.transactions function correctly handles the new accountId values
The implementation shows that queries.transactions is designed to handle all the specified accountId values:
- For
'budgeted': Returns transactions from non-closed accounts that are on-budget - For
'offbudget': Returns transactions from non-closed accounts that are off-budget - For
'uncategorized': Returns transactions that are uncategorized and from on-budget accounts - For regular account IDs: Returns transactions from that specific account
The code is properly handling all these cases through the accountFilter function which generates the appropriate query filters for each type of accountId.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'queries.transactions' supports the expanded accountId types.
# Locate the 'transactions' query function definition
rg 'function transactions\('
# Review how 'accountId' is processed within the function
rg 'accountId' -A 5 -B 2 $(fd '.*\.js$' src/)
Length of output: 138441
Script:
#!/bin/bash
# Let's check the implementation of queries.transactions function
rg -A 10 "export function transactions" packages/loot-core/src/client/queries.ts
Length of output: 726
Script:
#!/bin/bash
# Let's check the implementation of accountFilter function which handles the special values
rg -A 15 "export function accountFilter" packages/loot-core/src/client/queries.ts
Length of output: 531
Script:
#!/bin/bash
# Let's see the complete accountFilter implementation to understand how it handles all cases
rg -A 30 "export function accountFilter" packages/loot-core/src/client/queries.ts
Length of output: 897
packages/loot-core/src/client/query-helpers.ts (1)
62-62:
Clarify the type of _unsubscribeSyncEvent to avoid confusion with void | null union
The current type declaration () => void | null for _unsubscribeSyncEvent is confusing because it implies the function returns void or null. If the intention is that _unsubscribeSyncEvent is either a function returning void or null, you should adjust the parentheses to correctly represent this.
Apply this diff to fix the type:
-private _unsubscribeSyncEvent: () => void | null;
+private _unsubscribeSyncEvent: (() => void) | null;Likely invalid or redundant comment.
packages/desktop-client/src/components/reports/reports/CalendarCard.tsx (1)
1-522: Overall implementation is solid and well-structured
The CalendarCard and CalendarCardInner components are well-designed, making effective use of React hooks and memoization. The code is readable and follows consistent styling conventions.
🧰 Tools
🪛 Biome
[error] 487-487: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 494-494: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 501-501: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 508-508: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 515-515: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| Promise.all( | ||
| baseTrans.map(transaction => send('rules-run', { transaction })), | ||
| ).then(newTrans => { | ||
| const withDefaults = newTrans.map(t => ({ | ||
| ...t, | ||
| category: scheduleData.statuses.get(t.schedule), | ||
| schedule: t.schedule, | ||
| subtransactions: t.subtransactions?.map((st: TransactionEntity) => ({ | ||
| ...st, | ||
| id: 'preview/' + st.id, | ||
| schedule: t.schedule, | ||
| subtransactions: t.subtransactions?.map((st: TransactionEntity) => ({ | ||
| ...st, | ||
| id: 'preview/' + st.id, | ||
| schedule: t.schedule, | ||
| })), | ||
| })); | ||
| setPreviewTransactions(ungroupTransactions(withDefaults)); | ||
| if (collapseTransactions) { | ||
| collapseTransactions(withDefaults.map(t => t.id)); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| return previewTransactions; | ||
| })), | ||
| })); | ||
| setPreviewTransactions(ungroupTransactions(withDefaults)); | ||
| if (collapseTransactions) { | ||
| collapseTransactions(withDefaults.map(t => t.id)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Promise.all operation.
The Promise.all operation should include error handling to gracefully handle failures during rules processing.
Consider adding error handling:
Promise.all(
baseTrans.map(transaction => send('rules-run', { transaction })),
- ).then(newTrans => {
+ )
+ .then(newTrans => {
const withDefaults = newTrans.map(t => ({
...t,
category: scheduleData.statuses.get(t.schedule),
schedule: t.schedule,
subtransactions: t.subtransactions?.map((st: TransactionEntity) => ({
...st,
id: 'preview/' + st.id,
schedule: t.schedule,
})),
}));
setPreviewTransactions(ungroupTransactions(withDefaults));
if (collapseTransactions) {
collapseTransactions(withDefaults.map(t => t.id));
}
+ })
+ .catch(error => {
+ console.error('Failed to process preview transactions:', error);
+ setPreviewTransactions([]);
+ });📝 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.
| Promise.all( | |
| baseTrans.map(transaction => send('rules-run', { transaction })), | |
| ).then(newTrans => { | |
| const withDefaults = newTrans.map(t => ({ | |
| ...t, | |
| category: scheduleData.statuses.get(t.schedule), | |
| schedule: t.schedule, | |
| subtransactions: t.subtransactions?.map((st: TransactionEntity) => ({ | |
| ...st, | |
| id: 'preview/' + st.id, | |
| schedule: t.schedule, | |
| subtransactions: t.subtransactions?.map((st: TransactionEntity) => ({ | |
| ...st, | |
| id: 'preview/' + st.id, | |
| schedule: t.schedule, | |
| })), | |
| })); | |
| setPreviewTransactions(ungroupTransactions(withDefaults)); | |
| if (collapseTransactions) { | |
| collapseTransactions(withDefaults.map(t => t.id)); | |
| } | |
| }); | |
| } | |
| return previewTransactions; | |
| })), | |
| })); | |
| setPreviewTransactions(ungroupTransactions(withDefaults)); | |
| if (collapseTransactions) { | |
| collapseTransactions(withDefaults.map(t => t.id)); | |
| } | |
| }); | |
| Promise.all( | |
| baseTrans.map(transaction => send('rules-run', { transaction })), | |
| ) | |
| .then(newTrans => { | |
| const withDefaults = newTrans.map(t => ({ | |
| ...t, | |
| category: scheduleData.statuses.get(t.schedule), | |
| schedule: t.schedule, | |
| subtransactions: t.subtransactions?.map((st: TransactionEntity) => ({ | |
| ...st, | |
| id: 'preview/' + st.id, | |
| schedule: t.schedule, | |
| })), | |
| })); | |
| setPreviewTransactions(ungroupTransactions(withDefaults)); | |
| if (collapseTransactions) { | |
| collapseTransactions(withDefaults.map(t => t.id)); | |
| } | |
| }) | |
| .catch(error => { | |
| console.error('Failed to process preview transactions:', error); | |
| setPreviewTransactions([]); | |
| }); |
| const onAction = useCallback( | ||
| async (name: ScheduleItemAction, id: ScheduleEntity['id']) => { | ||
| switch (name) { | ||
| case 'post-transaction': | ||
| await send('schedule/post-transaction', { id }); | ||
| break; | ||
| case 'skip': | ||
| await send('schedule/skip-next-date', { id }); | ||
| break; | ||
| case 'complete': | ||
| await send('schedule/update', { | ||
| schedule: { id, completed: true }, | ||
| }); | ||
| break; | ||
| case 'restart': | ||
| await send('schedule/update', { | ||
| schedule: { id, completed: false }, | ||
| resetNextDate: true, | ||
| }); | ||
| break; | ||
| case 'delete': | ||
| await send('schedule/delete', { id }); | ||
| break; | ||
| default: | ||
| throw new Error(`Unknown action: ${name}`); | ||
| } | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add send to the dependency array of onAction.
While the function implementation is solid, the empty dependency array ([]) in useCallback should include the send function since it's used within the callback.
const onAction = useCallback(
async (name: ScheduleItemAction, id: ScheduleEntity['id']) => {
// ... existing implementation
},
- [],
+ [send],
);📝 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 onAction = useCallback( | |
| async (name: ScheduleItemAction, id: ScheduleEntity['id']) => { | |
| switch (name) { | |
| case 'post-transaction': | |
| await send('schedule/post-transaction', { id }); | |
| break; | |
| case 'skip': | |
| await send('schedule/skip-next-date', { id }); | |
| break; | |
| case 'complete': | |
| await send('schedule/update', { | |
| schedule: { id, completed: true }, | |
| }); | |
| break; | |
| case 'restart': | |
| await send('schedule/update', { | |
| schedule: { id, completed: false }, | |
| resetNextDate: true, | |
| }); | |
| break; | |
| case 'delete': | |
| await send('schedule/delete', { id }); | |
| break; | |
| default: | |
| throw new Error(`Unknown action: ${name}`); | |
| } | |
| }, | |
| [], | |
| ); | |
| const onAction = useCallback( | |
| async (name: ScheduleItemAction, id: ScheduleEntity['id']) => { | |
| switch (name) { | |
| case 'post-transaction': | |
| await send('schedule/post-transaction', { id }); | |
| break; | |
| case 'skip': | |
| await send('schedule/skip-next-date', { id }); | |
| break; | |
| case 'complete': | |
| await send('schedule/update', { | |
| schedule: { id, completed: true }, | |
| }); | |
| break; | |
| case 'restart': | |
| await send('schedule/update', { | |
| schedule: { id, completed: false }, | |
| resetNextDate: true, | |
| }); | |
| break; | |
| case 'delete': | |
| await send('schedule/delete', { id }); | |
| break; | |
| default: | |
| throw new Error(`Unknown action: ${name}`); | |
| } | |
| }, | |
| [send], | |
| ); |
| async waitFor() { | ||
| await this.transactionTable.waitFor(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using waitFor in key interaction methods.
The waitFor method is a good addition for test stability. Consider calling it in key methods like enterSingleTransaction and createSplitTransaction to ensure the transaction table is ready before interactions:
async enterSingleTransaction(transaction) {
+ await this.waitFor();
await this.addNewTransactionButton.click();
await this._fillTransactionFields(this.newTransactionRow, transaction);
}
async createSplitTransaction([rootTransaction, ...transactions]) {
+ await this.waitFor();
await this.addNewTransactionButton.click();Committable suggestion skipped: line range outside the PR's diff.
| @@ -345,7 +347,7 @@ export function SchedulesTable({ | |||
| }); | |||
| }, [payees, accounts, schedules, filter, statuses]); | |||
|
|
|||
| const items: SchedulesTableItem[] = useMemo(() => { | |||
| const items: readonly SchedulesTableItem[] = useMemo(() => { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety by removing type assertion.
The items array is correctly marked as readonly, but there's a type assertion at line 430 that casts it back to ScheduleEntity[]. This defeats the purpose of readonly and might hide potential mutation bugs.
Consider updating the Table component's type to accept readonly arrays:
-items={items as ScheduleEntity[]}
+items={items}You'll need to update the Table component's props interface to accept readonly ScheduleEntity[].
Also applies to: 430-430
| const { isLoading, schedules = [] } = useCachedSchedules(); | ||
|
|
||
| if (isLoading) { | ||
| return null; | ||
| } | ||
|
|
||
| const schedule = scheduleId ? schedules.find(s => s.id === scheduleId) : null; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for schedule loading failure.
The schedule loading logic should handle potential errors that may occur during the fetch operation. Currently, it only checks if the data is loading but doesn't handle error states.
- const { isLoading, schedules = [] } = useCachedSchedules();
+ const { isLoading, error, schedules = [] } = useCachedSchedules();
- if (isLoading) {
+ if (isLoading || error) {
+ if (error) {
+ console.error('Failed to load schedules:', error);
+ }
return 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.
| const { isLoading, schedules = [] } = useCachedSchedules(); | |
| if (isLoading) { | |
| return null; | |
| } | |
| const schedule = scheduleId ? schedules.find(s => s.id === scheduleId) : null; | |
| const { isLoading, error, schedules = [] } = useCachedSchedules(); | |
| if (isLoading || error) { | |
| if (error) { | |
| console.error('Failed to load schedules:', error); | |
| } | |
| return null; | |
| } | |
| const schedule = scheduleId ? schedules.find(s => s.id === scheduleId) : null; |
| const name = newName || t('Net Worth'); | ||
| await send('dashboard-update-widget', { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update default widget name to 'Calendar'.
In the onSaveWidgetName function, the default name is set to 'Net Worth', which may not be appropriate for the Calendar component. Consider updating it to 'Calendar' to reflect the correct widget type.
Apply this diff to update the default widget name:
-const name = newName || t('Net Worth');
+const name = newName || t('Calendar');📝 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 name = newName || t('Net Worth'); | |
| await send('dashboard-update-widget', { | |
| const name = newName || t('Calendar'); | |
| await send('dashboard-update-widget', { |
| send('make-filters-from-conditions', { | ||
| conditions: conditions.filter(cond => !cond.customName), | ||
| }).then((data: { filters: unknown[] }) => { | ||
| let query = q('transactions') | ||
| .filter({ | ||
| [conditionsOpKey]: data.filters, | ||
| }) | ||
| .filter({ | ||
| $and: [ | ||
| { date: { $gte: monthUtils.firstDayOfMonth(start) } }, | ||
| { date: { $lte: monthUtils.lastDayOfMonth(end) } }, | ||
| ], | ||
| }) | ||
| .select('*'); | ||
|
|
||
| if (sortField) { | ||
| query = query.orderBy({ | ||
| [getField(sortField)]: ascDesc, | ||
| }); | ||
| } | ||
|
|
||
| setQuery(query); | ||
| }); | ||
| }, [start, end, conditions, conditionsOp, sortField, ascDesc]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for asynchronous operations.
The send('make-filters-from-conditions', ...) call lacks error handling, which could lead to unhandled promise rejections if the request fails. Adding error handling ensures the application can gracefully handle errors.
Apply this diff to add error handling:
useEffect(() => {
const conditionsOpKey = conditionsOp === 'or' ? '$or' : '$and';
- send('make-filters-from-conditions', {
+ send('make-filters-from-conditions', {
conditions: conditions.filter(cond => !cond.customName),
- }).then((data: { filters: unknown[] }) => {
+ })
+ .then((data: { filters: unknown[] }) => {
let query = q('transactions')
.filter({
[conditionsOpKey]: data.filters,
})
.filter({
$and: [
{ date: { $gte: monthUtils.firstDayOfMonth(start) } },
{ date: { $lte: monthUtils.lastDayOfMonth(end) } },
],
})
.select('*');
if (sortField) {
query = query.orderBy({
[getField(sortField)]: ascDesc,
});
}
setQuery(query);
- });
+ })
+ .catch(error => {
+ console.error('Failed to make filters from conditions:', error);
+ // Handle the error appropriately, e.g., show a notification to the user
+ });
}, [start, end, conditions, conditionsOp, sortField, ascDesc]);📝 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.
| send('make-filters-from-conditions', { | |
| conditions: conditions.filter(cond => !cond.customName), | |
| }).then((data: { filters: unknown[] }) => { | |
| let query = q('transactions') | |
| .filter({ | |
| [conditionsOpKey]: data.filters, | |
| }) | |
| .filter({ | |
| $and: [ | |
| { date: { $gte: monthUtils.firstDayOfMonth(start) } }, | |
| { date: { $lte: monthUtils.lastDayOfMonth(end) } }, | |
| ], | |
| }) | |
| .select('*'); | |
| if (sortField) { | |
| query = query.orderBy({ | |
| [getField(sortField)]: ascDesc, | |
| }); | |
| } | |
| setQuery(query); | |
| }); | |
| }, [start, end, conditions, conditionsOp, sortField, ascDesc]); | |
| send('make-filters-from-conditions', { | |
| conditions: conditions.filter(cond => !cond.customName), | |
| }) | |
| .then((data: { filters: unknown[] }) => { | |
| let query = q('transactions') | |
| .filter({ | |
| [conditionsOpKey]: data.filters, | |
| }) | |
| .filter({ | |
| $and: [ | |
| { date: { $gte: monthUtils.firstDayOfMonth(start) } }, | |
| { date: { $lte: monthUtils.lastDayOfMonth(end) } }, | |
| ], | |
| }) | |
| .select('*'); | |
| if (sortField) { | |
| query = query.orderBy({ | |
| [getField(sortField)]: ascDesc, | |
| }); | |
| } | |
| setQuery(query); | |
| }) | |
| .catch(error => { | |
| console.error('Failed to make filters from conditions:', error); | |
| // Handle the error appropriately, e.g., show a notification to the user | |
| }); | |
| }, [start, end, conditions, conditionsOp, sortField, ascDesc]); |
|
changed this back to WIP since it depends on useTransactions from @joel-jeremy and its not merged yet |
lelemm
force-pushed
the
calendar_report
branch
from
1f5ca75 to
ee050ba
Compare
lelemm
force-pushed
the
calendar_report
branch
from
ee050ba to
df17843
Compare
A calendar view report:
Actual.-.Google.Chrome.2024-10-31.15-43-38.mp4