Add SnapTrade connection management UI for freeing up connection slots #747
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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. 📝 WalkthroughWalkthroughAdds SnapTrade connection management: controller actions to list/delete connections and orphaned users, provider model helpers for SnapTrade API operations, a lazy-load Stimulus controller, a connections partial and settings panel update, member routes, and new locale keys. Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as Browser / Lazy Load
participant Controller as SnaptradeItemsController
participant Model as SnaptradeItem::Provided
participant SnapTradeAPI as SnapTrade API
participant DB as Local Database
participant View as Connections Partial
User->>Browser: open "Manage connections" details
Browser->>Browser: lazy controller detects open & calls load()
Browser->>Controller: GET /snaptrade_items/:id/connections
Controller->>Controller: build_connections_list()
par fetch remote connections
Controller->>Model: fetch_connections()
Model->>SnapTradeAPI: GET /connections
SnapTradeAPI-->>Model: connections or ApiError
and fetch local accounts
Controller->>DB: query `snaptrade_accounts` grouped by authorization_id
DB-->>Controller: local accounts
and fetch orphaned users
Controller->>Model: orphaned_users()
Model->>SnapTradeAPI: GET /users
SnapTradeAPI-->>Model: users or ApiError
end
Controller->>View: render _connections_list (connections, orphaned_users, error)
View-->>Browser: HTML partial
Browser->>Browser: insert HTML, hide loading indicator
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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: bf8a8790d5
ℹ️ 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".
| def orphaned_users | ||
| return [] unless credentials_configured? && user_registered? | ||
|
|
||
| all_users = list_all_users | ||
| all_users.reject { |uid| uid == snaptrade_user_id } |
There was a problem hiding this comment.
The orphaned user list is built by calling list_all_users and only filtering out the current snaptrade_user_id, which means any other user registered under the same SnapTrade client will be treated as “orphaned.” Because the schema does not enforce unique client_id/consumer_key per family, multiple families can share a client in the same deployment; in that case, this UI will surface other families’ user IDs and delete_orphaned_user will delete them, disconnecting those families. Consider scoping orphaned users to IDs known to belong to the current family (e.g., stored history) or disallowing deletes unless you can guarantee per-family credentials.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intended, due to SnapTrade not providing users any other way to manage connections from their dashboard. It an orphaned family ever exists the user will have no way to clean. This can be reconsidered in the future if conditions change.
![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
🤖 Fix all issues with AI agents
In `@app/controllers/snaptrade_items_controller.rb`:
- Around line 294-297: The Turbo Stream branch currently calls redirect_to
inside respond_to (format.turbo_stream { redirect_to settings_providers_path,
alert: t(".failed") }), which Turbo ignores; instead render a turbo_stream
response that updates the page with the error flash. Replace the
format.turbo_stream block to render a turbo_stream (e.g. using the app's
flash_notification_stream_items or render turbo_stream.update to target an error
container) and pass the translated message t(".failed") as the flash/error
content so the UI updates without relying on a redirect.
- Around line 259-268: The code calls provider.delete_connection with
creds[:user_id] and creds[:user_secret] but
`@snaptrade_item.snaptrade_credentials` can be nil, causing NoMethodError; update
the block that runs when accounts_deleted == 0 to first fetch creds =
`@snaptrade_item.snaptrade_credentials` and guard it (e.g., return/skip or log and
do not call delete_connection) or use safe access to fallback to
`@snaptrade_item.snaptrade_user_id` / snaptrade_user_secret; ensure you only call
provider.delete_connection when both user_id and user_secret are present (check
creds.present? and creds[:user_id].present? && creds[:user_secret].present? or
use safe navigation) and otherwise avoid invoking provider.delete_connection to
prevent a nil dereference.
In `@app/javascript/controllers/lazy_load_controller.js`:
- Around line 9-21: Replace the manual event listener pattern in
connect()/disconnect() by using a declarative Stimulus action and rename the
handler: remove this.boundHandleToggle and the
addEventListener/removeEventListener calls in connect() and disconnect(); rename
the method handleToggle() to toggled() and ensure the template uses
data-action="toggle-><controllerName>#toggled" so Stimulus invokes toggled()
when the element fires "toggle"; keep the immediate-load logic that checks
this.element.open and this.loadedValue and call this.load() as before.
🧹 Nitpick comments (3)
app/javascript/controllers/lazy_load_controller.js (1)
29-60: Guard against duplicate fetches while a load is in flight.Without an in-flight flag, rapid toggling can fire overlapping requests.
♻️ Proposed change
async load() { - if (this.loadedValue) return; + if (this.loadedValue || this.loading) return; + this.loading = true; try { const csrfToken = document.querySelector('meta[name="csrf-token"]')?.content; const response = await fetch(this.urlValue, { headers: { Accept: "text/html", "X-Requested-With": "XMLHttpRequest", "X-CSRF-Token": csrfToken, }, credentials: "same-origin", }); @@ - this.loadedValue = true; + this.loadedValue = true; } else { console.error("Lazy load failed:", response.status, response.statusText); this.showError(`Failed to load (${response.status})`); } } catch (error) { console.error("Lazy load error:", error); this.showError("Network error"); + } finally { + this.loading = false; } }config/locales/views/snaptrade_items/en.yml (1)
42-47: Consider placing delete_ messages under their own action scopes.*These keys are under
snaptrade_items.link_existing_account, but they belong to distinct actions. Moving them tosnaptrade_items.delete_connection.*andsnaptrade_items.delete_orphaned_user.*keeps the hierarchy consistent and easier to navigate.As per coding guidelines, ...
app/views/snaptrade_items/_connections_list.html.erb (1)
12-110: This partial is complex enough to merit a ViewComponent.Multiple branches, menus, and confirmation flows make it a good candidate for a component with a Ruby class to keep view logic thin.
As per coding guidelines, ...
…loading state management; improve SnapTrade deletion logic with additional safeguards and logging.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/controllers/snaptrade_items_controller.rb`:
- Around line 409-412: Replace the hardcoded user-facing string "Unknown
Brokerage" with a localized I18n lookup where the connections hash is built (the
block that pushes into result[:connections], referencing variables result,
auth_id, and api_conn). Use I18n.t with a clear key (e.g.
"snaptrade_items.unknown_brokerage") instead of the literal and ensure you add
that key and translation to the locale YAML files so the fallback brokerage name
is translatable.
- Around line 250-273: The delete_connection action currently uses
params[:authorization_id] without validating it, which can lead to deleting
nil-scoped records or calling the provider API with an invalid ID; add a guard
at the start of delete_connection to validate presence (and non-empty) of
authorization_id, return early with an appropriate Rails flash.alert or render
status if missing, and avoid executing
`@snaptrade_item.snaptrade_accounts.where`(...).destroy_all or
provider.delete_connection when authorization_id is blank to prevent unintended
deletions or invalid API calls.
In `@app/views/settings/providers/_snaptrade_panel.html.erb`:
- Around line 74-98: Replace the custom Stimulus lazy-load pattern inside the
details block (data-controller="lazy-load", data-lazy-load-url-value and targets
like data-lazy-load-target="content"/"loading"/"frame") with a Turbo Frame: add
a turbo_frame_tag whose id matches the frame returned by the connections view
and set its src to connections_snaptrade_item_path(item) and loading="lazy" so
Turbo defers fetching until the details opens; then update the server
partial/view that renders the fetched content to return a matching
turbo_frame_tag with the same id (use turbo_frame_tag helper) but no src so the
frame content is rendered into the frame.
…okerage message using i18n.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/controllers/snaptrade_items_controller.rb`:
- Around line 253-255: Replace the hard-coded "Missing authorization ID" with an
i18n key: add a translation like missing_authorization_id under the appropriate
scope in your locale YAML (e.g., under controllers.snaptrade_items or the action
scope), then update the redirect to use the translator (e.g., alert:
t(".missing_authorization_id") or alert:
t("snaptrade_items.missing_authorization_id")). Keep the key name consistent
with existing keys and use interpolation if you later need to pass dynamic data;
change the conditional in SnaptradeItemsController where authorization_id.blank?
is handled to reference that i18n key.
♻️ Duplicate comments (1)
app/controllers/snaptrade_items_controller.rb (1)
264-277: Avoid returning success when API deletion is skipped due to missing credentials.
If local accounts are absent and credentials are missing, the connection is not deleted but a success response is still sent. Treat this as a failure and return an alert instead.🔧 Suggested fix
- if accounts_deleted == 0 + if accounts_deleted == 0 provider = `@snaptrade_item.snaptrade_provider` creds = `@snaptrade_item.snaptrade_credentials` - if provider && creds&.dig(:user_id) && creds&.dig(:user_secret) - provider.delete_connection( - user_id: creds[:user_id], - user_secret: creds[:user_secret], - authorization_id: authorization_id - ) - else - Rails.logger.warn "SnapTrade: Cannot delete orphaned connection #{authorization_id} - missing credentials" - end + unless provider && creds&.dig(:user_id) && creds&.dig(:user_secret) + Rails.logger.warn "SnapTrade: Cannot delete orphaned connection #{authorization_id} - missing credentials" + redirect_to settings_providers_path, alert: t(".failed", message: t(".missing_credentials")) + return + end + + provider.delete_connection( + user_id: creds[:user_id], + user_secret: creds[:user_secret], + authorization_id: authorization_id + ) end
…n ID messages using i18n.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/controllers/snaptrade_items_controller.rb`:
- Around line 275-282: The controller currently logs a warning when credentials
are missing for authorization_id but always returns a success response; update
the action in SnaptradeItemsController to track the deletion result (e.g., a
boolean like deletion_succeeded) when attempting to call the SnapTrade API and,
if credentials are missing or deletion fails, respond with an error path — for
HTML render/redirect with an error flash (instead of notice) and for
turbo_stream render a failure replacement or no-remove (instead of
turbo_stream.remove("connection_#{authorization_id}")); ensure the respond_to
block uses this deletion_succeeded flag to choose the success vs failure
responses.
- Around line 284-286: The rescue block for Provider::Snaptrade::ApiError in
SnaptradeItemsController currently always calls redirect_to (the line with
redirect_to settings_providers_path, alert: t(".failed", message: e.message)),
which breaks Turbo Stream requests; update the rescue to respond to both formats
by using respond_to/format handling: for HTML keep redirect_to with the alert,
and for turbo_stream return a turbo_stream response (e.g., render a turbo_stream
that updates the relevant frame/flash partial or uses flash.now) so Turbo
clients receive a stream instead of a redirect; locate the rescue handling
Provider::Snaptrade::ApiError => e and modify it accordingly.
🧹 Nitpick comments (2)
config/locales/views/snaptrade_items/en.yml (1)
49-51: Consider adding a%{message}placeholder todelete_orphaned_user.failedfor consistency.The
delete_connection.failedkey includes a%{message}placeholder to surface error details, butdelete_orphaned_user.faileddoes not. Adding this would help users understand why deletion failed.Suggested change
delete_orphaned_user: success: "Orphaned registration deleted successfully." - failed: "Failed to delete orphaned registration." + failed: "Failed to delete orphaned registration: %{message}"app/controllers/snaptrade_items_controller.rb (1)
303-306: Failure path uses redirect for Turbo Stream requests.Similar to
delete_connection, usingredirect_tofor Turbo Stream requests causes unexpected behavior (full page reload). Consider handling both formats explicitly.Proposed fix
else - # Redirect for both formats - Turbo will follow the redirect - redirect_to settings_providers_path, alert: t(".failed") + respond_to do |format| + format.html { redirect_to settings_providers_path, alert: t(".failed") } + format.turbo_stream do + flash.now[:alert] = t(".failed") + render turbo_stream: flash_notification_stream_items + end + end end
…ing, i18n support for API deletion failures, and consistent Turbo Stream responses.
2 participants
Summary
Why
SnapTrade's free tier has a 5-connection limit, and connections can become orphaned when users cancel mid-linking or app state drifts. SnapTrade has no dashboard UI for managing connections - the only way to delete them is via API. This gives users a way to reclaim slots without needing API access.
Summary by CodeRabbit
New Features
UI
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.