Add head titles for each page#7603
Add head titles for each page#7603danielh-official wants to merge 13 commits intoactualbudget:masterfrom
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hello contributor! We would love to review your PR! Before we can do that, please make sure:
We do this to reduce the TOIL the core contributor team has to go through for each PR and to allow for speedy reviews and merges. For more information, please see our Contributing Guide. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/desktop-client/src/components/Title.tsx (1)
1-3: Minor: simplify the JSX expression.The template literal is unnecessary — JSX can interpolate
valuedirectly with adjacent text.♻️ Proposed simplification
export function Title({ value }: { value: string }) { - return <title>{`${value} | Actual`}</title>; + return <title>{value} | Actual</title>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/Title.tsx` around lines 1 - 3, The Title component uses an unnecessary template literal; simplify JSX by interpolating the prop directly: update the Title function (export function Title) to return a title element that uses {value} with the adjacent string separator instead of `${value} | Actual` (e.g., <title>{value} | Actual</title>), leaving prop typing ({ value }: { value: string }) unchanged.upcoming-release-notes/7603.md (1)
6-6: Nit: tighten the release-note wording.The sentence starts with a dangling pronoun ("It"). Release-note entries typically read as a direct statement of the change.
✏️ Proposed phrasing
-It adds unique titles for each page. +Add unique page titles so browser tabs and bookmarks show page-specific names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upcoming-release-notes/7603.md` at line 6, The release note sentence uses a dangling pronoun ("It adds unique titles for each page."); change it to a direct present-tense statement by replacing that sentence with "Adds unique titles for each page." so the entry reads as a concise change summary in the upcoming-release-notes/7603.md content.packages/desktop-client/src/components/payees/ManagePayees.tsx (1)
40-57: Consider renderingTitleat the top level ofManagePayeesrather than insidePayeeTableHeader.Other pages in this PR (e.g.,
ManageRules,ManageTags,Schedules,BankSync,Budget) renderTitleat the root of the page component. Embedding it insidePayeeTableHeader— a sub-component whose name suggests table-header concerns — couples head-title semantics to a presentational row. Moving it to the top ofManagePayees's returned tree keeps the pattern consistent and makes the title's scope obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/payees/ManagePayees.tsx` around lines 40 - 57, The Title component is currently rendered inside the PayeeTableHeader sub-component which couples page-level semantics to a presentational header; update ManagePayees so Title (using Title value={t('Payees')}) is moved to the top-level return of the ManagePayees component (above the TableHeader/PayeeTableHeader) and remove the embedded Title from PayeeTableHeader so the table header only contains table-related cells (SelectCell, Cell) and the page title stays consistent with other pages like ManageRules/ManageTags.packages/desktop-client/src/components/settings/index.tsx (1)
15-15: PreferuseTranslation()over module-levelimport { t } from 'i18next'in React components.
About()is a React component, sotshould come fromuseTranslation()(consistent withAdvancedAbout/Settingsin this same file). The module-leveltwon't subscribe the component to language-change re-renders.♻️ Proposed fix
-import { t } from 'i18next'; - import { getLatestAppVersion } from '#app/appSlice';function About() { + const { t } = useTranslation(); const version = useServerVersion();Based on learnings: "importing
tdirectly from 'i18next' is best avoided, and we should almost always prefer using the useTranslation hook if possible." (qedi-r, PR 3527)Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/settings/index.tsx` at line 15, Replace the module-level import of t from 'i18next' with the useTranslation hook inside the About React component so it subscribes to language changes: remove the top-level "import { t } from 'i18next'" and inside the About function call const { t } = useTranslation() (matching how AdvancedAbout and Settings do it), then update any uses of t in About to use that local t; ensure you import useTranslation from 'react-i18next' if not already present.packages/desktop-client/src/components/accounts/Account.tsx (1)
2092-2107: Avoid shadowing: twogetAccountTitlehelpers with different return shapes.There's now a module-level
getAccountTitle(returns"… Account Transactions"/`${account.name} — Account Transactions`) and a class-methodAccountInternal.getAccountTitle(returns"On Budget Accounts"/account.name) with intentionally different strings. Same name, different results is an easy foot-gun for future edits. Consider renaming the module-level helper to clarify intent, e.g.getAccountPageTitle(and using it only for the<Title>tag).Also, this helper calls module-scoped
tfromi18next; that's fine here since the function is invoked per render, but wrapping it into a small hook or passingtin would be more consistent with the rest of the file.♻️ Proposed rename
- const accountTitle = getAccountTitle(account, params.id); + const accountTitle = getAccountPageTitle(account, params.id); @@ -function getAccountTitle(account?: AccountEntity, id?: string) { +function getAccountPageTitle(account?: AccountEntity, id?: string) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/accounts/Account.tsx` around lines 2092 - 2107, There's a shadowing issue: the module-level function getAccountTitle returns page-title-style strings while the class method AccountInternal.getAccountTitle returns account-name-style strings; rename the module-level helper to something explicit (e.g., getAccountPageTitle) and update its call sites (such as the <Title> usage) so only the renamed helper is used for page/title text, leaving AccountInternal.getAccountTitle unchanged; additionally, to match file conventions, either accept a t parameter or wrap the renamed helper in a small hook so it doesn't rely on the module-scoped i18next t directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/desktop-client/src/components/accounts/Account.tsx`:
- Around line 2040-2044: getAccountTitle can return null, which when rendered
via the template literal `${accountTitle}` produces the string "null" in the
document title; change the rendering so Title is only rendered when accountTitle
is non-null (or use a safe fallback string) — locate the accountTitle variable
and the Title component usage in this file (around the accountTitle =
getAccountTitle(...) and the Title value={`${accountTitle}`} line) and replace
it with a conditional render or conditional value: if accountTitle is null omit
Title entirely or pass a default like the app's base title.
In `@packages/desktop-client/src/components/budget/index.tsx`:
- Line 13: Replace the direct static import "import { t } from 'i18next'" with
the hook-based API inside the React component: remove the top-level t import and
add "const { t } = useTranslation()" inside the functional component (e.g., in
the component that renders the budget UI), then update any usages of t(...) to
use this hooked t so the component reacts to language changes; ensure you import
useTranslation from 'react-i18next' and remove the unused i18next import.
In `@packages/desktop-client/src/components/reports/Overview.tsx`:
- Around line 738-740: The Title usage is building a dynamic i18n key with
t(`${dashboard.name || 'Untitled'} — Report Dashboard`) which prevents
extracting and translating the static suffix and the 'Untitled' fallback;
replace this with an interpolated translation key (e.g. a key like
"reportDashboard.title") and pass dashboard.name (or a translated fallback from
t('Untitled')) as a variable to t so the static "— Report Dashboard" string and
the fallback are localizable while the dashboard.name remains dynamic; update
the Title call to use t('reportDashboard.title', { name: dashboard.name ||
t('Untitled') }) (and add the corresponding translation entries).
In `@packages/desktop-client/src/components/reports/reports/Summary.tsx`:
- Line 351: The translation key is being built dynamically in the Title
component call (Title and t(`${title} — Report Summary`)), which prevents
extraction of the fixed suffix; change the call to use a stable i18n key and
interpolate the widget title via the t function (e.g. t('reports.summary', {
title })) so the static "— Report Summary" text is translatable while passing
the dynamic title as a variable to be inserted by the localization system.
---
Nitpick comments:
In `@packages/desktop-client/src/components/accounts/Account.tsx`:
- Around line 2092-2107: There's a shadowing issue: the module-level function
getAccountTitle returns page-title-style strings while the class method
AccountInternal.getAccountTitle returns account-name-style strings; rename the
module-level helper to something explicit (e.g., getAccountPageTitle) and update
its call sites (such as the <Title> usage) so only the renamed helper is used
for page/title text, leaving AccountInternal.getAccountTitle unchanged;
additionally, to match file conventions, either accept a t parameter or wrap the
renamed helper in a small hook so it doesn't rely on the module-scoped i18next t
directly.
In `@packages/desktop-client/src/components/payees/ManagePayees.tsx`:
- Around line 40-57: The Title component is currently rendered inside the
PayeeTableHeader sub-component which couples page-level semantics to a
presentational header; update ManagePayees so Title (using Title
value={t('Payees')}) is moved to the top-level return of the ManagePayees
component (above the TableHeader/PayeeTableHeader) and remove the embedded Title
from PayeeTableHeader so the table header only contains table-related cells
(SelectCell, Cell) and the page title stays consistent with other pages like
ManageRules/ManageTags.
In `@packages/desktop-client/src/components/settings/index.tsx`:
- Line 15: Replace the module-level import of t from 'i18next' with the
useTranslation hook inside the About React component so it subscribes to
language changes: remove the top-level "import { t } from 'i18next'" and inside
the About function call const { t } = useTranslation() (matching how
AdvancedAbout and Settings do it), then update any uses of t in About to use
that local t; ensure you import useTranslation from 'react-i18next' if not
already present.
In `@packages/desktop-client/src/components/Title.tsx`:
- Around line 1-3: The Title component uses an unnecessary template literal;
simplify JSX by interpolating the prop directly: update the Title function
(export function Title) to return a title element that uses {value} with the
adjacent string separator instead of `${value} | Actual` (e.g., <title>{value} |
Actual</title>), leaving prop typing ({ value }: { value: string }) unchanged.
In `@upcoming-release-notes/7603.md`:
- Line 6: The release note sentence uses a dangling pronoun ("It adds unique
titles for each page."); change it to a direct present-tense statement by
replacing that sentence with "Adds unique titles for each page." so the entry
reads as a concise change summary in the upcoming-release-notes/7603.md content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddbb4112-4da6-44f4-88df-af50fcbc3185
📒 Files selected for processing (12)
packages/desktop-client/src/components/ManageRules.tsxpackages/desktop-client/src/components/Title.tsxpackages/desktop-client/src/components/accounts/Account.tsxpackages/desktop-client/src/components/banksync/index.tsxpackages/desktop-client/src/components/budget/index.tsxpackages/desktop-client/src/components/payees/ManagePayees.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/reports/Summary.tsxpackages/desktop-client/src/components/schedules/index.tsxpackages/desktop-client/src/components/settings/index.tsxpackages/desktop-client/src/components/tags/ManageTags.tsxupcoming-release-notes/7603.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/desktop-client/src/components/accounts/Account.tsx (1)
2092-2107: Duplicate-but-divergentgetAccountTitleimplementations;filterNamestate is ignored for the document title.There are now two
getAccountTitlefunctions with subtly different semantics:
AccountInternal.getAccountTitle(Lines 897-918) readslocation.state.filterNameand returns short labels like"On Budget Accounts"/"Uncategorized"/ account name.- The new top-level
getAccountTitle(Lines 2092-2107) ignoresfilterNameand returns"… Account Transactions"variants.Consequences:
- When the page is reached via a
filterNamelink (e.g.,onShowTransactions→t('Selected transactions')), the<h1>shows"Selected transactions"but the browser tab shows the generic account/route title — the document title no longer reflects what the user is actually viewing.- The fallback on Line 2103 (
return t('Account Transactions')) is only hit whenaccountis undefined ANDidis a truthy non-special value. In that windowAccountInternalwill redirect to/accounts(Lines 1738-1742), so users briefly see "Account Transactions | Actual" for unknown IDs. Consider returning an empty string (or skipping<Title>) instead to let the default"Actual"title fromindex.htmlstand.Consider consolidating the two helpers (e.g., parameterize the suffix) and threading
filterNameinto the standalone version so the tab title matches the header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/accounts/Account.tsx` around lines 2092 - 2107, There are two mismatched title helpers (getAccountTitle and AccountInternal.getAccountTitle) causing the document title to ignore location.state.filterName; to fix, consolidate them by updating the top-level getAccountTitle to accept an optional filterName parameter (the same values read from location.state.filterName) and make it return the short labels when filterName is present (e.g., "On Budget Accounts", "Uncategorized", "Selected transactions") before falling back to account/name-based strings; also change the fallback for unknown truthy ids to return an empty string (so the <Title> can be omitted and the default "Actual" remains) and update callers (including AccountInternal and wherever <Title> is set) to pass through location.state.filterName to the unified getAccountTitle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/desktop-client/src/components/accounts/Account.tsx`:
- Around line 2040-2044: Replace the direct i18next import and usage with
react-i18next's useTranslation inside the Account component: call const { t } =
useTranslation() at the top of the Account function and remove the standalone t
import; update getAccountTitle usage so any hardcoded "Account Transactions"
suffix in getAccountTitle or where accountTitle is composed uses t('Account
Transactions') instead of a literal string (ensure special account types still
use t()), and pass the resulting plain string accountTitle (no template literal
wrapper) into <Title value={accountTitle} /> so the component re-renders on
language changes and the suffix is translated.
---
Nitpick comments:
In `@packages/desktop-client/src/components/accounts/Account.tsx`:
- Around line 2092-2107: There are two mismatched title helpers (getAccountTitle
and AccountInternal.getAccountTitle) causing the document title to ignore
location.state.filterName; to fix, consolidate them by updating the top-level
getAccountTitle to accept an optional filterName parameter (the same values read
from location.state.filterName) and make it return the short labels when
filterName is present (e.g., "On Budget Accounts", "Uncategorized", "Selected
transactions") before falling back to account/name-based strings; also change
the fallback for unknown truthy ids to return an empty string (so the <Title>
can be omitted and the default "Actual" remains) and update callers (including
AccountInternal and wherever <Title> is set) to pass through
location.state.filterName to the unified getAccountTitle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cfc0b9b-f37f-4c30-b3c3-33ea7ade8840
📒 Files selected for processing (4)
packages/desktop-client/src/components/accounts/Account.tsxpackages/desktop-client/src/components/budget/index.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/reports/Summary.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/reports/Overview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/reports/reports/Summary.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/desktop-client/src/components/accounts/Account.tsx`:
- Around line 2093-2116: Rename the getAccountTitle function's translator
parameter to a required t (i.e., getAccountTitle(account?: AccountEntity, id?:
string, t: (key: string) => string): string), remove the optional identity
fallback so t is mandatory, and update all call sites to pass the i18n function
by destructuring (e.g., const { t } = useTranslation() or props) before calling
getAccountTitle; keep the existing returned keys/strings but ensure all
translate(...) usages inside getAccountTitle are renamed to t(...) so
i18next-parser and the actual/no-untranslated-strings lint rule will detect
them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6d1e995-9ecc-4ba1-81a2-c43cbfd27a0b
📒 Files selected for processing (1)
packages/desktop-client/src/components/accounts/Account.tsx
Replace the direct import of `t` from 'i18next' with the `useTranslation` hook from 'react-i18next' in the Budget component. This moves `const { t } = useTranslation()` into the component so translations update correctly (e.g. on language change) and follow React i18next best practices.
Stop passing the whole interpolated title to t(). Previously t() was called on the combined string, which prevented proper localization of the fallback 'Untitled' and the 'Report Dashboard' label. This change calls t('Untitled') and t('Report Dashboard') separately while interpolating dashboard.name, ensuring correct translations.
Stop passing the full template string to the i18n function. Previously `${title} — Report Summary` was sent to `t()`, causing the dynamic title to be treated as a translation key. This change concatenates the raw `title` with `t('Report Summary')` so only the fixed suffix is translated, preserving the dynamic title and fixing translation lookup/interpolation issues.
Co-authored-by: Copilot <copilot@github.com>
Change getAccountTitle to accept a required translator function parameter named `t` and tighten types for `account` and `id`. Remove the previous internal fallback `translate` implementation and replace all uses of `translate` with `t`. This enforces explicit localization handling and clarifies typings for callers (they must now pass a translation function).
e16c9b1 to
f6c6931
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/desktop-client/src/components/accounts/Account.tsx (1)
2045-2045: Drop the redundant template literal.
accountTitleis already typed asstring(return type ofgetAccountTitle), so the wrapping template literal on Line 2045 is unnecessary.♻️ Proposed tweak
- <Title value={`${accountTitle}`} /> + <Title value={accountTitle} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/accounts/Account.tsx` at line 2045, The Title usage wraps accountTitle in an unnecessary template literal; since getAccountTitle returns a string, change the prop to pass accountTitle directly (i.e., use <Title value={accountTitle} />) instead of <Title value={`${accountTitle}`} />—update the JSX where Title is rendered and ensure accountTitle comes from getAccountTitle unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/desktop-client/src/components/accounts/Account.tsx`:
- Line 2045: The Title usage wraps accountTitle in an unnecessary template
literal; since getAccountTitle returns a string, change the prop to pass
accountTitle directly (i.e., use <Title value={accountTitle} />) instead of
<Title value={`${accountTitle}`} />—update the JSX where Title is rendered and
ensure accountTitle comes from getAccountTitle unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 506c4bec-ca02-41d3-ba19-24ca371ebb7e
📒 Files selected for processing (12)
packages/desktop-client/src/components/ManageRules.tsxpackages/desktop-client/src/components/Title.tsxpackages/desktop-client/src/components/accounts/Account.tsxpackages/desktop-client/src/components/banksync/index.tsxpackages/desktop-client/src/components/budget/index.tsxpackages/desktop-client/src/components/payees/ManagePayees.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/reports/Summary.tsxpackages/desktop-client/src/components/schedules/index.tsxpackages/desktop-client/src/components/settings/index.tsxpackages/desktop-client/src/components/tags/ManageTags.tsxupcoming-release-notes/7603.md
✅ Files skipped from review due to trivial changes (7)
- packages/desktop-client/src/components/ManageRules.tsx
- packages/desktop-client/src/components/banksync/index.tsx
- packages/desktop-client/src/components/tags/ManageTags.tsx
- upcoming-release-notes/7603.md
- packages/desktop-client/src/components/Title.tsx
- packages/desktop-client/src/components/payees/ManagePayees.tsx
- packages/desktop-client/src/components/settings/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/desktop-client/src/components/schedules/index.tsx
Description
It adds unique titles for each page as described in #7588.
The original title in the
index.htmlfile (Actual) still remains, and is shown before the page is fully mounted. It also serves as a fallback for newer pages that might be missing titles.Starting with React 19, title elements are detected and hoisted up to head. Each title is written into the HTML as a title above the default.
For instance, the /budget page has the following:
Why?
This makes it so that a user can bookmark different pages of the app and not have to manually change the bookmark name to have its own title instead of "Actual".
It also makes having multiple browser tabs open for the web app less of a pain because the user can tell at a glance which tab aligns with which page.
Related issue(s)
Relates to #7588
Testing
yarn startat the root.Checklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
Changeset
src/components/Title.tsxsrc/components/banksync/index.tsxsrc/components/schedules/index.tsxsrc/components/tags/ManageTags.tsxsrc/components/budget/index.tsxsrc/components/payees/ManagePayees.tsxsrc/components/ManageRules.tsxsrc/components/accounts/Account.tsxsrc/components/reports/Overview.tsxsrc/components/reports/reports/Summary.tsxsrc/components/settings/index.tsxView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
loot-core
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
api
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
cli
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
crdt
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged