Code review

[VibeNL]

No description provided.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Dividend charts now display confirmed and predicted dividends as separate bars for clearer visual distinction.
  • Y-axis labels now dynamically include currency symbols for improved readability.

• Improvements

  • Enhanced primary currency handling with automatic fallback to native currency when conversion is unavailable.
  • Improved null-safety handling in dividend details modals for primary currency values.

Walkthrough

Refactors dividend retrieval into helper methods with defensive holdings and currency-conversion handling, updates UI to show confirmed vs predicted monthly bars and moves modal state to code-behind, changes UpcomingDividendModel currency fields to nullable, and fixes median computation to handle empty and even-length lists correctly.

Changes

Cohort / File(s)	Summary
Median Calculation GhostfolioSidekick/MarketDataMaintainer/PredictDividendsTask.cs	Median now returns 0 for empty input, computes the true statistical median (averages two middle values for even-length lists) and adds boundary checks.
Dividend Service Refactor PortfolioViewer/PortfolioViewer.WASM.Data/Services/UpcomingDividendsService.cs	Extracts helpers (GetHoldingsDictionaryAsync, GetUpcomingDividendsWithProfilesAsync) and DividendWithProfile type; replaces inline logic with helpers; skips items with no positive holdings; wraps currency conversion in try/catch and falls back to native currency; maps both native and primary-currency amounts.
UI: Upcoming Dividends Page PortfolioViewer/PortfolioViewer.WASM/Pages/UpcomingDividends.razor, PortfolioViewer/PortfolioViewer.WASM/Pages/UpcomingDividends.razor.cs	Removes modal state from the .razor markup (modal open/close handlers moved/added to code-behind); replaces single monthly bar series with two stacked Bars ("Confirmed" and "Predicted"); chart Y-axis now shows dynamic currency symbol; introduces selectedDividend, ShowDetails, and CloseDetails in code-behind.
Model: Nullable Primary Currency Fields PortfolioViewer/PortfolioViewer.WASM.Data/Models/UpcomingDividendModel.cs	Changes AmountPrimaryCurrency and DividendPerSharePrimaryCurrency from decimal to decimal? (nullable) to reflect possible conversion fallbacks or absent primary-currency values.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibbled numbers, found the middle true,
Bars stack confirmed beside predicted blue,
Helpers hop in, currencies fall back,
Models grow мягко nullable on track,
A rabbit cheers — dividends in view! 🥕

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (1 warning, 2 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Code review' is vague and generic, failing to convey meaningful information about the specific changes made to the codebase.	Consider using a more descriptive title that summarizes the primary changes, such as 'Improve dividend calculations and add currency conversion handling' or 'Refactor UpcomingDividendsService with enhanced data handling'.
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to assess whether meaningful context was given about the changes.	Add a pull request description explaining the changes, their rationale, and any relevant context for reviewers.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch code-review

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Development container published

Install with:

docker pull vibenl/ghostfoliosidekick:pr-934

[coderabbitai]

Note

Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it.

---

Generating unit tests... This may take up to 20 minutes.

[coderabbitai]

✅ Created PR with unit tests: #935

[VibeNL]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@PortfolioViewer/PortfolioViewer.WASM.Data/Services/UpcomingDividendsService.cs`:
- Around line 47-60: The code in UpcomingDividendsService sets
DividendPerSharePrimaryCurrency to the native amount when
currencyExchange.ConvertMoney fails but still leaves PrimaryCurrency as the
configured primary symbol, which can mislabel amounts; in the catch block of the
conversion attempt (the block that calls currencyExchange.ConvertMoney and sets
dividendPerSharePrimaryCurrency), update the fallback so you do NOT claim the
value is in the primary currency—either set dividendPerSharePrimaryCurrency to
null/nullable or set the accompanying currency label to item.Dividend.Currency
(native currency) instead of the primaryCurrency; apply the same change to the
similar fallback block around lines 73-75 so UI gets the correct
currency/nullable amount when conversion fails.
- Line 109: The private DTO class DividendWithProfile is not intended for
inheritance—mark the nested class DividendWithProfile as sealed to enforce this;
update its declaration from "private class DividendWithProfile" to "private
sealed class DividendWithProfile" so the compiler prevents subclassing and
expresses the intended usage.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c71d23c and 4613ab9.

📒 Files selected for processing (4)

• GhostfolioSidekick/MarketDataMaintainer/PredictDividendsTask.cs
• PortfolioViewer/PortfolioViewer.WASM.Data/Services/UpcomingDividendsService.cs
• PortfolioViewer/PortfolioViewer.WASM/Pages/UpcomingDividends.razor
• PortfolioViewer/PortfolioViewer.WASM/Pages/UpcomingDividends.razor.cs

💤 Files with no reviewable changes (1)

• PortfolioViewer/PortfolioViewer.WASM/Pages/UpcomingDividends.razor

📜 Review details

🧰 Additional context used

🧬 Code graph analysis (1)

PortfolioViewer/PortfolioViewer.WASM.Data/Services/UpcomingDividendsService.cs (2)

GhostfolioSidekick.UnitTests/MarketDataMaintainer/GatherDividendsTaskTests.cs (2)

• SymbolProfile (237-245)
• Dividend (247-262)

GhostfolioSidekick/MarketDataMaintainer/GatherDividendsTask.cs (1)

• Task (21-79)

🪛 GitHub Check: build-test-analyze

PortfolioViewer/PortfolioViewer.WASM.Data/Services/UpcomingDividendsService.cs

[warning] 47-47:
Remove this useless assignment to local variable 'dividendPerSharePrimaryCurrency'.

---

[warning] 109-109:
Private classes which are not derived in the current assembly should be marked as 'sealed'.

🔇 Additional comments (3)

GhostfolioSidekick/MarketDataMaintainer/PredictDividendsTask.cs (1)

228-239: Median update is correct and safer.

Good improvement: empty-input guard plus true median handling for even-sized lists.

PortfolioViewer/PortfolioViewer.WASM/Pages/UpcomingDividends.razor.cs (2)

65-99: Confirmed vs predicted stacked bars are a solid UX improvement.

The split-series chart makes forecast confidence clearer without changing data semantics.

---

63-63: This null-dereference concern is not valid.

The PrimaryCurrency property is guaranteed to be non-null by design. Its getter returns _cachedPrimaryCurrency ?? Currency.EUR, ensuring it always returns a Currency object (falling back to EUR if not yet cached). The service is initialized at app startup via Program.cs, and the interface contract explicitly documents this behavior. The code at line 63 is safe.

Likely an incorrect or invalid review comment.

[coderabbitai]

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
`@PortfolioViewer/PortfolioViewer.WASM.Data/Services/UpcomingDividendsService.cs`:
- Around line 49-65: The conversion call in UpcomingDividendsService is
unconditionally calling currencyExchange.ConvertMoney and on failure nulling out
valid same-currency values; change the logic in the block that sets
primaryCurrencyLabel/ dividendPerSharePrimaryCurrency/
expectedAmountPrimaryCurrency so that if nativeCurrency equals
primaryCurrency.Symbol (or primaryCurrency currency code), skip calling
ConvertMoney entirely and instead set dividendPerSharePrimaryCurrency =
item.Dividend.Amount and expectedAmountPrimaryCurrency = item.Dividend.Amount *
quantity and keep primaryCurrencyLabel = primaryCurrency.Symbol; only call
currencyExchange.ConvertMoney for differing currencies and keep the try/catch
around that call to handle conversion failures without affecting same-currency
assignments.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4613ab9 and 5ce790c.

📒 Files selected for processing (4)

• PortfolioViewer/PortfolioViewer.WASM.Data/Models/UpcomingDividendModel.cs
• PortfolioViewer/PortfolioViewer.WASM.Data/Services/UpcomingDividendsService.cs
• PortfolioViewer/PortfolioViewer.WASM/Pages/UpcomingDividends.razor
• PortfolioViewer/PortfolioViewer.WASM/Pages/UpcomingDividends.razor.cs

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: build-test-analyze

🧰 Additional context used

🧬 Code graph analysis (1)

PortfolioViewer/PortfolioViewer.WASM.Data/Services/UpcomingDividendsService.cs (2)

PortfolioViewer/PortfolioViewer.WASM.Data/Models/UpcomingDividendModel.cs (1)

• UpcomingDividendModel (3-23)

Database/Migrations/20251207160427_Dividends.cs (1)

• Dividends (8-60)

🔇 Additional comments (5)

PortfolioViewer/PortfolioViewer.WASM.Data/Models/UpcomingDividendModel.cs (1)

16-18: Nullable primary-currency fields are correctly modeled.

This aligns with conversion-failure semantics and avoids forcing misleading numeric defaults.

PortfolioViewer/PortfolioViewer.WASM.Data/Services/UpcomingDividendsService.cs (1)

87-129: Nice decomposition into focused data-access helpers.

Splitting holdings aggregation and dividend/profile retrieval improves readability and keeps concerns localized.

PortfolioViewer/PortfolioViewer.WASM/Pages/UpcomingDividends.razor (1)

125-139: Null-safe formatting update is correct.

Using nullable-aware formatting here matches the updated model and prevents null-related rendering issues.

PortfolioViewer/PortfolioViewer.WASM/Pages/UpcomingDividends.razor.cs (2)

65-101: Confirmed vs predicted chart series wiring looks good.

Trace setup, stacked mode, and legend configuration are consistent with the intended visualization.

---

103-113: Modal selection state in code-behind is a clean improvement.

ShowDetails / CloseDetails are straightforward and correctly manage selection lifecycle.

[sonarqubecloud]

Quality Gate failed

Failed conditions
79.1% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[github-actions]

Development container published

Install with:

docker pull vibenl/ghostfoliosidekick:pr-934