Conversation
Branch naming uses hyphens (feature/lite-*) instead of slashes (feature/lite/*) because git can't create refs under an existing branch name. Mark all plan-enum tasks as complete.
…miting - Add `require_write_api!` guard to PointsController for create, update, destroy, and bulk_destroy actions. Lite users receive 403 with clear JSON error and upgrade URL. - Add `scoped_points` helper to ApiController that filters Lite users to a 12-month timestamp window. Pro and self-hoster users see all data. - Add rack-attack gem with Redis backend for per-plan rate limiting: Lite 200 req/hr, Pro 1,000 req/hr, self-hosters exempt. - Add X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset headers on all API responses for rate-limited plans. - Custom 429 response with Retry-After header and upgrade URL. - Full test coverage: 14 new specs (points write gating, read scoping, rate limit headers, throttle behavior).
Lite users see a lock badge + "Pro" label on gated layer toggles (heatmap, fog-of-war, scratch map, globe view). Clicking a gated toggle shows a 20-second live preview using the user's actual data, then auto-hides with an upgrade prompt. Globe view (which requires a page reload) shows the upgrade prompt immediately instead. - Add layer_gate.js utility with gatedToggle() and isGatedPlan() - Update routes_manager.js toggleHeatmap/toggleFog/toggleScratch to use gatedToggle — skips SettingsManager.updateSetting for Lite - Update settings_manager.js toggleGlobe to block for Lite users - Update syncToggleStates to force gated layers off on page load - Add lock icon badges to _settings_panel.html.erb for Lite users - Update lite.md with completed map-layers tasks
Backend: - Add `scoped_points` helper to ApiController for 12-month Lite window - Add `archived_points` helper returning only data older than 12 months - Support `archived=true` param on Points index — returns only archived points for Lite users, empty array for Pro/self-hoster (no archived concept) - PointsController index uses scoped_points by default, archived_points when param is set Frontend: - Add `fetchArchivedPoints` and `fetchAllArchivedPoints` to ApiClient - After main map data loads, Lite users get a background fetch of archived points rendered as a separate MapLibre layer at 30% opacity (gray circles with stroke) - Click on archived point shows upgrade prompt toast with link - Cursor changes to pointer on hover over archived data - Archived layer inserted below all active layers in z-order Tests: 4 new specs for archived param behavior (Lite returns archived, Lite excludes recent, Pro returns empty, self-hoster returns empty). 3592 examples, 0 failures.
Daily Sidekiq job checks all Lite users for data approaching archival: - 11 months: in-app warning notification - 11.5 months: email via UsersMailer#archival_approaching - 12 months: in-app banner about archived data Each threshold is tracked in user settings to prevent duplicate warnings. Scheduled daily at 03:00 via sidekiq-cron on the existing :archival queue.
1: Add lite plan
2: Feature/lite api gating
3: feat: gate map layers for Lite plan with timed preview
…miting - Add `require_write_api!` guard to PointsController for create, update, destroy, and bulk_destroy actions. Lite users receive 403 with clear JSON error and upgrade URL. - Add `scoped_points` helper to ApiController that filters Lite users to a 12-month timestamp window. Pro and self-hoster users see all data. - Add rack-attack gem with Redis backend for per-plan rate limiting: Lite 200 req/hr, Pro 1,000 req/hr, self-hosters exempt. - Add X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset headers on all API responses for rate-limited plans. - Custom 429 response with Retry-After header and upgrade URL. - Full test coverage: 14 new specs (points write gating, read scoping, rate limit headers, throttle behavior).
Lite users see a lock badge + "Pro" label on gated layer toggles (heatmap, fog-of-war, scratch map, globe view). Clicking a gated toggle shows a 20-second live preview using the user's actual data, then auto-hides with an upgrade prompt. Globe view (which requires a page reload) shows the upgrade prompt immediately instead. - Add layer_gate.js utility with gatedToggle() and isGatedPlan() - Update routes_manager.js toggleHeatmap/toggleFog/toggleScratch to use gatedToggle — skips SettingsManager.updateSetting for Lite - Update settings_manager.js toggleGlobe to block for Lite users - Update syncToggleStates to force gated layers off on page load - Add lock icon badges to _settings_panel.html.erb for Lite users - Update lite.md with completed map-layers tasks
# Conflicts: # app/controllers/api/v1/points_controller.rb # app/controllers/api_controller.rb # app/javascript/maps_maplibre/utils/layer_gate.js # config/initializers/rack_attack.rb # spec/requests/api/v1/points_spec.rb
4: feat: add data retention archival filter and archived data rendering
5: feat: add Lite archival warning jobs
Lite plan for Cloud
📝 WalkthroughWalkthroughThis PR introduces a comprehensive Lite plan system with tiered features and data windowing. It adds a two-tier plan enum (Lite/Pro), implements 12-month data retention for Lite users, gates premium features (heatmap, fog of war, scratch map, globe), introduces plan-based API rate limiting via rack-attack, adds archival warnings for aging data, and scatters plan-awareness throughout controllers, views, and frontend code. A new database migration establishes the plan column, scheduled jobs manage data archival notifications, and extensive test coverage validates plan gating across API and UI layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Lite as Lite User<br/>(Cloud)
participant UI as Web UI /<br/>Frontend
participant Controller as Web/API<br/>Controller
participant Model as User Model<br/>(plan_scopable)
participant DB as Database<br/>(filtered)
Note over Lite,DB: Plan-Based Data Scoping Flow
Lite->>UI: Request stats/points
UI->>Controller: GET /stats or /api/v1/points
Controller->>Model: current_user.scoped_points<br/>current_user.scoped_stats
Model->>Model: Check plan_restricted?<br/>(lite + cloud = true)
Model->>Model: Calculate data_window_start<br/>(12 months ago)
Model->>DB: SELECT * WHERE timestamp >= cutoff
DB-->>Model: Filtered records (12mo window)
Model-->>Controller: Scoped relation
Controller->>Controller: Build response / render view
Controller-->>UI: Response + alert/lock UI
UI-->>Lite: Render with<br/>upgrade banner/<br/>locked cards
sequenceDiagram
participant User as Lite User
participant Map as MapLibre Map
participant Manager as SettingsManager<br/>(Gating)
participant Toggle as Layer Toggle
participant Gate as LayerGate<br/>gatedToggle()
participant Banner as UpgradeBanner
Note over User,Banner: Layer Gating & Preview Flow
User->>Map: Load map with Lite plan
Map->>Manager: syncToggleStates()
Manager->>Manager: Check isGatedPlan(plan)
Manager->>Toggle: Force gated toggles OFF
Toggle-->>Manager: Confirmed
User->>Toggle: Click Heatmap toggle (ON)
Toggle->>Gate: gatedToggle({heatmap...})
Gate->>Gate: Validate isGatedPlan = true
Gate->>Toggle: Show preview countdown toast
Gate->>Map: showFn() → display heatmap
Note over Gate: Wait PREVIEW_SECONDS
Gate->>Map: hideFn() → hide heatmap
Gate->>Toggle: Force toggle OFF
Gate->>Banner: UpgradeBanner.show({message, upgradeUrl})
Banner-->>User: Display upgrade prompt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
app/controllers/map/leaflet_controller.rb (1)
43-72:⚠️ Potential issue | 🟠 MajorPlan scoping bypassed in distance calculation.
The
calculate_distancemethod queries thepointstable directly usinguser_id, bypassing the plan-scoped filtering applied inpoints_from_user(line 113). For Lite users, this will calculate distances including points outside their 12-month data window, creating inconsistency between the displayed points and the reported distance.Consider either:
- Filtering the SQL query to respect the plan window, or
- Using
@points(already scoped) for the calculationPotential approach using already-scoped points
def calculate_distance - return 0 if `@points.count`(:id) < 2 + return 0 if `@points.size` < 2 - # Use PostGIS window function for efficient distance calculation - # This is O(1) database operation vs O(n) Ruby iteration - import_filter = params[:import_id].present? ? 'AND import_id = :import_id' : '' - - sql = <<~SQL.squish - SELECT COALESCE(SUM(distance_m) / 1000.0, 0) as total_km FROM ( - SELECT ST_Distance( - lonlat::geography, - LAG(lonlat::geography) OVER (ORDER BY timestamp) - ) as distance_m - FROM points - WHERE user_id = :user_id - AND timestamp >= :start_at - AND timestamp <= :end_at - #{import_filter} - ) distances - SQL - - query_params = { user_id: current_user.id, start_at: start_at, end_at: end_at } - query_params[:import_id] = params[:import_id] if params[:import_id].present? - - result = Point.connection.select_value( - ActiveRecord::Base.sanitize_sql_array([sql, query_params]) - ) - - result&.to_f&.round || 0 + # Calculate distance using already-scoped and filtered points + point_ids = `@points.pluck`(:id) + return 0 if point_ids.size < 2 + + sql = <<~SQL.squish + SELECT COALESCE(SUM(distance_m) / 1000.0, 0) as total_km FROM ( + SELECT ST_Distance( + lonlat::geography, + LAG(lonlat::geography) OVER (ORDER BY timestamp) + ) as distance_m + FROM points + WHERE id = ANY(:point_ids) + ) distances + SQL + + result = Point.connection.select_value( + ActiveRecord::Base.sanitize_sql_array([sql, { point_ids: point_ids }]) + ) + + result&.to_f&.round || 0 end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/map/leaflet_controller.rb` around lines 43 - 72, The calculate_distance method bypasses plan-scoped filtering by querying points directly (using user_id) instead of the already-scoped `@points` created by points_from_user, causing Lite users to see distances outside their allowed window; fix by reusing the scoped relation (`@points` or points_from_user) for the distance calculation—either (A) build the same ST_Distance SQL as a fragment and execute it against `@points` (e.g., via `@points.select/connection` with sanitized SQL and the same window/order) so the import_id and plan window are respected, or (B) compute the distance in Ruby from `@points` enum if performance allows; ensure the final query still uses LAG(... ORDER BY timestamp) and returns total_km as before, and remove the direct points WHERE user_id = :user_id clause.app/javascript/controllers/maps/maplibre/data_loader.js (1)
146-163:⚠️ Potential issue | 🟠 MajorDon't zero out
totalPointsInRangewhen point layers are disabled.The new fallback returns
{ points: [], totalPointsInRange: 0 }, and that value is copied intodata.totalPointsInRange. If a Lite user hides points/routes/heatmap/fog/scratch, the map will now look like the selected range has no truncated data at all, so the upgrade/banner logic never gets a chance to fire. This count needs to be fetched independently of layer visibility.Also applies to: 228-235, 289-289
app/views/visits/index.html.erb (1)
45-50:⚠️ Potential issue | 🟠 MajorRender the plan-window alert for non-empty visit lists too.
At Line 49, the warning only appears when
@visits.empty?, so Lite users with actual visits never see the archival notice on this page. Move it above the conditional so both states get the same warning.Proposed fix
- <% if `@visits.empty`? %> + <%= render 'shared/plan_data_window_alert', utm_content: 'visits' %> + + <% if `@visits.empty`? %> <div class="hero min-h-80 bg-base-200"> <div class="hero-content text-center"> <div class="max-w-md"> - <%= render 'shared/plan_data_window_alert', utm_content: 'visits' %> <h1 class="text-5xl font-bold">Hello there!</h1> <p class="py-6"> Here you'll find your <%= params[:status] %> visits, but now there are none. Create some areas on your map and pretty soon you'll see visit suggestions on this page!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/visits/index.html.erb` around lines 45 - 50, The plan-data alert partial is currently rendered inside the `@visits.empty`? branch so Lite users with visits never see it; move the render 'shared/plan_data_window_alert', utm_content: 'visits' out of the conditional and place it above the if `@visits.empty`? block so it is always rendered regardless of whether `@visits` is empty or not, keeping the rest of the conditional (the hero/content headings and visit list) unchanged.app/controllers/api/v1/subscriptions_controller.rb (1)
6-32:⚠️ Potential issue | 🟡 MinorConsider rescuing
ActiveRecord::RecordNotFoundfor user lookup.If the JWT token contains an invalid
user_id,User.findwill raiseActiveRecord::RecordNotFoundwhich isn't currently rescued, potentially returning a 500 error instead of a more appropriate response.🛡️ Proposed fix
def callback decoded_token = Subscription::DecodeJwtToken.new(params[:token]).call user = User.find(decoded_token[:user_id]) # ... rest of method rescue JWT::DecodeError => e ExceptionReporter.call(e) render json: { message: 'Failed to verify subscription update.' }, status: :unauthorized + rescue ActiveRecord::RecordNotFound => e + ExceptionReporter.call(e) + render json: { message: 'User not found.' }, status: :not_found rescue ArgumentError => e ExceptionReporter.call(e) render json: { message: 'Invalid subscription data received.' }, status: :unprocessable_content end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/v1/subscriptions_controller.rb` around lines 6 - 32, The callback action uses User.find and can raise ActiveRecord::RecordNotFound for an invalid decoded_token[:user_id]; add a rescue ActiveRecord::RecordNotFound branch (similar to the existing JWT::DecodeError/ArgumentError handlers) that calls ExceptionReporter.call(e) and renders a clear JSON error (e.g., "User not found") with an appropriate status like :not_found so the controller returns a 404 instead of a 500 when the user lookup fails.app/controllers/insights_controller.rb (1)
119-119:⚠️ Potential issue | 🟡 Minor
digests.yearlyis not scoped - same issue as API controller.This mirrors the concern in
Api::V1::InsightsController. The digest data may include aggregations from outside the plan's 12-month window. Consider applying consistent plan scoping to digests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/insights_controller.rb` at line 119, The call current_user.digests.yearly.find_by(year: `@selected_year`) is not scoped to the current plan and can return digests outside the plan's 12-month window; update this lookup to apply the same plan scoping used in the API controller (e.g. use the current plan's digests scope or filter current_user.digests by current_plan/id) so the digest lookup for `@selected_year` only searches digests belonging to the current plan.app/controllers/api/v1/insights_controller.rb (1)
63-64:⚠️ Potential issue | 🟠 Major
digestsis not scoped like other data sources; inconsistency affects both yearly and monthly digests.While
scoped_statsandscoped_visitsare used throughout to enforce plan-based data windowing,digests.yearlyon line 63 anddigests.monthlyon line 76 remain unscoped. For Lite users, this allows access to aggregated data from outside the 12-month plan window, leaking historical travel patterns and statistics.Consider either:
- Adding a
scoped_digestsmethod toPlanScopablethat filters digests by the plan window, or- Filtering the digest's travel patterns in both
load_travel_patternsandcalculate_yearly_day_of_weekto exclude data outside the plan window🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/v1/insights_controller.rb` around lines 63 - 64, The digests lookup (current_api_user.digests.yearly / monthly) is not being filtered by the user's plan window like scoped_stats/scoped_visits; either add a scoped_digests method to PlanScopable that mirrors scoped_stats/scoped_visits and replace current_api_user.digests.yearly/monthly with current_api_user.scoped_digests.yearly/monthly, or (if you prefer a smaller change) filter the digest contents after loading by trimming travel_patterns and any aggregated fields in load_travel_patterns and calculate_yearly_day_of_week to only include records within the plan window (use the same plan window logic used by scoped_stats/scoped_visits to determine start/end dates). Ensure you update references to yearly_digest and monthly_digest where they are used so no unscoped data remains.
🟡 Minor comments (11)
app/controllers/concerns/safe_timestamp_parser.rb-11-12 (1)
11-12:⚠️ Potential issue | 🟡 MinorClamp Unix timestamps against UTC bounds.
Lines 11-12 make the allowed range depend on
Time.zone, so the numeric path can reject0in west-of-UTC zones or allow negative epochs in east-of-UTC zones. Since Line 15 treats these values as Unix timestamps, the bounds should be fixed UTC epoch values instead.Suggested fix
- min_timestamp = Time.zone.parse('1970-01-01').to_i - max_timestamp = Time.zone.parse('2100-01-01').to_i + min_timestamp = Time.utc(1970, 1, 1).to_i + max_timestamp = Time.utc(2100, 1, 1).to_i🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/concerns/safe_timestamp_parser.rb` around lines 11 - 12, The min_timestamp and max_timestamp are currently computed with Time.zone.parse which makes numeric parsing vary by local zone; change them to fixed UTC epoch bounds (e.g. compute min_timestamp and max_timestamp using Time.utc (or Time.new(...).utc) for '1970-01-01' and '2100-01-01') so the numeric path in SafeTimestampParser always clamps against absolute Unix UTC values; update the assignments to min_timestamp and max_timestamp accordingly and keep the rest of the timestamp parsing logic unchanged.app/models/points/raw_data_archive.rb-63-64 (1)
63-64:⚠️ Potential issue | 🟡 MinorReject malformed
format_versionvalues instead of coercing them to legacy archives.The
metadata['format_version'].to_ion line 64 silently treats non-numeric values like'abc'as0, bypassing the count-field validation. Similarly,false.blank?also returns true, allowing invalid metadata to skip validation. The current tests cover format_version 1 and 2, but don't test these edge cases. If format_version is meant to be strictly a version number, parse it withInteger()and add an error for invalid values.Suggested fix
- # Count fields were introduced in format_version 2; don't enforce on older archives - return if metadata['format_version'].blank? || metadata['format_version'].to_i < 2 + # Count fields were introduced in format_version 2; don't enforce on older archives + format_version = metadata['format_version'] + return if format_version.blank? + + version = Integer(format_version) + return if version < 2 + + rescue ArgumentError, TypeError + errors.add(:metadata, 'format_version must be a valid integer') + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/points/raw_data_archive.rb` around lines 63 - 64, The code currently uses metadata['format_version'].to_i which coerces non-numeric or falsey values to 0 and skips count-field validation; instead parse and validate the version strictly: check metadata['format_version'] for presence, attempt Integer(metadata['format_version']) (rescue ArgumentError/TypeError) and add a model validation error (e.g., errors.add(:metadata, "...invalid format_version...")) for missing or malformed values; only when Integer(...) succeeds use the numeric value to decide whether to return early for versions < 2 and proceed with count-field validation otherwise.app/controllers/application_controller.rb-76-78 (1)
76-78:⚠️ Potential issue | 🟡 MinorDon’t redirect Turbo Stream requests from
require_pro!.Protected update actions now using this helper fall back to a redirect instead of a Turbo Stream response, so they bypass the inline flash/update flow and degrade the Turbo UX. Handle the
turbo_streambranch with a streamed flash response instead.As per coding guidelines,
app/controllers/**/*_controller.rb: Use Turbo Stream responses for CRUD actions (create, update, destroy) instead of redirects or JSON responses. Use theFlashStreamableconcern to send flash messages via Turbo Streams with thestream_flashmethod.Also applies to: 92-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/application_controller.rb` around lines 76 - 78, The turbo_stream branch in the require_pro! handler currently issues a redirect which breaks Turbo UX; replace the redirect_to call inside format.turbo_stream with a streamed flash response using the FlashStreamable concern: call stream_flash (or the controller's stream_flash method) to emit the alert and appropriate status, and ensure the format.turbo_stream branch renders nothing else so the client receives the Turbo Stream flash update; update the same pattern for the other affected block at lines ~92-96 to use stream_flash instead of redirect_to.spec/controllers/application_controller_spec.rb-32-41 (1)
32-41:⚠️ Potential issue | 🟡 MinorThis example never exercises the self-hosted bypass.
DawarichSettings.self_hosted?is never stubbed totrue, and the user is already Pro, so this spec currently passes through the same path as the previous context. Use a Lite user and enable self-hosted mode here.Suggested test fix
context 'when on a self-hosted instance' do - let(:user) { create(:user) } # default plan is pro + let(:user) { create(:user, plan: :lite) } - before { sign_in user } + before do + allow(DawarichSettings).to receive(:self_hosted?).and_return(true) + sign_in user + end it 'allows access regardless of plan' do get :index expect(response).to have_http_status(:ok) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/controllers/application_controller_spec.rb` around lines 32 - 41, The spec in context 'when on a self-hosted instance' never toggles the self-hosted flag and uses a Pro user, so it doesn't exercise the bypass; change the user to a Lite user (e.g. replace let(:user) { create(:user) } with a Lite variant like create(:user, :lite) or create(:user, plan: :lite)) and stub DawarichSettings.self_hosted? to true in the before block (use allow(DawarichSettings).to receive(:self_hosted?).and_return(true)) so the test actually verifies the self-hosted bypass in ApplicationController.app/views/settings/integrations/index.html.erb-7-23 (1)
7-23:⚠️ Potential issue | 🟡 MinorMove
Connected Accountsoutside the Pro gate.The new top-level
if@pro_required`` branch hides the OAuth provider summary for Lite users, even though that section is unrelated to Immich/Photoprism access. This turns a photo-integration gate into a general settings-page regression.Also applies to: 116-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/settings/integrations/index.html.erb` around lines 7 - 23, The "Connected Accounts" / OAuth provider summary is mistakenly placed inside the `@pro_required` branch, hiding it for Lite users; move the markup that renders the "Connected Accounts" section (the OAuth provider summary and its heading/button block) out of the if `@pro_required` ... else ... end conditional so it renders for all users, leaving only the Immich/Photoprism-specific card inside the pro-only branch; update the surrounding ERB so `@pro_required` only wraps the Immich/Photoprism card and ensure HTML/ERB tags (the opening <% if `@pro_required` %>, the else branch and the corresponding end) are adjusted accordingly to keep the Connected Accounts markup outside the conditional.spec/models/user_spec.rb-48-50 (1)
48-50:⚠️ Potential issue | 🟡 MinorThis example still creates the user before the self-hosted stub is applied.
Because the context uses
let!, the user is materialized before the laterbeforehook runs. So this assertion can pass or fail based on the suite default instead of the self-hosted branch you're trying to cover. Makeuserlazy here (or stub earlier) so creation happens afterDawarichSettings.self_hosted?is forced totrue.Suggested test setup fix
- let!(:user) { create(:user, :inactive) } - before do allow(DawarichSettings).to receive(:self_hosted?).and_return(true) end + + let(:user) { create(:user, :inactive) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/user_spec.rb` around lines 48 - 50, The test is creating user too early because the spec uses let! which materializes the user before the before hook that stubs DawarichSettings.self_hosted?; change the spec to use a lazy let (replace let! with let for the user) or move the DawarichSettings.self_hosted? stub into an earlier setup (e.g., a top-level before block or use allow(DawarichSettings).to receive(:self_hosted?).and_return(true) before user is referenced) so that User creation (the user factory used in the example) happens after self_hosted? is forced to true and the pro? assertion reliably exercises the self-hosted branch.spec/serializers/api/user_serializer_spec.rb-118-121 (1)
118-121:⚠️ Potential issue | 🟡 MinorAssert the serialized
planvalue, not just its type.At Lines 118-121,
be_a(String)still passes if the serializer returns the wrong enum name or a stale value. Compare it directly withtest_user.plan, like the other subscription fields.Proposed test fix
expect(subscription).to include(:status, :active_until, :plan) expect(subscription[:status]).to be_a(String) expect(subscription[:active_until]).to be_a(ActiveSupport::TimeWithZone) - expect(subscription[:plan]).to be_a(String) + expect(subscription[:plan]).to eq(test_user.plan)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/serializers/api/user_serializer_spec.rb` around lines 118 - 121, The test currently only asserts the serialized subscription[:plan] is a String which can hide wrong enum values; replace that loose type assertion by asserting the actual value matches the object's plan (i.e. change the expectation on subscription[:plan] to compare to test_user.plan so the serializer returns the correct enum name), keeping the other expectations for :status and :active_until unchanged.e2e/setup/auth-lite.setup.js-12-15 (1)
12-15:⚠️ Potential issue | 🟡 MinorSwitch these sign-in locators to
data-testids.Lines 12-15 rely on field names and submit text, which makes this setup brittle to markup or copy changes. Please target stable test ids instead.
Proposed locator update
- await page.fill('input[name="user[email]"]', "lite@dawarich.app") - await page.fill('input[name="user[password]"]', "password") + await page.getByTestId("sign-in-email").fill("lite@dawarich.app") + await page.getByTestId("sign-in-password").fill("password") - await page.click('input[type="submit"][value="Log in"]') + await page.getByTestId("sign-in-submit").click()As per coding guidelines,
e2e/**/*.{js,ts}: Use data-testid selectors in browser test files for DOM queries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/setup/auth-lite.setup.js` around lines 12 - 15, Replace the brittle name/text-based selectors used in the sign-in flow by switching the page.fill and page.click calls to target stable data-testid attributes; update the two page.fill calls that currently use 'input[name="user[email]"]' and 'input[name="user[password]"]' to use their corresponding data-testid values (e.g., the email and password input testids), and change the page.click that targets 'input[type="submit"][value="Log in"]' to click the submit element using its data-testid. Ensure the testids you use match the app's markup (add them if missing) and keep using the existing page.fill and page.click calls (only change the selector strings).spec/requests/users/digests_spec.rb-90-91 (1)
90-91:⚠️ Potential issue | 🟡 MinorPin cloud mode in the Pro-plan examples.
These examples currently inherit whatever
DawarichSettings.self_hosted?defaults to in test, so they can still pass via the self-hosted bypass instead of the Pro-only branch. Stubbing it tofalsehere keeps the spec targeted.💡 Suggested change
context 'when user is on Pro plan' do - before { user.update_column(:plan, User.plans[:pro]) } + before do + allow(DawarichSettings).to receive(:self_hosted?).and_return(false) + user.update_column(:plan, User.plans[:pro]) + end it 'shows full digest with monthly chart and detailed stats' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/users/digests_spec.rb` around lines 90 - 91, The Pro-plan examples can pass via the self-hosted bypass because DawarichSettings.self_hosted? is not pinned; in the context 'when user is on Pro plan' (where user.update_column(:plan, User.plans[:pro]) is used) stub DawarichSettings.self_hosted? to return false for the duration of the examples so the test exercises the Pro-only branch (use a before block to allow(DawarichSettings).to receive(:self_hosted?).and_return(false) and ensure it is reset after if required).app/javascript/controllers/maps/maplibre/map_data_manager.js-376-383 (1)
376-383:⚠️ Potential issue | 🟡 MinorDismiss the data-window banner when it no longer applies.
This helper only shows the upsell. If a Lite user narrows the query so
totalInRange <= loadedCount, the previous banner will stay visible because there is no hide/reset path here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/maps/maplibre/map_data_manager.js` around lines 376 - 383, The _showDataWindowBanner helper only shows the banner but never hides it; update _showDataWindowBanner(totalInRange, loadedCount) to call UpgradeBanner.hide() (or the banner's dismiss method if named differently) when totalInRange <= loadedCount so the upsell is removed as soon as it no longer applies; keep the existing UpgradeBanner.show call (using this.controller.upgradeUrlValue and utmContent) for the > case and add the complementary hide call in the else branch to reset the banner state.app/views/users/digests/show.html.erb-126-163 (1)
126-163:⚠️ Potential issue | 🟡 MinorDon’t gate away the
No tracking datafallback.Line 126 only checks
@digest.top_countries_by_time.any?, so the placeholder on Lines 148-159 never renders for a fully untracked year — exactly when the explanation is most useful. Widen the outer condition to include positiveuntracked_days.Suggested fix
- <% if `@digest.top_countries_by_time.any`? %> + <% if `@digest.top_countries_by_time.any`? || `@digest.untracked_days.to_i.positive`? %> <div class="card bg-base-200 shadow-xl mb-8"> <div class="card-body text-center items-center"> @@ - <% if `@digest.untracked_days` > 0 %> + <% if `@digest.untracked_days.to_i.positive`? %> <div class="flex justify-between items-center p-3 bg-base-100 rounded-lg border-2 border-dashed border-gray-200">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/users/digests/show.html.erb` around lines 126 - 163, The outer conditional currently only renders the country card when `@digest.top_countries_by_time.any`?, which hides the "No tracking data" fallback when a year is fully untracked; change the top conditional that wraps the card to check either `@digest.top_countries_by_time.any`? OR `@digest.untracked_days.to_i` > 0 so the card always renders when there are untracked days, leaving the inner logic (the .take(5).each_with_index loop and the `@digest.untracked_days` conditional) unchanged.
🧹 Nitpick comments (16)
spec/models/points/raw_data_archive_spec.rb (1)
67-85: Add a one-missing-count example to protect the validator branch.These cases cover “both missing” and “both present”, but not the actual
expected_count.blank? || actual_count.blank?branch. A regression to&&would still pass this block, so it’s worth adding a case where exactly one count is missing.Suggested spec addition
it 'rejects format_version 2 archives missing count fields' do archive = build(:points_raw_data_archive, user: user, metadata: { 'format_version' => 2, 'compression' => 'gzip', 'encryption' => 'aes-256-gcm' }) expect(archive).not_to be_valid expect(archive.errors[:metadata]).to include('must contain expected_count and actual_count') end + + it 'rejects format_version 2 archives when only one count field is present' do + archive = build(:points_raw_data_archive, user: user, metadata: { + 'format_version' => 2, + 'compression' => 'gzip', + 'encryption' => 'aes-256-gcm', + 'expected_count' => 100 + }) + expect(archive).not_to be_valid + expect(archive.errors[:metadata]).to include('must contain expected_count and actual_count') + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/points/raw_data_archive_spec.rb` around lines 67 - 85, Add a spec that exercises the validator branch where exactly one count is missing: in spec/models/points/raw_data_archive_spec.rb add an example (e.g. it 'rejects format_version 2 archives with one missing count field') that builds a points_raw_data_archive via build(:points_raw_data_archive, user: user, metadata: { 'format_version' => 2, 'compression' => 'gzip', 'encryption' => 'aes-256-gcm', 'expected_count' => 100 }) (omit 'actual_count') and asserts archive is invalid and archive.errors[:metadata] includes 'must contain expected_count and actual_count'; you can also add the mirrored case (only 'actual_count' present) to fully protect the expected_count.blank? || actual_count.blank? branch.lib/tasks/demo.rake (1)
125-131: Keep Lite visits and areas sourced from recent points.These calls read from the full Lite dataset, not the recent slice. In particular,
create_areasorders bytimestamp, so with 20 recent + 10 old points andcount = 2, one of the two Lite areas will be built from the archived portion. If this fixture is meant to demo the 12-month Lite window, pass a recent-only scope or dedicated Lite helpers here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tasks/demo.rake` around lines 125 - 131, The Lite demo is using the full dataset when calling create_visits and create_areas, so areas can be built from archived points; update the calls that create Lite fixtures (create_visits and create_areas invoked for lite_user) to use the recent-only slice or the Lite-specific helper scope (e.g., pass a recent_points scope or use create_areas_for_lite / create_visits_for_lite) so both visits and areas are sourced only from the recent 12-month Lite window; ensure the create_areas call no longer relies on the global ordering by timestamp but instead receives the filtered recent dataset for lite_user.app/controllers/settings/integrations_controller.rb (1)
19-22: Consider Turbo Stream response for update action.The coding guidelines specify using Turbo Stream responses for CRUD actions instead of redirects. Consider using
FlashStreamableconcern withstream_flashfor the flash messages.Example using Turbo Stream
+ include FlashStreamable + def update result = Settings::Update.new( current_user, settings_params, refresh_photos_cache: params[:refresh_photos_cache].present? ).call - flash[:notice] = result[:notices].join('. ') if result[:notices].any? - flash[:alert] = result[:alerts].join('. ') if result[:alerts].any? - - redirect_to settings_integrations_path + respond_to do |format| + format.turbo_stream do + stream_flash(notice: result[:notices].join('. ')) if result[:notices].any? + stream_flash(alert: result[:alerts].join('. ')) if result[:alerts].any? + end + format.html { redirect_to settings_integrations_path } + end endAs per coding guidelines: "Use Turbo Stream responses for CRUD actions (create, update, destroy) instead of redirects or JSON responses" and "Use the
FlashStreamableconcern to send flash messages via Turbo Streams with thestream_flashmethod".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/settings/integrations_controller.rb` around lines 19 - 22, The update action currently sets flash[:notice]/[:alert] from result and then calls redirect_to settings_integrations_path; change this to emit a Turbo Stream response using the FlashStreamable concern: include FlashStreamable in IntegrationsController (if not already), set the flash entries as you do from result (flash[:notice] = ...; flash[:alert] = ...), then call stream_flash to stream those flash messages and render a turbo_stream response that updates the settings integrations UI (e.g., render a turbo_stream that replaces the integrations list or a specific partial) instead of redirecting; replace the existing redirect_to settings_integrations_path with a render turbo_stream flow that uses stream_flash and updates the relevant DOM.spec/controllers/application_controller_spec.rb (1)
18-74: Add JSON and Turbo Stream coverage forrequire_pro!.The helper has separate
html,json, andturbo_streambranches, but these specs only send HTML requests. A regression in either non-HTML response path would currently go unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/controllers/application_controller_spec.rb` around lines 18 - 74, Add specs that exercise the JSON and turbo_stream branches of require_pro! in addition to the existing HTML tests: for the pro plan and self-hosted contexts send requests with format: :json and format: :turbo_stream (or appropriate ACCEPT headers) and assert 200/ok responses and body as expected; for the lite plan and unsigned contexts send requests with format: :json and :turbo_stream and assert the correct redirect/status and alert payloads (JSON error body or turbo_stream redirect/flash behavior) so the require_pro! branches for json and turbo_stream are covered alongside the existing HTML examples.app/views/insights/details.html.erb (1)
10-58: Consider extracting the repeated gating pattern into a helper.The
if show_plan_data_window_alert? ... else cache ... endpattern is repeated 6 times. This could be refactored into a helper method for better maintainability.♻️ Example helper extraction
# app/helpers/insights_helper.rb def render_insight_section(title:, utm_content:, cache_key:, &block) if show_plan_data_window_alert? render 'pro_locked_card', title: title, utm_content: utm_content else cache(cache_key, expires_in: 24.hours, &block) end endThen in the view:
<%= render_insight_section( title: 'Year Comparison', utm_content: 'year_comparison', cache_key: insights_cache_key + ["year_comparison"]) do %> <%= render 'year_comparison' %> <% end %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/insights/details.html.erb` around lines 10 - 58, Extract the repeated gating pattern around show_plan_data_window_alert? into a helper (e.g., render_insight_section) that accepts title, utm_content, cache_key and a block; replace each repeated if/else cache ... render 'pro_locked_card' logic in the view with calls to this helper using insights_cache_key + ["..."] (and include `@selected_month` where appropriate) so the view only calls render_insight_section(title:, utm_content:, cache_key:) { render 'partial_name' } while the helper renders the pro_locked_card when show_plan_data_window_alert? is true or otherwise wraps the provided block in cache(..., expires_in: 24.hours).app/views/stats/_locked_year_card.html.erb (1)
12-13: Avoidrandin the view for placeholder chart data.This makes the locked card change on every render, which is noisy for users and undermines deterministic caching/tests. A fixed sample series is safer here.
♻️ Suggested change
<%= column_chart( - (1..12).map { |m| [Date::MONTHNAMES[m], rand(5_000..80_000)] }, + [ + ['January', 18_000], ['February', 26_000], ['March', 34_000], + ['April', 29_000], ['May', 41_000], ['June', 52_000], + ['July', 48_000], ['August', 57_000], ['September', 44_000], + ['October', 36_000], ['November', 31_000], ['December', 24_000] + ], height: '200px', suffix: " #{current_user.safe_settings.distance_unit}",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/stats/_locked_year_card.html.erb` around lines 12 - 13, The view uses a non-deterministic data series in the column_chart call by mapping (1..12).map { |m| [Date::MONTHNAMES[m], rand(5_000..80_000)] }, causing the chart to change on every render; replace that rand-based series with a fixed sample array of 12 month/value pairs (keep Date::MONTHNAMES for labels or a literal array of month names) so the partial (_locked_year_card.html.erb) renders deterministic, cacheable, and testable data instead of random values.e2e/lite/plan-gates.spec.js (2)
144-148: Consider using more stable selectors for settings gating test.The current selectors use
text=andinput[placeholder*="Immich"]which may be fragile if text changes. Per coding guidelines, preferdata-testidselectors for DOM queries in E2E tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/lite/plan-gates.spec.js` around lines 144 - 148, Replace fragile text and placeholder-based locators with stable data-testid selectors: swap page.locator("text=Upgrade to Pro") for a data-testid-based locator (e.g. page.locator('[data-testid="upgrade-to-pro"]')) and swap page.locator('input[placeholder*="Immich"]').first() for a data-testid like '[data-testid="immich-url-input"]'; update the app templates to add those data-testid attributes if they do not exist and adjust the assertions in the test accordingly (keep the same visibility checks using the new locators).
125-131: Minor: 12-month calculation approximation may drift from server logic.The calculation
12 * 30 * 24 * 60 * 60assumes 30-day months (360 days), while actual 12 months is ~365 days. If the server uses12.months.ago(Rails), this could cause edge-case test failures.Consider aligning with the server's approach or adding a small buffer:
- const twelveMonthsAgo = Date.now() / 1000 - 12 * 30 * 24 * 60 * 60 + // Use 365 days to match Rails' 12.months.ago more closely + const twelveMonthsAgo = Date.now() / 1000 - 365 * 24 * 60 * 60🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/lite/plan-gates.spec.js` around lines 125 - 131, The 12-month window calculation using 12 * 30 * 24 * 60 * 60 (variable twelveMonthsAgo) underestimates a true year and can drift from server-side 12.months.ago; update the twelveMonthsAgo computation to use a 365-day year (365 * 24 * 60 * 60) or add a small safety buffer (e.g., +7 days) so the expect(point.timestamp).toBeGreaterThanOrEqual(Math.floor(twelveMonthsAgo)) check aligns with server logic and avoids edge-case failures in the points loop.spec/requests/api/v1/points_spec.rb (1)
468-490: Self-hosted context may need explicit stub for consistency.The self-hosted context doesn't explicitly stub
DawarichSettings.self_hosted?. While it may default totruein the test environment, other contexts explicitly stub it. For consistency and to avoid test brittleness if defaults change, consider adding an explicit stub.Suggested addition
context 'when on a self-hosted instance' do let!(:self_hosted_user) { create(:user) } # default plan is pro + before do + allow(DawarichSettings).to receive(:self_hosted?).and_return(true) + end + let!(:recent_point) do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/points_spec.rb` around lines 468 - 490, The self-hosted context relies on a default but doesn't explicitly stub DawarichSettings.self_hosted? which can cause brittle tests; inside the "when on a self-hosted instance" context add an explicit stub (e.g., a before block that uses allow(DawarichSettings).to receive(:self_hosted?).and_return(true)) so the behavior is deterministic for the tests that create self_hosted_user, recent_point, and old_point.app/views/insights/index.html.erb (1)
24-37: Consider accessibility for locked content cards.The locked cards provide visual indication that features are Pro-only, but ensure the
pro_locked_cardpartial includes appropriate ARIA attributes or screen reader text to communicate the locked state to assistive technology users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/insights/index.html.erb` around lines 24 - 37, The pro_locked_card partial currently only provides visual cues; update the pro_locked_card partial to expose the locked state to assistive tech by adding a screen-reader-only text (e.g., "Pro feature — locked") and/or ARIA attributes such as aria-disabled="true" and aria-label or aria-describedby that conveys the lock, ensure any interactive element inside pro_locked_card uses tabindex="-1" when locked (or role="button" with aria-disabled) so it's not focusable/announced as actionable, and verify the turbo_frame_tag "insights_details" content still reads correctly to screen readers with these additions.config/initializers/rack_attack.rb (1)
32-35: Consider cache invalidation after plan upgrade.The 2-minute cache for plan lookup (
expires_in: 2.minutes) means users who upgrade from Lite to Pro won't see their new rate limits for up to 2 minutes. This is generally acceptable, but consider if immediate reflection is needed.If so, you could invalidate the cache key when a user's plan changes:
# In User model or plan upgrade service Rails.cache.delete("rack_attack/plan/#{api_key}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/initializers/rack_attack.rb` around lines 32 - 35, The current two-minute cache of User plan (created via Rails.cache.fetch in rack_attack.rb using user_plan and api_key) can delay new rate limits after a plan upgrade; fix this by invalidating that cache entry when a user's plan changes—add a cache delete call for the "rack_attack/plan/#{api_key}" key in the User model (e.g., an after_commit callback detecting plan change) or in the plan upgrade service so the next Rack::Attack lookup reads the updated plan immediately.spec/models/concerns/plan_scopable_spec.rb (1)
39-42: Consider usingfreeze_timefor time-sensitive assertions.The
be_within(1.second)matcher works but could be flaky if tests run slowly. Consider wrapping time-sensitive tests withfreeze_timeortravel_tofor deterministic behavior.♻️ Suggested improvement
describe '#data_window_start' do + around do |example| + freeze_time { example.run } + end + it 'returns approximately 12 months ago' do - expect(user.data_window_start).to be_within(1.second).of(12.months.ago) + expect(user.data_window_start).to eq(12.months.ago) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/concerns/plan_scopable_spec.rb` around lines 39 - 42, Wrap the time-sensitive example that asserts user.data_window_start in a deterministic time block (e.g., use freeze_time or travel_to from ActiveSupport::Testing::TimeHelpers) so it doesn't flake; locate the example in spec/models/concerns/plan_scopable_spec.rb that calls user.data_window_start and either wrap the expectation in freeze_time { ... } or travel_to a fixed time (e.g., travel_to 12.months.ago and assert equality) to make the assertion stable.app/javascript/maps_maplibre/utils/layer_gate.js (1)
70-75: Countdown timer updates every 5 seconds but decrements by 5.The countdown shows "20s → 15s → 10s → 5s" which works, but the first update happens after 5 seconds, so users initially see "20 seconds" for the first 5 seconds. Consider showing "15s remaining" immediately or adjusting the messaging.
💡 Alternative: start countdown immediately
let remaining = PREVIEW_SECONDS + const updateToast = () => { + if (remaining > 0 && toast?.parentNode) { + toast.textContent = `Previewing ${layerName} — ${remaining}s remaining.` + } + } const countdownInterval = setInterval(() => { remaining -= 5 - if (remaining > 0 && toast?.parentNode) { - toast.textContent = `Previewing ${layerName} — ${remaining}s remaining.` - } + updateToast() }, 5000)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/maps_maplibre/utils/layer_gate.js` around lines 70 - 75, The countdown currently waits 5s before the first decrement so users see the full starting value; update the UI immediately by adjusting the logic around countdownInterval: either decrement remaining once before starting the setInterval or set toast.textContent using remaining-5 right away (referencing remaining, countdownInterval, toast, and layerName), then keep the existing 5s interval for subsequent updates; ensure you also handle the case where remaining <= 0 and clearInterval(countdownInterval) as needed.app/controllers/api_controller.rb (1)
41-63: Consider extracting shared logic fromrequire_pro_api!andrequire_write_api!.These two methods are nearly identical, differing only in error codes and messages. Consider extracting a shared helper:
♻️ DRY refactor suggestion
def require_pro_api! require_pro_plan!('pro_plan_required', 'This feature requires a Pro plan.') end def require_write_api! require_pro_plan!('write_api_restricted', 'Write API access requires a Pro plan. Your data was not modified.') end private def require_pro_plan!(error_code, message) return unless current_api_user return if DawarichSettings.self_hosted? return if current_api_user.pro? render json: { error: error_code, message: message, upgrade_url: upgrade_url_for(current_api_user) }, status: :forbidden end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api_controller.rb` around lines 41 - 63, The two nearly identical methods require_pro_api! and require_write_api! should be DRYed by extracting the shared logic into a private helper (e.g., require_pro_plan!) that accepts the error code and message; update require_pro_api! and require_write_api! to call require_pro_plan! with their specific error strings, and move the existing checks (current_api_user guard, DawarichSettings.self_hosted? check, current_api_user.pro? check, and render json with upgrade_url_for(current_api_user) and status :forbidden) into that helper.app/javascript/maps_maplibre/components/upgrade_banner.js (1)
5-81: Consider refactoring to a plain module with exported functions.Static analysis flags this class as containing only static members. A plain module with exported functions would be more idiomatic JavaScript:
♻️ Alternative module-based approach
let activeBanner = null export function showUpgradeBanner({ message, upgradeUrl, utmContent }) { // ... implementation } export function dismissUpgradeBanner() { // ... implementation }That said, the current class-based approach provides a clean namespace and the singleton pattern is explicit. This is a minor stylistic preference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/maps_maplibre/components/upgrade_banner.js` around lines 5 - 81, The UpgradeBanner class only contains static members; refactor it into a plain module by replacing the class with a module-level variable activeBanner and two exported functions showUpgradeBanner and dismissUpgradeBanner that implement the same logic currently in UpgradeBanner.show and UpgradeBanner.dismiss; move static activeBanner -> let activeBanner = null, implement showUpgradeBanner({ message, upgradeUrl, utmContent }) to create and append the banner (same DOM logic, use encodeURIComponent for utmContent) and set activeBanner, and implement dismissUpgradeBanner() to remove activeBanner, clear it, and set sessionStorage, then update any callers to use the new exported function names instead of UpgradeBanner.show/UpgradeBanner.dismiss.app/controllers/insights_controller.rb (1)
30-31: Guard conditions may be redundant.If
year_locked?returns true, thencurrent_user.plan_restricted?must also be true (sinceyear_locked?checksplan_restricted?internally on line 52). The|| current_user.plan_restricted?condition appears to cover the case where the year itself isn't locked but the user is still restricted.However, the intent could be clearer. If you want to block all restricted users from the details page regardless of year, consider adding a comment or restructuring:
💡 Clarify intent with a comment
load_year_stats -return if `@year_locked` || current_user.plan_restricted? +# Block access if viewing a locked year OR if restricted user selects unlocked year +# (Lite users can view scoped years but not full details) +return if `@year_locked` || current_user.plan_restricted?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/insights_controller.rb` around lines 30 - 31, The guard `return if `@year_locked` || current_user.plan_restricted?` is redundant because the `year_locked?` check (see year_locked? method) already inspects `current_user.plan_restricted?`; either drop the explicit `current_user.plan_restricted?` from this guard and leave `return if `@year_locked`` (or `return if year_locked?` if you prefer the method call), or keep both but add a clarifying comment above the line explaining why both are needed; update the line referencing the `@year_locked`/`year_locked?` symbols accordingly to remove ambiguity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d55fa68c-e454-4d1a-a091-0ef5d77965dc
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockapp/assets/svg/icons/brands/github.svgis excluded by!**/*.svgapp/assets/svg/icons/brands/google.svgis excluded by!**/*.svgapp/assets/svg/icons/lucide/outline/lock.svgis excluded by!**/*.svg
📒 Files selected for processing (106)
.app_versionCHANGELOG.mdCLAUDE.mdGemfileapp/assets/builds/tailwind.cssapp/assets/stylesheets/maps_maplibre.cssapp/controllers/api/v1/insights_controller.rbapp/controllers/api/v1/plan_controller.rbapp/controllers/api/v1/points_controller.rbapp/controllers/api/v1/settings_controller.rbapp/controllers/api/v1/subscriptions_controller.rbapp/controllers/api/v1/tracks/points_controller.rbapp/controllers/api/v1/visits_controller.rbapp/controllers/api_controller.rbapp/controllers/application_controller.rbapp/controllers/concerns/safe_timestamp_parser.rbapp/controllers/imports_controller.rbapp/controllers/insights_controller.rbapp/controllers/map/leaflet_controller.rbapp/controllers/settings/integrations_controller.rbapp/controllers/shared/digests_controller.rbapp/controllers/shared/stats_controller.rbapp/controllers/stats_controller.rbapp/controllers/users/digests_controller.rbapp/controllers/visits_controller.rbapp/helpers/application_helper.rbapp/javascript/controllers/maps/maplibre/data_loader.jsapp/javascript/controllers/maps/maplibre/map_data_manager.jsapp/javascript/controllers/maps/maplibre/routes_manager.jsapp/javascript/controllers/maps/maplibre/settings_manager.jsapp/javascript/controllers/maps/maplibre_controller.jsapp/javascript/maps_maplibre/components/toast.jsapp/javascript/maps_maplibre/components/upgrade_banner.jsapp/javascript/maps_maplibre/services/api_client.jsapp/javascript/maps_maplibre/utils/layer_gate.jsapp/jobs/lite/archival_warning_job.rbapp/mailers/users_mailer.rbapp/models/concerns/plan_scopable.rbapp/models/points/raw_data_archive.rbapp/models/user.rbapp/queries/tracks/index_query.rbapp/serializers/api/user_serializer.rbapp/services/location_search/spatial_matcher.rbapp/services/timeline/day_assembler.rbapp/services/users/safe_settings.rbapp/services/users/transportation_thresholds_updater.rbapp/services/visits/find_in_time.rbapp/services/visits/find_within_bounding_box.rbapp/views/devise/sessions/new.html.erbapp/views/devise/shared/_links.html.erbapp/views/insights/_header.html.erbapp/views/insights/_pro_locked_card.html.erbapp/views/insights/details.html.erbapp/views/insights/index.html.erbapp/views/map/leaflet/index.html.erbapp/views/map/maplibre/_settings_panel.html.erbapp/views/map/maplibre/index.html.erbapp/views/map/timeline_feeds/_feed.html.erbapp/views/settings/integrations/index.html.erbapp/views/shared/_plan_data_window_alert.html.erbapp/views/shared/_sharing_modal.html.erbapp/views/shared/map/_upgrade_banner.html.erbapp/views/stats/_locked_year_card.html.erbapp/views/stats/_month.html.erbapp/views/stats/index.html.erbapp/views/stats/month.html.erbapp/views/stats/show.html.erbapp/views/users/digests/public_year.html.erbapp/views/users/digests/show.html.erbapp/views/users_mailer/archival_approaching.html.erbapp/views/users_mailer/archival_approaching.text.erbapp/views/visits/index.html.erbconfig/initializers/03_dawarich_settings.rbconfig/initializers/rack_attack.rbconfig/initializers/rails_icons.rbconfig/routes.rbconfig/schedule.ymldb/migrate/20260301201446_add_plan_to_users.rbdb/migrate/20260301202147_set_plan_for_existing_users.rbdb/schema.rbe2e/lite/plan-gates.spec.jse2e/setup/auth-lite.setup.jse2e/v2/helpers/constants.jslib/tasks/demo.rakeplaywright.config.jsspec/controllers/api_controller_spec.rbspec/controllers/application_controller_spec.rbspec/factories/users.rbspec/helpers/application_helper_spec.rbspec/jobs/lite/archival_warning_job_spec.rbspec/models/concerns/plan_scopable_spec.rbspec/models/points/raw_data_archive_spec.rbspec/models/user_spec.rbspec/requests/api/v1/plan_spec.rbspec/requests/api/v1/points_spec.rbspec/requests/api/v1/rate_limiting_spec.rbspec/requests/api/v1/subscriptions_spec.rbspec/requests/api/v1/tracks/points_spec.rbspec/requests/imports_spec.rbspec/requests/settings/integrations_spec.rbspec/requests/shared/stats_spec.rbspec/requests/users/digests_spec.rbspec/serializers/api/user_serializer_spec.rbspec/services/users/safe_settings_spec.rbspec/services/users/transportation_thresholds_updater_spec.rbspec/support/devise.rb
Comment on lines
24
to
+25
| def show | ||
| visit = current_api_user.visits.find(params[:id]) | ||
| visit = current_api_user.scoped_visits.find(params[:id]) |
There was a problem hiding this comment.
Apply the same scope to the write endpoints.
show is now plan-scoped, but Lines 43, 57, and 95 still use current_api_user.visits. That leaves visits outside the active plan window writable, mergeable, and deletable by ID even though they are no longer readable via show.
Suggested fix
def update
- visit = current_api_user.visits.find(params[:id])
+ visit = current_api_user.scoped_visits.find(params[:id])
visit = update_visit(visit)
render json: Api::VisitSerializer.new(visit).call
end- visits = current_api_user.visits.where(id: visit_ids).order(started_at: :asc)
+ visits = current_api_user.scoped_visits.where(id: visit_ids).order(started_at: :asc) def destroy
- visit = current_api_user.visits.find(params[:id])
+ visit = current_api_user.scoped_visits.find(params[:id])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/api/v1/visits_controller.rb` around lines 24 - 25, The write
endpoints are still using current_api_user.visits.find which allows modifying
visits outside the active plan window; update those calls to use the same scoped
lookup as show by replacing current_api_user.visits.find(...) with
current_api_user.scoped_visits.find(...) in the actions that perform writes (the
update, merge, and destroy endpoints referenced around lines 43, 57, and 95).
Ensure any helper methods or before_actions that load a visit also use
scoped_visits so all read and write operations consistently apply the plan
scope.
Comment on lines
+14
to
+15
| # Treat purely numeric strings as Unix timestamps | ||
| return date_string.to_i.clamp(min_timestamp, max_timestamp) if date_string.match?(/\A\d+\z/) |
There was a problem hiding this comment.
Keep numeric inputs from bypassing the “safe” fallback.
Line 15 assumes date_string responds to match?. If a controller passes an actual Integer from JSON params, this raises NoMethodError, which is not rescued here and turns a valid numeric timestamp into a 500. Branch on Numeric first, or normalize before regex matching.
Suggested fix
- return date_string.to_i.clamp(min_timestamp, max_timestamp) if date_string.match?(/\A\d+\z/)
+ if date_string.is_a?(Numeric) || date_string.to_s.match?(/\A\d+\z/)
+ return date_string.to_i.clamp(min_timestamp, max_timestamp)
+ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/concerns/safe_timestamp_parser.rb` around lines 14 - 15, The
current check assumes date_string responds to match? and fails for Integer
inputs; update the logic in SafeTimestampParser so numeric params are handled
first (e.g., if date_string.is_a?(Numeric) then use
date_string.to_i.clamp(min_timestamp, max_timestamp)), or normalize the input to
a String before running the regex (e.g., convert date_string.to_s and then call
match?(/\A\d+\z/)); adjust the branch that currently uses
date_string.match?(/\A\d+\z/) to avoid NoMethodError and ensure numeric
timestamps hit the safe clamp path.
Comment on lines
11
to
+17
| visits = current_user | ||
| .visits | ||
| .scoped_visits | ||
| .where(status:) | ||
| .includes(%i[suggested_places area points place]) | ||
| .order(started_at: order_by) | ||
|
|
||
| @suggested_visits_count = current_user.visits.suggested.count | ||
| @suggested_visits_count = current_user.scoped_visits.suggested.count |
There was a problem hiding this comment.
Scope set_visit as well.
Line 50 still loads @visit via current_user.visits.find(params[:id]). That lets a user update a visit outside the active plan window if they already have the ID, so the new scoping here only protects the list view.
Suggested fix
def set_visit
- `@visit` = current_user.visits.find(params[:id])
+ `@visit` = current_user.scoped_visits.find(params[:id])
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/visits_controller.rb` around lines 11 - 17, The set_visit
finder still uses current_user.visits.find(params[:id]) and allows access
outside the scoped window; update the set_visit method (the before_action that
assigns `@visit`) to use current_user.scoped_visits.find(params[:id]) instead so
it enforces the same scoped_visits restrictions as the index/listing and
prevents loading visits outside the active plan window.
Comment on lines
+69
to
+88
| // Gated layer toggles that Lite users cannot persist | ||
| const gatedToggles = new Set([ | ||
| "heatmapToggle", | ||
| "fogToggle", | ||
| "scratchToggle", | ||
| ]) | ||
|
|
||
| Object.entries(toggleMap).forEach(([targetName, settingKey]) => { | ||
| const target = `${targetName}Target` | ||
| const hasTarget = `has${targetName.charAt(0).toUpperCase()}${targetName.slice(1)}Target` | ||
| if (controller[hasTarget]) { | ||
| controller[target].checked = this.settings[settingKey] | ||
| // Force gated layers off for Lite users on page load | ||
| if ( | ||
| gatedToggles.has(targetName) && | ||
| isGatedPlan(controller.userPlanValue) | ||
| ) { | ||
| controller[target].checked = false | ||
| } else { | ||
| controller[target].checked = this.settings[settingKey] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Gated setting reads/writes ==\n'
rg -n -C2 '\b(heatmapEnabled|fogEnabled|scratchEnabled|globeProjection)\b' app/javascript
printf '\n== Globe projection initialization ==\n'
rg -n -C2 '\bsetProjection\s*\(|\bglobeProjection\b' app/javascript/controllers/maps app/javascript/maps_maplibre
printf '\n== Existing gated-plan normalization ==\n'
rg -n -C2 '\bisGatedPlan\b|userPlanValue' app/javascriptRepository: Freika/dawarich
Length of output: 20490
The Lite gate only flips checkboxes; it does not clear the stored gated flags.
syncToggleStates() leaves this.settings.heatmapEnabled, fogEnabled, scratchEnabled, and globeProjection untouched. Since these settings are read directly during map initialization (line 346), layer creation, and data loading, a Lite user with previously enabled gated features will still have them active on the map despite the checkbox being forced off. Globe View is especially risky because the projection is applied during MapInitializer.initialize() before checkbox sync runs.
To fix this, normalize these settings to false for Lite users alongside the checkbox updates, or guard their use throughout initialization and rendering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/controllers/maps/maplibre/settings_manager.js` around lines 69
- 88, syncToggleStates() currently forces checkbox state for gated toggles but
does not update the underlying settings, so Lite users can still have gated
features active; update syncToggleStates() (the block that iterates toggleMap
and uses gatedToggles) to also set this.settings[settingKey] = false when
gatedToggles.has(targetName) && isGatedPlan(controller.userPlanValue), and
specifically ensure globeProjection (and the keys heatmapEnabled, fogEnabled,
scratchEnabled) are normalized to false for Lite users so
MapInitializer.initialize() and layer creation/read paths read the disabled
value rather than relying solely on the checkbox.
Comment on lines
+49
to
+52
| def sanitize_gated_layers | ||
| if @user.settings.key?('enabled_map_layers') | ||
| @user.settings['enabled_map_layers'] -= Users::SafeSettings::GATED_MAP_LAYERS | ||
| end |
There was a problem hiding this comment.
Guard enabled_map_layers before subtracting gated layers.
key? does not guarantee an array value. If this setting is present but nil, the -= will raise and turn a Lite settings update into a 500.
🛠️ Suggested fix
def sanitize_gated_layers
if `@user.settings.key`?('enabled_map_layers')
- `@user.settings`['enabled_map_layers'] -= Users::SafeSettings::GATED_MAP_LAYERS
+ `@user.settings`['enabled_map_layers'] =
+ Array.wrap(`@user.settings`['enabled_map_layers']) - Users::SafeSettings::GATED_MAP_LAYERS
end
`@user.settings`['globe_projection'] = false if `@settings_params.key`?('globe_projection')
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def sanitize_gated_layers | |
| if @user.settings.key?('enabled_map_layers') | |
| @user.settings['enabled_map_layers'] -= Users::SafeSettings::GATED_MAP_LAYERS | |
| end | |
| def sanitize_gated_layers | |
| if `@user.settings.key`?('enabled_map_layers') | |
| `@user.settings`['enabled_map_layers'] = | |
| Array.wrap(`@user.settings`['enabled_map_layers']) - Users::SafeSettings::GATED_MAP_LAYERS | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/users/transportation_thresholds_updater.rb` around lines 49 -
52, sanitize_gated_layers currently assumes `@user.settings`['enabled_map_layers']
is an Array and does a -= which will blow up if it's nil or another type; change
it to guard the value first by fetching the setting into a local (e.g. enabled =
`@user.settings`['enabled_map_layers']) and only perform the subtraction if
enabled.is_a?(Array) (or coerce via Array(enabled) if you intend to treat nil as
empty array), otherwise leave the setting untouched; reference the
sanitize_gated_layers method, `@user.settings`['enabled_map_layers'], and
Users::SafeSettings::GATED_MAP_LAYERS when making the change.
| @@ -0,0 +1,36 @@ | |||
| <div class="card w-full bg-base-200 shadow-xl"> | |||
| <div class="card-body"> | |||
| <h2 class="card-title justify-between text-<%= header_colors[year % header_colors.size] %>"> | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "== Tailwind config =="
fd 'tailwind\.config\.(js|cjs|ts)$' . -x sh -c 'echo "-- $1 --"; sed -n "1,240p" "$1"' sh {}
echo
echo "== Dynamic class usage =="
rg -n 'text-<%=|header_colors' appRepository: Freika/dawarich
Length of output: 1519
🏁 Script executed:
#!/bin/bash
set -e
echo "== header_colors definition =="
rg -A 5 'def header_colors' app/helpers/application_helper.rb
echo
echo "== Full tailwind config =="
cat config/tailwind.config.jsRepository: Freika/dawarich
Length of output: 752
Add a safelist for the dynamic header color utilities in Tailwind config.
The text-* classes generated by <%= header_colors[...] %> are never scanned as complete literals (the helper only contains the bare color names info, success, warning, error, accent, secondary, primary). Tailwind will purge them in production. Add a safelist entry to config/tailwind.config.js:
safelist: [
{ pattern: /^text-(info|success|warning|error|accent|secondary|primary)$/ }
]This pattern also affects other files using the same dynamic approach (app/views/stats/index.html.erb, app/views/stats/_year.html.erb, app/views/insights/_year_comparison.html.erb).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/stats/_locked_year_card.html.erb` at line 3, The dynamic class
generation using header_colors (the text-<%= header_colors[...] %> expressions)
produces partial class names (info, success, etc.) that Tailwind can't detect,
so add a safelist entry to your Tailwind config to preserve those utilities at
build time; update your Tailwind config's safelist to include the regex pattern
{ pattern: /^text-(info|success|warning|error|accent|secondary|primary)$/ }
(this will cover the dynamic uses of header_colors and the same pattern in other
views that build text-* classes).
Comment on lines
+126
to
+159
| <!-- Countries & Cities (Pro / self-hosted only) --> | ||
| <div class="card bg-base-100 shadow-xl mb-8"> | ||
| <div class="card-body text-center items-center"> | ||
| <h2 class="card-title"> | ||
| <%= icon 'map-pin' %> Where They Spent the Most Time | ||
| <%= icon 'earth' %> Countries & Cities | ||
| </h2> | ||
| <ul class="space-y-2 w-full"> | ||
| <% @digest.top_countries_by_time.take(3).each do |country| %> | ||
| <li class="flex justify-between items-center p-3 bg-base-200 rounded-lg"> | ||
| <span class="font-semibold"> | ||
| <span class="mr-1"><%= country_flag(country['name']) %></span> | ||
| <%= country['name'] %> | ||
| </span> | ||
| <span class="text-gray-600"><%= format_time_spent(country['minutes']) %></span> | ||
| </li> | ||
| <% end %> | ||
| </ul> | ||
| </div> | ||
| </div> | ||
| <% end %> | ||
|
|
||
| <!-- Countries & Cities --> | ||
| <div class="card bg-base-100 shadow-xl mb-8"> | ||
| <div class="card-body text-center items-center"> | ||
| <h2 class="card-title"> | ||
| <%= icon 'earth' %> Countries & Cities | ||
| </h2> | ||
| <div class="space-y-4 w-full"> | ||
| <% @digest.toponyms&.each_with_index do |country, index| %> | ||
| <div class="space-y-2"> | ||
| <div class="flex justify-between items-center"> | ||
| <span class="font-semibold"> | ||
| <span class="mr-1"><%= country_flag(country['country']) %></span> | ||
| <%= country['country'] %> | ||
| </span> | ||
| <span class="text-sm"><%= country['cities']&.length || 0 %> cities</span> | ||
| <div class="space-y-4 w-full"> | ||
| <% @digest.toponyms&.each_with_index do |country, index| %> | ||
| <div class="space-y-2"> | ||
| <div class="flex justify-between items-center"> | ||
| <span class="font-semibold"> | ||
| <span class="mr-1"><%= country_flag(country['country']) %></span> | ||
| <%= country['country'] %> | ||
| </span> | ||
| <span class="text-sm"><%= country['cities']&.length || 0 %> cities</span> | ||
| </div> | ||
| <progress class="progress progress-primary w-full" value="<%= 100 - (index * 15) %>" max="100"></progress> | ||
| </div> | ||
| <progress class="progress progress-primary w-full" value="<%= 100 - (index * 15) %>" max="100"></progress> | ||
| </div> | ||
| <% end %> | ||
| </div> | ||
| <% end %> | ||
| </div> | ||
|
|
||
| <div class="divider"></div> | ||
| <div class="divider"></div> | ||
|
|
||
| <div class="flex flex-wrap gap-2 justify-center w-full"> | ||
| <span class="text-sm font-medium">Cities visited:</span> | ||
| <% @digest.toponyms&.each do |country| %> | ||
| <% country['cities']&.take(5)&.each do |city| %> | ||
| <div class="badge badge-outline"><%= city['city'] %></div> | ||
| <% end %> | ||
| <% if country['cities']&.length.to_i > 5 %> | ||
| <div class="badge badge-ghost">+<%= country['cities'].length - 5 %> more</div> | ||
| <div class="flex flex-wrap gap-2 justify-center w-full"> | ||
| <span class="text-sm font-medium">Cities visited:</span> | ||
| <% @digest.toponyms&.each do |country| %> | ||
| <% country['cities']&.take(5)&.each do |city| %> | ||
| <div class="badge badge-outline"><%= city['city'] %></div> | ||
| <% end %> | ||
| <% if country['cities']&.length.to_i > 5 %> | ||
| <div class="badge badge-ghost">+<%= country['cities'].length - 5 %> more</div> | ||
| <% end %> | ||
| <% end %> | ||
| <% end %> | ||
| </div> |
There was a problem hiding this comment.
Use real city coverage values in the public Countries & Cities card.
Line 142’s 100 - (index * 15) is just rank-based decoration, so the bar stops matching the city counts immediately and becomes negative from the eighth country onward. Also, because this section only uses safe navigation, Lines 147-159 still render the divider and Cities visited: label when @digest.toponyms is blank. Please switch this view to the same data-driven progress/empty-state approach already used in app/views/users/digests/show.html.erb.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/users/digests/public_year.html.erb` around lines 126 - 159, The
progress bars and empty-state handling need to be data-driven: compute the max
city count from `@digest.toponyms` (e.g., max_count = `@digest.toponyms.map`{|c|
c['cities']&.length.to_i}.max.to_i) and for each country calculate percentage =
((country['cities']&.length.to_i * 100) / [max_count,1].max).clamp(0,100) and
use that value for the progress value attribute instead of 100 - (index * 15);
also wrap the whole "Countries & Cities" content (the progress loop, the divider
and the "Cities visited:" block that uses country['cities']&.take(5) and the "+n
more" badge) in a conditional that checks `@digest.toponyms.present`? and render
the same empty-state markup used in show.html.erb when absent.
Comment on lines
+5
to
+12
| if DawarichSettings.self_hosted? | ||
| # Self-hosted: all users get pro plan (already the default 1) | ||
| # Explicit update for clarity in case any user has a non-default value | ||
| User.update_all(plan: :pro) | ||
| else | ||
| # Cloud: active/trial users get pro plan (the current plan, renamed) | ||
| User.where(status: %i[active trial]).update_all(plan: :pro) | ||
| User.where(status: :inactive).update_all(plan: :lite) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid coupling this migration to the runtime User model.
Using User directly here means future enum/default-scope/model changes can break a fresh setup on this historical migration. Prefer a migration-local model or SQL against persisted values instead of application enums.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20260301202147_set_plan_for_existing_users.rb` around lines 5 -
12, The migration currently references the runtime User model (User.update_all
and User.where(...).update_all), which couples the migration to future
model/enum changes; change it to use a migration-local model (e.g., define class
MigrationUser < ActiveRecord::Base; self.table_name = "users"; end) or issue raw
SQL updates instead of using User enums: use DawarichSettings.self_hosted? to
branch, then run MigrationUser.update_all(plan: "pro") or execute("UPDATE users
SET plan='pro' WHERE status IN ('active','trial')") and execute("UPDATE users
SET plan='lite' WHERE status='inactive'") so the migration relies only on
persistent DB values.
Comment on lines
+34
to
+41
| context 'when on a self-hosted instance' do | ||
| let(:user) { create(:user) } # default plan is pro | ||
|
|
||
| it 'allows access regardless of plan' do | ||
| get :index, params: { api_key: user.api_key } | ||
|
|
||
| expect(response).to have_http_status(:ok) | ||
| end |
There was a problem hiding this comment.
This context is still exercising the Pro-user path.
There is no self_hosted? stub here, and the example itself says the default user plan is Pro. That means this test can pass even if the self-hosted bypass is broken, because it never forces the Lite/self-hosted combination that matters.
Suggested test setup
context 'when on a self-hosted instance' do
- let(:user) { create(:user) } # default plan is pro
+ let(:user) { create(:user, plan: :lite) }
+
+ before do
+ allow(DawarichSettings).to receive(:self_hosted?).and_return(true)
+ end
it 'allows access regardless of plan' do
get :index, params: { api_key: user.api_key }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| context 'when on a self-hosted instance' do | |
| let(:user) { create(:user) } # default plan is pro | |
| it 'allows access regardless of plan' do | |
| get :index, params: { api_key: user.api_key } | |
| expect(response).to have_http_status(:ok) | |
| end | |
| context 'when on a self-hosted instance' do | |
| let(:user) { create(:user, plan: :lite) } | |
| before do | |
| allow(DawarichSettings).to receive(:self_hosted?).and_return(true) | |
| end | |
| it 'allows access regardless of plan' do | |
| get :index, params: { api_key: user.api_key } | |
| expect(response).to have_http_status(:ok) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/controllers/api_controller_spec.rb` around lines 34 - 41, The test is
exercising a Pro user path and never stubs the self-hosted condition; update the
context 'when on a self-hosted instance' to stub the self-hosted flag (e.g.,
make Site.self_hosted? or equivalent return true) and create a Lite user instead
of the default Pro (use create(:user, plan: :lite) or the factory trait your
suite uses) so the example calling get :index, params: { api_key: user.api_key }
actually verifies the self-hosted bypass for Lite users.
Comment on lines
+56
to
+67
| context 'when on a self-hosted instance' do | ||
| let!(:user) { create(:user) } | ||
|
|
||
| it 'returns full features regardless of plan' do | ||
| get api_v1_plan_url(api_key: user.api_key) | ||
|
|
||
| expect(response).to have_http_status(:ok) | ||
|
|
||
| json = JSON.parse(response.body) | ||
| expect(json['features']['heatmap']).to be(true) | ||
| expect(json['features']['data_window']).to be_nil | ||
| end |
There was a problem hiding this comment.
This self-hosted spec never proves the self-hosted override.
DawarichSettings.self_hosted? is not stubbed here, and the user is not forced onto Lite. A normal cloud path can satisfy these assertions too, so this example can pass without ever exercising the self-hosted bypass.
Suggested test setup
context 'when on a self-hosted instance' do
- let!(:user) { create(:user) }
+ let!(:user) { create(:user, plan: :lite) }
+
+ before do
+ allow(DawarichSettings).to receive(:self_hosted?).and_return(true)
+ end
it 'returns full features regardless of plan' do
get api_v1_plan_url(api_key: user.api_key)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| context 'when on a self-hosted instance' do | |
| let!(:user) { create(:user) } | |
| it 'returns full features regardless of plan' do | |
| get api_v1_plan_url(api_key: user.api_key) | |
| expect(response).to have_http_status(:ok) | |
| json = JSON.parse(response.body) | |
| expect(json['features']['heatmap']).to be(true) | |
| expect(json['features']['data_window']).to be_nil | |
| end | |
| context 'when on a self-hosted instance' do | |
| let!(:user) { create(:user, plan: :lite) } | |
| before do | |
| allow(DawarichSettings).to receive(:self_hosted?).and_return(true) | |
| end | |
| it 'returns full features regardless of plan' do | |
| get api_v1_plan_url(api_key: user.api_key) | |
| expect(response).to have_http_status(:ok) | |
| json = JSON.parse(response.body) | |
| expect(json['features']['heatmap']).to be(true) | |
| expect(json['features']['data_window']).to be_nil | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/requests/api/v1/plan_spec.rb` around lines 56 - 67, The spec is not
proving the self-hosted override because DawarichSettings.self_hosted? isn't
stubbed and the user isn't placed on Lite; update the test to explicitly stub
DawarichSettings.self_hosted? to return true (so the self-hosted code path runs)
and create or set the test user to the Lite plan (e.g. create(:user, :lite) or
set user.plan = Plan::LITE) before calling api_v1_plan_url so the assertions
verify the self-hosted override behavior.
Merged
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
Release Notes
New Features
Chores