Conversation
📝 WalkthroughWalkthroughIntroduces UI enhancements for sorting imports/exports by name, file size, points, and creation date through a new Sortable concern. Adds sortable column headers, updates pagination styling and structure across multiple views, and expands API documentation with comprehensive Swagger specs covering numerous endpoints with richer request/response schemas and detailed field descriptions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/imports_controller.rb (1)
7-23:⚠️ Potential issue | 🟠 Major
byte_sizesorting hides imports without attachments.Line 7 enables
byte_sizesorting, and Line 22 routes index throughsorted(scope). Inapp/controllers/concerns/sortable.rb, that path uses an inner join (joins(file_attachment: :blob)), which drops rows with no attached file. The Import model has no validation enforcing file presence, andapp/services/tasks/imports/google_records.rb(Line 28) creates imports without attachments, making these records invisible when sorting by byte size.Change
joinstoleft_outer_joinsinapp/controllers/concerns/sortable.rb(Line 10):- scope.joins(file_attachment: :blob) + scope.left_outer_joins(file_attachment: :blob)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/imports_controller.rb` around lines 7 - 23, Sorting by byte_size currently uses an inner join that drops imports without attachments; update the Sortable concern so the method used by sorted(scope) (the code that currently does joins(file_attachment: :blob)) uses left_outer_joins(file_attachment: :blob) instead, so SORTABLE_COLUMNS can include "byte_size" without excluding records that have no attached file.
♻️ Duplicate comments (2)
app/views/kaminari/_last_page.html.erb (1)
9-10: Duplicate: icon-only pagination control needs an explicit accessible label.Same issue already raised for the next-page partial.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/kaminari/_last_page.html.erb` around lines 9 - 10, The icon-only pagination link in the partial _last_page.html.erb (the link_to that renders "»»" with class "join-item btn") lacks an accessible label; add an explicit label by providing an aria-label (or adding visually-hidden text inside the link) that clearly describes the control (e.g., "Last page") while preserving remote: remote and url parameters so screen readers can announce the purpose of the link.app/views/kaminari/_first_page.html.erb (1)
9-10: Duplicate: icon-only pagination control needs an explicit accessible label.Same issue already raised for the next-page partial.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/kaminari/_first_page.html.erb` around lines 9 - 10, The first-page pagination control in _first_page.html.erb renders only an icon («) and needs an explicit accessible label; update the link_to in the partial (the same helper that renders the «) to include an aria-label (or title) with a localized string (e.g. t('pagination.first') or "First page") or add visually-hidden text inside the block so screen readers can identify it—mirror the change made in the next-page partial to ensure parity.
🧹 Nitpick comments (9)
app/controllers/concerns/sortable.rb (1)
17-19: Missing constant guard —NameErrorif included withoutSORTABLE_COLUMNS
self.class::SORTABLE_COLUMNSwill raiseNameErrorif the concern is ever included in a controller that forgets to define the constant. A safe default is easy to add.♻️ Proposed fix
def sort_column - self.class::SORTABLE_COLUMNS.include?(params[:sort_by]) ? params[:sort_by] : 'created_at' + sortable_columns = self.class.const_defined?(:SORTABLE_COLUMNS) ? self.class::SORTABLE_COLUMNS : [] + sortable_columns.include?(params[:sort_by]) ? params[:sort_by] : 'created_at' end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/concerns/sortable.rb` around lines 17 - 19, The method sort_column currently calls self.class::SORTABLE_COLUMNS which raises NameError if a controller including the concern hasn’t defined SORTABLE_COLUMNS; change the guard to check for the constant before using it (e.g., use self.class.const_defined?(:SORTABLE_COLUMNS) or defined?(self.class::SORTABLE_COLUMNS)) and only call include? when the constant exists, otherwise fall back to the default 'created_at'; update the logic in sort_column to safely handle missing SORTABLE_COLUMNS while keeping the existing default behavior.spec/requests/authentication_spec.rb (1)
129-132: Avoid unnecessaryENVstubbing—pass the secret directly to both the encoder and decoder.The
EncodeJwtTokenclass acceptssecret_keyas a parameter, andJWT.decodeis a library function that can accept any secret value. Since the test already defines a localsecret_keyvariable, stubbingENVadds unnecessary coupling to global state and violates the guideline of avoiding over-stubbing. Passsecret_keydirectly to bothEncodeJwtToken.new()andJWT.decode()instead.♻️ Suggested refactor
secret_key = 'test-jwt-secret-key' - allow(ENV).to receive(:[]).and_call_original - allow(ENV).to receive(:[]).with('AUTH_JWT_SECRET_KEY').and_return(secret_key) # Test JWT token generation directly using the same logic as after_sign_in_path_for payload = { api_key: user.api_key, exp: 5.minutes.from_now.to_i } # Create JWT token using the same service token = Subscription::EncodeJwtToken.new( - payload, ENV['AUTH_JWT_SECRET_KEY'] + payload, secret_key ).call # Decode the token to verify the payload decoded_payload = JWT.decode( token, - ENV['AUTH_JWT_SECRET_KEY'], + secret_key, true, { algorithm: 'HS256' } ).first🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/authentication_spec.rb` around lines 129 - 132, Remove the ENV stubbing and pass the local secret_key directly to the encoder and decoder: stop calling allow(ENV).to receive... and instead instantiate EncodeJwtToken with secret_key (EncodeJwtToken.new(secret_key) or appropriate constructor param) and call JWT.decode with the same secret_key value; update any expectations that relied on ENV to use the local secret_key variable so the test no longer couples to global ENV.spec/swagger/api/v1/visits_controller_spec.rb (1)
5-5: UseRSpec.describefor consistency.Same issue as
users_controller_spec.rb— baredescribewithoutRSpec.prefix is inconsistent with other specs in this PR.♻️ Proposed fix
-describe 'Visits API', type: :request do +RSpec.describe 'Visits API', type: :request do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/swagger/api/v1/visits_controller_spec.rb` at line 5, Replace the bare top-level `describe 'Visits API', type: :request do` with `RSpec.describe 'Visits API', type: :request do` to match the other specs; update the spec file `visits_controller_spec.rb` so the top-level example group uses `RSpec.describe` instead of `describe`.spec/swagger/api/v1/families/locations_controller_spec.rb (1)
29-36: Consider seeding location data for a more useful Swagger example.The
beforeblock creates a family and membership but no actual location records.SwaggerResponseExample.capturewill embedlocations: []into the generated Swagger docs, making the example less informative for API consumers.💡 Suggested addition
before do family = create(:family, creator: user) create(:family_membership, :owner, family: family, user: user) + # Create a second member with a recent point so the example shows real shape + member = create(:user) + create(:family_membership, family: family, user: member) + create(:point, user: member, timestamp: Time.current) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/swagger/api/v1/families/locations_controller_spec.rb` around lines 29 - 36, The test's before block creates a Family and FamilyMembership but no Location records, so SwaggerResponseExample.capture embeds an empty locations array; modify the before block that calls create(:family, creator: user) and create(:family_membership, :owner, family: family, user: user) to also seed one or more Location records associated with that family (e.g., using create(:location, family: family) or create_list(:location, 2, family: family)) so the response captured by SwaggerResponseExample.capture(response) contains meaningful location data for the generated docs.spec/swagger/api/v1/tags_controller_spec.rb (1)
16-45: Consider seeding a tag+place so the Swagger example is non-empty.No tags or privacy-zone data are created before the 200 test, so
SwaggerResponseExample.capturewill record an empty array. Adding at minimum one tag with a place would make the generated documentation example self-explanatory for API consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/swagger/api/v1/tags_controller_spec.rb` around lines 16 - 45, The 200 response example is empty because no test data is created; before the response '200' block (or inside it) seed at least one Tag and an associated Place so SwaggerResponseExample.capture(example, response) records a non-empty array—e.g. add a before/let! that creates a Tag (and its tag_id, tag_name, tag_icon, tag_color, radius_meters) and a Place (id, name, latitude, longitude) and associates them as your app expects so run_test! returns the populated payload.spec/swagger/api/v1/timeline_controller_spec.rb (1)
33-38: Consider creating tracking data so the captured Swagger example is non-empty.Without any points, visits, or tracks,
SwaggerResponseExample.capturewill embed{ "days": [] }as the documented example, which is not illustrative for API consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/swagger/api/v1/timeline_controller_spec.rb` around lines 33 - 38, The captured Swagger example is empty because no tracking data is created before the request; add setup to create at least one track/point/visit within the spec so SwaggerResponseExample.capture(response) embeds real data. In the spec around let(:start_at)/let(:end_at) and before running run_test!, insert a before hook (or use let! / FactoryBot.create_list) to create sample Track/Point/Visit records (matching your app factories) that fall between start_at and end_at so the controller returns a populated "days" array for SwaggerResponseExample.capture.spec/swagger/api/v1/users_controller_spec.rb (1)
5-5: UseRSpec.describefor consistency.All other new specs introduced in this PR (
families/locations_controller_spec.rb,timeline_controller_spec.rb,tags_controller_spec.rb,borders_controller_spec.rb) useRSpec.describe. The baredescribehere is inconsistent.♻️ Proposed fix
-describe 'Users API', type: :request do +RSpec.describe 'Users API', type: :request do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/swagger/api/v1/users_controller_spec.rb` at line 5, The spec uses the bare describe call for the "Users API" example group; change the top-level call from describe 'Users API', type: :request do to RSpec.describe 'Users API', type: :request do so it matches the other new specs (e.g., families/locations_controller_spec, timeline_controller_spec) and maintains consistency across the test suite.spec/swagger/api/v1/digests_controller_spec.rb (2)
72-72:let!(:stats)returns the Range(1..12), not the created records.
(1..12).each { ... }returns the range, not an array of createdStatobjects. While the variable isn't referenced for its value elsewhere in this spec (only for the side effect), usingmapinstead ofeachwould be more idiomatic if someone later tries to usestats:Suggested fix
- let!(:stats) { (1..12).each { |m| create(:stat, year: 2024, month: m, user: user) } } + let!(:stats) { (1..12).map { |m| create(:stat, year: 2024, month: m, user: user) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/swagger/api/v1/digests_controller_spec.rb` at line 72, The test's let! binding let!(:stats) uses (1..12).each { |m| create(:stat, year: 2024, month: m, user: user) } which returns the Range rather than the created records; change the block to use map (e.g., (1..12).map { |m| create(... ) }) so let!(:stats) will hold the array of created Stat objects while preserving the side effect; update the let!(:stats) declaration accordingly in the spec.
173-187: Consider extracting a:digestfactory instead of inlineUsers::Digest.create!.This pattern is duplicated at lines 173–187 and 215–222. A factory (e.g.,
create(:users_digest, user: user, year: 2024)) would reduce boilerplate and improve maintainability. Not blocking, but worth a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/swagger/api/v1/digests_controller_spec.rb` around lines 173 - 187, Replace the inline Users::Digest.create! block used in the let!(:digest) declarations with a dedicated factory (e.g., :users_digest) so both occurrences reuse it; create a factory that sets sensible defaults for attributes used in the spec (user, year, period_type, distance, toponyms, monthly_distances, time_spent_by_location, first_time_visits, year_over_year, all_time_stats, travel_patterns) and update the specs to call create(:users_digest, user: user, year: 2024) (or override any attributes needed) instead of repeating Users::Digest.create! in both places.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/concerns/sortable.rb`:
- Around line 8-15: The current sorted(scope) method uses joins(file_attachment:
:blob) which performs an INNER JOIN and drops records without attachments;
change it to use a LEFT OUTER JOIN (e.g., left_outer_joins(file_attachment:
:blob)) and adjust the ORDER BY to handle NULLs and missing byte_size by using
COALESCE(active_storage_blobs.byte_size, 0) (or wrap in Arel.sql) and append a
NULLS LAST/NULLS FIRST clause as appropriate so unattached records are preserved
and sorted to the end; update references to sort_column == 'byte_size',
sort_direction, and the scope.order call inside sorted to use the new LEFT OUTER
JOIN + COALESCE ordering approach.
In `@app/views/exports/_table_row.html.erb`:
- Line 3: The current call number_to_human_size(export.file&.byte_size) wrongly
assumes export.file can be nil; ActiveStorage::Attached::One is never nil and
will delegate to a nil blob causing NoMethodError when no file is attached. Fix
by reading the blob size safely (e.g. use export.file.blob&.byte_size or guard
with export.file.attached?) and pass that value to number_to_human_size (keeping
the 'N/A' fallback). Update the _table_row view to use
export.file.blob&.byte_size (or an attached? check) instead of
export.file&.byte_size.
In `@app/views/imports/_table_row.html.erb`:
- Around line 20-21: The tooltip span showing import.error_message is not
keyboard-accessible; make it focusable and screen-reader friendly by adding
tabindex="0" to the element and an accessible label (use aria-label or
aria-describedby pointing to a visually hidden element containing
import.error_message.presence || 'Import failed'); ensure the
icon('circle-alert') remains decorative (aria-hidden="true") so the screen
reader reads the provided label. Target the span with the tooltip and the
import.error_message expression to implement this.
In `@app/views/kaminari/_gap.html.erb`:
- Line 8: Replace the non-interactive <button> used in the pagination gap
partial (_gap.html.erb) with a purely presentational inline element: render a
<span> (or similar non-interactive element) that preserves the same classes
("join-item", "btn", "btn-disabled") for styling and add accessibility
attributes such as aria-hidden="true" (and/or role="presentation") so the "..."
is treated as display-only and not announced as a control.
In `@app/views/kaminari/_next_page.html.erb`:
- Around line 9-10: The "next" pagination link rendered by the link_to block in
_next_page.html.erb currently outputs only the glyph (») so it's not
accessible; update the link_to call for the next control to include descriptive
attributes (e.g., aria-label="Next page" and title="Next page") and do the same
in _prev_page.html.erb for the previous control (aria-label="Previous page" and
title="Previous page") so screen readers and hover tooltips can announce the
control's purpose.
In `@app/views/kaminari/_page.html.erb`:
- Around line 10-13: The active-page branch inside the ERB partial (the
page.current? conditional in app/views/kaminari/_page.html.erb) renders a
clickable <button>; change that to a non-interactive element (e.g., a <span> or
<div>) with the same styling class (join-item btn btn-active) and add
aria-current="page" to convey the current page to assistive tech; keep the else
branch using link_to for non-current pages unchanged.
In `@app/views/kaminari/_prev_page.html.erb`:
- Around line 9-10: The previous-page link in the _prev_page.html.erb partial
renders only a symbol and lacks an accessible name; update the link_to call that
renders « to include an aria-label (preferably localized via I18n.t, e.g.
I18n.t('pagination.previous')) so screen readers announce "Previous page".
Ensure the aria-label is added alongside existing attributes (rel, remote,
class) on the same link_to invocation.
In `@spec/swagger/api/v1/areas_controller_spec.rb`:
- Around line 18-32: The request body schemas defining the top-level "area"
object (the parameter declared as name: :area in the Swagger spec) are missing a
top-level required key, but the controller uses params.require(:area); update
both occurrences of the body schema to include required: %w[area] at the top
level (i.e., the schema that wraps the inner area properties) so the OpenAPI
contract requires the top-level area key and matches the controller's
params.require(:area).
In `@spec/swagger/api/v1/insights_controller_spec.rb`:
- Around line 107-108: The continuation line for the dayOfWeek property is
misaligned; adjust the indentation so the description: 'Distance by day of week
(Mon-Sun)' line is indented two spaces to align with the other hash entries
under dayOfWeek (i.e., align the continuation with the dayOfWeek: { ... }
block), ensuring Ruby two-space indentation is used for the property hash.
In `@spec/swagger/api/v1/photos_controller_spec.rb`:
- Around line 112-113: The continuation line for the localDateTime schema entry
is misindented (the `description:` key is at column 0); update the indentation
so the `description:` line is indented two spaces to match Ruby two-space style
and align with the `localDateTime:` block in
spec/swagger/api/v1/photos_controller_spec.rb (look for the `localDateTime: {
type: :string, format: 'date-time',` line and re-indent the following
`description:` line), and apply the same two-space fix to any other `.rb` files
with similar misaligned continuation lines.
In `@spec/swagger/api/v1/points_controller_spec.rb`:
- Line 220: Update the Swagger path parameter declaration for the Points
endpoint so the :id is documented as an integer rather than a string: locate the
parameter line "parameter name: :id, in: :path, type: :string, required: true,
description: 'Point ID'" in spec/swagger/api/v1/points_controller_spec.rb and
change the type to :integer to match the controller behavior (Points controller
uses .find(params[:id]) and Rails integer IDs) and to be consistent with other
specs (tracks, visits, places, areas).
In `@spec/swagger/api/v1/subscriptions_controller_spec.rb`:
- Around line 29-35: The test's JWT token uses ENV['JWT_SECRET_KEY'] without
setting it, so make it deterministic by stubbing the ENV key before the token is
generated: add a setup like stub_const('ENV', ENV.to_hash.merge('JWT_SECRET_KEY'
=> 'test_jwt_secret')) (or equivalent) in the spec so the let(:token) that calls
JWT.encode uses a known secret; ensure the stub appears before the let(:token)
reference so JWT.decode in the code under test will match.
In `@spec/swagger/api/v1/tracks_controller_spec.rb`:
- Around line 55-56: The continuation of the hash for the schema field
`dominant_mode_emoji` is misaligned (starts at column 0); fix the indentation to
use two spaces and align the continuation line with the start of the hash on the
previous line in spec/swagger/api/v1/tracks_controller_spec.rb so the block
reads with consistent two-space indentation for the `dominant_mode_emoji: {
type: :string, nullable: true, description: 'Emoji for transportation mode' }`
entry.
In `@swagger/v1/swagger.yaml`:
- Around line 1658-1676: The OpenAPI for the /api/v1/photos/{id}/thumbnail
endpoint is inconsistent: add a content block for the '200' response with media
type image/jpeg and schema type: string, format: binary to represent binary
image data, and remove the incorrect image/jpeg media type from the '404'
response so the '404' only has application/json with a schema object containing
an error string (retain the error property description); update the response
blocks in swagger.yaml accordingly.
- Around line 953-966: The OpenAPI parameters "lat" and "lon" currently place
format at the parameter level; move each format into its schema by editing the
parameter objects for lat and lon so their schema includes both type: number and
format: float (i.e., inside the schema block for the parameters named "lat" and
"lon" update the schema to contain type: number and format: float and remove the
top-level format fields).
---
Outside diff comments:
In `@app/controllers/imports_controller.rb`:
- Around line 7-23: Sorting by byte_size currently uses an inner join that drops
imports without attachments; update the Sortable concern so the method used by
sorted(scope) (the code that currently does joins(file_attachment: :blob)) uses
left_outer_joins(file_attachment: :blob) instead, so SORTABLE_COLUMNS can
include "byte_size" without excluding records that have no attached file.
---
Duplicate comments:
In `@app/views/kaminari/_first_page.html.erb`:
- Around line 9-10: The first-page pagination control in _first_page.html.erb
renders only an icon («) and needs an explicit accessible label; update the
link_to in the partial (the same helper that renders the «) to include an
aria-label (or title) with a localized string (e.g. t('pagination.first') or
"First page") or add visually-hidden text inside the block so screen readers can
identify it—mirror the change made in the next-page partial to ensure parity.
In `@app/views/kaminari/_last_page.html.erb`:
- Around line 9-10: The icon-only pagination link in the partial
_last_page.html.erb (the link_to that renders "»»" with class "join-item btn")
lacks an accessible label; add an explicit label by providing an aria-label (or
adding visually-hidden text inside the link) that clearly describes the control
(e.g., "Last page") while preserving remote: remote and url parameters so screen
readers can announce the purpose of the link.
---
Nitpick comments:
In `@app/controllers/concerns/sortable.rb`:
- Around line 17-19: The method sort_column currently calls
self.class::SORTABLE_COLUMNS which raises NameError if a controller including
the concern hasn’t defined SORTABLE_COLUMNS; change the guard to check for the
constant before using it (e.g., use self.class.const_defined?(:SORTABLE_COLUMNS)
or defined?(self.class::SORTABLE_COLUMNS)) and only call include? when the
constant exists, otherwise fall back to the default 'created_at'; update the
logic in sort_column to safely handle missing SORTABLE_COLUMNS while keeping the
existing default behavior.
In `@spec/requests/authentication_spec.rb`:
- Around line 129-132: Remove the ENV stubbing and pass the local secret_key
directly to the encoder and decoder: stop calling allow(ENV).to receive... and
instead instantiate EncodeJwtToken with secret_key
(EncodeJwtToken.new(secret_key) or appropriate constructor param) and call
JWT.decode with the same secret_key value; update any expectations that relied
on ENV to use the local secret_key variable so the test no longer couples to
global ENV.
In `@spec/swagger/api/v1/digests_controller_spec.rb`:
- Line 72: The test's let! binding let!(:stats) uses (1..12).each { |m|
create(:stat, year: 2024, month: m, user: user) } which returns the Range rather
than the created records; change the block to use map (e.g., (1..12).map { |m|
create(... ) }) so let!(:stats) will hold the array of created Stat objects
while preserving the side effect; update the let!(:stats) declaration
accordingly in the spec.
- Around line 173-187: Replace the inline Users::Digest.create! block used in
the let!(:digest) declarations with a dedicated factory (e.g., :users_digest) so
both occurrences reuse it; create a factory that sets sensible defaults for
attributes used in the spec (user, year, period_type, distance, toponyms,
monthly_distances, time_spent_by_location, first_time_visits, year_over_year,
all_time_stats, travel_patterns) and update the specs to call
create(:users_digest, user: user, year: 2024) (or override any attributes
needed) instead of repeating Users::Digest.create! in both places.
In `@spec/swagger/api/v1/families/locations_controller_spec.rb`:
- Around line 29-36: The test's before block creates a Family and
FamilyMembership but no Location records, so SwaggerResponseExample.capture
embeds an empty locations array; modify the before block that calls
create(:family, creator: user) and create(:family_membership, :owner, family:
family, user: user) to also seed one or more Location records associated with
that family (e.g., using create(:location, family: family) or
create_list(:location, 2, family: family)) so the response captured by
SwaggerResponseExample.capture(response) contains meaningful location data for
the generated docs.
In `@spec/swagger/api/v1/tags_controller_spec.rb`:
- Around line 16-45: The 200 response example is empty because no test data is
created; before the response '200' block (or inside it) seed at least one Tag
and an associated Place so SwaggerResponseExample.capture(example, response)
records a non-empty array—e.g. add a before/let! that creates a Tag (and its
tag_id, tag_name, tag_icon, tag_color, radius_meters) and a Place (id, name,
latitude, longitude) and associates them as your app expects so run_test!
returns the populated payload.
In `@spec/swagger/api/v1/timeline_controller_spec.rb`:
- Around line 33-38: The captured Swagger example is empty because no tracking
data is created before the request; add setup to create at least one
track/point/visit within the spec so SwaggerResponseExample.capture(response)
embeds real data. In the spec around let(:start_at)/let(:end_at) and before
running run_test!, insert a before hook (or use let! / FactoryBot.create_list)
to create sample Track/Point/Visit records (matching your app factories) that
fall between start_at and end_at so the controller returns a populated "days"
array for SwaggerResponseExample.capture.
In `@spec/swagger/api/v1/users_controller_spec.rb`:
- Line 5: The spec uses the bare describe call for the "Users API" example
group; change the top-level call from describe 'Users API', type: :request do to
RSpec.describe 'Users API', type: :request do so it matches the other new specs
(e.g., families/locations_controller_spec, timeline_controller_spec) and
maintains consistency across the test suite.
In `@spec/swagger/api/v1/visits_controller_spec.rb`:
- Line 5: Replace the bare top-level `describe 'Visits API', type: :request do`
with `RSpec.describe 'Visits API', type: :request do` to match the other specs;
update the spec file `visits_controller_spec.rb` so the top-level example group
uses `RSpec.describe` instead of `describe`.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/assets/svg/icons/lucide/outline/arrow-down-up.svgis excluded by!**/*.svg
📒 Files selected for processing (50)
CHANGELOG.mdapp/assets/builds/tailwind.cssapp/controllers/concerns/sortable.rbapp/controllers/exports_controller.rbapp/controllers/imports_controller.rbapp/helpers/application_helper.rbapp/views/exports/_table_row.html.erbapp/views/exports/index.html.erbapp/views/imports/_table_row.html.erbapp/views/imports/index.html.erbapp/views/kaminari/_first_page.html.erbapp/views/kaminari/_gap.html.erbapp/views/kaminari/_last_page.html.erbapp/views/kaminari/_next_page.html.erbapp/views/kaminari/_page.html.erbapp/views/kaminari/_paginator.html.erbapp/views/kaminari/_prev_page.html.erbapp/views/places/index.html.erbapp/views/points/index.html.erbapp/views/trips/index.html.erbapp/views/visits/index.html.erbspec/requests/authentication_spec.rbspec/support/geocoder_stubs.rbspec/support/swagger_response_example.rbspec/swagger/api/v1/areas_controller_spec.rbspec/swagger/api/v1/countries/borders_controller_spec.rbspec/swagger/api/v1/countries/visited_cities_spec.rbspec/swagger/api/v1/digests_controller_spec.rbspec/swagger/api/v1/families/locations_controller_spec.rbspec/swagger/api/v1/health_controller_spec.rbspec/swagger/api/v1/insights_controller_spec.rbspec/swagger/api/v1/locations_controller_spec.rbspec/swagger/api/v1/maps/hexagons_controller_spec.rbspec/swagger/api/v1/overland/batches_controller_spec.rbspec/swagger/api/v1/owntracks/points_controller_spec.rbspec/swagger/api/v1/photos_controller_spec.rbspec/swagger/api/v1/places_controller_spec.rbspec/swagger/api/v1/points/tracked_months_controller_spec.rbspec/swagger/api/v1/points_controller_spec.rbspec/swagger/api/v1/settings_controller_spec.rbspec/swagger/api/v1/stats_controller_spec.rbspec/swagger/api/v1/subscriptions_controller_spec.rbspec/swagger/api/v1/tags_controller_spec.rbspec/swagger/api/v1/timeline_controller_spec.rbspec/swagger/api/v1/tracks/points_controller_spec.rbspec/swagger/api/v1/tracks_controller_spec.rbspec/swagger/api/v1/users_controller_spec.rbspec/swagger/api/v1/visits_controller_spec.rbspec/swagger_helper.rbswagger/v1/swagger.yaml
| def sorted(scope) | ||
| if sort_column == 'byte_size' | ||
| scope.joins(file_attachment: :blob) | ||
| .order(Arel.sql('active_storage_blobs.byte_size').public_send(sort_direction)) | ||
| else | ||
| scope.order(sort_column => sort_direction) | ||
| end | ||
| end |
There was a problem hiding this comment.
INNER JOIN on byte_size sort silently drops records without attached files
joins(file_attachment: :blob) produces an INNER JOIN. Any export or import that doesn't yet have an attached file (e.g., in-progress, failed, or legacy records) will be excluded from the result set when the user sorts by file size. This creates a confusing UX where rows disappear depending on the selected sort column.
Use a LEFT OUTER JOIN and COALESCE to preserve all records:
🐛 Proposed fix
- def sorted(scope)
- if sort_column == 'byte_size'
- scope.joins(file_attachment: :blob)
- .order(Arel.sql('active_storage_blobs.byte_size').public_send(sort_direction))
- else
- scope.order(sort_column => sort_direction)
- end
- end
+ def sorted(scope)
+ if sort_column == 'byte_size'
+ scope.joins("LEFT OUTER JOIN active_storage_attachments ON active_storage_attachments.record_id = #{scope.klass.table_name}.id AND active_storage_attachments.record_type = '#{scope.klass.name}' AND active_storage_attachments.name = 'file'")
+ .joins("LEFT OUTER JOIN active_storage_blobs ON active_storage_blobs.id = active_storage_attachments.blob_id")
+ .order(Arel.sql("COALESCE(active_storage_blobs.byte_size, 0)").public_send(sort_direction))
+ else
+ scope.order(sort_column => sort_direction)
+ end
+ endAlternatively, use the Rails left_outer_joins helper if your association setup supports it cleanly, then fallback-sort unattached records to the end via NULLS LAST / NULLS FIRST.
📝 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.
| def sorted(scope) | |
| if sort_column == 'byte_size' | |
| scope.joins(file_attachment: :blob) | |
| .order(Arel.sql('active_storage_blobs.byte_size').public_send(sort_direction)) | |
| else | |
| scope.order(sort_column => sort_direction) | |
| end | |
| end | |
| def sorted(scope) | |
| if sort_column == 'byte_size' | |
| scope.joins("LEFT OUTER JOIN active_storage_attachments ON active_storage_attachments.record_id = #{scope.klass.table_name}.id AND active_storage_attachments.record_type = '#{scope.klass.name}' AND active_storage_attachments.name = 'file'") | |
| .joins("LEFT OUTER JOIN active_storage_blobs ON active_storage_blobs.id = active_storage_attachments.blob_id") | |
| .order(Arel.sql("COALESCE(active_storage_blobs.byte_size, 0)").public_send(sort_direction)) | |
| else | |
| scope.order(sort_column => sort_direction) | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/concerns/sortable.rb` around lines 8 - 15, The current
sorted(scope) method uses joins(file_attachment: :blob) which performs an INNER
JOIN and drops records without attachments; change it to use a LEFT OUTER JOIN
(e.g., left_outer_joins(file_attachment: :blob)) and adjust the ORDER BY to
handle NULLs and missing byte_size by using
COALESCE(active_storage_blobs.byte_size, 0) (or wrap in Arel.sql) and append a
NULLS LAST/NULLS FIRST clause as appropriate so unattached records are preserved
and sorted to the end; update references to sort_column == 'byte_size',
sort_direction, and the scope.order call inside sorted to use the new LEFT OUTER
JOIN + COALESCE ordering approach.
| @@ -0,0 +1,23 @@ | |||
| <tr id="<%= dom_id(export) %>" class="hover"> | |||
| <td><%= export.name %></td> | |||
| <td><%= number_to_human_size(export.file&.byte_size) || 'N/A' %></td> | |||
There was a problem hiding this comment.
export.file&.byte_size does not guard against unattached files — potential NoMethodError
export.file (a has_one_attached association) returns an ActiveStorage::Attached::One instance, which is never nil. The &. safe-navigation operator therefore never short-circuits. When the export has no file attached, export.file.byte_size delegates to a nil blob and will raise NoMethodError at runtime.
🐛 Proposed fix
- <td><%= number_to_human_size(export.file&.byte_size) || 'N/A' %></td>
+ <td><%= export.file.attached? ? number_to_human_size(export.file.byte_size) : 'N/A' %></td>📝 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.
| <td><%= number_to_human_size(export.file&.byte_size) || 'N/A' %></td> | |
| <td><%= export.file.attached? ? number_to_human_size(export.file.byte_size) : 'N/A' %></td> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/exports/_table_row.html.erb` at line 3, The current call
number_to_human_size(export.file&.byte_size) wrongly assumes export.file can be
nil; ActiveStorage::Attached::One is never nil and will delegate to a nil blob
causing NoMethodError when no file is attached. Fix by reading the blob size
safely (e.g. use export.file.blob&.byte_size or guard with
export.file.attached?) and pass that value to number_to_human_size (keeping the
'N/A' fallback). Update the _table_row view to use export.file.blob&.byte_size
(or an attached? check) instead of export.file&.byte_size.
| <span class="tooltip tooltip-left cursor-help inline-flex items-center" data-tip="<%= import.error_message.presence || 'Import failed' %>"> | ||
| <%= icon('circle-alert', class: ['w-4', 'h-4', 'text-red-500']) %> |
There was a problem hiding this comment.
Expose failed-import details to keyboard/screen-reader users.
Line 20 uses a non-focusable span for tooltip-only content. Keyboard users may miss the error detail. Add focusability and an accessible text label.
Suggested tweak
- <span class="tooltip tooltip-left cursor-help inline-flex items-center" data-tip="<%= import.error_message.presence || 'Import failed' %>">
+ <span class="tooltip tooltip-left cursor-help inline-flex items-center"
+ tabindex="0"
+ aria-label="<%= import.error_message.presence || 'Import failed' %>"
+ data-tip="<%= import.error_message.presence || 'Import failed' %>">
<%= icon('circle-alert', class: ['w-4', 'h-4', 'text-red-500']) %>
</span>📝 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.
| <span class="tooltip tooltip-left cursor-help inline-flex items-center" data-tip="<%= import.error_message.presence || 'Import failed' %>"> | |
| <%= icon('circle-alert', class: ['w-4', 'h-4', 'text-red-500']) %> | |
| <span class="tooltip tooltip-left cursor-help inline-flex items-center" | |
| tabindex="0" | |
| aria-label="<%= import.error_message.presence || 'Import failed' %>" | |
| data-tip="<%= import.error_message.presence || 'Import failed' %>"> | |
| <%= icon('circle-alert', class: ['w-4', 'h-4', 'text-red-500']) %> | |
| </span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/imports/_table_row.html.erb` around lines 20 - 21, The tooltip span
showing import.error_message is not keyboard-accessible; make it focusable and
screen-reader friendly by adding tabindex="0" to the element and an accessible
label (use aria-label or aria-describedby pointing to a visually hidden element
containing import.error_message.presence || 'Import failed'); ensure the
icon('circle-alert') remains decorative (aria-hidden="true") so the screen
reader reads the provided label. Target the span with the tooltip and the
import.error_message expression to implement this.
| per_page: number of items to fetch per page | ||
| remote: data-remote | ||
| -%> | ||
| <button class="join-item btn btn-disabled">...</button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and read its contents
find . -path "*/app/views/kaminari/_gap.html.erb" -type fRepository: Freika/dawarich
Length of output: 93
🏁 Script executed:
# Once found, read the file
cat -n app/views/kaminari/_gap.html.erbRepository: Freika/dawarich
Length of output: 445
🏁 Script executed:
# Check if there are other Kaminari partials to understand the pattern
find . -path "*/app/views/kaminari/*.erb" -type f | head -20Repository: Freika/dawarich
Length of output: 335
🏁 Script executed:
# Look for any similar gap or pagination patterns in the codebase
rg "aria-hidden|btn-disabled" --type html --type erb --max-count 10Repository: Freika/dawarich
Length of output: 488
🏁 Script executed:
# Check other Kaminari partials to see how they're structured
cat -n app/views/kaminari/_page.html.erbRepository: Freika/dawarich
Length of output: 688
🏁 Script executed:
# Look for accessibility patterns in the codebase
rg "role=|aria-label|aria-" --type erb app/views/kaminari/ | head -20Repository: Freika/dawarich
Length of output: 155
🏁 Script executed:
# Check the other pagination button elements to understand the full pattern
cat -n app/views/kaminari/_prev_page.html.erb app/views/kaminari/_next_page.html.erbRepository: Freika/dawarich
Length of output: 996
🏁 Script executed:
# Look at how the paginator wraps these elements
cat -n app/views/kaminari/_paginator.html.erbRepository: Freika/dawarich
Length of output: 997
Use non-interactive markup for the pagination gap token.
The gap partial renders static "..." content using a <button> element, which is semantically incorrect for non-clickable pagination markers. A <span> with aria-hidden="true" better represents display-only content.
Suggested fix
-<button class="join-item btn btn-disabled">...</button>
+<span class="join-item btn btn-disabled" aria-hidden="true">...</span>📝 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.
| <button class="join-item btn btn-disabled">...</button> | |
| <span class="join-item btn btn-disabled" aria-hidden="true">...</span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/kaminari/_gap.html.erb` at line 8, Replace the non-interactive
<button> used in the pagination gap partial (_gap.html.erb) with a purely
presentational inline element: render a <span> (or similar non-interactive
element) that preserves the same classes ("join-item", "btn", "btn-disabled")
for styling and add accessibility attributes such as aria-hidden="true" (and/or
role="presentation") so the "..." is treated as display-only and not announced
as a control.
| <%= link_to url, rel: 'next', remote: remote, class: "join-item btn" do %> | ||
| » |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file in the repository
find . -name "_next_page.html.erb" -type fRepository: Freika/dawarich
Length of output: 99
🏁 Script executed:
# Check the full content of the file to see if it matches the review snippet
cat -n "$(find . -name '_next_page.html.erb' -type f | head -1)"Repository: Freika/dawarich
Length of output: 523
🏁 Script executed:
# Look for similar pagination partials to check if they follow accessibility patterns
find . -path "*/kaminari/*" -name "*.erb" -type f | head -10Repository: Freika/dawarich
Length of output: 335
🏁 Script executed:
# Check if there's a similar _prev_page or other pagination controls for consistency patterns
cat -n "$(find . -path '*/kaminari/*' -name '*prev*' -type f | head -1)" 2>/dev/null || echo "No prev page file found"Repository: Freika/dawarich
Length of output: 531
Add an accessible name to the icon-only "next page" control.
Line 9 renders only a glyph (»), so screen readers won't announce the link's purpose. Add aria-label and title to make the navigation action explicit. The same issue exists in _prev_page.html.erb with the « glyph.
💡 Proposed fix
-<%= link_to url, rel: 'next', remote: remote, class: "join-item btn" do %>
+<%= link_to url, rel: 'next', remote: remote, class: "join-item btn",
+ aria: { label: 'Next page' }, title: 'Next page' do %>
»
<% 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.
| <%= link_to url, rel: 'next', remote: remote, class: "join-item btn" do %> | |
| » | |
| <%= link_to url, rel: 'next', remote: remote, class: "join-item btn", | |
| aria: { label: 'Next page' }, title: 'Next page' do %> | |
| » | |
| <% end %> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/kaminari/_next_page.html.erb` around lines 9 - 10, The "next"
pagination link rendered by the link_to block in _next_page.html.erb currently
outputs only the glyph (») so it's not accessible; update the link_to call
for the next control to include descriptive attributes (e.g., aria-label="Next
page" and title="Next page") and do the same in _prev_page.html.erb for the
previous control (aria-label="Previous page" and title="Previous page") so
screen readers and hover tooltips can announce the control's purpose.
| end | ||
|
|
||
| path '/api/v1/points/{id}' do | ||
| parameter name: :id, in: :path, type: :string, required: true, description: 'Point ID' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the spec file
find . -type f -name "*points_controller_spec.rb" | head -5Repository: Freika/dawarich
Length of output: 219
🏁 Script executed:
# Check the file size first
wc -l spec/swagger/api/v1/points_controller_spec.rb 2>/dev/null || echo "File not found at expected path"Repository: Freika/dawarich
Length of output: 108
🏁 Script executed:
# Let's search for the file more broadly
find . -type f -name "*points*" | grep -E "spec|controller" | head -10Repository: Freika/dawarich
Length of output: 481
🏁 Script executed:
# List the swagger spec directory structure
find . -path "*/swagger/api/v1/*" -type f 2>/dev/null | head -20Repository: Freika/dawarich
Length of output: 1103
🏁 Script executed:
# Try to locate the points controller or related swagger specs
find . -type f \( -name "*points*" -o -name "*point*" \) | grep -v node_modules | head -20Repository: Freika/dawarich
Length of output: 1089
🏁 Script executed:
# Read the spec file around lines 220 and 245
sed -n '210,260p' spec/swagger/api/v1/points_controller_spec.rbRepository: Freika/dawarich
Length of output: 1701
🏁 Script executed:
# Check the Points controller
cat app/controllers/api/v1/points_controller.rb | head -100Repository: Freika/dawarich
Length of output: 3208
🏁 Script executed:
# Check Point model to understand the ID type
head -50 app/models/point.rbRepository: Freika/dawarich
Length of output: 1787
🏁 Script executed:
# Check other swagger specs to see what pattern is used for path parameters with IDs
grep -r "parameter name.*:id.*in.*:path" spec/swagger/ | head -20Repository: Freika/dawarich
Length of output: 1174
Change the point id parameter type from string to integer in the Swagger documentation.
The Points API endpoint at /api/v1/points/{id} uses Rails record IDs (integers), but the Swagger spec incorrectly documents the :id path parameter as :string. This is inconsistent with similar endpoints (tracks, visits, places, areas all use :integer) and the actual controller implementation, which calls .find(params[:id]) and expects an integer ID.
Suggested fix
- parameter name: :id, in: :path, type: :string, required: true, description: 'Point ID'
+ parameter name: :id, in: :path, type: :integer, required: true, description: 'Point ID'📝 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.
| parameter name: :id, in: :path, type: :string, required: true, description: 'Point ID' | |
| parameter name: :id, in: :path, type: :integer, required: true, description: 'Point ID' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/swagger/api/v1/points_controller_spec.rb` at line 220, Update the
Swagger path parameter declaration for the Points endpoint so the :id is
documented as an integer rather than a string: locate the parameter line
"parameter name: :id, in: :path, type: :string, required: true, description:
'Point ID'" in spec/swagger/api/v1/points_controller_spec.rb and change the type
to :integer to match the controller behavior (Points controller uses
.find(params[:id]) and Rails integer IDs) and to be consistent with other specs
(tracks, visits, places, areas).
| let(:token) do | ||
| JWT.encode( | ||
| { user_id: user.id, status: 'active', active_until: 1.year.from_now.iso8601 }, | ||
| ENV['JWT_SECRET_KEY'], | ||
| 'HS256' | ||
| ) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for JWT_SECRET_KEY configuration in test environment
rg -n 'JWT_SECRET_KEY' --type ruby -C 3Repository: Freika/dawarich
Length of output: 6200
🏁 Script executed:
#!/bin/bash
# Check the test helper files for environment setup
find . -name 'rails_helper.rb' -o -name 'spec_helper.rb' | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat -n {}'Repository: Freika/dawarich
Length of output: 9407
🏁 Script executed:
#!/bin/bash
# Check the actual subscriptions_controller to see how it validates JWT
fd -e rb -path '*/controllers/*subscriptions*' | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -100 {}'Repository: Freika/dawarich
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Look for .env.test or similar configuration files
find . -maxdepth 2 -name '.env*' -o -name 'config.yml' | head -10Repository: Freika/dawarich
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check the .env.test file for JWT_SECRET_KEY
cat -n ./.env.test | grep -i 'jwt\|secret' || echo "No JWT/SECRET found in .env.test"
head -50 ./.env.testRepository: Freika/dawarich
Length of output: 249
🏁 Script executed:
#!/bin/bash
# Check the exact spec file under review to see its full setup
cat -n spec/swagger/api/v1/subscriptions_controller_spec.rbRepository: Freika/dawarich
Length of output: 2001
🏁 Script executed:
#!/bin/bash
# Check if there's a support file that sets up JWT_SECRET_KEY globally
ls -la spec/support/
fd -type f -name "*.rb" spec/support | xargs grep -l "JWT_SECRET_KEY" 2>/dev/null || echo "No support files set JWT_SECRET_KEY"Repository: Freika/dawarich
Length of output: 998
🏁 Script executed:
#!/bin/bash
# Find the subscriptions controller and check how it handles JWT decoding
find . -path '*/controllers/*subscriptions*' -name '*.rb' | xargs cat -n | head -150Repository: Freika/dawarich
Length of output: 966
🏁 Script executed:
#!/bin/bash
# Check the Subscription::DecodeJwtToken service more closely
cat -n app/services/subscription/decode_jwt_token.rbRepository: Freika/dawarich
Length of output: 434
Explicitly set JWT_SECRET_KEY in test to match codebase patterns and ensure token validity.
This test uses ENV['JWT_SECRET_KEY'] without explicit setup. While JWT.encode and JWT.decode will technically work if both use nil, this is inconsistent with other JWT tests in the codebase (e.g., spec/requests/api/v1/subscriptions_spec.rb and spec/jobs/users/trial_webhook_job_spec.rb both stub ENV with test secrets). Use stub_const to ensure the test encodes/decodes with the same deterministic secret:
Proposed fix
response '200', 'subscription updated' do
schema type: :object,
properties: {
message: { type: :string, description: 'Confirmation message' }
}
+ before do
+ stub_const('ENV', ENV.to_h.merge('JWT_SECRET_KEY' => 'test_secret'))
+ end
+
let(:user) { create(:user) }
let(:token) do
JWT.encode(
{ user_id: user.id, status: 'active', active_until: 1.year.from_now.iso8601 },
ENV['JWT_SECRET_KEY'],
'HS256'
)
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/swagger/api/v1/subscriptions_controller_spec.rb` around lines 29 - 35,
The test's JWT token uses ENV['JWT_SECRET_KEY'] without setting it, so make it
deterministic by stubbing the ENV key before the token is generated: add a setup
like stub_const('ENV', ENV.to_hash.merge('JWT_SECRET_KEY' => 'test_jwt_secret'))
(or equivalent) in the spec so the let(:token) that calls JWT.encode uses a
known secret; ensure the stub appears before the let(:token) reference so
JWT.decode in the code under test will match.
| dominant_mode_emoji: { type: :string, nullable: true, | ||
| description: 'Emoji for transportation mode' } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Misaligned continuation line — same indentation issue as in the insights spec.
Line 56 starts at column 0 but is a continuation of line 55.
Suggested fix
- dominant_mode_emoji: { type: :string, nullable: true,
-description: 'Emoji for transportation mode' }
+ dominant_mode_emoji: { type: :string, nullable: true,
+ description: 'Emoji for transportation mode' }As per coding guidelines, **/*.rb: Use two-space indentation.
📝 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.
| dominant_mode_emoji: { type: :string, nullable: true, | |
| description: 'Emoji for transportation mode' } | |
| dominant_mode_emoji: { type: :string, nullable: true, | |
| description: 'Emoji for transportation mode' } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/swagger/api/v1/tracks_controller_spec.rb` around lines 55 - 56, The
continuation of the hash for the schema field `dominant_mode_emoji` is
misaligned (starts at column 0); fix the indentation to use two spaces and align
the continuation line with the start of the hash on the previous line in
spec/swagger/api/v1/tracks_controller_spec.rb so the block reads with consistent
two-space indentation for the `dominant_mode_emoji: { type: :string, nullable:
true, description: 'Emoji for transportation mode' }` entry.
| - name: lat | ||
| in: query | ||
| format: float | ||
| required: true | ||
| description: Latitude coordinate to search near | ||
| schema: | ||
| type: number | ||
| - name: lon | ||
| in: query | ||
| format: float | ||
| required: true | ||
| description: Longitude coordinate to search near | ||
| schema: | ||
| type: number |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the specified lines
if [ -f "swagger/v1/swagger.yaml" ]; then
echo "=== Lines 953-966 of swagger/v1/swagger.yaml ==="
sed -n '953,966p' swagger/v1/swagger.yaml
echo ""
echo "=== Context (lines 950-970) ==="
sed -n '950,970p' swagger/v1/swagger.yaml
else
echo "File not found"
find . -name "swagger.yaml" -o -name "swagger.yml" 2>/dev/null
fiRepository: Freika/dawarich
Length of output: 1039
🌐 Web query:
OpenAPI 3.x parameter format location specification query parameter schema validation
💡 Result:
In OpenAPI 3.x, a parameter’s location is specified by the Parameter Object field in, whose allowed values are query, path, header, and cookie (a parameter is uniquely identified by name + in). [1]
For query parameters, OpenAPI defines two separate concerns:
-
Serialization / parsing (wire format in the URL)
Controlled bystyle(default forin: queryisform),explode(defaulttrueforform), and optionallyallowReserved(query-only). These govern how arrays/objects turn into?a=1&a=2vs?a=1,2, etc. [1][2] -
Schema validation (the value after parsing)
Controlled by the parameter’sschema(or alternativelycontent, but you must use eitherschemaorcontent, not both). [1][2]
The schema’sformatis not a “location” setting—it’s a Schema Object annotation/constraint applied to the parsed value (e.g.,type: string,format: uuid). [1]
Version differences that affect validation semantics:
- OpenAPI 3.0.x:
schemais an “extended subset” of JSON Schema (Wright Draft 00), so not all modern JSON Schema keywords/behaviors apply. [3] - OpenAPI 3.1.x:
schemaaligns with JSON Schema 2020-12 via the OpenAPI 3.1 schema dialect (i.e., full JSON Schema features are available by default). [2][4]
Move format inside schema for query parameters.
In OpenAPI 3.x, format must be part of the Schema Object, not at the parameter level. Lines 955 and 962 incorrectly place format on the parameter object itself. Per the OpenAPI 3.x specification, the Parameter Object's schema is defined via the schema property, and schema-level constraints like format belong inside that schema, not at the parameter level. This structure violation can cause validation failures in tooling.
Suggested fix
- name: lat
in: query
- format: float
required: true
description: Latitude coordinate to search near
schema:
type: number
+ format: float
- name: lon
in: query
- format: float
required: true
description: Longitude coordinate to search near
schema:
type: number
+ format: float🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@swagger/v1/swagger.yaml` around lines 953 - 966, The OpenAPI parameters "lat"
and "lon" currently place format at the parameter level; move each format into
its schema by editing the parameter objects for lat and lon so their schema
includes both type: number and format: float (i.e., inside the schema block for
the parameters named "lat" and "lon" update the schema to contain type: number
and format: float and remove the top-level format fields).
| '200': | ||
| description: photo found | ||
| '404': | ||
| description: photo not found | ||
| content: | ||
| application/json: | ||
| image/jpeg: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| id: | ||
| type: string | ||
| latitude: | ||
| type: number | ||
| format: float | ||
| longitude: | ||
| type: number | ||
| format: float | ||
| localDateTime: | ||
| type: string | ||
| format: date-time | ||
| originalFileName: | ||
| type: string | ||
| city: | ||
| type: string | ||
| state: | ||
| type: string | ||
| country: | ||
| type: string | ||
| type: | ||
| type: string | ||
| enum: | ||
| - IMAGE | ||
| - VIDEO | ||
| - image | ||
| - video | ||
| - raw | ||
| - live | ||
| - animated | ||
| orientation: | ||
| error: | ||
| type: string | ||
| enum: | ||
| - portrait | ||
| - landscape | ||
| source: | ||
| description: Error message | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| error: | ||
| type: string | ||
| enum: | ||
| - immich | ||
| - photoprism | ||
| '404': | ||
| description: photo not found | ||
| description: Error message |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n swagger/v1/swagger.yaml | sed -n '1650,1680p'Repository: Freika/dawarich
Length of output: 1122
🏁 Script executed:
cat -n swagger/v1/swagger.yaml | sed -n '1620,1680p'Repository: Freika/dawarich
Length of output: 2201
Fix thumbnail error response media type contract.
The /api/v1/photos/{id}/thumbnail endpoint's OpenAPI specification contradicts its documented behavior. The description states "On success returns binary image/jpeg data. On error returns JSON with error details," but the schema defines 404 errors with image/jpeg media type containing a JSON object. The 200 response also lacks a content definition despite the description promising binary image data.
Update the specification to add binary image/jpeg content to the 200 response and remove the incorrect image/jpeg media type from the 404 response, keeping only application/json:
Suggested fix
responses:
'200':
description: photo found
+ content:
+ image/jpeg:
+ schema:
+ type: string
+ format: binary
'404':
description: photo not found
content:
- image/jpeg:
- schema:
- type: object
- properties:
- error:
- type: string
- description: Error message
application/json:
schema:
type: object
properties:
error:
type: string
description: Error message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@swagger/v1/swagger.yaml` around lines 1658 - 1676, The OpenAPI for the
/api/v1/photos/{id}/thumbnail endpoint is inconsistent: add a content block for
the '200' response with media type image/jpeg and schema type: string, format:
binary to represent binary image data, and remove the incorrect image/jpeg media
type from the '404' response so the '404' only has application/json with a
schema object containing an error string (retain the error property
description); update the response blocks in swagger.yaml accordingly.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Documentation