show all occurrences of upcoming schedules within the upcoming period #4166
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 (largest 100 files by percent change)
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
|
/update-vrt |
matt-fidd
changed the title
WalkthroughThe pull request introduces enhancements to transaction handling and scheduling across multiple files in the Actual app. The changes primarily focus on improving the representation of transaction statuses by adding a new property, Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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: 5
🧹 Nitpick comments (6)
packages/loot-core/src/client/data-hooks/transactions.ts (2)
148-149: Ensure default value forupcomingLengthwhen user preference is unavailableWhen
upcomingLengthisnullor undefined, theparseIntfunction defaults to'7', but it's safer to set a default value upfront to prevent unexpected behavior.Initialize
upcomingLengthwith a default value using the nullish coalescing operator:const [upcomingLength] = useSyncedPref('upcomingScheduledTransactionLength'); +const upcomingLengthValue = upcomingLength ?? '7';And then use
upcomingLengthValuein subsequent code.
3-4: Organize imports for better readability and maintainabilityThere's an opportunity to group related imports together and remove any unused imports to enhance code clarity.
Consider organizing the imports as follows:
import { useEffect, useRef, useState, useMemo, useCallback } from 'react'; import debounce from 'lodash/debounce'; import { send } from '../../platform/client/fetch'; import { type Query } from '../../shared/query'; +import { useSyncedPref } from '@actual-app/web/src/hooks/useSyncedPref'; +import * as d from 'date-fns'; +import { currentDay, addDays, parseDate } from '../../shared/months'; +import { + getScheduledAmount, + extractScheduleConds, + getNextDate, +} from '../../shared/schedules';This groups external library imports separately from internal modules.
Also applies to: 8-14
packages/loot-core/src/shared/schedules.ts (2)
314-327: ValidatesolveModeingetDateWithSkippedWeekendto prevent errorsThe function assumes that
solveModeis either'after'or'before', but if an invalid value is passed, it throws a generic error.Add validation for
solveModeand provide a clearer error message:export function getDateWithSkippedWeekend( date: Date, solveMode: 'after' | 'before', ) { + if (solveMode !== 'after' && solveMode !== 'before') { + throw new Error(`Invalid solveMode '${solveMode}'. Expected 'after' or 'before'.`); + } if (d.isWeekend(date)) { if (solveMode === 'after') { return d.nextMonday(date); } else { return d.previousFriday(date); } } return date; }
319-322: Simplify the logic ingetDateWithSkippedWeekendThe
if-elsestructure can be simplified for better readability.Refactor the code:
if (solveMode === 'after') { return d.nextMonday(date); - } else if (solveMode === 'before') { + } + // solveMode === 'before' return d.previousFriday(date);Since the invalid
solveModecase is already handled, anelseclause is not necessary.packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2)
1033-1035: Consider using early returns for clearer status precedence.The current logic could be more explicit about status precedence. Consider refactoring to:
- let previewStatus = categoryId; - if (upcoming) previewStatus = 'upcoming'; + let previewStatus; + if (upcoming) { + previewStatus = 'upcoming'; + } else { + previewStatus = categoryId; + }
1611-1613: Simplify nested ternary operators for better readability.The status determination logic using nested ternary operators is hard to read. Consider refactoring to:
- status={ - isPreview - ? upcoming === true - ? 'upcoming' - : categoryId - : reconciled - ? 'reconciled' - : cleared - ? 'cleared' - : null - } + status={(() => { + if (isPreview) { + return upcoming ? 'upcoming' : categoryId; + } + if (reconciled) { + return 'reconciled'; + } + if (cleared) { + return 'cleared'; + } + return null; + })()}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4166.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx(4 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(3 hunks)packages/loot-core/src/server/budget/goalsSchedule.ts(1 hunks)packages/loot-core/src/server/schedules/app.test.ts(1 hunks)packages/loot-core/src/server/schedules/app.ts(1 hunks)packages/loot-core/src/shared/schedules.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loot-core/src/server/budget/goalsSchedule.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: Analyze
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (6)
packages/loot-core/src/server/schedules/app.test.ts (2)
5-5: Ensure consistent usage ofgetNextDateacross the codebaseBy importing
getNextDatefrom'../../shared/schedules', the test now uses the shared implementation, promoting consistency. Ensure that the sharedgetNextDatefunction behaves as expected in the test context.Run the tests to confirm that
getNextDateworks correctly with the shared implementation.
Line range hint
75-80: Update test cases to reflect changes ingetNextDatebehaviorSince
getNextDateis now imported from a shared module, there may be differences in its behavior. Verify that the test cases still accurately test the intended functionality.Ensure that the test inputs and expected outputs are still valid with the new
getNextDatefunction.packages/loot-core/src/shared/schedules.ts (2)
285-287: Ensure correct handling of condition values ingetNextDateWhen creating a new
Conditioninstance, verify that the condition value is valid and handle any potential errors.Add error checking to confirm that
cond.getValue()returns a valid value.
300-308: Account for possible empty dates array ingetNextDateIf no dates are found after attempting to get occurrences,
datesmay still be empty, leading to anullreturn. Ensure this is the intended behavior or handle it appropriately.Consider providing a default date or logging a warning if no dates are found.
packages/loot-core/src/server/schedules/app.ts (1)
16-17: LGTM! Good refactoring of date utilities.Moving date-related utility functions to a shared module improves code reusability and maintainability.
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
1028-1032: LGTM! Clean addition of the upcoming property.The
upcomingproperty is properly integrated into the transaction object destructuring.
|
@youngcw sorry, small variable naming change |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/loot-core/src/client/data-hooks/transactions.ts (2)
3-3: Consider moving shared hooks to a common package.The import from
@actual-app/webcrosses package boundaries. Consider moving shared hooks to a common package to maintain better architectural boundaries and prevent tight coupling between packages.
165-209: Consider memoizing date parsing operations.The implementation correctly handles schedule generation and date calculations. However,
parseDateis called multiple times within the loop and sort comparator. Consider memoizing parsed dates to optimize performance, especially for large schedules.const dates: Set<string> = new Set(); + const parsedDates = new Map<string, Date>(); + const getOrParseDateMemo = (date: string) => { + let parsed = parsedDates.get(date); + if (!parsed) { + parsed = parseDate(date); + parsedDates.set(date, parsed); + } + return parsed; + }; while (day <= upcomingPeriodEnd) { const nextDate = getNextDate(dateConditions, day); - day = parseDate(addDays(nextDate, 1)); + day = getOrParseDateMemo(addDays(nextDate, 1)); if (dates.has(nextDate)) break; - if (parseDate(nextDate) > upcomingPeriodEnd) break; + if (getOrParseDateMemo(nextDate) > upcomingPeriodEnd) break; dates.add(nextDate); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx(4 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/client/data-hooks/transactions.ts (3)
Learnt from: matt-fidd
PR: actualbudget/actual#4166
File: packages/loot-core/src/client/data-hooks/transactions.ts:155-158
Timestamp: 2025-01-16T14:30:36.518Z
Learning: In packages/loot-core/src/client/data-hooks/transactions.ts, the `upcomingLength` preference is always stored as a number in string format, so no additional type checking is needed when using `parseInt`.
Learnt from: matt-fidd
PR: actualbudget/actual#4166
File: packages/loot-core/src/client/data-hooks/transactions.ts:200-200
Timestamp: 2025-01-16T14:29:13.188Z
Learning: In the scheduled transactions implementation within `packages/loot-core/src/client/data-hooks/transactions.ts`, the `upcoming` flag is set based on `schedules.length > 0` to act as an override, where the first occurrence gets `false` and subsequent occurrences get `true`. This is intentional and should not be changed to date-based comparison.
Learnt from: matt-fidd
PR: actualbudget/actual#4166
File: packages/loot-core/src/client/data-hooks/transactions.ts:173-181
Timestamp: 2025-01-16T14:29:03.337Z
Learning: The `getNextDate` function in `packages/loot-core/src/shared/schedules.ts` is designed to always return a date value, either directly from the condition value or after processing schedule occurrences. It does not return null.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
packages/loot-core/src/client/data-hooks/transactions.ts (2)
148-158: LGTM! Clean implementation of the upcoming period calculation.The implementation correctly uses the synced preference for
upcomingLengthand properly calculates the upcoming period end date.
258-258: LGTM! Dependencies are correctly specified.The effect dependencies are properly updated to include
upcomingLength, ensuring the effect runs when the preference changes.
|
This should probable pull in the other change first before merging to check all is still good. So far all is good other than this still includes the message about only one instance |
related to #3660