Repository navigation
Conversation
Map authors can enable an "Auto-zoom minimap to play region" option. When set, the guess minimap zooms to the map's location bounds at the start of each round instead of the default world view. Bounds are precomputed at ingest time (min/max lat/lng over the map's locations) and delivered to the client through MatchConfig, so no answer locations are exposed and no per-round scan is required.
Add unit coverage for the play-region bounds helper (e7-to-degree conversion and the disabled/missing-bound guards), source assertions that ingest computes bounds and the match plan applies them, and frontend tests for deriving autoZoomBounds from the match config.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds configurable play-region auto-zooming: map bounds are stored and computed during ingestion, exposed through match configuration, editable in map metadata, and applied by the guess minimap with antimeridian-aware bounds. ChangesPlay-region auto-zoom
Sequence Diagram(s)sequenceDiagram
participant MapEditor
participant MapAPI
participant MatchPlanner
participant HomePageGame
participant GuessMap
MapEditor->>MapAPI: save autoZoomPlayRegion
MatchPlanner->>MapAPI: read setting and stored bounds
MapAPI-->>MatchPlanner: return play-region configuration
MatchPlanner->>HomePageGame: provide match configuration
HomePageGame->>GuessMap: pass autoZoomBounds
GuessMap->>GuessMap: fit bounds in guess mode
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/features/maps/lib/maps-client.test.ts (1)
153-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the enabled payload path as well.
This assertion only verifies the default-false case. Add a request with
autoZoomPlayRegion: trueand assert thatupdateMapsendstrue, covering the path used byMapEditMetadataModal.tsxLine [39].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/features/maps/lib/maps-client.test.ts` around lines 153 - 158, Extend the map update payload tests around the existing autoZoomPlayRegion assertion to include an enabled case. Submit a request with autoZoomPlayRegion set to true and verify updateMap sends true, while preserving the existing false-case coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/features/maps/lib/maps-client.ts`:
- Line 68: Update MapUpdateInput and its serialization path around
autoZoomPlayRegion so omitted updates preserve the stored setting instead of
becoming explicit false. Either make autoZoomPlayRegion required and update
every caller, or implement a tri-state representation that distinguishes
omission from false and propagates that distinction through the map update flow.
In `@db/migrations/000054_map_auto_zoom_play_region.up.sql`:
- Around line 7-21: Replace the longitude aggregation in the migration’s maps
backfill with the same longitude-aware wrapped-bounds algorithm used during
location ingest, so antimeridian-crossing locations produce narrow bounds. Reuse
the existing ingest-time representation or leave bounds unset for unsupported
cases; do not persist independent min(lng_e7)/max(lng_e7) values.
In `@pkg/persistence/map_ingest.go`:
- Around line 129-137: Represent longitude bounds as the shortest circular
interval rather than independent MIN/MAX values: update the official-map ingest
at pkg/persistence/map_ingest.go:129-137 and custom-map ingest at
pkg/persistence/map_ingest.go:251-259, using a consistent wrapped-interval
convention and recomputing existing bounds. In
apps/web/components/GuessMap.tsx:155-161, detect that convention, unwrap one
endpoint before calculating the reference longitude, and pass wrapped corners to
Leaflet; add an antimeridian regression case covering the behavior.
---
Nitpick comments:
In `@apps/web/features/maps/lib/maps-client.test.ts`:
- Around line 153-158: Extend the map update payload tests around the existing
autoZoomPlayRegion assertion to include an enabled case. Submit a request with
autoZoomPlayRegion set to true and verify updateMap sends true, while preserving
the existing false-case coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 003e7387-a761-469d-a4ba-72a69281a832
📒 Files selected for processing (18)
apps/web/components/GuessMap.tsxapps/web/components/ui/types.tsapps/web/features/home/model/derive-home-model.test.tsapps/web/features/home/model/derive-home-model.tsapps/web/features/home/model/types.tsapps/web/features/home/page/HomePageGame.tsxapps/web/features/lobby/components/MapMetadataFields.tsxapps/web/features/lobby/components/maps/MapEditMetadataModal.tsxapps/web/features/maps/lib/maps-client.test.tsapps/web/features/maps/lib/maps-client.tsdb/migrations/000054_map_auto_zoom_play_region.down.sqldb/migrations/000054_map_auto_zoom_play_region.up.sqlpkg/contracts/contracts.gopkg/persistence/map_ingest.gopkg/persistence/map_match_plans.gopkg/persistence/map_match_plans_test.gopkg/persistence/map_scan.gopkg/persistence/maps.go
| difficulty: CustomMap["difficulty"]; | ||
| thumbnailKey: string; | ||
| thumbnailVariant?: number; | ||
| autoZoomPlayRegion?: boolean; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve the setting when the update input omits it.
MapUpdateInput.autoZoomPlayRegion is optional, but Line [134] serializes omission as explicit false. Since pkg/persistence/map_ingest.go Lines [278-305] unconditionally persist the received value, callers updating unrelated metadata can silently disable auto-zoom. Make the field required and update all callers, or use a tri-state contract that preserves the stored value when omitted.
Also applies to: 134-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/features/maps/lib/maps-client.ts` at line 68, Update MapUpdateInput
and its serialization path around autoZoomPlayRegion so omitted updates preserve
the stored setting instead of becoming explicit false. Either make
autoZoomPlayRegion required and update every caller, or implement a tri-state
representation that distinguishes omission from false and propagates that
distinction through the map update flow.
Comment on lines
+129
to
+137
| if _, err := tx.Exec(ctx, ` | ||
| update maps set status='ready',location_count=$2, | ||
| bounds_min_lat_e7=(select min(lat_e7) from locations where map_storage_id=$3), | ||
| bounds_max_lat_e7=(select max(lat_e7) from locations where map_storage_id=$3), | ||
| bounds_min_lng_e7=(select min(lng_e7) from locations where map_storage_id=$3), | ||
| bounds_max_lng_e7=(select max(lng_e7) from locations where map_storage_id=$3), | ||
| updated_at=now() | ||
| where id=$1 | ||
| `, mapID, len(parsed), mapStorageID); err != nil { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Represent antimeridian bounds as a shortest circular interval. MIN/MAX turns locations near +/-180° into an almost-worldwide interval; Line 157 then centers it on 0°, so fitBounds zooms out instead of fitting the play region.
pkg/persistence/map_ingest.go#L129-L137: derive and store the shortest longitude arc for official maps, including a wrapped-interval convention.pkg/persistence/map_ingest.go#L251-L259: apply the same derivation for custom-map ingest.apps/web/components/GuessMap.tsx#L155-L161: detect the wrapped convention, unwrap one endpoint before computing the reference longitude, then pass wrapped corners to Leaflet.
Also recompute existing bounds and add an antimeridian regression case.
📍 Affects 2 files
pkg/persistence/map_ingest.go#L129-L137(this comment)pkg/persistence/map_ingest.go#L251-L259apps/web/components/GuessMap.tsx#L155-L161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/persistence/map_ingest.go` around lines 129 - 137, Represent longitude
bounds as the shortest circular interval rather than independent MIN/MAX values:
update the official-map ingest at pkg/persistence/map_ingest.go:129-137 and
custom-map ingest at pkg/persistence/map_ingest.go:251-259, using a consistent
wrapped-interval convention and recomputing existing bounds. In
apps/web/components/GuessMap.tsx:155-161, detect that convention, unwrap one
endpoint before calculating the reference longitude, and pass wrapped corners to
Leaflet; add an antimeridian regression case covering the behavior.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/persistence/map_match_plans_test.go (1)
53-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSource-text substring assertions are brittle regression guards.
TestMapIngestComputesPlayRegionBoundsreadsmap_ingest.goas text and greps for exact call-site strings and a hardcoded positional parameter (auto_zoom_play_region=$9). This passes today but breaks on any harmless refactor (renaming the variable, reordering SQL parameters, reformatting the call) even when behavior is unchanged, and it can't detect a subtly wrong implementation that still contains the right substrings. Prefer a behavior-driven test (e.g., invokingcomputePlayRegionBoundsE7/UpdateCustomMapdirectly, or a pgxmock-based check of the executed query) over parsing source as text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/persistence/map_match_plans_test.go` around lines 53 - 65, The TestMapIngestComputesPlayRegionBounds test should stop reading map_ingest.go and asserting source-text substrings. Replace it with a behavior-driven test that exercises computePlayRegionBoundsE7 and the relevant UpdateCustomMap/create-or-import flow, using real persistence setup or pgxmock to verify the computed bounds are passed and persisted without depending on SQL placeholder positions or call formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/persistence/map_match_plans_test.go`:
- Around line 53-65: The TestMapIngestComputesPlayRegionBounds test should stop
reading map_ingest.go and asserting source-text substrings. Replace it with a
behavior-driven test that exercises computePlayRegionBoundsE7 and the relevant
UpdateCustomMap/create-or-import flow, using real persistence setup or pgxmock
to verify the computed bounds are passed and persisted without depending on SQL
placeholder positions or call formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f202917-f913-4828-a267-1fd034e8c2f4
📒 Files selected for processing (8)
apps/web/components/GuessMap.tsxapps/web/components/guess-map-bounds.test.tsapps/web/components/guess-map-bounds.tsapps/web/features/maps/lib/maps-client.test.tsapps/web/features/maps/lib/maps-client.tsdb/migrations/000054_map_auto_zoom_play_region.up.sqlpkg/persistence/map_ingest.gopkg/persistence/map_match_plans_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- db/migrations/000054_map_auto_zoom_play_region.up.sql
- apps/web/components/GuessMap.tsx
Owner
|
Hi there, thanks for the PR. While reviewing it I noticed a bug with the map upload flow, so I committed a fix that is required for db to function properly. Please fix conflicts before I can proceed. |
…p-play-region # Conflicts: # pkg/persistence/maps.go
This branch has not been deployed
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
2 participants
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.
Adds an opt-in per-map setting that lets map authors have the guess minimap automatically zoom to the map's play region at the start of each round, instead of defaulting to a fully zoomed-out world view. This is useful for regional/country maps where the world view forces players to zoom in manually every round.
When enabled, the minimap fits to the geographic bounds (min/max latitude and longitude) of the map's locations when a round begins.
How it works:
Summary by CodeRabbit
New Features
Bug Fixes
Tests