Enhanced Import Amount Type Selection #179
Conversation
Introduces a new field, amount_type_identifier_value, to the import configuration and database schema. Updates controller, model logic, JavaScript, and view to support selecting and treating a specific identifier value for custom column amount type strategy. Adjusts tests to use the new field.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an amount-type identifier field and inflow-treatment handling across controller params, Stimulus UI, views, model validation and signage logic, a DB migration, and tests to support identifier-based amount-type mapping. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant B as Browser (Form)
participant S as Stimulus
participant R as Rails Controller
participant I as Import (model)
participant Row as Import::Row
participant DB as DB
U->>B: Select strategy = custom_column and pick identifier
B->>S: connect / identifier change
S->>S: handleAmountTypeIdentifierChange / show inflow UI
S-->>B: Reveal inflow-treatment selector
U->>B: Submit configuration (identifier + treatment)
B->>R: POST/PATCH configuration
R->>DB: Permit & persist amount_type_identifier_value and amount_type_inflow_value
DB-->>R: Save OK
R->>I: Validate (custom_column requires identifier or explicit treatment)
R-->>B: Response
loop Importing rows
R->>Row: apply_transaction_signage_convention(row)
Row->>Row: Determine selected_identifier (or legacy_identifier)
alt selected_identifier present
Row->>Row: Compare row.entity_type with selected_identifier
alt amount_type_inflow_value == inflows_positive
Row->>Row: Match => -1, No match => +1
else
Row->>Row: Match => +1, No match => -1
end
else
Row->>Row: Use legacy inflow comparison logic
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Pull Request Overview
Enhances the CSV import configuration to allow users to explicitly map a selected column value to either income or expense, rather than assuming a single inflow value. Introduces a new identifier field and updates UI, processing logic, and strong parameters accordingly.
- Added amount_type_identifier_value column and form control to distinguish the triggering CSV value.
- Updated transaction sign application logic to handle both sides based on chosen treatment.
- Enhanced Stimulus controller to progressively reveal dependent selection UI.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rb | Adds new amount_type_identifier_value column to imports table. |
| app/views/import/configurations/_transaction_import.html.erb | Splits configuration into identifier selection and treatment (income/expense) selection. |
| app/controllers/import/configurations_controller.rb | Permits new parameter in strong params. |
| app/models/import/row.rb | Adjusts sign logic to use identifier + treatment instead of a single inflow match. |
| app/javascript/controllers/import_controller.js | Adds logic to show/hide new dependent select and update displayed identifier. |
| test/models/transaction_import_test.rb | Updates fixture/setup to include new identifier field. |
| #showAmountTypeInflowValueTargets(amountTypeColumnKey) { | ||
| // This should be called when amount_type_identifier_value changes | ||
| // We need to update the displayed identifier value in the UI text | ||
| const identifierValueSelect = this.amountTypeValueTarget.querySelector("select"); | ||
| const selectedValue = identifierValueSelect.value; | ||
|
|
||
| if (!selectedValue) { | ||
| this.amountTypeInflowValueTarget.classList.add("hidden"); | ||
| this.amountTypeInflowValueTarget.classList.remove("flex"); | ||
| return; | ||
| } | ||
|
|
||
| // Show the inflow value dropdown | ||
| this.amountTypeInflowValueTarget.classList.remove("hidden"); | ||
| this.amountTypeInflowValueTarget.classList.add("flex"); | ||
|
|
||
| // Update the displayed identifier value in the text | ||
| const identifierSpan = this.amountTypeInflowValueTarget.querySelector("span.font-medium"); | ||
| if (identifierSpan) { | ||
| identifierSpan.textContent = selectedValue; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The parameter amountTypeColumnKey is never used and all invocations pass no argument, which can confuse future readers; remove the unused parameter from the method signature (and its comment if not needed) to clarify intent.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/import/configurations_controller.rb(1 hunks)app/javascript/controllers/import_controller.js(6 hunks)app/models/import/row.rb(1 hunks)app/views/import/configurations/_transaction_import.html.erb(1 hunks)db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rb(1 hunks)test/models/transaction_import_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (33)
db/migrate/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place database migrations under db/migrate/
Simple validations like null checks and unique constraints should be handled by database constraints in migration files.
ActiveRecord migrations must inherit from
ActiveRecord::Migration[7.2]; do not use version 8.0
Files:
db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rbtest/models/transaction_import_test.rbapp/controllers/import/configurations_controller.rbapp/models/import/row.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rbtest/models/transaction_import_test.rbapp/controllers/import/configurations_controller.rbapp/javascript/controllers/import_controller.jsapp/models/import/row.rb
**/*.{rb,erb,haml,slim}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb,haml,slim}: UseCurrent.userfor the current user; do not usecurrent_user
UseCurrent.familyfor the current family; do not usecurrent_family
Ignore i18n methods; hardcode strings in English for now (do not useI18n.t,t, or similar)
Files:
db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rbtest/models/transaction_import_test.rbapp/controllers/import/configurations_controller.rbapp/models/import/row.rbapp/views/import/configurations/_transaction_import.html.erb
db/migrate/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Enforce simple validations in the database (NULL constraints, unique indexes, etc.)
Files:
db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rb
test/**/*_test.rb
📄 CodeRabbit inference engine (AGENTS.md)
Name Minitest files with *_test.rb and mirror the app/ structure under test/
test/**/*_test.rb: ALWAYS use Minitest + fixtures + Mocha for tests; NEVER RSpec or FactoryBot.
Use Mocha for mocking in tests when necessary.
Use VCR for external API tests.
test/**/*_test.rb: Always use Minitest for tests; do not use RSpec
Do not use factories (e.g., FactoryBot) in tests; rely on fixtures instead
For tests needing many records, use Rails helpers to construct data and inline the creation in the test
Only write tests for critical and important code paths
Avoid tests that merely verify framework/ActiveRecord behavior
Test boundaries correctly: for queries, assert returned values; for commands, assert collaborators are called with correct params
Never test the implementation details of one class in another class’s test suite
Use the mocha gem for stubs and mocks
Prefer OpenStruct for mock instances; use a mock class for complex cases
Only mock what’s necessary; don’t mock return values unless they are under test
Files:
test/models/transaction_import_test.rb
**/test/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.rb: ALWAYS use Minitest and fixtures for Ruby tests; NEVER RSpec or factories.
Only test critical and important code paths; system tests should be used sparingly for critical user flows.
Files:
test/models/transaction_import_test.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/controllers/import/configurations_controller.rbapp/models/import/row.rb
**/app/**/*.{rb,erb,js,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Push Rails to its limits before adding new dependencies; a strong technical/business reason is required for new dependencies.
Files:
app/controllers/import/configurations_controller.rbapp/javascript/controllers/import_controller.jsapp/models/import/row.rbapp/views/import/configurations/_transaction_import.html.erb
**/app/controllers/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Controllers should be skinny; business logic should go in models (
app/models/), not inapp/services/.
Files:
app/controllers/import/configurations_controller.rb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/controllers/import/configurations_controller.rbapp/models/import/row.rbapp/views/import/configurations/_transaction_import.html.erb
app/javascript/**/*.{js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
app/javascript/**/*.{js,jsx}: Place JavaScript code under app/javascript/
JavaScript naming: lowerCamelCase for variables/functions; PascalCase for classes/components
Let Biome format and lint JavaScript code (npm run lint/format)
Files:
app/javascript/controllers/import_controller.js
app/javascript/controllers/**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/javascript/controllers/**/*.js: Stimulus controllers should remain lightweight, have fewer than 7 targets, use private methods and expose a clear public API.
Pass data viadata-*-valueattributes, not with inline JavaScript.
Component-specific controllers should stay near components; global controllers go inapp/javascript/controllers/.
Files:
app/javascript/controllers/import_controller.js
**/*.{html,erb,slim,js,jsx,ts,tsx,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always use functional design tokens (e.g., text-primary, bg-container) from the design system; do not use raw colors or ad-hoc classes.
Files:
app/javascript/controllers/import_controller.jsapp/views/import/configurations/_transaction_import.html.erb
**/app/javascript/controllers/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
**/app/javascript/controllers/**/*.js: Stimulus controllers should be lightweight and simple (<7 targets), use private methods, have clear public APIs, and follow single-responsibility principles.
Pass data viadata-*-valueattributes, not inline JavaScript, in Stimulus controllers.
Files:
app/javascript/controllers/import_controller.js
app/javascript/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use URL query params for state over localStorage/sessionStorage; only persist to DB if absolutely necessary
Files:
app/javascript/controllers/import_controller.js
app/javascript/controllers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/javascript/controllers/**/*.{js,ts}: Stimulus controllers should display preformatted values; avoid implementing formatting logic client-side
Keep client-side code for interactions where it truly shines (e.g., bulk selection); avoid unnecessary client-only solutionsPlace global Stimulus controllers under app/javascript/controllers so they can be used across any view
Files:
app/javascript/controllers/import_controller.js
{app/javascript/controllers/**/*.{js,ts},app/views/**/*.erb,app/components/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Use declarative Stimulus actions in ERB (data-action) instead of imperative event listeners in controller lifecycle (e.g., avoid addEventListener in connect); controllers should just respond to actions
Files:
app/javascript/controllers/import_controller.jsapp/views/import/configurations/_transaction_import.html.erb
{app/javascript/controllers/**/*.{js,ts},app/components/**/*.{js,ts}}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
{app/javascript/controllers/**/*.{js,ts},app/components/**/*.{js,ts}}: Keep Stimulus controllers lightweight: aim for fewer than 7 targets; use private methods and expose a clear public API
Keep Stimulus controllers focused: no domain logic; favor single responsibility; leverage Stimulus callbacks, actions, targets, values, and classes
Files:
app/javascript/controllers/import_controller.js
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Style UI using TailwindCSS v4.x with the custom design system defined in app/assets/tailwind/maybe-design-system.css
Always start by referencing app/assets/tailwind/maybe-design-system.css to understand available primitives, functional tokens, and component tokens before styling
Prefer functional tokens from the design system over raw Tailwind values (e.g., use text-primary, bg-container, border border-primary instead of text-white, bg-white, border-gray-200)
Files:
app/javascript/controllers/import_controller.jsapp/views/import/configurations/_transaction_import.html.erb
{app/javascript/controllers,app/components}/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Use Stimulus targets for DOM access instead of manual document.getElementById calls
Files:
app/javascript/controllers/import_controller.js
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Use ActiveRecord validations for forms and complex domain constraints.
app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required
Files:
app/models/import/row.rb
**/app/models/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Business logic should primarily reside in models; use concerns and POROs for organization.
Files:
app/models/import/row.rb
app/{helpers,models}/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates on the server side (Ruby) before sending to the client
Files:
app/models/import/row.rb
app/views/**/*.erb
📄 CodeRabbit inference engine (AGENTS.md)
app/views/**/*.erb: Keep heavy logic out of ERB views; prefer helpers/components instead
ERB templates are linted by erb-lint per .erb_lint.ymlAlways use the icon helper (icon) for icons; never call lucide_icon directly
Files:
app/views/import/configurations/_transaction_import.html.erb
app/views/**/*.html.*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/views/**/*.html.*: Use partials only for simple, context-specific, mostly static HTML content.
Prefer semantic HTML; use Turbo Frames where possible instead of client-side solutions.
Use query params for state, not localStorage or sessionStorage.
Always perform server-side formatting for currencies, numbers, and dates.
Files:
app/views/import/configurations/_transaction_import.html.erb
**/app/**/*.erb
📄 CodeRabbit inference engine (CLAUDE.md)
Always use the
iconhelper for icons in templates; never uselucide_icondirectly.
Files:
app/views/import/configurations/_transaction_import.html.erb
**/app/views/**/*.erb
📄 CodeRabbit inference engine (CLAUDE.md)
**/app/views/**/*.erb: Keep domain logic OUT of view templates; logic belongs in component files, not templates.
Use partials only for primarily static, simple HTML with minimal logic, and only when not likely to be reused.
Prefer native HTML over JavaScript components for UI elements (e.g., use<dialog>,<details><summary>).
Leverage Turbo frames for page sections over client-side JavaScript solutions.
Use query params (not localStorage/sessions) for client state management.
Perform server-side formatting for currencies, numbers, and dates in templates.
Files:
app/views/import/configurations/_transaction_import.html.erb
app/views/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/views/**/*.html.erb: Prefer native HTML elements (e.g., ,) over JS-based components
Leverage Turbo frames to break up pages instead of JS-driven client-side solutions
Prefer native client-side form validation when possible
Files:
app/views/import/configurations/_transaction_import.html.erb
{app/components/**/*.{js,ts,erb},app/views/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Component-scoped Stimulus controllers in app/components must be used only within their component views, not in app/views
Files:
app/views/import/configurations/_transaction_import.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/import/configurations/_transaction_import.html.erb
{app/views,app/components}/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
{app/views,app/components}/**/*.html.erb: Keep domain logic out of ERB templates; compute values in component/controller code and reference them in the template
Integrate Stimulus declaratively in ERB: templates declare data-controller/data-action; controllers respond to those declarations
Pass data from Rails to Stimulus via data-*-value attributes, not inline JavaScript
Files:
app/views/import/configurations/_transaction_import.html.erb
app/views/**/_*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Name partials with a leading underscore (e.g., _trend_change.html.erb, _form_errors.html.erb)
Files:
app/views/import/configurations/_transaction_import.html.erb
🧠 Learnings (1)
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/{entry,valuation,transaction,trade}.rb : Entry amounts are signed: negative = inflow, positive = outflow (applies to Entry and its types: Valuation, Transaction, Trade)
Applied to files:
test/models/transaction_import_test.rbapp/models/import/row.rb
🧬 Code graph analysis (1)
app/javascript/controllers/import_controller.js (1)
app/javascript/controllers/multi_select_controller.js (1)
event(17-17)
🪛 GitHub Actions: Pull Request
app/javascript/controllers/import_controller.js
[error] 25-25: lint/complexity/useOptionalChain: Unsafe fix suggested by linter. Change to optional chaining (identifierValueSelect?.value) to satisfy rule.
[error] 40-40: lint/complexity/useOptionalChain: Unsafe fix suggested by linter. Change to optional chaining (identifierValueSelect?.value) to satisfy rule.
🪛 GitHub Check: ci / lint
db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rb
[failure] 5-5:
Layout/TrailingEmptyLines: Final newline missing.
🔇 Additional comments (3)
app/javascript/controllers/import_controller.js (3)
14-14: LGTM!The new target follows Stimulus conventions and keeps the controller within the guideline of fewer than 7 targets.
57-59: LGTM!The public method follows Stimulus action patterns with a clear, focused responsibility.
140-144: LGTM!Properly hides both inflow-related targets when switching to signed amount strategy, maintaining consistent UI state.
db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rb
Outdated
Show resolved
Hide resolved
Refactored Import::Row to better handle legacy and new custom column strategies for amount type, ensuring correct identifier and inflow treatment logic. Simplified JavaScript controller logic for showing inflow value targets. Minor formatting fix in migration file.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/javascript/controllers/import_controller.js (1)
86-107: Consider optional chaining for defensive programming.Past concern about unused parameter is resolved. However, line 90 accesses
identifierValueSelect.valuewithout checking ifidentifierValueSelectis null. While call sites currently guard against this, using optional chaining would be more defensive and consistent with the pattern at lines 24 and 38.Apply this diff:
- const identifierValueSelect = this.amountTypeValueTarget.querySelector("select"); - const selectedValue = identifierValueSelect.value; + const selectedValue = this.amountTypeValueTarget.querySelector("select")?.value;This allows the existing check at line 92 to handle cases where either the select doesn't exist or has no value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/javascript/controllers/import_controller.js(6 hunks)app/models/import/row.rb(1 hunks)db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- db/migrate/20251002120000_add_amount_type_identifier_value_to_imports.rb
🧰 Additional context used
📓 Path-based instructions (19)
app/javascript/**/*.{js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
app/javascript/**/*.{js,jsx}: Place JavaScript code under app/javascript/
JavaScript naming: lowerCamelCase for variables/functions; PascalCase for classes/components
Let Biome format and lint JavaScript code (npm run lint/format)
Files:
app/javascript/controllers/import_controller.js
app/javascript/controllers/**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/javascript/controllers/**/*.js: Stimulus controllers should remain lightweight, have fewer than 7 targets, use private methods and expose a clear public API.
Pass data viadata-*-valueattributes, not with inline JavaScript.
Component-specific controllers should stay near components; global controllers go inapp/javascript/controllers/.
Files:
app/javascript/controllers/import_controller.js
**/*.{html,erb,slim,js,jsx,ts,tsx,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always use functional design tokens (e.g., text-primary, bg-container) from the design system; do not use raw colors or ad-hoc classes.
Files:
app/javascript/controllers/import_controller.js
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
app/javascript/controllers/import_controller.jsapp/models/import/row.rb
**/app/javascript/controllers/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
**/app/javascript/controllers/**/*.js: Stimulus controllers should be lightweight and simple (<7 targets), use private methods, have clear public APIs, and follow single-responsibility principles.
Pass data viadata-*-valueattributes, not inline JavaScript, in Stimulus controllers.
Files:
app/javascript/controllers/import_controller.js
**/app/**/*.{rb,erb,js,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Push Rails to its limits before adding new dependencies; a strong technical/business reason is required for new dependencies.
Files:
app/javascript/controllers/import_controller.jsapp/models/import/row.rb
app/javascript/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use URL query params for state over localStorage/sessionStorage; only persist to DB if absolutely necessary
Files:
app/javascript/controllers/import_controller.js
app/javascript/controllers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/javascript/controllers/**/*.{js,ts}: Stimulus controllers should display preformatted values; avoid implementing formatting logic client-side
Keep client-side code for interactions where it truly shines (e.g., bulk selection); avoid unnecessary client-only solutionsPlace global Stimulus controllers under app/javascript/controllers so they can be used across any view
Files:
app/javascript/controllers/import_controller.js
{app/javascript/controllers/**/*.{js,ts},app/views/**/*.erb,app/components/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Use declarative Stimulus actions in ERB (data-action) instead of imperative event listeners in controller lifecycle (e.g., avoid addEventListener in connect); controllers should just respond to actions
Files:
app/javascript/controllers/import_controller.js
{app/javascript/controllers/**/*.{js,ts},app/components/**/*.{js,ts}}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
{app/javascript/controllers/**/*.{js,ts},app/components/**/*.{js,ts}}: Keep Stimulus controllers lightweight: aim for fewer than 7 targets; use private methods and expose a clear public API
Keep Stimulus controllers focused: no domain logic; favor single responsibility; leverage Stimulus callbacks, actions, targets, values, and classes
Files:
app/javascript/controllers/import_controller.js
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Style UI using TailwindCSS v4.x with the custom design system defined in app/assets/tailwind/maybe-design-system.css
Always start by referencing app/assets/tailwind/maybe-design-system.css to understand available primitives, functional tokens, and component tokens before styling
Prefer functional tokens from the design system over raw Tailwind values (e.g., use text-primary, bg-container, border border-primary instead of text-white, bg-white, border-gray-200)
Files:
app/javascript/controllers/import_controller.js
{app/javascript/controllers,app/components}/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Use Stimulus targets for DOM access instead of manual document.getElementById calls
Files:
app/javascript/controllers/import_controller.js
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Use ActiveRecord validations for forms and complex domain constraints.
app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required
Files:
app/models/import/row.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/models/import/row.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
app/models/import/row.rb
**/app/models/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Business logic should primarily reside in models; use concerns and POROs for organization.
Files:
app/models/import/row.rb
**/*.{rb,erb,haml,slim}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb,haml,slim}: UseCurrent.userfor the current user; do not usecurrent_user
UseCurrent.familyfor the current family; do not usecurrent_family
Ignore i18n methods; hardcode strings in English for now (do not useI18n.t,t, or similar)
Files:
app/models/import/row.rb
app/{helpers,models}/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates on the server side (Ruby) before sending to the client
Files:
app/models/import/row.rb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/models/import/row.rb
🧠 Learnings (1)
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/{entry,valuation,transaction,trade}.rb : Entry amounts are signed: negative = inflow, positive = outflow (applies to Entry and its types: Valuation, Transaction, Trade)
Applied to files:
app/models/import/row.rb
🧬 Code graph analysis (1)
app/javascript/controllers/import_controller.js (1)
app/javascript/controllers/multi_select_controller.js (1)
event(17-17)
🔇 Additional comments (6)
app/models/import/row.rb (1)
72-78: LGTM! Correct implementation of identifier/treatment split.The new path logic correctly handles both:
- Legacy imports (via fallback to
legacy_identifier)- New imports with explicit identifier/treatment split
The sign calculations properly convert CSV values to Sure's convention (negative = inflow, positive = outflow) for all combinations of identifier matches and treatment types.
Based on learnings
app/javascript/controllers/import_controller.js (5)
14-14: LGTM! New target added for inflow value UI.The
amountTypeInflowValuetarget supports the new identifier/treatment split flow in the UI.
24-26: Past concern addressed: optional chaining implemented.The code now uses optional chaining as requested in previous reviews, ensuring safe access to the select element's value.
38-40: Past concern addressed: optional chaining implemented.Consistent use of optional chaining as requested in previous reviews.
55-57: LGTM! Event handler for identifier selection.The handler correctly responds to identifier changes and updates the inflow value UI accordingly. Follows Stimulus conventions.
138-142: LGTM! Properly hides both identifier and inflow value UI elements.When switching to signed_amount strategy, both the identifier dropdown and inflow treatment dropdown are correctly hidden, maintaining clean UI state.
app/models/import/row.rb
Outdated
| if selected_identifier.blank? | ||
| # Legacy import: amount_type_inflow_value is the identifier, treat as inflow | ||
| if entity_type == inflow_treatment | ||
| value * -1 | ||
| else | ||
| value | ||
| end |
There was a problem hiding this comment.
Clarify the legacy path logic or remove if unreachable.
The legacy path appears to have semantic issues:
- Line 67 compares
entity_type(a CSV value like "credit") toinflow_treatment(a signage convention like "inflows_positive") - This comparison doesn't make sense because these are different types of values
- This path only executes when both
amount_type_identifier_valueandamount_type_inflow_valueare blank, which shouldn't happen for valid custom_column imports
The main logic paths (both legacy and new imports with identifiers) correctly use the new path at lines 72-78, so this may be unreachable code.
Consider one of these approaches:
- If this path is unreachable, add a validation to ensure at least one identifier field is present for custom_column strategy, then remove lines 65-71
- If this is defensive code, add a comment explaining when this case occurs and what the intended behavior is
🤖 Prompt for AI Agents
In app/models/import/row.rb around lines 65 to 71 the legacy branch that runs
when selected_identifier.blank? compares entity_type (a CSV token like "credit")
to inflow_treatment (a signage convention) and is likely unreachable; remove the
entire legacy branch (lines 65–71) and add a validation on the custom_column
import strategy to require at least one of amount_type_identifier_value or
amount_type_inflow_value be present (fail fast with a clear error message), or
alternatively keep the branch only if you add a clarifying comment explaining
the exact CSV/legacy case it defends and adjust the comparison to use compatible
semantics — prefer removing the branch and enforcing the validation to avoid
dead/incorrect logic.
Introduces a model validation to ensure custom column imports have either an identifier value or valid legacy inflow values. Also refactors Import::Row logic to simplify inflow treatment handling for legacy and new imports.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/import.rb(2 hunks)app/models/import/row.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Use ActiveRecord validations for forms and complex domain constraints.
app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required
Files:
app/models/import/row.rbapp/models/import.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/models/import/row.rbapp/models/import.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
app/models/import/row.rbapp/models/import.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
app/models/import/row.rbapp/models/import.rb
**/app/**/*.{rb,erb,js,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Push Rails to its limits before adding new dependencies; a strong technical/business reason is required for new dependencies.
Files:
app/models/import/row.rbapp/models/import.rb
**/app/models/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Business logic should primarily reside in models; use concerns and POROs for organization.
Files:
app/models/import/row.rbapp/models/import.rb
**/*.{rb,erb,haml,slim}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb,haml,slim}: UseCurrent.userfor the current user; do not usecurrent_user
UseCurrent.familyfor the current family; do not usecurrent_family
Ignore i18n methods; hardcode strings in English for now (do not useI18n.t,t, or similar)
Files:
app/models/import/row.rbapp/models/import.rb
app/{helpers,models}/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates on the server side (Ruby) before sending to the client
Files:
app/models/import/row.rbapp/models/import.rb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/models/import/row.rbapp/models/import.rb
🧠 Learnings (1)
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/{entry,valuation,transaction,trade}.rb : Entry amounts are signed: negative = inflow, positive = outflow (applies to Entry and its types: Valuation, Transaction, Trade)
Applied to files:
app/models/import/row.rb
|
Sounds like this would have helped you @sokie? Can you review the PR? 🙏 |
tbh I'm unsure of the usefulness of this, and problem is unclear.. |
|
the reason for this is not al my csv imports have incomes, my bank exports an csv with a column |
3 participants
Summary
Improved the import configuration flow to allow users to specify whether a selected CSV column value should be treated as income (inflow) or expense (outflow). This provides more flexibility when importing transaction data with custom amount type columns.
Problem
Previously, when using the "custom column" amount type strategy, users could only select which value from their CSV represented "inflow". This was limiting when:
before(csv only contains expense)
after
Summary by CodeRabbit
New Features
Chores
Tests