Merged
Conversation
|
Warning Rate limit exceeded
⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR introduces an expense distribution feature comprising new API endpoints, application services using a typed service aggregation pattern, domain interfaces, and a dashboard UI component to visualize expense breakdown by category over a date range. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Blazor Component
participant HttpClient as MoneyFlowHttpClient
participant Controller as ExpenseDistributionController
participant Service as ExpenseDistributionService
participant TypedService as CurrencyExpenseDistributionService
participant Repository as IFinancialAccountRepository
Client->>HttpClient: GetExpenseDistribution(userId, currency, start, end)
HttpClient->>Controller: GET /api/expensedistribution/{userId}/{currencyId}/{start}/{end}
Controller->>Repository: GetAccounts(userId)
Repository-->>Controller: List of accounts
Controller->>Service: GetExpenseDistribution(userId, currency, start, end)
Service->>TypedService: GetExpenseDistribution(userId, currency, start, end)
TypedService->>Repository: Iterate accounts
TypedService->>TypedService: Aggregate expenses by category
TypedService-->>Service: List<NameValueResult>
Service->>Service: Combine typed services results
Service-->>Controller: Aggregated List<NameValueResult>
Controller-->>HttpClient: Ok(result)
HttpClient-->>Client: List<NameValueResult>
Client->>Client: Render pie chart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an “Expense distribution” feature to the FinanceManager dashboard by introducing a new backend endpoint + application services to aggregate expenses by label classification, along with UI components and admin tooling to assign classifications to labels.
Changes:
- Add expense distribution domain/service layer (typed services + aggregator) and API controller endpoint.
- Add dashboard “Expense distribution” pie chart card and enhance labels list visualization.
- Add label “Spending classification” editor UI and ensure currency entry queries include label classifications.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| code/FinanceManager.Infrastructure/Repositories/Account/Entry/CurrencyEntryRepository.cs | Eager-load label classifications for currency entries to support classification-based analytics. |
| code/FinanceManager.Domain/Services/IExpenseDistributionServiceTyped.cs | New typed service interface for per-account-type expense distribution providers. |
| code/FinanceManager.Domain/Services/IExpenseDistributionService.cs | New service contract for retrieving expense distribution results. |
| code/FinanceManager.Domain/Entities/Shared/Accounts/FinancialLabelClassificationCatalog.cs | Adds helper to resolve a single classification from multiple labels/classifications. |
| code/FinanceManager.Components/HttpClients/MoneyFlowHttpClient.cs | Adds client method to call the new ExpenseDistribution API endpoint. |
| code/FinanceManager.Components/Components/Dashboard/Dashboard.razor | Adds the new expense distribution card to the dashboard layout. |
| code/FinanceManager.Components/Components/Dashboard/Cards/FinancialLabelsListCard.razor.cs | Adds helpers for progress-bar percentage calculation and icon selection. |
| code/FinanceManager.Components/Components/Dashboard/Cards/FinancialLabelsListCard.razor | Updates label list UI to include icons, progress bars, and color-coded values. |
| code/FinanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razor.cs | New dashboard card code-behind to fetch distribution data and prepare chart series/labels. |
| code/FinanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razor | New dashboard card UI showing a pie chart + totals/loading/empty states. |
| code/FinanceManager.Components/Components/Admin/EditLabelPage.razor.cs | Adds state + action to set a label’s spending classification via API. |
| code/FinanceManager.Components/Components/Admin/EditLabelPage.razor | Adds admin UI controls to select and set spending classification for a label. |
| code/FinanceManager.Application/Services/Seeders/FinancialLabelSeeder.cs | Adds a default “Undisclosed Expense” label to seed data. |
| code/FinanceManager.Application/Services/ExpenseDistributionService.cs | New aggregator service combining results from typed distribution services. |
| code/FinanceManager.Application/Services/Currencies/CurrencyExpenseDistributionService.cs | Implements currency-account expense aggregation by resolved label classification. |
| code/FinanceManager.Application/ServiceCollectionExtension.cs | Registers the new expense distribution services with DI. |
| code/FinanceManager.Api/Controllers/ExpenseDistributionController.cs | New authorized API endpoint exposing expense distribution results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
code/FinanceManager.Infrastructure/Repositories/Account/Entry/CurrencyEntryRepository.cs
Show resolved
Hide resolved
code/FinanceManager.Domain/Entities/Shared/Accounts/FinancialLabelClassificationCatalog.cs
Outdated
Show resolved
Hide resolved
...inanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razor.cs
Show resolved
Hide resolved
...inanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razor.cs
Outdated
Show resolved
Hide resolved
Comment on lines
+16
to
+22
| [HttpGet("GetExpenseDistribution/{userId:int}/{currencyId:int}/{start:DateTime}/{end:DateTime}")] | ||
| [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(List<NameValueResult>))] | ||
| public async Task<IActionResult> GetExpenseDistribution(int userId, int currencyId, DateTime start, DateTime end, CancellationToken cancellationToken = default) | ||
| { | ||
| var currency = await currencyRepository.GetCurrencies(cancellationToken).SingleAsync(x => x.Id == currencyId, cancellationToken); | ||
| return Ok(await expenseDistributionService.GetExpenseDistribution(userId, currency, start, end)); | ||
| } |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/FinanceManager.Application/Services/Seeders/FinancialLabelSeeder.cs (1)
30-35:⚠️ Potential issue | 🟠 MajorExisting deployments won't receive new labels.
The current logic skips seeding entirely when any labels exist (
count > 0). This means deployments that already have the original label set will never receive the 8 newly added labels. Consider either:
- Adding a database migration to insert the new labels, or
- Changing the logic to seed only missing labels:
♻️ Alternative approach: seed missing labels
public async Task Seed(CancellationToken cancellationToken = default) { try { - var count = await financialLabelsRepository.GetCount(); - if (count > 0) - { - logger.LogInformation("Financial labels already present: {Count}", count); - return; - } + var existingLabels = await financialLabelsRepository.GetAll(cancellationToken); + var existingSet = existingLabels.ToHashSet(StringComparer.OrdinalIgnoreCase); foreach (var label in _defaultLabels) { + if (existingSet.Contains(label)) + continue; + if (await financialLabelsRepository.Add(label, cancellationToken)) logger.LogInformation("Seeded default financial label: {Label}", label); else logger.LogWarning("Failed to seed default financial label: {Label}", label); }Also, consider passing
cancellationTokentoGetCount()for consistency if the repository method supports it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/FinanceManager.Application/Services/Seeders/FinancialLabelSeeder.cs` around lines 30 - 35, The seeding currently aborts when any labels exist (using financialLabelsRepository.GetCount()), preventing new labels from being added to existing deployments; change the logic to compute and insert only missing labels instead of returning early: retrieve existing labels (e.g., via financialLabelsRepository.GetAllAsync or a GetByNamesAsync variant, passing cancellationToken if supported), compare against the full desired label set to find missing names, then create and persist only those missing labels via the repository's add/insert methods and save/commit; also update the GetCount() call (or any repository calls) to accept and forward the cancellationToken for consistency and keep the existing logger.LogInformation calls but log the number of labels added rather than returning silently.
🧹 Nitpick comments (3)
code/FinanceManager.Components/Components/Dashboard/Cards/FinancialLabelsListCard.razor.cs (1)
60-65: Avoid recomputing max value for every row.
GetPercentagerecalculatesMaxon each call, which becomes quadratic when rendering many labels. Cache max absolute value once when_datais loaded.♻️ Suggested refactor
+private decimal _maxAbsValue; + protected override async Task OnParametersSetAsync() { @@ - if (userId is not null) - _data = (await MoneyFlowHttpClient.GetLabelsValue(userId.UserId, StartDateTime, EndDateTime)).Where(x => x.Value != 0).ToList(); + if (userId is not null) + { + _data = (await MoneyFlowHttpClient.GetLabelsValue(userId.UserId, StartDateTime, EndDateTime)) + .Where(x => x.Value != 0) + .ToList(); + _maxAbsValue = _data.Count == 0 ? 0 : _data.Max(x => Math.Abs(x.Value)); + } @@ private double GetPercentage(decimal value) { - if (_data.Count == 0) return 0; - var maxValue = _data.Max(x => Math.Abs(x.Value)); - return maxValue == 0 ? 0 : (double)(Math.Abs(value) / maxValue * 100); + return _maxAbsValue == 0 ? 0 : (double)(Math.Abs(value) / _maxAbsValue * 100); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/FinanceManager.Components/Components/Dashboard/Cards/FinancialLabelsListCard.razor.cs` around lines 60 - 65, GetPercentage currently recomputes _data.Max(...) on every call causing O(n^2) rendering; introduce a cached field (e.g., _maxAbsValue) computed once when _data is loaded/updated (in the method that sets/loads _data such as the data loader, OnInitialized/OnParametersSet or the setter that assigns _data) and update/invalidate that cache whenever _data changes; then change GetPercentage to use the cached _maxAbsValue and still return 0 when _data is empty or _maxAbsValue == 0.code/FinanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razor.cs (1)
44-46: Remove unnecessaryawait Task.CompletedTask.
OnInitializedAsyncperforms only synchronous operations. Theawait Task.CompletedTaskis unnecessary and can be removed by making the method synchronous or simply returningTask.CompletedTask.Proposed fix
- protected override async Task OnInitializedAsync() + protected override Task OnInitializedAsync() { _currency = SettingsService.GetCurrency(); _chartOptions = new ChartOptions { ChartPalette = ColorsProvider.GetColors().ToArray(), ShowLegend = false, TooltipTitleFormat = "{{Y_VALUE}} " + _currency.ShortName, }; - await Task.CompletedTask; + return Task.CompletedTask; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/FinanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razor.cs` around lines 44 - 46, The OnInitializedAsync method in ExpenseDistributionOverviewCard.razor.cs contains an unnecessary await Task.CompletedTask; remove it by either changing OnInitializedAsync into a synchronous OnInitialized override (if all work is synchronous) or keep the async signature but return Task.CompletedTask directly; update the method signature and body accordingly in the ExpenseDistributionOverviewCard class so there is no redundant await.code/FinanceManager.Application/Services/Currencies/CurrencyExpenseDistributionService.cs (1)
10-12:IsOfType<T>()method is defined but never called.The
IsOfType<T>()method exists butExpenseDistributionServicedoesn't use it when iterating through typed services. This is consistent with theBalanceServicepattern, but the method appears to serve no purpose currently.If type filtering is not needed, consider removing
IsOfType<T>()fromIExpenseDistributionServiceTypedand this implementation to reduce dead code. Otherwise, document its intended use case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/FinanceManager.Application/Services/Currencies/CurrencyExpenseDistributionService.cs` around lines 10 - 12, The IsOfType<T>() implementation in CurrencyExpenseDistributionService is dead code; remove the IsOfType<T>() signature from the IExpenseDistributionServiceTyped interface and delete the IsOfType<T>() method from CurrencyExpenseDistributionService (and any other typed service implementations) so the interface no longer defines an unused method, update all implementing classes to compile against the trimmed interface, and run build/tests to ensure no callers rely on it; alternatively, if you intend to keep type filtering, add a clear usage in ExpenseDistributionService where typed services are iterated (use IsOfType<T>() to filter before invoking) and document the behavior—choose one approach and apply consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/FinanceManager.Api/Controllers/ExpenseDistributionController.cs`:
- Around line 18-22: The GetExpenseDistribution method currently uses
currencyRepository.GetCurrencies(...).SingleAsync(...) which throws if the
currencyId is invalid; change it to call the repository method that returns an
optional Currency (e.g., currencyRepository.GetCurrency(currencyId,
cancellationToken) or GetCurrency(int id) as exposed), check for null and return
NotFound() when null, otherwise pass the found currency into
expenseDistributionService.GetExpenseDistribution(userId, currency, start, end)
and return Ok(...); ensure you preserve the CancellationToken usage when calling
the repository and service.
In
`@code/FinanceManager.Application/Services/Currencies/CurrencyExpenseDistributionService.cs`:
- Around line 14-40: GetExpenseDistribution currently ignores the Currency
parameter: either implement currency-aware aggregation or remove the parameter
from the contract. Fix option A: add a Currency property to the CurrencyAccount
model and propagate it through the repository so
financialAccountRepository.GetAccounts<CurrencyAccount>(userId, start, end) can
accept a currency filter (or filter results by account.Currency) and then only
aggregate entries from accounts matching the passed Currency parameter in
GetExpenseDistribution. Fix option B: if all currency-specific services should
not require a currency, remove the Currency parameter from
IExpenseDistributionService (and analogous methods in
CurrencyBalanceService/CurrencyEssentialSpendingService) and update all
implementations and callers to match. Ensure changes reference
GetExpenseDistribution, CurrencyAccount, financialAccountRepository, and the
interface IExpenseDistributionService so all compile-time usages are updated.
In `@code/FinanceManager.Components/Components/Admin/EditLabelPage.razor.cs`:
- Around line 52-73: SetClassificationAsync currently calls
FinancialLabelHttpClient.AddClassification and Get without error handling; wrap
the await calls in a try/catch that catches exceptions from AddClassification
and Get, add a descriptive error entry to _errors (and optionally log the
exception), and return gracefully so exceptions don't bubble out of the event
handler; keep the existing success path (clearing _errors, inserting _info,
refreshing _labelData, and updating _selectedClassification) inside the try
block and ensure the catch does not overwrite successful _info entries.
In
`@code/FinanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razor.cs`:
- Around line 75-78: The catch block in ExpenseDistributionOverviewCard (in
ExpenseDistributionOverviewCard.razor.cs) is calling Logger.LogError with the
message first and the exception as an argument, which misuses the overload;
change the call to pass the Exception as the first parameter and a message
template as the second (e.g., Logger.LogError(ex, "Error while handling expense
distribution in ExpenseDistributionOverviewCard") or similar) so the exception
and stack trace are captured correctly.
In
`@code/FinanceManager.Components/Components/Dashboard/Cards/FinancialLabelsListCard.razor.cs`:
- Line 67: The GetCategoryIcon method currently calls categoryName.ToLower()
which can throw for null; update GetCategoryIcon(string categoryName) to guard
against null/empty inputs (e.g., check string.IsNullOrWhiteSpace(categoryName)
or use categoryName ?? string.Empty) before normalizing, and return a sensible
default icon when the name is missing; ensure any subsequent switch/lookup
operates on the safe, lower-cased value (for example use var key = (categoryName
?? string.Empty).ToLowerInvariant()) and reference that key in the switch.
---
Outside diff comments:
In `@code/FinanceManager.Application/Services/Seeders/FinancialLabelSeeder.cs`:
- Around line 30-35: The seeding currently aborts when any labels exist (using
financialLabelsRepository.GetCount()), preventing new labels from being added to
existing deployments; change the logic to compute and insert only missing labels
instead of returning early: retrieve existing labels (e.g., via
financialLabelsRepository.GetAllAsync or a GetByNamesAsync variant, passing
cancellationToken if supported), compare against the full desired label set to
find missing names, then create and persist only those missing labels via the
repository's add/insert methods and save/commit; also update the GetCount() call
(or any repository calls) to accept and forward the cancellationToken for
consistency and keep the existing logger.LogInformation calls but log the number
of labels added rather than returning silently.
---
Nitpick comments:
In
`@code/FinanceManager.Application/Services/Currencies/CurrencyExpenseDistributionService.cs`:
- Around line 10-12: The IsOfType<T>() implementation in
CurrencyExpenseDistributionService is dead code; remove the IsOfType<T>()
signature from the IExpenseDistributionServiceTyped interface and delete the
IsOfType<T>() method from CurrencyExpenseDistributionService (and any other
typed service implementations) so the interface no longer defines an unused
method, update all implementing classes to compile against the trimmed
interface, and run build/tests to ensure no callers rely on it; alternatively,
if you intend to keep type filtering, add a clear usage in
ExpenseDistributionService where typed services are iterated (use IsOfType<T>()
to filter before invoking) and document the behavior—choose one approach and
apply consistently.
In
`@code/FinanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razor.cs`:
- Around line 44-46: The OnInitializedAsync method in
ExpenseDistributionOverviewCard.razor.cs contains an unnecessary await
Task.CompletedTask; remove it by either changing OnInitializedAsync into a
synchronous OnInitialized override (if all work is synchronous) or keep the
async signature but return Task.CompletedTask directly; update the method
signature and body accordingly in the ExpenseDistributionOverviewCard class so
there is no redundant await.
In
`@code/FinanceManager.Components/Components/Dashboard/Cards/FinancialLabelsListCard.razor.cs`:
- Around line 60-65: GetPercentage currently recomputes _data.Max(...) on every
call causing O(n^2) rendering; introduce a cached field (e.g., _maxAbsValue)
computed once when _data is loaded/updated (in the method that sets/loads _data
such as the data loader, OnInitialized/OnParametersSet or the setter that
assigns _data) and update/invalidate that cache whenever _data changes; then
change GetPercentage to use the cached _maxAbsValue and still return 0 when
_data is empty or _maxAbsValue == 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d348aa67-87a7-4fb0-817a-369070fb05d6
📒 Files selected for processing (17)
code/FinanceManager.Api/Controllers/ExpenseDistributionController.cscode/FinanceManager.Application/ServiceCollectionExtension.cscode/FinanceManager.Application/Services/Currencies/CurrencyExpenseDistributionService.cscode/FinanceManager.Application/Services/ExpenseDistributionService.cscode/FinanceManager.Application/Services/Seeders/FinancialLabelSeeder.cscode/FinanceManager.Components/Components/Admin/EditLabelPage.razorcode/FinanceManager.Components/Components/Admin/EditLabelPage.razor.cscode/FinanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razorcode/FinanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razor.cscode/FinanceManager.Components/Components/Dashboard/Cards/FinancialLabelsListCard.razorcode/FinanceManager.Components/Components/Dashboard/Cards/FinancialLabelsListCard.razor.cscode/FinanceManager.Components/Components/Dashboard/Dashboard.razorcode/FinanceManager.Components/HttpClients/MoneyFlowHttpClient.cscode/FinanceManager.Domain/Entities/Shared/Accounts/FinancialLabelClassificationCatalog.cscode/FinanceManager.Domain/Services/IExpenseDistributionService.cscode/FinanceManager.Domain/Services/IExpenseDistributionServiceTyped.cscode/FinanceManager.Infrastructure/Repositories/Account/Entry/CurrencyEntryRepository.cs
Comment on lines
+18
to
+22
| public async Task<IActionResult> GetExpenseDistribution(int userId, int currencyId, DateTime start, DateTime end, CancellationToken cancellationToken = default) | ||
| { | ||
| var currency = await currencyRepository.GetCurrencies(cancellationToken).SingleAsync(x => x.Id == currencyId, cancellationToken); | ||
| return Ok(await expenseDistributionService.GetExpenseDistribution(userId, currency, start, end)); | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for invalid currencyId.
SingleAsync will throw InvalidOperationException if no currency matches the provided currencyId, resulting in a 500 error instead of a proper 404. While this follows the existing pattern in MoneyFlowController, it provides a poor API experience.
The repository already exposes GetCurrency(int id) which returns Currency? and could be used with a null check:
Proposed fix
public async Task<IActionResult> GetExpenseDistribution(int userId, int currencyId, DateTime start, DateTime end, CancellationToken cancellationToken = default)
{
- var currency = await currencyRepository.GetCurrencies(cancellationToken).SingleAsync(x => x.Id == currencyId, cancellationToken);
+ var currency = await currencyRepository.GetCurrency(currencyId, cancellationToken);
+ if (currency is null)
+ return NotFound($"Currency with id {currencyId} not found");
return Ok(await expenseDistributionService.GetExpenseDistribution(userId, currency, start, end));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/FinanceManager.Api/Controllers/ExpenseDistributionController.cs` around
lines 18 - 22, The GetExpenseDistribution method currently uses
currencyRepository.GetCurrencies(...).SingleAsync(...) which throws if the
currencyId is invalid; change it to call the repository method that returns an
optional Currency (e.g., currencyRepository.GetCurrency(currencyId,
cancellationToken) or GetCurrency(int id) as exposed), check for null and return
NotFound() when null, otherwise pass the found currency into
expenseDistributionService.GetExpenseDistribution(userId, currency, start, end)
and return Ok(...); ensure you preserve the CancellationToken usage when calling
the repository and service.
Comment on lines
+14
to
+40
| public async Task<List<NameValueResult>> GetExpenseDistribution(int userId, Currency currency, DateTime start, DateTime end) | ||
| { | ||
| if (end > DateTime.UtcNow) end = DateTime.UtcNow; | ||
|
|
||
| Dictionary<string, decimal> totals = []; | ||
|
|
||
| await foreach (var account in financialAccountRepository.GetAccounts<CurrencyAccount>(userId, start, end)) | ||
| { | ||
| if (account?.Entries is null) continue; | ||
|
|
||
| foreach (var entry in account.Entries) | ||
| { | ||
| if (entry.ValueChange >= 0) continue; | ||
| if (entry.PostingDate < start || entry.PostingDate > end) continue; | ||
|
|
||
| var category = FinancialLabelClassificationCatalog.ResolveClassification(entry.Labels); | ||
| if (category is null) continue; | ||
|
|
||
| totals[category] = totals.GetValueOrDefault(category) + Math.Abs(entry.ValueChange); | ||
| } | ||
| } | ||
|
|
||
| return totals | ||
| .Select(kv => new NameValueResult(kv.Key, Math.Round(kv.Value, 2))) | ||
| .OrderBy(r => r.Name) | ||
| .ToList(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other typed services use the currency parameter
rg -n "currency" code/FinanceManager.Application/Services/Currencies/ -A 2 -B 2Repository: avresial/FinanceManager
Length of output: 17165
🏁 Script executed:
# Check if other services actually use the currency parameter in their implementations
# Look at CurrencyBalanceService implementation
head -100 code/FinanceManager.Application/Services/Currencies/CurrencyBalanceService.cs | tail -50Repository: avresial/FinanceManager
Length of output: 2285
🏁 Script executed:
# Check CurrencyEssentialSpendingService implementation
head -50 code/FinanceManager.Application/Services/Currencies/CurrencyEssentialSpendingService.csRepository: avresial/FinanceManager
Length of output: 2512
🏁 Script executed:
# Check if there's an interface that defines these method signatures
fd -n "ICurrency.*Service" code/FinanceManager.Application/Services/Repository: avresial/FinanceManager
Length of output: 297
🏁 Script executed:
# Find interface definitions for these typed services
rg "IEssentialSpendingServiceTyped|ICurrencyBalanceService|interface.*Typed" code/FinanceManager.Domain --type cs -B 2 -A 5Repository: avresial/FinanceManager
Length of output: 2191
🏁 Script executed:
# Check CurrencyAccount to see if it has a Currency property
rg "class CurrencyAccount" code/ --type cs -A 15Repository: avresial/FinanceManager
Length of output: 50379
🏁 Script executed:
# Check the base interface IExpenseDistributionService to see the method signature
rg "interface IExpenseDistributionService" code/FinanceManager.Domain/Services/ -A 10Repository: avresial/FinanceManager
Length of output: 899
🏁 Script executed:
# Check if CurrencyAccount has any Currency-related property
rg "public.*Currency" code/FinanceManager.Domain/Entities/FinancialAccounts/Currencies/CurrencyAccount.csRepository: avresial/FinanceManager
Length of output: 794
🏁 Script executed:
# Look for how the currency parameter is used in the controller or caller
rg "GetExpenseDistribution" code/ --type cs -B 2 -A 5 | head -60Repository: avresial/FinanceManager
Length of output: 6959
🏁 Script executed:
# Full CurrencyAccount class definition to see all properties
head -40 code/FinanceManager.Domain/Entities/FinancialAccounts/Currencies/CurrencyAccount.csRepository: avresial/FinanceManager
Length of output: 1667
The currency parameter is unused—incomplete filtering implementation.
The method receives a Currency parameter required by the IExpenseDistributionService interface but never uses it. All currency accounts are aggregated regardless of their currency. Since callers explicitly pass the currency ID through the API endpoint, and CurrencyAccount has no currency field to filter by, this appears to be an incomplete implementation.
Similar patterns exist in other typed services (CurrencyBalanceService, CurrencyEssentialSpendingService), suggesting a broader architectural issue. Either implement currency-based filtering by adding a currency field to CurrencyAccount and filtering during aggregation, or remove the unused parameter from the interface contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@code/FinanceManager.Application/Services/Currencies/CurrencyExpenseDistributionService.cs`
around lines 14 - 40, GetExpenseDistribution currently ignores the Currency
parameter: either implement currency-aware aggregation or remove the parameter
from the contract. Fix option A: add a Currency property to the CurrencyAccount
model and propagate it through the repository so
financialAccountRepository.GetAccounts<CurrencyAccount>(userId, start, end) can
accept a currency filter (or filter results by account.Currency) and then only
aggregate entries from accounts matching the passed Currency parameter in
GetExpenseDistribution. Fix option B: if all currency-specific services should
not require a currency, remove the Currency parameter from
IExpenseDistributionService (and analogous methods in
CurrencyBalanceService/CurrencyEssentialSpendingService) and update all
implementations and callers to match. Ensure changes reference
GetExpenseDistribution, CurrencyAccount, financialAccountRepository, and the
interface IExpenseDistributionService so all compile-time usages are updated.
Comment on lines
+52
to
+73
| private async Task SetClassificationAsync() | ||
| { | ||
| if (_labelData is null || string.IsNullOrEmpty(_selectedClassification)) return; | ||
|
|
||
| var result = await FinancialLabelHttpClient.AddClassification( | ||
| new(_labelData.Id, FinancialLabelClassificationCatalog.SpendingNecessityKind, _selectedClassification)); | ||
|
|
||
| if (!result) | ||
| { | ||
| _errors.Insert(0, "Failed to set classification."); | ||
| return; | ||
| } | ||
|
|
||
| _errors.Clear(); | ||
| _info.Insert(0, $"Classification set to '{_selectedClassification}'."); | ||
|
|
||
| _labelData = await FinancialLabelHttpClient.Get(_labelData.Id); | ||
| if (_labelData is not null) | ||
| _selectedClassification = _labelData.Classifications | ||
| .FirstOrDefault(c => c.Kind == FinancialLabelClassificationCatalog.SpendingNecessityKind)?.Value | ||
| ?? string.Empty; | ||
| } |
There was a problem hiding this comment.
Handle HTTP exceptions inside SetClassificationAsync.
Lines 56-68 call async client methods without a try/catch; transient API failures can bubble out of the event callback and break the interaction flow.
🩹 Suggested fix
private async Task SetClassificationAsync()
{
if (_labelData is null || string.IsNullOrEmpty(_selectedClassification)) return;
-
- var result = await FinancialLabelHttpClient.AddClassification(
- new(_labelData.Id, FinancialLabelClassificationCatalog.SpendingNecessityKind, _selectedClassification));
-
- if (!result)
+ try
{
- _errors.Insert(0, "Failed to set classification.");
- return;
+ var result = await FinancialLabelHttpClient.AddClassification(
+ new(_labelData.Id, FinancialLabelClassificationCatalog.SpendingNecessityKind, _selectedClassification));
+
+ if (!result)
+ {
+ _errors.Insert(0, "Failed to set classification.");
+ return;
+ }
+
+ _errors.Clear();
+ _info.Insert(0, $"Classification set to '{_selectedClassification}'.");
+
+ _labelData = await FinancialLabelHttpClient.Get(_labelData.Id);
+ if (_labelData is not null)
+ _selectedClassification = _labelData.Classifications
+ .FirstOrDefault(c => c.Kind == FinancialLabelClassificationCatalog.SpendingNecessityKind)?.Value
+ ?? string.Empty;
}
-
- _errors.Clear();
- _info.Insert(0, $"Classification set to '{_selectedClassification}'.");
-
- _labelData = await FinancialLabelHttpClient.Get(_labelData.Id);
- if (_labelData is not null)
- _selectedClassification = _labelData.Classifications
- .FirstOrDefault(c => c.Kind == FinancialLabelClassificationCatalog.SpendingNecessityKind)?.Value
- ?? string.Empty;
+ catch (Exception ex)
+ {
+ _errors.Insert(0, $"Failed to set classification: {ex.Message}");
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/FinanceManager.Components/Components/Admin/EditLabelPage.razor.cs`
around lines 52 - 73, SetClassificationAsync currently calls
FinancialLabelHttpClient.AddClassification and Get without error handling; wrap
the await calls in a try/catch that catches exceptions from AddClassification
and Get, add a descriptive error entry to _errors (and optionally log the
exception), and return gracefully so exceptions don't bubble out of the event
handler; keep the existing success path (clearing _errors, inserting _info,
refreshing _labelData, and updating _selectedClassification) inside the try
block and ensure the catch does not overwrite successful _info entries.
...inanceManager.Components/Components/Dashboard/Cards/ExpenseDistributionOverviewCard.razor.cs
Show resolved
Hide resolved
| return maxValue == 0 ? 0 : (double)(Math.Abs(value) / maxValue * 100); | ||
| } | ||
|
|
||
| private string GetCategoryIcon(string categoryName) => categoryName.ToLower() switch |
There was a problem hiding this comment.
Guard against null category names before normalization.
Line 67 dereferences categoryName with ToLower() and can throw during render if upstream data contains null/empty names.
🛡️ Suggested fix
-private string GetCategoryIcon(string categoryName) => categoryName.ToLower() switch
+private string GetCategoryIcon(string? categoryName)
{
+ if (string.IsNullOrWhiteSpace(categoryName))
+ return Icons.Material.Filled.LocalAtm;
+
+ var normalized = categoryName.ToLowerInvariant();
+ return normalized switch
+ {
var n when n.Contains("salary") => Icons.Material.Filled.Paid,
@@
- _ => Icons.Material.Filled.LocalAtm
- };
+ _ => Icons.Material.Filled.LocalAtm
+ };
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@code/FinanceManager.Components/Components/Dashboard/Cards/FinancialLabelsListCard.razor.cs`
at line 67, The GetCategoryIcon method currently calls categoryName.ToLower()
which can throw for null; update GetCategoryIcon(string categoryName) to guard
against null/empty inputs (e.g., check string.IsNullOrWhiteSpace(categoryName)
or use categoryName ?? string.Empty) before normalizing, and return a sensible
default icon when the name is missing; ensure any subsequent switch/lookup
operates on the safe, lower-cased value (for example use var key = (categoryName
?? string.Empty).ToLowerInvariant()) and reference that key in the switch.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
UI Improvements