Skip to content

Feat/financial position - #208

Merged
nfebe merged 3 commits into
devfrom
feat/financial-position
Jul 13, 2026
Merged

Feat/financial position#208
nfebe merged 3 commits into
devfrom
feat/financial-position

Conversation

@austin047

Copy link
Copy Markdown
Collaborator

Description

  • AI chat fixes and improvements
  • Implementation for financial position
  • Implementation for holdings

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@austin047
austin047 requested review from Wuyika and nfebe and removed request for Wuyika July 13, 2026 04:14
@austin047
austin047 marked this pull request as ready for review July 13, 2026 04:32
@sourceant

sourceant Bot commented Jul 13, 2026

Copy link
Copy Markdown

Code Review Summary

This PR implements the 'Financial Position' and 'Holdings' features, including database schema updates, remote data sources, repositories, and UI screens. It also enhances the AI chat experience and improves exchange rate handling by shifting the logic to the backend.

🚀 Key Improvements

  • Introduction of TransactionIntent to classify money movement (loans, investments, etc.) beyond simple income/expense.
  • Offline-first approach for Financial Position using a dedicated cache table.
  • Structured holding management with support for both manual and auto-priced (CoinGecko) assets.

💡 Minor Suggestions

  • Centralize currency formatting logic into a single utility class.
  • Update non-English translation files to fix placeholders and untranslated English keys.

🚨 Critical Issues

  • Database migration v5 -> v6 is missing on_delete or on_update constraints for new FK-like references if they existed, though current implementation seems to follow existing patterns.

@sourceant sourceant Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review complete. See the overview comment for a summary.

"myGroup": "Meine gruppe",
"homeDescText": "Bist du bereit, dein Geld zu verfolgen?",
"groups": "Gruppen",
"groups": "Groups",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The translation is set to English. It should be translated to German.

Suggested change
"groups": "Groups",
+ "groups": "Gruppen",

isScrollControlled: true,
useSafeArea: true,
constraints: BoxConstraints(
maxHeight: MediaQuery.of(context).size.height * maxHeightRatio,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hardcoding the screen height ratio can cause issues on devices with unusual aspect ratios or when the keyboard is open. It is better to rely on the sheet's built-in scrollable behavior or provide a more flexible constraint.

Suggested change
maxHeight: MediaQuery.of(context).size.height * maxHeightRatio,
maxHeight: MediaQuery.sizeOf(context).height * maxHeightRatio,

final apiResponse = ApiResponse.fromJson(response.data);
final data = apiResponse.data as Map<String, dynamic>;

final rates = (data['rates'] as Map<String, dynamic>).map(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Mapping the rates and then manually adding the base key is redundant. The backend usually provides this, but if synthesizing, ensure null safety for the value conversion.

Suggested change
final rates = (data['rates'] as Map<String, dynamic>).map(
final rates = (data['rates'] as Map<String, dynamic>).map(
(key, value) => MapEntry(key, (value as num).toDouble()),
)..putIfAbsent(base, () => 1.0);

@nfebe
nfebe merged commit 02cb0cb into dev Jul 13, 2026
3 checks passed
@nfebe
nfebe deleted the feat/financial-position branch July 13, 2026 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants