feat(api): expose reset status polling#1598
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 an admin-protected asynchronous family account reset flow: Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as Users API
participant Queue as Job Queue
participant DB as Database
Client->>API: DELETE /api/v1/users/reset (auth, admin)
activate API
API->>Queue: Enqueue FamilyResetJob (family_id)
Queue-->>API: job_id (enqueued) or error
API-->>Client: 200 {status: rgba(0,128,0,0.5), job_id, family_id, status_url} or 500 {error: "reset_enqueue_failed"}
deactivate API
Client->>API: GET /api/v1/users/reset/status (auth, admin)
activate API
API->>DB: Aggregate per-family target counts
DB-->>API: counts (per-entity totals)
API-->>Client: 200 {status: rgba(0,128,0,0.5) or rgba(255,165,0,0.5), reset_complete, family_id, counts}
deactivate API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
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 |
superagent-security
Bot
added
the
contributor:flagged
|
| Dimension | Score | What it measures |
|---|---|---|
| Identity | 30 | Account age, contribution history, GPG keys, org memberships |
| Behavior | 80 | PR patterns, unsolicited contribution ratio, activity cadence |
| Content | 100 | PR body substance, issue linkage, contribution quality |
| Graph | 30 | Cross-repo trust, co-contributor relationships |
Analyzed by Brin · Full profile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/api/v1/users_controller.rb`:
- Around line 9-17: Wrap the FamilyResetJob.perform_later call in a begin/rescue
block to explicitly handle enqueue failures: call
FamilyResetJob.perform_later(current_resource_owner.family) inside the block,
rescue StandardError (or the specific ActiveJob/adapter exception if preferred),
log the exception, and return a controlled JSON error payload (e.g., message,
error, status: "failed_to_enqueue", and an appropriate HTTP status like 503)
instead of proceeding to render the success JSON that references job.job_id and
api_v1_users_reset_status_path; only render the original success response when
perform_later succeeds.
In `@spec/requests/api/v1/users_spec.rb`:
- Around line 92-102: The 401 and 403 response examples in the specs (response
'401' and response '403' blocks) lack explicit response schemas so the generated
OpenAPI omits the standard error contract; update those response blocks in
spec/requests/api/v1/users_spec.rb to include the same error response schema
used elsewhere (e.g., reference the shared error schema or add a schema block
with properties like code/message/details) so the OpenAPI generator emits the
standard error body for both unauthorized and forbidden responses.
In `@test/controllers/api/v1/users_controller_test.rb`:
- Around line 90-100: The test "reset status requires admin role" uses
ApiKey.create! with scopes ["read"], which could make the 403 come from scope
enforcement instead of role checks; change the non_admin_api_key creation in
this test (ApiKey.create!, users(:family_member), display_key) to use the
"read_write" scope so the key has sufficient scopes and the assertion (get
"/api/v1/users/reset/status" with api_headers(non_admin_api_key) and
assert_response :forbidden) isolates failure to admin-role enforcement rather
than scope restrictions.
🪄 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: 19aaa22f-97ef-4b71-ab64-631a42b0d497
📥 Commits
Reviewing files that changed from the base of the PR and between c91b730 and e7df60921d41bcf3af1d25ad68d4c4972c58f7f4.
📒 Files selected for processing (6)
app/controllers/api/v1/users_controller.rbconfig/routes.rbdocs/api/openapi.yamlspec/requests/api/v1/users_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/users_controller_test.rb
JSONbored
force-pushed
the
feat/api-reset-status
branch
from
e7df609 to
baf22e0
Compare
![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: baf22e034f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/controllers/api/v1/users_controller_test.rb (1)
71-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the reset job receives the family.
This only verifies that
FamilyResetJobwas enqueued. Add an argument assertion so the test fails if the controller enqueues the wrong family.Suggested patch
- assert_enqueued_with(job: FamilyResetJob) do + assert_enqueued_with(job: FamilyResetJob, args: [ `@user.family` ]) do delete "/api/v1/users/reset", headers: api_headers(`@api_key`) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/api/v1/users_controller_test.rb` around lines 71 - 83, Update the assertion that the FamilyResetJob was enqueued to also verify the job was given the correct family argument: change the assert_enqueued_with in the test "reset enqueues FamilyResetJob and returns 200" to include args: [`@user.family`] (i.e. assert_enqueued_with(job: FamilyResetJob, args: [`@user.family`]) do ... end) so the test fails if the controller enqueues the wrong family when calling delete "/api/v1/users/reset".docs/api/openapi.yaml (1)
2800-2857:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the enqueue-failure response.
The controller now returns a 500
ErrorResponsewhenFamilyResetJob.perform_laterfails, but this OpenAPI block still only documents 200/401/403. Add the 500 response so the checked-in contract matches runtime behavior.Suggested patch
responses: '200': description: account reset initiated content: application/json: schema: "$ref": "#/components/schemas/ResetInitiatedResponse" '401': description: unauthorized content: application/json: schema: "$ref": "#/components/schemas/ErrorResponse" '403': description: "forbidden — requires read_write scope and admin role" content: application/json: schema: "$ref": "#/components/schemas/ErrorResponse" + '500': + description: enqueue failure + content: + application/json: + schema: + "$ref": "#/components/schemas/ErrorResponse"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/openapi.yaml` around lines 2800 - 2857, The OpenAPI spec for DELETE "/api/v1/users/reset" is missing the 500 response that the controller can return when FamilyResetJob.perform_later fails; update the DELETE operation to add a '500' response entry that uses the existing ErrorResponse schema (`#/components/schemas/ErrorResponse`) so the contract matches runtime behavior and documents the enqueue-failure case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/api/openapi.yaml`:
- Around line 2800-2857: The OpenAPI spec for DELETE "/api/v1/users/reset" is
missing the 500 response that the controller can return when
FamilyResetJob.perform_later fails; update the DELETE operation to add a '500'
response entry that uses the existing ErrorResponse schema
(`#/components/schemas/ErrorResponse`) so the contract matches runtime behavior
and documents the enqueue-failure case.
In `@test/controllers/api/v1/users_controller_test.rb`:
- Around line 71-83: Update the assertion that the FamilyResetJob was enqueued
to also verify the job was given the correct family argument: change the
assert_enqueued_with in the test "reset enqueues FamilyResetJob and returns 200"
to include args: [`@user.family`] (i.e. assert_enqueued_with(job: FamilyResetJob,
args: [`@user.family`]) do ... end) so the test fails if the controller enqueues
the wrong family when calling delete "/api/v1/users/reset".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 989b76cf-7f91-46b4-a600-58b5716fae4f
📥 Commits
Reviewing files that changed from the base of the PR and between e7df60921d41bcf3af1d25ad68d4c4972c58f7f4 and baf22e034f3bd1db7d4890d885f6256e1dbafe7c.
📒 Files selected for processing (6)
app/controllers/api/v1/users_controller.rbconfig/routes.rbdocs/api/openapi.yamlspec/requests/api/v1/users_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/users_controller_test.rb
There was a problem hiding this comment.
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 `@app/controllers/api/v1/users_controller.rb`:
- Around line 60-64: The ensure_admin method in UsersController uses a localized
I18n.t("users.reset.unauthorized") message which makes API v1 responses
locale-dependent; update ensure_admin so the render_json call returns a stable
hardcoded English message (e.g. message: "You are not authorized to perform this
action" or the project’s standard API v1 error text) instead of I18n.t(...),
keeping the same status :forbidden and preserving the error key and return false
behavior; locate this change in the ensure_admin method referencing
current_resource_owner and render_json.
🪄 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: 8dec3865-97df-4666-9b0a-69c3c2732822
📥 Commits
Reviewing files that changed from the base of the PR and between baf22e034f3bd1db7d4890d885f6256e1dbafe7c and d9b5582662062bb12b725c6ed59e172af5a34ef9.
📒 Files selected for processing (2)
app/controllers/api/v1/users_controller.rbtest/controllers/api/v1/users_controller_test.rb
There was a problem hiding this comment.
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 `@app/controllers/api/v1/users_controller.rb`:
- Around line 19-25: The rescue is too broad in the UsersController#reset
action—limit error handling to the FamilyResetJob enqueue call so other
exceptions still propagate; wrap only the FamilyResetJob.perform_later (or
enqueue) invocation in a begin/rescue that catches the exception as e, log
Rails.logger.error with the error details and render the 500 JSON using the API
v1 format (error: "reset_enqueue_failed", message: "Error: #{e.message}",
status: :internal_server_error). Leave the rest of the reset action unrescued so
non-enqueue errors are not misclassified.
🪄 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: fc01d668-1f54-47ad-89e9-203e9c8e4c91
📥 Commits
Reviewing files that changed from the base of the PR and between d9b5582662062bb12b725c6ed59e172af5a34ef9 and 874389405d65523587ed5451a45bc1817718d7a8.
📒 Files selected for processing (1)
app/controllers/api/v1/users_controller.rb
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/controllers/api/v1/users_controller.rb (1)
15-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep API v1 500 error message format consistent.
On Line 17, the enqueue-failure payload diverges from the API v1 convention used elsewhere (
"Error: #{e.message}"), which can create inconsistent client/test expectations.Suggested patch
- render json: { + render_json({ error: "reset_enqueue_failed", - message: "Account reset could not be queued" - }, status: :internal_server_error + message: "Error: #{e.message}" + }, status: :internal_server_error)Based on learnings: In
app/controllers/api/v1/**/*.rb, 500 responses should followmessage: "Error: #{e.message}".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/v1/users_controller.rb` around lines 15 - 18, The 500 response in UsersController currently returns a static message ("Account reset could not be queued") which breaks the API v1 convention; update the render json in the error branch (the render json block that returns error: "reset_enqueue_failed") to follow the standard pattern and return message: "Error: #{e.message}" (use the exception variable used in the rescue/error handler), so the payload matches other app/controllers/api/v1/*.rb 500 responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/controllers/api/v1/users_controller.rb`:
- Around line 15-18: The 500 response in UsersController currently returns a
static message ("Account reset could not be queued") which breaks the API v1
convention; update the render json in the error branch (the render json block
that returns error: "reset_enqueue_failed") to follow the standard pattern and
return message: "Error: #{e.message}" (use the exception variable used in the
rescue/error handler), so the payload matches other app/controllers/api/v1/*.rb
500 responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b66e84c-f62c-4a45-89be-3c45331e760a
📥 Commits
Reviewing files that changed from the base of the PR and between 874389405d65523587ed5451a45bc1817718d7a8 and 667bdf3fe5573378d47ee66c09befdd4f423b6f5.
📒 Files selected for processing (1)
app/controllers/api/v1/users_controller.rb
jjmata
removed
the
contributor:flagged
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
JSONbored
force-pushed
the
feat/api-reset-status
branch
from
667bdf3 to
7349ae9
Compare
superagent-security
Bot
added
the
contributor:flagged
JSONbored
force-pushed
the
feat/api-reset-status
branch
2 times, most recently
from
c4dfab7 to
ab1922f
Compare
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Another one I missed yesterday:
PR #1598 Review: feat(api): expose reset status polling
Overview
This PR adds two things:
- Enriches
DELETE /api/v1/users/resetto returnjob_id,family_id, andstatus_urlinstead of just a plain message. - Adds
GET /api/v1/users/reset/status— a polling endpoint that returns counts of surviving family resources so API clients can determine when an async reset has materialized.
The motivation is sound: async reset with no observable completion state is fragile for automation. The implementation is reasonably clean but has a few issues worth addressing.
Issues
Bare rescue catches too broadly (users_controller.rb:19)
rescue => eThis catches all Exception subclasses, including SignalException, SystemExit, NoMemoryError, etc. It should be rescue StandardError => e. This was flagged by CodeRabbit and is still unresolved.
family_id declared as format: uuid in OpenAPI schemas
Both ResetInitiatedResponse and ResetStatusResponse declare family_id as type: string, format: uuid, but the Rails app uses integer primary keys by default. If Family uses integer IDs, the returned value is a number not a UUID string, and the schema is wrong. This needs verification against the actual model.
reset_status calls current_resource_owner.family twice and counts.values.sum twice
def reset_status
counts = reset_target_counts(current_resource_owner.family)
render json: {
status: counts.values.sum.zero? ? "complete" : "data_remaining",
family_id: current_resource_owner.family.id, # second call
reset_complete: counts.values.sum.zero?, # computed again
...
}
endcurrent_resource_owner.family hits the association twice; counts.values.sum is computed twice. Use local variables:
def reset_status
family = current_resource_owner.family
counts = reset_target_counts(family)
complete = counts.values.sum.zero?
render json: { status: complete ? "complete" : "data_remaining",
family_id: family.id, reset_complete: complete, counts: counts }
endjob_id is returned but the status endpoint is not job-scoped
The reset response includes job_id suggesting per-job status tracking, but GET /api/v1/users/reset/status returns family-wide resource counts, not job state. The job_id has no practical use with the current polling design. Either remove job_id from the response or document clearly that it is informational only.
Status heuristic is racy
Determining reset_complete via resource counts will return false if the user has any accounts/categories/etc. (which they likely will before a reset). After a reset completes, if the user immediately creates new data, the endpoint says "data_remaining" again. The description acknowledges this is counts-based but the field name reset_complete implies stronger semantics than what's actually implemented. Consider renaming it resources_cleared or improving the docstring.
Positives
- Correct access control split: read scope for status, write + admin for reset. This is exactly right.
Current.family→current_resource_owner.family: Fixing this for the API context is correct.- Error message does not leak
e.messageto the client: The deliberate decision to use"Account reset could not be queued"instead of exposinge.messageis the right security choice, regardless of what CodeRabbit suggested. api_headersfix (plain_keyvsdisplay_key): This is a real bug fix that likely made the existing tests pass incorrectly before.- Test for no leakage of internal error message (
assert_not_includes response.body, "queue down"): Good security-conscious test. - OpenAPI schemas are complete and consistent with the rswag helper.
source: "web"added to testApiKey.create!calls: Likely fixes a validation that was previously being silently skipped.
Summary
The feature logic is solid and the access control is correct. The three things worth fixing before merge are: (1) rescue StandardError, (2) verifying family_id type in the OpenAPI schema, and (3) the duplicate current_resource_owner.family call in reset_status. The racy reset_complete semantics are worth at minimum a documentation note, and the job_id field should either be removed or its limited utility called out explicitly.
jjmata
removed
the
contributor:flagged
|
Your last few PRs are all coming in as "draft" - you planning to flip them all to ready for review at some point in batch, @JSONbored? |
JSONbored
force-pushed
the
feat/api-reset-status
branch
from
ab1922f to
2fcfd8f
Compare
superagent-security
Bot
added
the
contributor:flagged
Yep, I split them intentionally so each PR is independently reviewable and mergeable. I realize 18 at once is a lot, so please treat them as a queue, not a demand to review everything at once. Suggested review order:
Happy to re-draft lower-priority PRs if the open queue feels too noisy. |
jjmata
removed
the
contributor:flagged
github-project-automation
Bot
moved this from Todo
to Done
in Latest release train (v0.7.1) - May 2026
2 participants
Summary
What changed
DELETE /api/v1/userreturnstatus,job_id,family_id, andstatus_url.GET /api/v1/users/reset/statusto expose reset-target counts and areset_completeboolean.Why
The destructive reset endpoint currently queues async work without giving API clients a reliable way to know when it is safe to import into a clean family. Polling status makes automation and recovery workflows less fragile without changing the reset job itself.
Validation
/usr/bin/ruby -c app/controllers/api/v1/users_controller.rb/usr/bin/ruby -c test/controllers/api/v1/users_controller_test.rb/usr/bin/ruby -c spec/requests/api/v1/users_spec.rbruby -e 'require "yaml"; YAML.load_file("docs/api/openapi.yaml")'git diff --checkBlocked locally:
bin/rails test ...andbin/rubocoprequire the repository Ruby/Bundler toolchain. This workstation currently exposes Ruby 2.6.10, while Sure requires Ruby 3.4.7 and Bundler 2.6.7.Notes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests