Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ (typescript) convert Balance.jsx to .tsx #3874

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useRef } from 'react';
import React, { useRef, type ComponentProps } from 'react';
import { useTranslation } from 'react-i18next';

import { useHover } from 'usehooks-ts';
Expand All @@ -7,6 +7,7 @@ import { isPreviewId } from 'loot-core/shared/transactions';
import { useCachedSchedules } from 'loot-core/src/client/data-hooks/schedules';
import { q } from 'loot-core/src/shared/query';
import { getScheduledAmount } from 'loot-core/src/shared/schedules';
import { type AccountEntity } from 'loot-core/types/models';

import { useSelectedItems } from '../../hooks/useSelected';
import { SvgArrowButtonRight1 } from '../../icons/v2';
Expand All @@ -16,10 +17,23 @@ import { Text } from '../common/Text';
import { View } from '../common/View';
import { PrivacyFilter } from '../PrivacyFilter';
import { CellValue, CellValueText } from '../spreadsheet/CellValue';
import { type SheetFields, type SheetNames } from '../spreadsheet/index';
import { useFormat } from '../spreadsheet/useFormat';
import { useSheetValue } from '../spreadsheet/useSheetValue';

function DetailedBalance({ name, balance, isExactBalance = true }) {
import { type ReconcilingMessage } from './Reconcile';

type DetailedBalanceProps = {
name: string;
balance: number | null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be non-nullable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

isExactBalance: boolean;
};

function DetailedBalance({
name,
balance,
isExactBalance = true,
Copy link
Author

@cindywu cindywu Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isExactBalance is intentionally and explicitly set to true because it seems that is what the original author intended.

}: DetailedBalanceProps) {
const format = useFormat();
return (
<Text
Expand All @@ -42,32 +56,40 @@ function DetailedBalance({ name, balance, isExactBalance = true }) {
);
}

function SelectedBalance({ selectedItems, account }) {
function SelectedBalance({
selectedItems,
account,
}: {
selectedItems: Set<string>;
account: AccountEntity;
}) {
const { t } = useTranslation();

type SelectedBalanceName = `selected-balance-${string}`;
cindywu marked this conversation as resolved.
Show resolved Hide resolved
const name = `selected-balance-${[...selectedItems].join('-')}`;
cindywu marked this conversation as resolved.
Show resolved Hide resolved

const rows = useSheetValue({
name,
const rows = useSheetValue<'balance', SelectedBalanceName>({
name: name as SelectedBalanceName,
query: q('transactions')
.filter({
id: { $oneof: [...selectedItems] },
parent_id: { $oneof: [...selectedItems] },
})
.select('id'),
});
const ids = new Set((rows || []).map(r => r.id));
const ids = new Set(Array.isArray(rows) ? rows.map(r => r.id) : []);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this always returns an array

Copy link
Author

@cindywu cindywu Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const ids = new Set((rows || []).map(r => r.id));

complains because useSheetValue doesn't always return an array. It can return a number.


const finalIds = [...selectedItems].filter(id => !ids.has(id));
let balance = useSheetValue({
name: name + '-sum',
type SelectedBalanceSumName = `selected-balance-${string}-sum`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please declare outside the component

let balance = useSheetValue<'balance', SelectedBalanceName>({
name: (name + '-sum') as SelectedBalanceSumName,
query: q('transactions')
.filter({ id: { $oneof: finalIds } })
.options({ splits: 'all' })
.calculate({ $sum: '$amount' }),
});

let scheduleBalance = null;
let scheduleBalance = 0;
cindywu marked this conversation as resolved.
Show resolved Hide resolved
cindywu marked this conversation as resolved.
Show resolved Hide resolved

const { isLoading, schedules = [] } = useCachedSchedules();

Expand Down Expand Up @@ -114,7 +136,7 @@ function SelectedBalance({ selectedItems, account }) {
);
}

function FilteredBalance({ filteredAmount }) {
function FilteredBalance({ filteredAmount }: { filteredAmount: number }) {
cindywu marked this conversation as resolved.
Show resolved Hide resolved
const { t } = useTranslation();

return (
Expand All @@ -126,34 +148,58 @@ function FilteredBalance({ filteredAmount }) {
);
}

function MoreBalances({ balanceQuery }) {
function MoreBalances({
balanceQuery,
}: {
balanceQuery: ComponentProps<typeof ReconcilingMessage>['balanceQuery'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extract to a Props type and this should be a Binding

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes you say that this should be a Binding? Do you have a suggested edit?

I am seeing this in a different file, so I updated it to that for now.

balanceQuery: { name: `balance-query-${string}`; query: Query };

}) {
const { t } = useTranslation();

const cleared = useSheetValue({
name: balanceQuery.name + '-cleared',
type SelectedBalanceClearedName = `balance-query-${string}-cleared`;
cindywu marked this conversation as resolved.
Show resolved Hide resolved
const cleared = useSheetValue<'balance', SelectedBalanceClearedName>({
name: (balanceQuery.name + '-cleared') as SelectedBalanceClearedName,
Copy link
Author

@cindywu cindywu Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works.

There must be a better way to write this, but I can't figure out how rn. So, I just "made it work"...

cindywu marked this conversation as resolved.
Show resolved Hide resolved
query: balanceQuery.query.filter({ cleared: true }),
});
const uncleared = useSheetValue({
name: balanceQuery.name + '-uncleared',

type SelectedBalanceUnclearedName = `balance-query-${string}-uncleared`;
cindywu marked this conversation as resolved.
Show resolved Hide resolved
const uncleared = useSheetValue<'balance', SelectedBalanceUnclearedName>({
name: (balanceQuery.name + '-uncleared') as SelectedBalanceUnclearedName,
query: balanceQuery.query.filter({ cleared: false }),
});

return (
<View style={{ flexDirection: 'row' }}>
<DetailedBalance name={t('Cleared total:')} balance={cleared} />
<DetailedBalance name={t('Uncleared total:')} balance={uncleared} />
<DetailedBalance
name={t('Cleared total:')}
balance={cleared}
isExactBalance
/>
<DetailedBalance
name={t('Uncleared total:')}
balance={uncleared}
isExactBalance
/>
</View>
);
}

type BalancesProps = {
balanceQuery: ComponentProps<typeof ReconcilingMessage>['balanceQuery'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a Binding

Copy link
Author

@cindywu cindywu Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes you say that? Do you have a suggested edit?

I am seeing this in a different file.

balanceQuery: { name: `balance-query-${string}`; query: Query };

Copy link
Author

@cindywu cindywu Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one way I could think of to set it as a Binding

balanceQuery: Extract<
    Binding<'balance', `balance-query-${string}`>,
    { name: string; query?: Query }
  >;

The root issue seems to be in

https://github.com/actualbudget/actual/blob/339fac280605bbd344ecb3387751a9ee43ffab69/packages/desktop-client/src/components/spreadsheet/index.ts#L81C1-L90C7

export type Binding<
  SheetName extends SheetNames,
  SheetFieldName extends SheetFields<SheetName>,
> =
  | SheetFieldName
  | {
      name: SheetFieldName;
      value?: Spreadsheets[SheetName][SheetFieldName];
      query?: Query;
    };

Possibly we could refactor this removing SheetFieldName as a potential type to be

export type Binding<
  SheetName extends SheetNames,
  SheetFieldName extends SheetFields<SheetName>,
> =
  {
      name: SheetFieldName;
      value?: Spreadsheets[SheetName][SheetFieldName];
      query?: Query;
    };

showExtraBalances: boolean;
onToggleExtraBalances: () => void;
account: AccountEntity;
isFiltered: boolean;
filteredAmount: number;
};

export function Balances({
balanceQuery,
showExtraBalances,
onToggleExtraBalances,
account,
isFiltered,
filteredAmount,
}) {
}: BalancesProps) {
const selectedItems = useSelectedItems();
const buttonRef = useRef(null);
const isButtonHovered = useHover(buttonRef);
Expand All @@ -177,7 +223,14 @@ export function Balances({
paddingBottom: 1,
}}
>
<CellValue binding={{ ...balanceQuery, value: 0 }} type="financial">
<CellValue
binding={{
name: balanceQuery.name as SheetFields<SheetNames>,
query: balanceQuery.query,
value: 0,
}}
type="financial"
>
{props => (
<CellValueText
{...props}
Expand Down
4 changes: 4 additions & 0 deletions packages/desktop-client/src/components/spreadsheet/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ export type Spreadsheets = {
'uncategorized-balance': number;

// Balance fields
[key: `balance-query-${string}`]: number;
[key: `balance-query-${string}-cleared`]: number;
[key: `balance-query-${string}-uncleared`]: number;
[key: `selected-balance-${string}`]: number;
[key: `selected-balance-${string}-sum`]: number;
};
};

Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/3874.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Maintenance
authors: [cindywu]
---

Convert Balance.jsx to tsx