feat: add "last reconciled" timestamp to accounts #4459
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 (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
Smaller No assets were smaller Unchanged No assets were unchanged |
tostasmistas
force-pushed
the
feat/accounts-last-reconciled
branch
2 times, most recently
from
5eaaf45 to
51a53f6
Compare
tostasmistas
changed the title
|
Warning Rate limit exceeded@tostasmistas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe changes enhance the reconciliation process for accounts across various components of the codebase. In the desktop client, the 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: 0
🧹 Nitpick comments (1)
packages/loot-core/src/mocks/index.ts (1)
25-25: Consider initializinglast_reconciledto null for new accounts.Currently, all mock accounts are initialized with the current timestamp as their
last_reconciledvalue, implying they've already been reconciled. This doesn't match the expected behavior where new accounts would start with a "Not yet reconciled" status.- last_reconciled: new Date().getTime().toString(), + last_reconciled: null,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4459.mdis excluded by!**/*.md
📒 Files selected for processing (8)
packages/desktop-client/src/components/accounts/Account.tsx(2 hunks)packages/desktop-client/src/components/accounts/Balance.tsx(5 hunks)packages/desktop-client/src/components/spreadsheet/index.ts(1 hunks)packages/loot-core/migrations/1740506588539_add_last_reconciled_at.sql(1 hunks)packages/loot-core/src/mocks/index.ts(1 hunks)packages/loot-core/src/server/accounts/app.ts(1 hunks)packages/loot-core/src/server/aql/schema/index.ts(1 hunks)packages/loot-core/src/types/models/account.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loot-core/migrations/1740506588539_add_last_reconciled_at.sql
🧰 Additional context used
🧠 Learnings (1)
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-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
🔇 Additional comments (10)
packages/desktop-client/src/components/spreadsheet/index.ts (1)
17-17: LGTM: Properly typed new property for account reconciliation.The
lastReconciledproperty has been correctly added to the account type with the appropriatestring | nulltype, allowing it to represent both reconciled accounts (with timestamp strings) and unreconciled accounts (with null values).packages/loot-core/src/types/models/account.d.ts (1)
7-7: LGTM: Properly typed core data model property.The
last_reconciledproperty has been correctly added to theAccountEntitytype with the appropriatestring | nulltype, consistent with other timestamp fields in the model and allowing for accounts that haven't been reconciled yet.packages/loot-core/src/server/aql/schema/index.ts (1)
77-77: LGTM: Schema field added with correct type.The
last_reconciledfield has been correctly added to the schema with the string type, which aligns with the type definition in theAccountEntityinterface.packages/loot-core/src/server/accounts/app.ts (1)
64-74: Good implementation for conditional field updates.The function signature has been properly updated to accept the new
last_reconciledfield using TypeScript's utility types, and the implementation uses a conditional spread operator to only include the field when it's provided. This maintains backward compatibility with existing code while supporting the new functionality.packages/desktop-client/src/components/accounts/Account.tsx (2)
1013-1018: Good defensive programming with account existence check.Adding this account existence check before attempting to update the last_reconciled timestamp helps prevent potential runtime errors and provides a clear error message for debugging.
1044-1049: Last reconciled timestamp implementation looks good.The code properly captures the current timestamp and updates the account record with the reconciliation time when the user completes reconciliation. This ensures the timestamp is updated in the database for future reference.
packages/desktop-client/src/components/accounts/Balance.tsx (4)
7-8: Appropriate imports for date handling.The date-fns library is a good choice for handling relative time formatting and provides good localization support.
150-159: Well-implemented relative time formatter.The
tsToRelativeTimefunction handles null values gracefully, correctly parses the timestamp, and properly applies localization based on the user's preferred language.One small suggestion: Add a fallback for invalid date parsing that could occur if the timestamp format changes in the future.
const tsToRelativeTime = (ts: string | null, language: string): string => { if (!ts) return 'Unknown'; const parsed = new Date(parseInt(ts, 10)); + if (isNaN(parsed.getTime())) return 'Unknown'; const locale = locales[language.replace('-', '') as keyof typeof locales] ?? locales['enUS']; return formatDistanceToNow(parsed, { addSuffix: true, locale }); };
188-208: Good UI implementation for reconciliation status.The UI element is consistent with the existing design language and provides useful information to the user about the reconciliation status of the account.
294-299: Proper prop passing for the reconciliation status.The code correctly passes the
last_reconciledproperty from the account object to theMoreBalancescomponent, maintaining the data flow.
| locales[language.replace('-', '') as keyof typeof locales] ?? | ||
| locales['enUS']; | ||
|
|
||
| return formatDistanceToNow(parsed, { addSuffix: true, locale }); |
There was a problem hiding this comment.
We can also use formatDistanceToNowStrict which will not include the prefix "about".
|
I was thinking about the location. I don't love that it's always there if you want to view the cleared amounts, but I don't have any better ideas yet |
What about a popover when you hover the reconcile button? Downside is that it's not as prominent or discoverable. |
|
just a thought, what about an open padlock icon when no recon has been performed, closed once it has, and the last recon date in the tool tip? Could also be not very noticeable tho |
|
Yeah, I completely understand all the opinions, I too couldn't come up with a better place for it. I also don't know if it's something people prefer to always have visible at a glance, which would be the issue with having it in a tooltip. |
|
Tool tip would be best imo, plenty of people never use reconcile, but do monitor cleared vs not. |
add a `last_reconciled` column to the `accounts` table, which stores a UNIX timestamp indicating when each account was last reconciled upon initial release, all accounts will display "Not yet reconciled"; however, after completing reconciliation and pressing the "Done reconciling" button, the timestamp will be updated accordingly
generate mock accounts with the `last_reconciled` value set to `null`, to match the expected behaviour that new accounts will start with a "Not yet reconciled" status
tostasmistas
force-pushed
the
feat/accounts-last-reconciled
branch
from
49be922 to
a42b559
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/loot-core/src/shared/util.ts (1)
490-499: Function implementation looks good, but could be more robust.The timestamp conversion function is well-implemented with proper error handling and locale support. However, there are a few improvements that could be made:
Consider these enhancements:
export function tsToRelativeTime(ts: string | null, language: string): string { - if (!ts) return 'Unknown'; + if (!ts || ts === '0') return 'Unknown'; - const parsed = new Date(parseInt(ts, 10)); + const timestamp = parseInt(ts, 10); + if (isNaN(timestamp) || timestamp <= 0) return 'Unknown'; + const parsed = new Date(timestamp); const locale = locales[language.replace('-', '') as keyof typeof locales] ?? locales['enUS']; - return formatDistanceToNow(parsed, { addSuffix: true, locale }); + try { + return formatDistanceToNow(parsed, { addSuffix: true, locale }); + } catch (error) { + console.error('Error formatting relative time:', error); + return 'Unknown'; + } }This improves error handling for invalid timestamps and wraps the date formatting in a try-catch block to prevent potential runtime errors.
packages/desktop-client/src/components/accounts/Header.tsx (3)
390-393: Consider extracting tooltip content to a separate function.For better readability and maintainability, consider extracting the tooltip content logic to a separate function.
+ const getReconciliationTooltipContent = (account: AccountEntity | undefined, language: string) => { + return account?.last_reconciled + ? `${t('Reconciled')} ${tsToRelativeTime(account?.last_reconciled, language || 'en-US')}` + : t('Not yet reconciled'); + }; // Then in the JSX: content={getReconciliationTooltipContent(account, language)}
391-391: Check for possible null reference.There's a potential null reference issue when accessing
account?.last_reconciledwhile also using the optional chaining operator within the tsToRelativeTime function call.-`${t('Reconciled')} ${tsToRelativeTime(account?.last_reconciled, language || 'en-US')}` +`${t('Reconciled')} ${tsToRelativeTime(account.last_reconciled, language || 'en-US')}`Since you're already checking if
account?.last_reconciledexists in the conditional, you can safely useaccount.last_reconciledwithin the function call.
408-410: Unnecessary View wrapper around the icon.The SvgLockClosed icon is wrapped in a View component that doesn't provide any styling or functionality.
-<View> <SvgLockClosed width={14} height={14} /> -</View>Removing the unnecessary View component would simplify the code without changing the functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4459.mdis excluded by!**/*.md
📒 Files selected for processing (10)
packages/desktop-client/src/components/accounts/Account.tsx(2 hunks)packages/desktop-client/src/components/accounts/Balance.tsx(1 hunks)packages/desktop-client/src/components/accounts/Header.tsx(5 hunks)packages/desktop-client/src/components/spreadsheet/index.ts(1 hunks)packages/loot-core/migrations/1740506588539_add_last_reconciled_at.sql(1 hunks)packages/loot-core/src/mocks/index.ts(1 hunks)packages/loot-core/src/server/accounts/app.ts(1 hunks)packages/loot-core/src/server/aql/schema/index.ts(1 hunks)packages/loot-core/src/shared/util.ts(2 hunks)packages/loot-core/src/types/models/account.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/accounts/Balance.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/loot-core/src/types/models/account.d.ts
- packages/loot-core/migrations/1740506588539_add_last_reconciled_at.sql
- packages/desktop-client/src/components/spreadsheet/index.ts
- packages/loot-core/src/server/aql/schema/index.ts
- packages/loot-core/src/mocks/index.ts
- packages/loot-core/src/server/accounts/app.ts
- packages/desktop-client/src/components/accounts/Account.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (3)
packages/desktop-client/src/components/accounts/Header.tsx (3)
17-17: New imports look good.Appropriate imports added for the new functionality.
Also applies to: 20-20, 28-28
191-192: Good use of global preferences.Correctly uses the useGlobalPref hook to get the user's language preference for localized timestamp display.
384-425: Great enhancement to the reconciliation UI.The tooltip implementation adds valuable context about the account's reconciliation status without cluttering the interface. The conditional rendering and placement are well thought out.
Nicely done with:
- Conditionally showing either "Reconciled X time ago" or "Not yet reconciled"
- Disabling the tooltip when the reconciliation menu is open to avoid UI conflicts
- Proper positioning of the tooltip ("top start")
- Leveraging the new tsToRelativeTime utility with localization support
|
I have relocated the timestamp from the account balances header to a tooltip displayed when hovering over the "Reconcile" button. I think this looks good and is practical for those that make use of reconciliation.
|
tostasmistas
force-pushed
the
feat/accounts-last-reconciled
branch
from
5347320 to
6f0d8d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/loot-core/src/shared/util.ts (3)
490-499: Add error handling for timestamp parsing and consider more robust date validationThe function works correctly for valid timestamps, but could be improved with better error handling and validation.
export function tsToRelativeTime(ts: string | null, language: string): string { if (!ts) return 'Unknown'; - const parsed = new Date(parseInt(ts, 10)); + // Handle potential parsing errors and invalid dates + try { + const parsedTimestamp = parseInt(ts, 10); + if (isNaN(parsedTimestamp)) return 'Unknown'; + + const parsed = new Date(parsedTimestamp); + if (isNaN(parsed.getTime())) return 'Unknown'; + + const locale = + locales[language.replace('-', '') as keyof typeof locales] ?? + locales['enUS']; + + return formatDistanceToNow(parsed, { addSuffix: true, locale }); + } catch (error) { + console.error('Error parsing timestamp:', error); + return 'Unknown'; + } - const locale = - locales[language.replace('-', '') as keyof typeof locales] ?? - locales['enUS']; - - return formatDistanceToNow(parsed, { addSuffix: true, locale }); }
488-489: Add JSDoc comments for the date utilities sectionAdding documentation for the new date utilities section would improve code maintainability.
// Date utilities +/** + * Date-related utility functions for formatting and displaying dates + */
490-490: Add JSDoc comments for the tsToRelativeTime functionAdding documentation for the new function would improve code usability and maintainability.
+/** + * Converts a UNIX timestamp string to a human-readable relative time format + * @param ts - UNIX timestamp in milliseconds as a string, or null + * @param language - Language code (e.g., 'en-US', 'fr-FR') for localization + * @returns A localized string describing the relative time (e.g., "2 days ago") or "Unknown" if invalid + */ export function tsToRelativeTime(ts: string | null, language: string): string {packages/desktop-client/src/components/accounts/Header.tsx (2)
384-398: Improve tooltip configuration for better accessibility and user experienceThe current tooltip implementation could be enhanced for better accessibility and user experience, especially considering the discussions about visibility preferences mentioned in the PR objectives.
Consider adding a delay to the tooltip to prevent it from appearing too quickly when users hover over the button unintentionally:
<Tooltip style={{ ...styles.tooltip, marginBottom: 10, }} content={ account?.last_reconciled ? `${t('Reconciled')} ${tsToRelativeTime(account.last_reconciled, language || 'en-US')}` : t('Not yet reconciled') } placement="top start" + delay={500} triggerProps={{ isDisabled: reconcileOpen, }} >
391-392: Ensure consistent translation approach for reconciliation timestamp displayWhile the implementation correctly uses the translation function for static text, the combined message could be improved to ensure proper localization.
Consider using a translation parameter instead of string concatenation to allow for different sentence structures in different languages:
content={ account?.last_reconciled - ? `${t('Reconciled')} ${tsToRelativeTime(account.last_reconciled, language || 'en-US')}` + ? t('Reconciled {{timeAgo}}', { + timeAgo: tsToRelativeTime(account.last_reconciled, language || 'en-US') + }) : t('Not yet reconciled') }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/desktop-client/src/components/accounts/Balance.tsx(1 hunks)packages/desktop-client/src/components/accounts/Header.tsx(5 hunks)packages/loot-core/src/shared/util.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/accounts/Balance.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
packages/loot-core/src/shared/util.ts (1)
2-3: Optimize bundle size by importing specific date-fns localesFollowing up on the previous review, importing all locales from 'date-fns/locale' will significantly increase your bundle size. Since we can see you're using it selectively based on language in the
tsToRelativeTimefunction, you should consider importing only the specific locales needed.import { formatDistanceToNow } from 'date-fns'; -import * as locales from 'date-fns/locale'; +// Import only the locales you need or use dynamic imports +import { enUS } from 'date-fns/locale'; +// Other commonly used locales can be imported as neededIf you need to support multiple locales dynamically, consider implementing a more efficient approach such as dynamic imports or a locale registry pattern.
packages/desktop-client/src/components/accounts/Header.tsx (1)
384-425: Implementation successfully addresses the PR objectivesThe tooltip implementation provides an elegant solution to show the reconciliation status without cluttering the interface. This aligns well with the discussion points in the PR comments, where users expressed concerns about constant visibility and preferences for more flexible solutions.
The tooltip approach provides:
- Information about reconciliation status that's discoverable but not always visible
- A non-intrusive way to display the timestamp
- Localized relative time formatting that's easily understood
This implementation balances visibility and minimalism effectively, addressing the concerns raised in the PR discussion.
tostasmistas
force-pushed
the
feat/accounts-last-reconciled
branch
from
6f0d8d2 to
d131dde
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/loot-core/src/shared/util.ts (1)
488-499: Improve error handling and type safety in the new timestamp utilityThe current implementation works for valid timestamps but could be improved with better error handling and type safety.
export function tsToRelativeTime(ts: string | null, language: string): string { if (!ts) return 'Unknown'; - const parsed = new Date(parseInt(ts, 10)); + try { + const timestamp = parseInt(ts, 10); + + // Check if the parsed timestamp is valid + if (isNaN(timestamp)) { + return 'Invalid date'; + } + + const parsed = new Date(timestamp); + + // Normalize language code to match date-fns locale format + const localeKey = language.replace('-', '') as keyof typeof locales; + const locale = locales[localeKey] ?? locales['enUS']; - const locale = - locales[language.replace('-', '') as keyof typeof locales] ?? - locales['enUS']; - return formatDistanceToNow(parsed, { addSuffix: true, locale }); + return formatDistanceToNow(parsed, { addSuffix: true, locale }); + } catch (error) { + console.error('Error formatting relative time:', error); + return 'Unknown'; + } }This implementation:
- Adds a try/catch block to handle any unexpected errors
- Validates the parsed timestamp before creating a Date object
- Improves code readability
- Provides better error recovery
packages/desktop-client/src/components/accounts/Header.tsx (1)
390-393: Consider adding fallback text for internationalizationThe conditional rendering for the tooltip content is good, but should provide a fallback for missing translations.
content={ account?.last_reconciled - ? `${t('Reconciled')} ${tsToRelativeTime(account.last_reconciled, language || 'en-US')}` - : t('Not yet reconciled') + ? `${t('Reconciled', 'Reconciled')} ${tsToRelativeTime(account.last_reconciled, language || 'en-US')}` + : t('Not yet reconciled', 'Not yet reconciled') }This ensures that even if translations are missing, the user will still see readable text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/desktop-client/src/components/accounts/Balance.tsx(1 hunks)packages/desktop-client/src/components/accounts/Header.tsx(4 hunks)packages/loot-core/src/shared/util.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/accounts/Balance.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (4)
packages/loot-core/src/shared/util.ts (1)
2-3: Bundle size optimization needed for date-fns importsThe current implementation imports all locales from date-fns, which will significantly increase bundle size.
Per the previous review comment, instead of importing all locales, consider importing only the specific locales needed or implement dynamic imports to improve tree-shaking and reduce bundle size.
import { formatDistanceToNow } from 'date-fns'; -import * as locales from 'date-fns/locale'; +// Import only specific locales that are needed +import { enUS, de, fr } from 'date-fns/locale'; + +// Create a map of supported locales +const supportedLocales: Record<string, Locale> = { enUS, de, fr };packages/desktop-client/src/components/accounts/Header.tsx (3)
17-17: LGTM! Appropriate imports for the new functionality.The imports are correctly added to support the tooltip functionality and relative time display.
Also applies to: 20-20, 28-28
191-192: LGTM! Clean implementation of language preference.Using
useGlobalPref('language')is the appropriate way to access user language settings for localization.
382-426: Good improvement to the reconciliation UX with tooltipThis tooltip implementation provides valuable context about reconciliation status to users, making the feature more intuitive and informative.
The tooltip is well-implemented with:
- Clear conditional content based on reconciliation status
- Proper positioning above the button
- Appropriate disabling when the reconciliation menu is open
One small suggestion to consider: add a simple fade-in animation to make the tooltip appearance more polished.
relocate the "last reconciled" timestamp from the account balances header to a tooltip displayed when hovering over the "Reconcile" button
tostasmistas
force-pushed
the
feat/accounts-last-reconciled
branch
from
d131dde to
7b263f7
Compare
|
Since this uses locales would be good to use the hook useLocale from this PR #4211 |
This PR resolves #2967.
It adds a
last_reconciledcolumn to theaccountstable, which stores a UNIX timestamp indicating when each account was last reconciled. Upon initial release, all accounts will display "Not yet reconciled"; however, after completing reconciliation and pressing the "Done reconciling" button, the timestamp will be updated accordingly.