feat: implement device registration, admin-secured management, and rate limiting for push notifications - #6
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
WalkthroughThe change adds a ChangesDevice API
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RackAttack
participant DevicesController
participant Device
participant DevicesDatabase
Client->>RackAttack: POST /v1/devices
alt within five requests per minute
RackAttack->>DevicesController: allow request
DevicesController->>Device: find or initialize by fcm_token
Device->>DevicesDatabase: validate and save
DevicesDatabase-->>DevicesController: device record
DevicesController-->>Client: JSON response
else sixth request
RackAttack-->>Client: HTTP 429
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/application_controller.rb`:
- Around line 4-11: Update authenticate_admin! so every unauthorized branch
renders a JSON response containing the error message "Unauthorized" with status
:unauthorized instead of calling head :unauthorized. Preserve the existing token
presence, byte-length, and secure comparison checks, and leave successful
authentication returning normally.
In `@app/controllers/devices_controller.rb`:
- Around line 4-6: Update DevicesController#index to paginate the ordered Device
relation with will_paginate before calling pluck, using the requested page and a
per-page value defaulting to 100 and capped at 1000. Add the corresponding
constants and private device_per_page helper, and include current_page,
total_entries, and per_page in the response pagination metadata.
- Around line 4-6: Update the DevicesController#index action to call no_store
before rendering the FCM token response, ensuring GET /v1/devices is not cached
while preserving the existing query and JSON structure.
- Around line 8-17: Update the create action in DevicesController to rescue
ActiveRecord::RecordNotUnique from concurrent device saves, reload the existing
device by device_params[:fcm_token], and return its id/type payload with HTTP
200; preserve the current created, successful update, and validation-error
responses.
In `@app/models/device.rb`:
- Around line 2-3: Define a service-specific maximum token length in the Device
model and add a maximum-length validation alongside presence and uniqueness
without restricting token characters or whitespace. In
db/migrate/20260805000000_create_devices.rb, enforce the same limit on the
fcm_token database column. In test/models/device_test.rb, add coverage
confirming a token exceeding the limit is invalid.
In `@test/models/device_test.rb`:
- Around line 4-22: Extend the Device model tests with a case covering invalid
FCM tokens: reject tokens containing whitespace, control characters such as
newlines, and values exceeding the database limit. Add these inputs to
test/models/device_test.rb alongside the existing fcm_token uniqueness coverage
and assert each new Device is invalid.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f25ae498-ec0a-4485-8c01-9e7c23bb0a92
⛔ Files ignored due to path filters (1)
db/schema.rbis excluded by!db/schema.rb
📒 Files selected for processing (11)
.env.exampleREADME.mdapp/controllers/application_controller.rbapp/controllers/devices_controller.rbapp/models/device.rbconfig/initializers/rack_attack.rbconfig/routes.rbdb/migrate/20260805000000_create_devices.rbtest/controllers/devices_controller_test.rbtest/integration/rate_limiting_test.rbtest/models/device_test.rb
Euler-B
force-pushed
the
feature/device-notifications-api
branch
from
August 6, 2026 03:58
e3d839d to
38dc75f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@config/routes.rb`:
- Line 7: Add DELETE to the CORS configuration for the /v1/* resource while
leaving ALLOWED_ORIGIN unchanged, so the existing devices destroy route supports
browser preflight requests.
In `@db/migrate/20260805000000_create_devices.rb`:
- Around line 5-9: Add the requested check constraint in the migration alongside
the existing devices index, using the devices_platform_is_web name and
restricting platform to web. Keep the existing column definition and index
unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 27c0de70-131e-489f-adcb-f5cae3e3513a
⛔ Files ignored due to path filters (1)
db/schema.rbis excluded by!db/schema.rb
📒 Files selected for processing (11)
.env.exampleREADME.mdapp/controllers/application_controller.rbapp/controllers/devices_controller.rbapp/models/device.rbconfig/initializers/rack_attack.rbconfig/routes.rbdb/migrate/20260805000000_create_devices.rbtest/controllers/devices_controller_test.rbtest/integration/rate_limiting_test.rbtest/models/device_test.rb
…te limiting for push notifications
Euler-B
force-pushed
the
feature/device-notifications-api
branch
from
August 6, 2026 04:08
38dc75f to
6c1c422
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation