feat(imports): validate Sure NDJSON before publish#1833
Conversation
|
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 SureImport NDJSON preflight validation, a persisted ChangesSplit Transactions & Taxonomy Merging Import Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as ImportsController
participant Preflight as SureImport::Preflight
participant Importer as SureImport
participant Background as ImportJob
Client->>API: POST /imports (NDJSON + merge_existing_taxonomy?)
API->>Preflight: SureImport::Preflight.new(family:, content:, merge_existing_taxonomy:).call
Preflight-->>API: Result (valid?, stats, errors, warnings)
API->>Importer: create import record (persist merge flag)
alt publish requested
API->>Importer: publish_later (runs validate_sure_preflight!)
Importer->>Background: ImportJob.perform_later(import)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
|
The preflight design is solid and the test coverage is thorough. One structural limitation worth flagging before merge: Reference validation only checks NDJSON source IDs, not existing family records The same applies to For the strict "full package" import case this is intentional and correct. But it means the preflight gate is currently incompatible with any incremental or additive NDJSON that builds on the existing family state. The PR description hints at this being intentional ("strict default"), but it's worth making the limitation explicit in a comment on If incremental imports are a future goal, the fix would be to seed Generated by Claude Code |
superagent-security
Bot
added
contributor:verified
Addressed in e4bb70b. I kept strict mode package-scoped rather than seeding What changed:
So yes, this is now explicit in code, tested, and reflected in the generated API surface without quietly expanding strict mode into incremental import behavior. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/family/data_exporter.rb (1)
305-323: ⚡ Quick winPotential N+1 queries when serializing split lines.
The includes on line 305 does not eager load
child_entriesor their associations. For each split parent,serialize_split_lines_for_exporttriggers separate queries forchild_entries, theirentryable, and potentiallytag_ids.♻️ Suggested fix to eager load child entries
- ndjson_exportable_transactions.includes(:category, :merchant, :tags, entry: :account).find_each do |transaction| + ndjson_exportable_transactions.includes(:category, :merchant, :tags, entry: [:account, { child_entries: { entryable: :tags } }]).find_each do |transaction|🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/family/data_exporter.rb` around lines 305 - 323, The code is causing N+1 queries because ndjson_exportable_transactions is not eager-loading split child entries and their associations; update the query used in ndjson_exportable_transactions/includes(...) to also preload entry.child_entries and their entryable and tags (so that serialize_split_lines_for_export doesn’t trigger per-transaction queries). Specifically, ensure the relation passed into ndjson_exportable_transactions.includes includes nested associations like entry: [:account, { child_entries: [:entryable, :tags] }] or equivalent so serialize_split_lines_for_export, child_entries, entryable and tag_ids are loaded up front.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/sure_import.rb`:
- Around line 154-156: Change the bare rescue in the SureImport exception
handling to only catch application-level errors: replace "rescue => error" with
"rescue StandardError => error" in the block that calls update! status: :failed,
error: error.message (the rescue around the SureImport import logic), so
SignalException/SystemExit aren't swallowed while preserving the existing
update! behavior.
---
Nitpick comments:
In `@app/models/family/data_exporter.rb`:
- Around line 305-323: The code is causing N+1 queries because
ndjson_exportable_transactions is not eager-loading split child entries and
their associations; update the query used in
ndjson_exportable_transactions/includes(...) to also preload entry.child_entries
and their entryable and tags (so that serialize_split_lines_for_export doesn’t
trigger per-transaction queries). Specifically, ensure the relation passed into
ndjson_exportable_transactions.includes includes nested associations like entry:
[:account, { child_entries: [:entryable, :tags] }] or equivalent so
serialize_split_lines_for_export, child_entries, entryable and tag_ids are
loaded up front.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e345822-4081-42b5-b232-57759fa9c635
📒 Files selected for processing (19)
app/controllers/api/v1/imports_controller.rbapp/controllers/import/uploads_controller.rbapp/controllers/imports_controller.rbapp/models/family/data_exporter.rbapp/models/family/data_importer.rbapp/models/import/preflight.rbapp/models/sure_import.rbapp/models/sure_import/preflight.rbapp/views/imports/_failure.html.erbdb/migrate/20260518170000_add_import_options_to_imports.rbdb/schema.rbdocs/api/openapi.yamlspec/requests/api/v1/imports_spec.rbtest/controllers/api/v1/imports_controller_test.rbtest/models/family/data_exporter_test.rbtest/models/family/data_importer_test.rbtest/models/import/preflight_test.rbtest/models/sure_import_test.rbtest/views/imports/failure_view_test.rb
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4bb70be7d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
JSONbored
force-pushed
the
codex/import-preflight-readiness
branch
from
71b0793 to
5f8f3c9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/sure_import.rb (1)
133-142: 💤 Low valueConsider using a custom error class for consistency.
The method raises
MaxRowCountExceededErrorandPreflightErrorfor specific failure conditions, but line 137 raises a bareRuntimeError. For consistent error handling by callers, consider introducing aNotPublishableErroror similar.♻️ Suggested refactor
class SureImport < Import PreflightError = Class.new(StandardError) + NotPublishableError = Class.new(StandardError)- raise "Import is not publishable" unless publishable? + raise NotPublishableError, "Import is not publishable" unless publishable?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/sure_import.rb` around lines 133 - 142, The publish_later method raises a bare RuntimeError when the import is not publishable; replace that with a custom error (e.g., NotPublishableError) so callers can handle it consistently: add a new NotPublishableError class (parallel to MaxRowCountExceededError/PreflightError) and change the raise "Import is not publishable" in publish_later to raise NotPublishableError, keeping the existing checks row_count_exceeded?, validate_sure_preflight!, and publishable? intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/models/sure_import.rb`:
- Around line 133-142: The publish_later method raises a bare RuntimeError when
the import is not publishable; replace that with a custom error (e.g.,
NotPublishableError) so callers can handle it consistently: add a new
NotPublishableError class (parallel to MaxRowCountExceededError/PreflightError)
and change the raise "Import is not publishable" in publish_later to raise
NotPublishableError, keeping the existing checks row_count_exceeded?,
validate_sure_preflight!, and publishable? intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 283211e2-ee1d-456b-8397-1822c92ac086
📒 Files selected for processing (19)
app/controllers/api/v1/imports_controller.rbapp/controllers/import/uploads_controller.rbapp/controllers/imports_controller.rbapp/models/family/data_exporter.rbapp/models/family/data_importer.rbapp/models/import/preflight.rbapp/models/sure_import.rbapp/models/sure_import/preflight.rbapp/views/imports/_failure.html.erbdb/migrate/20260518170000_add_import_options_to_imports.rbdb/schema.rbdocs/api/openapi.yamlspec/requests/api/v1/imports_spec.rbtest/controllers/api/v1/imports_controller_test.rbtest/models/family/data_exporter_test.rbtest/models/family/data_importer_test.rbtest/models/import/preflight_test.rbtest/models/sure_import_test.rbtest/views/imports/failure_view_test.rb
✅ Files skipped from review due to trivial changes (3)
- db/migrate/20260518170000_add_import_options_to_imports.rb
- app/controllers/import/uploads_controller.rb
- db/schema.rb
🚧 Files skipped from review as they are similar to previous changes (14)
- app/controllers/imports_controller.rb
- app/controllers/api/v1/imports_controller.rb
- test/models/import/preflight_test.rb
- test/models/family/data_exporter_test.rb
- test/views/imports/failure_view_test.rb
- spec/requests/api/v1/imports_spec.rb
- app/models/import/preflight.rb
- app/views/imports/_failure.html.erb
- test/models/family/data_importer_test.rb
- docs/api/openapi.yaml
- test/models/sure_import_test.rb
- app/models/family/data_importer.rb
- test/controllers/api/v1/imports_controller_test.rb
- app/models/family/data_exporter.rb
|
Ready for maintainer review. Addressed the CodeRabbit nitpick by replacing SureImport's generic publish_later RuntimeError with SureImport::NotPublishableError and added focused model coverage. Current head 82ffbde has required CI green. |
Track split parent entries for Sure import revert, preserve explicit empty split-line taxonomy values, and eager-load split child data during NDJSON export. Also narrows SureImport publish failure handling to StandardError and adds regression coverage for the reviewed paths.
JSONbored
force-pushed
the
codex/import-preflight-readiness
branch
from
9ca9551 to
c726e03
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/imports_controller.rb (1)
22-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRescue
SureImport::NotPublishableErrorin web publish flow.Line 23 can raise
SureImport::NotPublishableError; it is not rescued here, so users can hit a 500 for a predictable validation condition.Proposed fix
def publish `@import.publish_later` redirect_to import_path(`@import`), notice: t(".started") rescue Import::MaxRowCountExceededError redirect_back_or_to import_path(`@import`), alert: t(".max_rows_exceeded", max: `@import.max_row_count`) + rescue SureImport::NotPublishableError => e + redirect_back_or_to import_path(`@import`), alert: e.message rescue SureImport::PreflightError => e redirect_to import_path(`@import`), alert: e.message end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/imports_controller.rb` around lines 22 - 30, The publish action can raise SureImport::NotPublishableError when calling `@import.publish_later`; add a rescue clause for SureImport::NotPublishableError in the publish method (alongside the existing SureImport::PreflightError rescue) to handle this predictable validation failure by redirecting to import_path(`@import`) (or redirect_back_or_to if preferred) and showing an appropriate alert (use the exception message or a localized i18n key). Ensure you reference the publish method and the exception class SureImport::NotPublishableError when making the change.
🧹 Nitpick comments (1)
app/views/imports/_failure.html.erb (1)
15-21: ⚡ Quick winPrefer
DS::Alertover custom alert container markup.This block is implementing design-system alert behavior manually; use the existing DS alert component for consistency and maintainability.
As per coding guidelines, "Check
app/components/DS/for existing design system components (DS::Alert,DS::Button,DS::Disclosure,DS::Dialog,DS::Menu, etc.) before writing custom component logic."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/imports/_failure.html.erb` around lines 15 - 21, Replace the custom alert markup with the design-system component DS::Alert: remove the manual div/list markup and render DS::Alert (from app/components/DS/) with the same styling/intent (destructive variant) and move the existing iteration over import.error.lines.map(&:strip).reject(&:blank?) into the DS::Alert body so each message is rendered as a list item; ensure you use the DS::Alert API (variant/role/heading slots if available) to preserve semantics and visual behavior while keeping the iteration logic tied to the import object and the same message filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/api/v1/imports_controller.rb`:
- Around line 245-261: Add a specific rescue for SureImport::NotPublishableError
before the generic StandardError rescue so that when `@import.publish_later`
raises NotPublishableError it returns a 422 instead of falling through to 500;
render a JSON response matching the other error blocks (error:
"not_publishable", a helpful message like "Import was uploaded but has no
publishable records.", include import_id: `@import.id` and any relevant
`@import.error` details if available) and set status: :unprocessable_entity, then
return to stop further handling.
In `@app/models/sure_import.rb`:
- Around line 156-159: In publish_later, avoid leaving the record stuck in
:importing if ImportJob.perform_later(self) raises: capture the current status
(e.g., previous_status = status), call update! status: :importing, then wrap
ImportJob.perform_later(self) in a begin/rescue; on rescue update! status:
previous_status (or the appropriate fallback) and re-raise or propagate the
error so callers see the failure; optionally wrap the status change and enqueue
in a transaction if supported. Ensure you reference the update! status:
:importing line and the ImportJob.perform_later(self) call when making the
change.
---
Outside diff comments:
In `@app/controllers/imports_controller.rb`:
- Around line 22-30: The publish action can raise
SureImport::NotPublishableError when calling `@import.publish_later`; add a rescue
clause for SureImport::NotPublishableError in the publish method (alongside the
existing SureImport::PreflightError rescue) to handle this predictable
validation failure by redirecting to import_path(`@import`) (or
redirect_back_or_to if preferred) and showing an appropriate alert (use the
exception message or a localized i18n key). Ensure you reference the publish
method and the exception class SureImport::NotPublishableError when making the
change.
---
Nitpick comments:
In `@app/views/imports/_failure.html.erb`:
- Around line 15-21: Replace the custom alert markup with the design-system
component DS::Alert: remove the manual div/list markup and render DS::Alert
(from app/components/DS/) with the same styling/intent (destructive variant) and
move the existing iteration over
import.error.lines.map(&:strip).reject(&:blank?) into the DS::Alert body so each
message is rendered as a list item; ensure you use the DS::Alert API
(variant/role/heading slots if available) to preserve semantics and visual
behavior while keeping the iteration logic tied to the import object and the
same message filtering.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04177f49-0d8d-469e-9f8a-e3c44b4b037e
📒 Files selected for processing (19)
app/controllers/api/v1/imports_controller.rbapp/controllers/import/uploads_controller.rbapp/controllers/imports_controller.rbapp/models/family/data_exporter.rbapp/models/family/data_importer.rbapp/models/import/preflight.rbapp/models/sure_import.rbapp/models/sure_import/preflight.rbapp/views/imports/_failure.html.erbdb/migrate/20260518170000_add_import_options_to_imports.rbdb/schema.rbdocs/api/openapi.yamlspec/requests/api/v1/imports_spec.rbtest/controllers/api/v1/imports_controller_test.rbtest/models/family/data_exporter_test.rbtest/models/family/data_importer_test.rbtest/models/import/preflight_test.rbtest/models/sure_import_test.rbtest/views/imports/failure_view_test.rb
✅ Files skipped from review due to trivial changes (1)
- db/schema.rb
2 participants
Summary
What changed
Why
Validation
bin/rails test test/models/sure_import_test.rb test/models/family/data_importer_test.rb test/models/import/preflight_test.rb test/controllers/api/v1/imports_controller_test.rb test/views/imports/failure_view_test.rbRAILS_ENV=test bundle exec rake rswag:specs:swaggerizebin/rubocopbundle exec erb_lint app/views/imports/_failure.html.erbbin/rails zeitwerk:checkbundle exec brakeman -q -f jsongit diff --checkbin/rails test test/models/sure_import_test.rbRAILS_ENV=test bundle exec rake rswag:specs:swaggerizebin/rubocop app/models/sure_import/preflight.rb test/models/sure_import_test.rb spec/requests/api/v1/imports_spec.rbgit diff --checkNotes
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements
Documentation