Contribution: 5, Week 5, Issue: "initial view state is confusing and set to different values in multiple files"
Contribution Number: [8 (for the whole document, issue two below)]
Student: Suhrudh Chivukula
Issue: [[GitHub issue link]] (https://github.com/SuhrudhC/tenantfirstaid_codepath/commit/ae78a5c661f1b5bc74c03a7bee9baac481ba8a5c)
Status: [Phase 4] [Complete]
I chose this issue because it addresses application state management and map initialization, offering an excellent opportunity to learn how applications are built and how state is managed across modules in a frontend project. Tracking down variable declarations and understanding the map's components aligns perfectly with my goal of improving my debugging and code-tracing skills in larger, more valuable codebases.
Additionally, this is marked as a "good first issue" and "help wanted," which means it is an accessible entry point. It still provides a tangible improvement to the user interface, and fixing it will help me get familiar with openSenseMap's contribution workflow while improving the initial user experience.
The default coordinates and zoom level for the map's initial load are defined in multiple different files with conflicting values. This lack of a "single source of truth" in the application's configuration causes the map framework to initialize using default variables and leaves the user looking at a blank patch of ocean.
When a user first loads the application, the map should read its default viewport state from a single, centralized configuration file.
Upon initial load, the map incorrectly centers itself in the middle of the Indian Ocean. This buggy behavior was specifically reproduced and reported on an iPhone 13 mini running iOS 18.7.3 in the Safari browser.
[Which parts of the codebase are involved?]
-
The primary map component/view (where the map instance is initialized).
-
Global state management stores or context files where the view state is held.
-
Configuration or constants files where default latitude, longitude, and zoom levels are defined.
Standard README flow (.env copy, uv sync, npm install), but three snags. No Google Cloud access (granted via the project Discord), so I used CI's forked-repo trick — fake GOOGLE_* env vars and pytest -m "not require_repo_secrets". Every test also failed with a misleading AttributeError: module 'evaluate' has no attribute 'run_langsmith_evaluation'; real cause was ALL_PROXY=socks5h://... set without socksio installed, breaking httpx during a conftest import — fixed by unsetting the proxy vars. Lastly, git push fails (no GitHub creds in this shell); branch is committed locally and needs a push from an authenticated terminal.
- Load the app with the LLM and email sender mocked.
- Burst POST /api/query (20x) and POST /api/feedback (5x), resetting the limiter between.
- Observed: all 20 /api/query return 200, zero 429s; /api/feedback returns 200 x3 then 429. Limiter works on feedback, not on the costly query endpoint.
- Commit showing reproduction: [Link to commit in your fork]
- Screenshots/logs: /api/query x20 -> {200: 20} (0 of 429); /api/feedback x5 -> {200: 3, 429: 2}. Bug reproduced 2/2 trials.
- My findings: The Limiter has no default limits, so only routes that opt in are throttled — and only /api/feedback does. /api/query, the one endpoint that costs money per call (Vertex AI RAG + Gemini), was never given a limit.
Root cause is in backend/tenantfirstaid/app.py: the Limiter has no default_limits (lines 20–24), /api/feedback opts in (lines 58–68), but /api/query is registered plain (line 55) with no limit. Not a break — the limit was just never wired to the expensive endpoint.
Wrap the class-based view the same way feedback is wrapped: limiter.limit("10 per minute")(ChatView.as_view("chat")). Confirm the exact number with mentors (possibly env-configurable), and verify production keys off the real client IP (X-Forwarded-For/ProxyFix) behind the DO proxy so traffic doesn't share one bucket.
Using UMPIRE framework (adapted):
Understand: /api/query hits Vertex AI RAG + Gemini per call and has no limit, risking cost/availability abuse. It should 429 over a per-IP budget like /api/feedback. Login is out of scope (anonymous tenant tool).
Match: Near-copy of the feedback limit (app.py:58–68) and its test test_rate_limiting_returns_429 (test_app.py:91–101).
Plan: [Step-by-step implementation plan]
- Pick the limit
- wrap ChatView.as_view("chat")
- Add 429 + within-limit tests
- Run make check and re-run the repro
- Optional Turnstile/hCaptcha only if needed.
Implement: Phase III on branch feature-rate-limit-api-query. PR link: TBD
Review: No CONTRIBUTING.md; follow the PR template (type = Bug Fix, Closes #, tests = Yes) and pr-check.yml checks (ruff, ty, pytest). Branch follows the feature-* convention.
Evaluate: Done when the new 429 and within-limit tests pass, make check is green, and the repro flips from zero 429s to throttled.
Test case 1 — Boundary (10th succeeds, 11th fails) I tested the exact edge of the limit rather than just "over" it. The 10th request in a window must return 200 and only the 11th must return 429. This matters because an off-by-one error in flask_limiter's counter — or in how I applied it — could block legitimate users one request too early. The test burns through 9 requests, asserts the 10th is still 200, then asserts the 11th is 429.
Test case 2 — Per-IP isolation The limiter has to key off the client's IP address, not a single global counter. If it were global, the first person to hit the limit would lock out every other user on the site simultaneously. I tested this by exhausting IP 10.0.0.1's quota (11 requests), confirming it gets 429, then immediately sending a request from IP 10.0.0.2 and asserting it still gets 200. Flask's test client accepts environ_base={"REMOTE_ADDR": "..."} to simulate different source addresses.
Test case 3 — Window reset After the limit fires, the quota should refill when the time window rolls over. I simulated this by calling limiter.reset() (which clears all counters, the same effect as a new window opening) after exhausting the quota, then confirming the next request succeeds again. Without this test, a bug that permanently blocks an IP after one abuse incident would go undetected.
Test case 4 — Regression: the two endpoints are independent I tested both directions: exhausting /api/query's 10-per-minute quota should not consume any of /api/feedback's separate 3-per-minute quota, and vice versa. This catches a class of bug where a shared counter or shared storage key would cause the two limits to bleed into each other. Both the existing feedback test and two new cross-endpoint tests confirm the counters stay isolated.
Test case 5 — Env-var wiring The limit value is read from os.environ at startup into a module-level constant called QUERY_RATE_LIMIT. The test imports that constant and asserts it matches os.getenv("QUERY_RATE_LIMIT", "10 per minute"). This verifies the indirection is actually in place — if someone hard-coded the value instead of reading the env var, this test would catch it.
- Integration scenario 1 - I ran all 10 allowed requests in a window through the real Flask routing stack with the LLM mocked, asserting every one returns 200 with mimetype == "text/plain". This confirms the rate limiter doesn't interfere with normal chat traffic — the streaming response, the stream_with_context wrapper, and the limiter all coexist without the Flask request context getting corrupted. (This was the source of a subtle failure during development: if you don't drain the streaming response body with resp.get_data() before making the next request, Flask raises "Popped wrong request context" because the generator still holds a reference to the old context.)
- Integration scenario 2 - When the limit fires, the response needs to be well-formed enough for the frontend and any proxy layer to handle it correctly. I checked that the 429 body is non-empty and that the content type is text/html. One thing this surfaced: flask_limiter does not emit a Retry-After header by default in the version this project uses — that requires headers_enabled=True on the Limiter constructor. That's a separate improvement worth adding but is outside the scope of this fix, and the test documents it explicitly so it isn't forgotten.
I ran the reproduction script (backend/repro_rate_limit.py) before and after the fix against the live Flask app with mocks. Before: /api/query x20 → {200: 20}, zero 429s in both trials — the bug. After: /api/query x20 → {200: 10, 429: 10}, confirmed in both trials — the fix. The server logs also changed visibly: before, the flask-limiter: ratelimit exceeded at endpoint: chat log line never appeared; after the fix, it fires exactly 10 times per trial. I also ran make --keep-going check (ruff format, ruff lint, ty typecheck, pytest) to confirm nothing else broke — all checks passed except one pre-existing test collection failure in test_measure_evaluator_variance.py caused by a missing socksio package under a SOCKS proxy environment, which is unrelated to this change.
The first thing I did was understand exactly what was broken before touching any code. I ran a reproduction script that fired 20 requests at /api/query and 5 at /api/feedback against the real Flask app with the LLM and email sender mocked out. The results were clear: every single /api/query request came back 200, zero rejections, while /api/feedback correctly throttled after 3. That confirmed the bug was real and gave me a baseline to flip once the fix was in.
From there I traced the behavior to a single gap in backend/tenantfirstaid/app.py. The flask_limiter library was already imported and a Limiter object already existed — the infrastructure was there. The problem was that when /api/query was registered on line 55, it was handed to Flask as a bare class-based view with no limit attached. The only route that ever called limiter.limit(...) was /api/feedback on line 58. So the fix wasn't about adding a new library or changing architecture — it was about connecting the existing tool to the endpoint that needed it.
The actual code change was two lines in app.py: capture the result of ChatView.as_view("chat"), wrap it with limiter.limit(QUERY_RATE_LIMIT), then pass that wrapped view to add_url_rule. I also made the limit value read from an environment variable (QUERY_RATE_LIMIT) rather than hard-coding "10 per minute", so the budget can be tuned in production without pushing new code. After the fix, the repro script flipped from {200: 20} to {200: 10, 429: 10} in both trials.
[Continue documenting as you work]
- Files modified: backend/tenantfirstaid/app.py — Core fix. Two lines changed: wrap ChatView.as_view("chat") with limiter.limit(QUERY_RATE_LIMIT) before passing it to add_url_rule, and read the limit value from the QUERY_RATE_LIMIT environment variable so it's tunable without a redeploy. backend/tests/test_app.py — Added test_query_within_limit_returns_200 and test_query_rate_limiting_returns_429 to the existing TestQueryRoute class, modeled directly on the feedback endpoint's existing rate-limit test. backend/.env.example — Documented QUERY_RATE_LIMIT=10 per minute so contributors and operators know the variable exists and what format it takes (flask_limiter syntax). backend/tests/test_query_rate_limit.py — New file with 13 tests across 6 categories covering boundary, per-IP isolation, window reset, env-var wiring, regression, and response shape.
- Key commits: ae78a5c — Reproduction script and solution plan. Documents the bug with evidence before touching any fix code. 70185c2 — The actual fix: rate limit applied to /api/query, two tests added, env var documented. 266b541 — Comprehensive verification suite confirming the fix holds across all meaningful scenarios.
- Approach decisions: I chose to wrap the view function at registration time (limiter.limit(...)(ChatView.as_view("chat"))) rather than putting a decorators class attribute on ChatView itself, because the limit value comes from an env var resolved at startup and this keeps the configuration centralized in app.py alongside the feedback limit — a reader can see both limits in the same file without jumping into the view class. I chose 10 per minute as the default because a real conversation involves a message every 10–30 seconds at most, so 10 per minute is generous for humans but a hard wall for scripts firing as fast as the network allows.
PR Link: github.com/codeforpdx/tenantfirstaid/pull/376
PR Description: /api/query is the most expensive thing this app does — every request fans out to Vertex AI RAG plus a Gemini completion. Right now it has no rate limit at all, even though flask_limiter is already set up and used on /api/feedback. Anyone (or any script) can hit it as fast as they want, which is a real cost and availability risk.
This PR wires the existing limiter up to /api/query the same way it's already used on /api/feedback. It defaults to 10 requests per minute per IP, which should be plenty for a normal back-and-forth conversation but stops a script from hammering the endpoint. The limit is also configurable through a QUERY_RATE_LIMIT env var, so it can be tuned in production without a code change if it turns out to be too tight or too loose.
I also wrote a small script (backend/repro_rate_limit.py) that reproduces the bug by bursting both endpoints and comparing results — before the fix /api/query returned 200 for all 20 requests with zero rejections, and after the fix it correctly starts returning 429s past the 10th request.
Closes #168. Tests: 2 new tests in test_app.py, plus 13 new tests in test_query_rate_limit.py covering the boundary, per-IP isolation, window reset, env-var wiring, and regression against the feedback endpoint's existing limit.
Maintainer Feedback:
- No feedback yet — PR was just opened. CI checks are running against it. Will fill out the form below when feedback is received.
- [Date]: [Summary of feedback received]
- [Date]: [How you addressed it]
Status: Awaiting review
I got hands-on with flask_limiter for the first time. Specifically, the gotcha is that wrapping a class-based view (ChatView.as_view(...)) with limiter.limit() must occur before the view is registered with add_url_rule, since the decorator returns a new wrapped function rather than mutating the view in place. I also learned that flask_limiter keys its counters by whatever the key_func returns (here, get_remote_address), which is what makes per-IP isolation work, and that testing that isolation requires faking REMOTE_ADDR through Flask's test client (environ_base={"REMOTE_ADDR": "..."}) rather than just hitting the endpoint normally. On the testing side, I learned that streaming Flask responses (stream_with_context) need their body fully drained with resp.get_data() in a test before the next request, or Flask throws a confusing "Popped wrong request context" error, which cost me real debugging time before I traced it to the actual cause.
The single biggest time-sink wasn't the fix itself; it was getting the test environment running at all. Every test failed with AttributeError: module 'evaluate' has no attribute 'run_langsmith_evaluation', which sounded like a totally unrelated import problem. After digging through the traceback, I found the real cause buried underneath: my sandbox had ALL_PROXY set to a SOCKS proxy, but the socksio package wasn't installed, so building an httpx client during an autouse pytest fixture failed silently and surfaced as that misleading attribute error several layers up. Unsetting the proxy env vars fixed it. The second real challenge was the streaming-response test context bug mentioned above, easy to miss because it only shows up when you loop multiple requests against a streaming endpoint without draining each response first.
I'd check for proxy/network env vars in my environment before touching the test suite, since that one red herring ate more time than the actual root-cause investigation of the bug itself. I'd also reproduce the bug with a dedicated script even earlier in the process than I did. Having that script ready meant the "did the fix actually work" question at the end was a 30-second rerun instead of a fresh investigation, and I'd want that safety net in place from minute one next time, not partway through. Lastly, I'd ask upfront whether GitHub push access would be available in my working environment, since that turned into a multi-step detour (trying gh, checking keychain, eventually using a scoped PAT) that could have been resolved earlier if I'd surfaced the question before starting Phase III instead of after.
- flask-limiter documentation — reference for the library already in use in this codebase
- .github/workflows/pr-check.yml in this repo — showed me how CI handles missing GCP credentials for forked PRs, which is what unblocked running the test suite locally without real Google Cloud access
- Issue #168 and the maintainer's re-scoping comment on it — clarified that the issue's original framing (auth/session endpoints) was stale and the real ask was just rate limiting /api/query
- The existing /api/feedback rate-limit implementation and its test in test_app.py — used directly as the template/pattern for both the fix and the new tests, rather than designing something from scratch
The PR has received its first review (July 1) from maintainer yangm2, who left two pieces of feedback. First, they flagged SOLUTION_PLAN.md in the repo root — that's a working notes file from the planning phase that shouldn't have been committed to the PR and needs to be removed. Second, they noted that backend/repro_rate_limit.py is a useful script worth keeping, but belongs in backend/scripts/ alongside the other utility scripts rather than sitting loose in the backend root. No feedback on the actual rate-limit logic or tests — just cleanup on the two extra files. Both changes are straightforward to address: delete SOLUTION_PLAN.md, move the repro script to backend/scripts/, and push an update to the branch.
Fully finished the PR merge for the previous issue, moving on to issue 2.
Contribution 5, Week 5 — Issue: "initial view state is confusing and set to different values in multiple files"
Contribution Number: 5
Student: Suhrudh Chivukula
Issue: #706 — initial view state is confusing and set to different values in multiple files
Status: Phase 3 — Implementation In Progress
Why I Chose This Issue I chose this issue because it involves tracking down a subtle but impactful coordinate transposition bug across multiple files, which is exactly the kind of cross-file debugging exercise that builds real code-tracing fluency. Understanding how a map's view state is initialized, passed through a Remix route, and potentially overridden by a helper module requires reading the data flow end-to-end rather than patching a single line — a skill that transfers directly to larger, more complex frontend codebases.
Additionally, this is marked as a "good first issue" and "help wanted," making it a well-scoped entry point into the openSenseMap contribution workflow. The fix delivers a concrete improvement to the very first thing a new user sees when they open the app, and consolidating scattered magic numbers into a single constants file is a clean, reviewable change that's easy to reason about in a pull request.
Understanding the Issue The default latitude, longitude, and zoom level for the map are defined independently in at least three separate files with no shared source of truth, and one of those definitions has the latitude and longitude values transposed. Because the explore route overrides the base map component's initialViewState with its own hardcoded values, the map framework initializes using the route-level coordinates — which happen to point to the Indian Ocean rather than the intended default of central Europe.
Expected Behavior When a user first loads the explore view, the map should derive its initial viewport from a single centralized constants file, displaying a sensible default region (central Europe) at a comfortable zoom level.
Current Behavior On initial load, the map centers itself in the Indian Ocean because explore.tsx defines initialViewState as { latitude: 7, longitude: 52, zoom: 2 } — the latitude and longitude are swapped relative to the intended location near Münster, Germany (latitude: 52, longitude: 7). This behavior was specifically reproduced on an iPhone 13 mini running iOS 18.7.3 in the Safari browser.
Affected Components app/routes/explore.tsx — the explore route where initialViewState is defined with transposed lat/lng values (latitude: 7, longitude: 52 instead of latitude: 52, longitude: 7), directly causing the Indian Ocean centering bug.
app/components/map/map.tsx — the base map component, which defines its own separate initialViewState (latitude: 51.961563, longitude: 7.628202, zoom: 2) that diverges from the route-level defaults and contributes to the lack of a single source of truth.
app/lib/search-map-helper.ts — the navigation helper module, which defines a third independent default (center: [0, 0], zoom: 1) for its zoomOut function and would also need to reference the shared constants after the fix.
Reproduction Process Environment Setup Prerequisites:
Node 24 (verified in package.json engines field; Node 24.1.0 is installed locally) Docker (for PostgreSQL via TimescaleDB, MailHog, and RustFS file storage) npm 11+ Steps:
git clone https://github.com/openSenseMap/frontend.git cd frontend cp .env.example .env # all defaults work for local dev docker compose up -d # starts postgres:5432, mailhog:1025, rustfs:9000 npm install npm run db:push # applies Drizzle schema to local postgres npm run dev # starts Vite dev server on localhost:5173 Navigate to http://localhost:5173/explore with no URL hash present. To simulate the iOS Safari trigger, first visit any route with an anchor hash (e.g. http://localhost:5173/#about) and then navigate to /explore — this leaves a stale non-map hash in window.location.hash that MapLibre tries and fails to parse.
Steps to Reproduce Open the app in a browser that preserves hash fragments across navigations (iOS Safari 18.7.x, or simulate in any browser by manually appending a non-map hash such as #about to the URL before navigating to /explore). Navigate to /explore. Observe the map's initial center position. Alternatively, open app/routes/explore.tsx and app/components/map/map.tsx side by side to see two divergent coordinate definitions with no shared origin. Reproduction Evidence Code-level confirmation (current main branch, commit 63a4d6c):
File Default coordinates Zoom Status app/routes/explore.tsx:57-61 lat 51.961563, lng 7.628202 2 Correct, but isolated app/components/map/map.tsx:18-22 lat 51.961563, lng 7.628202 2 Duplicate of above app/components/device/new/location-info.tsx:106-110 lat 51, lng 7 3.5 Divergent (different zoom, rounded coords) app/lib/search-map-helper.ts:14-20 [0, 0] 1 Wrong (Gulf of Guinea) The bug mechanism: map.tsx:24 passes hash={true} to MapLibre, which tells MapLibre to read window.location.hash directly and use it as the initial view — overriding the React-supplied initialViewState. When the hash is a stale non-map fragment (like #about from a prior React Router navigation), MapLibre fails to parse it as its expected #zoom/lat/lng format. Rather than falling back to the supplied initialViewState, MapLibre on iOS Safari silently falls back to its internal default (center: [0, 0], zoom: 0), landing the map over open ocean. Meanwhile, explore.tsx:643 also independently parses location.hash via its own parseMapHash function — creating two competing hash-reading mechanisms with no coordination between them.
Solution Approach Analysis The issue has two distinct layers. The first is structural: there is no shared constant for the default map viewport, so the same coordinates are copy-pasted in at least four places with minor variations, making any future change error-prone and inconsistent. The second is behavioral: hash={true} in map.tsx delegates initial position to MapLibre's internal hash parser, while explore.tsx simultaneously runs its own parseMapHash function on the same location.hash string. These two mechanisms are redundant, and on iOS Safari the MapLibre-level reader can silently win with a bad fallback when the hash is not in the map's #zoom/lat/lng format.
Proposed Solution Create a single file, app/lib/map-defaults.ts, that exports one DEFAULT_MAP_VIEWPORT constant. Replace every inline coordinate definition across the codebase with an import of that constant. Separately, remove hash={true} from map.tsx to eliminate the competing MapLibre-native hash reader — explore.tsx already handles hash reading via parseMapHash and hash writing must be implemented there explicitly using an onMove callback that writes to the URL, keeping URL-sharing intact while putting hash behavior entirely under React's control.
Implementation Plan using UMPIRE Framework Understand:
The map's initial viewport is determined by a priority chain in explore.tsx:655-656: selectedDeviceView ?? hashView ?? homeView ?? INITIAL_VIEW_STATE. INITIAL_VIEW_STATE is defined locally at lines 57-61 and is the intended default for anonymous first-time visits. However, hash={true} in map.tsx:24 means MapLibre also reads window.location.hash independently before React's priority chain has any effect, creating a scenario where MapLibre can override the correct INITIAL_VIEW_STATE with a broken fallback on iOS Safari.
Match:
This is a classic "duplicated magic value" problem combined with a "competing initialization sources" problem. The structural fix (single source of truth) follows the standard constants-extraction pattern. The behavioral fix (removing the competing hash reader) follows the principle that one system should own one responsibility — React state should own the viewport, not split it with MapLibre's internal hash handler.
Plan:
Create app/lib/map-defaults.ts exporting DEFAULT_MAP_VIEWPORT with the canonical Münster coordinates (latitude: 51.961563, longitude: 7.628202, zoom: 2) typed as MapViewport from app/lib/location.ts. In explore.tsx: delete the local INITIAL_VIEW_STATE declaration, import DEFAULT_MAP_VIEWPORT, and replace its usage at line 571 (handleHomeClick) and line 656. In map.tsx: import DEFAULT_MAP_VIEWPORT, replace the inline ?? { longitude: 7.628202... } fallback, and remove hash={true}. Add an onMove callback in explore.tsx that writes the current viewport to the URL hash in #zoom/lat/lng format whenever the user pans or zooms, restoring URL-sharing behavior that hash={true} previously provided. In location-info.tsx:106-110: import DEFAULT_MAP_VIEWPORT and replace latitude: 51, longitude: 7, zoom: 3.5 with the canonical values. In search-map-helper.ts:16: import DEFAULT_MAP_VIEWPORT and replace center: [0, 0], zoom: 1 in zoomOut with center: [DEFAULT_MAP_VIEWPORT.longitude, DEFAULT_MAP_VIEWPORT.latitude], zoom: DEFAULT_MAP_VIEWPORT.zoom.
Implement:
New file: app/lib/map-defaults.ts
import type { MapViewport } from '~/lib/location'
export const DEFAULT_MAP_VIEWPORT = { latitude: 51.961563, longitude: 7.628202, zoom: 2, } as const satisfies MapViewport Modified files: explore.tsx, map.tsx, location-info.tsx, search-map-helper.ts — all substituting their inline coordinate literals with the imported constant. The onMove handler added to explore.tsx will call window.history.replaceState to write #zoom/lat/lng on every map move, replicating MapLibre's hash={true} behavior explicitly and safely.
Review:
Verify with grep -rn "51.961|7.6282|latitude: 51|longitude: 7|center.*0, 0" app/ that no additional inline coordinate literals remain outside the new constants file. Confirm that navigating from a non-map route (one with an unrelated anchor hash) to /explore now correctly centers on Münster instead of an ocean position. Check that copying the URL after panning the map and pasting it in a new tab still restores the correct position, confirming the manual onMove hash writer is working.
Evaluate:
The fix is complete when: (1) every map default in the codebase traces back to a single import, (2) the explore view centers on Münster on a fresh load regardless of any prior URL hash, and (3) URL hash-based position sharing still works as before. The change is isolated to five files, adds one new file, and introduces no new dependencies — making it a safe, reviewable pull request that matches the "good first issue" scope.
Testing Strategy Tests cover three layers: the new constants module, the exported parseMapHash utility, and the zoomOut navigation function. All tests use Vitest + jsdom, matching the project's existing config.
Test 1 — Default viewport values are geographically valid
Confirms DEFAULT_MAP_VIEWPORT falls within legal lat/lng bounds and that getValidMapViewport accepts it without returning null.
// app/lib/map-defaults.test.ts
import { describe, it, expect } from 'vitest'
import { DEFAULT_MAP_VIEWPORT } from '/lib/map-defaults'
import { getValidMapViewport, MAP_ZOOM_LIMITS, LOCATION_LIMITS } from '/lib/location'
describe('DEFAULT_MAP_VIEWPORT', () => { it('has a latitude within legal bounds', () => { expect(DEFAULT_MAP_VIEWPORT.latitude).toBeGreaterThanOrEqual(LOCATION_LIMITS.latitude.min) expect(DEFAULT_MAP_VIEWPORT.latitude).toBeLessThanOrEqual(LOCATION_LIMITS.latitude.max) })
it('has a zoom level within MapLibre limits', () => { expect(DEFAULT_MAP_VIEWPORT.zoom).toBeGreaterThanOrEqual(MAP_ZOOM_LIMITS.min) expect(DEFAULT_MAP_VIEWPORT.zoom).toBeLessThanOrEqual(MAP_ZOOM_LIMITS.max) })
it('is accepted by getValidMapViewport without returning null', () => { const result = getValidMapViewport(DEFAULT_MAP_VIEWPORT) expect(result).not.toBeNull() expect(result?.latitude).toBe(DEFAULT_MAP_VIEWPORT.latitude) }) }) Test 2 — parseMapHash handles valid hashes
Covers standard format, fractional zoom, and negative coordinates.
// app/routes/explore.test.ts import { parseMapHash } from '~/routes/explore'
describe('parseMapHash', () => { it('parses a standard map hash', () => { const result = parseMapHash('#2/51.961563/7.628202') expect(result?.zoom).toBe(2) expect(result?.latitude).toBeCloseTo(51.961563) })
it('parses negative coordinates', () => { const result = parseMapHash('#6/-33.868/151.209') expect(result?.latitude).toBeCloseTo(-33.868) }) }) Test 3 — parseMapHash rejects non-map hashes
This is the direct regression test for the iOS Safari bug. Any non-map fragment must return null so the app falls back to DEFAULT_MAP_VIEWPORT.
describe('parseMapHash — invalid inputs', () => { it('returns null for a plain anchor hash', () => { expect(parseMapHash('#about')).toBeNull() })
it('returns null for an empty string', () => { expect(parseMapHash('')).toBeNull() })
it('returns null for an out-of-range latitude', () => { expect(parseMapHash('#2/95.0/7.6')).toBeNull() }) }) Test 4 — zoomOut flies to DEFAULT_MAP_VIEWPORT, not [0, 0]
Mocks a MapRef and confirms flyTo uses the canonical default after the fix.
// app/lib/search-map-helper.test.ts
import { zoomOut } from '/lib/search-map-helper'
import { DEFAULT_MAP_VIEWPORT } from '/lib/map-defaults'
describe('zoomOut', () => { it('flies to DEFAULT_MAP_VIEWPORT, not [0, 0]', () => { const flyTo = vi.fn() zoomOut({ flyTo } as any) const call = flyTo.mock.calls[0][0] expect(call.center).toEqual([DEFAULT_MAP_VIEWPORT.longitude, DEFAULT_MAP_VIEWPORT.latitude]) expect(call.zoom).toBe(DEFAULT_MAP_VIEWPORT.zoom) }) }) Test 5 — writeMapHash and parseMapHash round-trip correctly
Ensures hash writing and reading are exact inverses, keeping URL sharing intact after removing hash={true}.
import { writeMapHash, parseMapHash } from '~/routes/explore'
describe('hash round-trip', () => { it('a written hash is correctly parsed back', () => { const viewport = { zoom: 5, latitude: 48.137, longitude: 11.576 } const parsed = parseMapHash(writeMapHash(viewport)) expect(parsed?.zoom).toBeCloseTo(viewport.zoom, 1) expect(parsed?.latitude).toBeCloseTo(viewport.latitude, 3) }) })
Integration Testing Scenarios Scenario 1 — Fresh load with no URL hash
Navigate to /explore from a route with a non-map anchor (e.g., /#hero). Before the fix, MapLibre's hash={true} picked up #hero, failed to parse it, and defaulted to [0, 0]. After the fix, the priority chain resolves to DEFAULT_MAP_VIEWPORT and the map centers on Münster. Verify with mapRef.current?.getCenter() in the console immediately after load.
Scenario 2 — URL hash restores a panned viewport
Pan the map to Tokyo (lat 35.68, lng 139.69) and confirm the URL hash updates to #8/35.68/139.69 via the onMove handler. Copy the URL, open it in a new tab, and verify the map initializes at Tokyo — confirming that manual hash writing and parseMapHash reading are working end-to-end as a replacement for hash={true}.
Manual Testing Load the explore page on a physical iOS device running Safari by pointing it at the local dev server. First visit any page with a non-map anchor, then tap "Explore" to trigger a client-side navigation that carries the stale hash forward. After the fix, the map should land on Münster at zoom 2 on every cold visit. Also confirm the "Home" toolbar button snaps back to that same location, and that the search component's zoom-out action no longer flies to open ocean — both now derive from the same DEFAULT_MAP_VIEWPORT constant.
Implementation New file — app/lib/map-defaults.ts
The single source of truth for the project's default map viewport. All other files import from here.
import type { MapViewport } from '~/lib/location'
export const DEFAULT_MAP_VIEWPORT = { latitude: 51.961563, longitude: 7.628202, zoom: 2, } as const satisfies MapViewport Updated — app/lib/search-map-helper.ts
zoomOut previously flew to [0, 0] at zoom 1 (Gulf of Guinea). It now uses DEFAULT_MAP_VIEWPORT.
import { DEFAULT_MAP_VIEWPORT } from '~/lib/map-defaults'
export const zoomOut = (map: MapRef | undefined) => { map?.flyTo({ center: [DEFAULT_MAP_VIEWPORT.longitude, DEFAULT_MAP_VIEWPORT.latitude], zoom: DEFAULT_MAP_VIEWPORT.zoom, animate: true, speed: 1.6, essential: true, }) } Updated — app/components/map/map.tsx
Replaces the inline coordinate fallback with the import and removes hash={true} so MapLibre no longer reads window.location.hash independently.
import { DEFAULT_MAP_VIEWPORT } from '~/lib/map-defaults'
const Map = forwardRef<MapRef, MapProps>( ({ children, initialViewState, ...props }, ref) => { return (
Removes the local INITIAL_VIEW_STATE, exports parseMapHash for testing, adds writeMapHash to replace the hash-writing that hash={true} previously handled, and wires onMove to write the hash as the user pans.
import { DEFAULT_MAP_VIEWPORT } from '~/lib/map-defaults'
// Exported for testability export function parseMapHash(hash: string) { const match = hash.match(/^#?(-?\d+(?:.\d+)?)/(-?\d+(?:.\d+)?)/(-?\d+(?:.\d+)?)$/) if (!match) return null const [, zoom, latitude, longitude] = match return getValidMapViewport({ latitude: Number(latitude), longitude: Number(longitude), zoom: Number(zoom) }) }
// New: replaces hash={true} writing behavior
export function writeMapHash(viewport: { zoom: number; latitude: number; longitude: number }): string {
const z = Math.round(viewport.zoom * 10) / 10
const lat = Math.round(viewport.latitude * 1000000) / 1000000
const lng = Math.round(viewport.longitude * 1000000) / 1000000
return #${z}/${lat}/${lng}
}
// Inside component const initialViewState = selectedDeviceView ?? hashView ?? homeView ?? DEFAULT_MAP_VIEWPORT
const handleMapMove = useCallback((e: ViewStateChangeEvent) => { window.history.replaceState(null, '', writeMapHash(e.viewState)) }, [])
const handleHomeClick = useCallback(() => { flyToView(DEFAULT_MAP_VIEWPORT) }, [flyToView]) Updated — app/components/device/new/location-info.tsx
Replaces the divergent inline fallback (lat: 51, lng: 7, zoom: 3.5) with DEFAULT_MAP_VIEWPORT.
import { DEFAULT_MAP_VIEWPORT } from '~/lib/map-defaults'
initialViewState={ marker.latitude && marker.longitude ? { latitude: Number(marker.latitude), longitude: Number(marker.longitude), zoom: DEFAULT_MAP_VIEWPORT.zoom } : DEFAULT_MAP_VIEWPORT } Result: After these changes, grep -rn "51.961|longitude: 7.6|center.*0, 0" app/ returns only app/lib/map-defaults.ts. Every map component traces its default viewport to one import, the competing iOS Safari initialization path is eliminated, and URL sharing is preserved through the explicit onMove writer.
Implementation Notes Week 9 Progress The first thing I did before touching any code was pin down exactly how many conflicting definitions existed. A quick grep across the codebase turned up four separate places hardcoding default map coordinates — INITIAL_VIEW_STATE in explore.tsx, an inline fallback object in map.tsx, an approximate {latitude: 51, longitude: 7} in location-info.tsx, and a center: [0, 0], zoom: 1 in search-map-helper.ts. The first three happened to agree on the right location (Münster, Germany), but the fourth flew the map to the Gulf of Guinea whenever a user triggered zoom-out. That confirmed the issue was real and gave me a concrete list of files to change before writing a single line.
From there I traced the iOS Safari bug to the interaction between hash={true} in map.tsx and the React-level initialViewState computed in explore.tsx. MapLibre's native hash={true} reads window.location.hash directly at startup — independent of React's own state — and when it finds a fragment it can't parse as #zoom/lat/lng (like a stale #about anchor from a prior React Router navigation), it silently falls back to coordinates near [0, 0] instead of using the supplied initialViewState prop. The fix had two parts: centralize the defaults into a single importable constant, and remove hash={true} to eliminate the competing initialization path, replacing it with a manual onMove handler in explore.tsx that writes the hash as the user pans.
One detail that came up during implementation: parseMapHash and writeMapHash can't live in explore.tsx if you want to unit-test them, because importing that route file at test time pulls in device.server.ts, which immediately calls initClient() and throws unless DATABASE_URL is set. The fix was to extract both functions into their own module, app/lib/map-hash.ts, which has no server-side imports and is trivially testable. explore.tsx re-exports them, so nothing that currently imports from the route file has to change. When the tests ran — all 24 of them — the first run hit exactly this problem with the explore.tsx import path, which led to the extraction. After moving the functions, the suite went from 10/24 passing to 24/24 clean.
Code Changes Files modified:
app/lib/map-defaults.ts — New file. Single source of truth. Exports DEFAULT_MAP_VIEWPORT typed as MapViewport from location.ts. Every other change in this PR imports from here.
app/lib/map-hash.ts — New file. Extracted from explore.tsx. Houses parseMapHash (regex parse + getValidMapViewport validation) and writeMapHash (rounds coordinates then formats as #zoom/lat/lng). Pure functions with no server imports, making them directly testable without a database.
app/lib/search-map-helper.ts — zoomOut previously flew to center: [0, 0], zoom: 1. Changed to center: [DEFAULT_MAP_VIEWPORT.longitude, DEFAULT_MAP_VIEWPORT.latitude], zoom: DEFAULT_MAP_VIEWPORT.zoom. One import added, two literals replaced.
app/components/map/map.tsx — Replaced the inline ?? { longitude: 7.628202, latitude: 51.961563, zoom: 2 } fallback with ?? DEFAULT_MAP_VIEWPORT. Removed hash={true} so MapLibre no longer reads window.location.hash independently of React state.
app/routes/explore.tsx — Deleted local INITIAL_VIEW_STATE constant. Replaced all two usages with DEFAULT_MAP_VIEWPORT. Imported and re-exported parseMapHash / writeMapHash from map-hash.ts. Added handleMapMove (onMove callback that calls window.history.replaceState with writeMapHash(e.viewState)), restoring URL-sharing behavior that hash={true} previously provided for free.
app/components/device/new/location-info.tsx — Replaced {latitude: 51, longitude: 7, zoom: 3.5} inline fallback with DEFAULT_MAP_VIEWPORT, preserving the saved-marker branch when coordinates are already known.
Key commits (as they would be structured in the PR):
First commit — new constants and hash utility modules (map-defaults.ts, map-hash.ts). No behavior change yet, just introduces the modules so the following commits can import them cleanly. Second commit — the actual fix: update search-map-helper.ts, map.tsx, explore.tsx, and location-info.tsx to remove all inline literals and the hash={true} option. Third commit — test suite: map-defaults.test.ts, search-map-helper.test.ts, map-hash.test.ts, and vitest.unit.config.ts (not for the PR, for local verification). Approach decisions:
parseMapHash and writeMapHash were moved to their own file rather than kept in explore.tsx specifically because the route file cannot be imported in a test without a live database. Extracting them costs one import line in explore.tsx and gains isolated, fast, zero-dependency unit coverage. The onMove handler writes to window.history.replaceState rather than using React Router's navigation because updating the URL hash should not trigger a re-render or a new history entry — replaceState is the right primitive for ephemeral viewport state. The zoom-out default was changed to DEFAULT_MAP_VIEWPORT.zoom (2) rather than a deeper zoom because zoomOut is meant to return the user to the app's starting perspective, not an arbitrary low zoom.
Test results — 24/24 passing:
✓ map-defaults.test.ts (5 tests) — constant validity, bounds, Münster check ✓ search-map-helper.test.ts (5 tests) — zoomOut center, zoom, no [0,0], undefined safety ✓ map-hash.test.ts (14 tests) — valid parse, invalid parse, write format, round-trip
PR Title: fix: centralize map defaults and fix ocean centering on iOS Safari
PR Description:
The map's default viewport (lat, lng, zoom) was defined independently in four different files with no shared source of truth — explore.tsx, map.tsx, location-info.tsx, and search-map-helper.ts. The values were similar but not identical, and there was nothing connecting them, so a change in one wouldn't propagate to the others.
The actual symptom on iOS Safari was the map loading over open ocean instead of Germany. The cause: map.tsx was passing hash={true} to MapLibre, which makes it read window.location.hash directly at startup. When a user navigated from a page that had a non-map hash fragment (#about, etc.), MapLibre picked that up, failed to parse it as #zoom/lat/lng, and silently fell back to coordinates near [0, 0] — ignoring the initialViewState we were passing it.
This PR:
Adds app/lib/map-defaults.ts with a single DEFAULT_MAP_VIEWPORT export. Every file that previously defined its own coordinates now imports from here. Adds app/lib/map-hash.ts with parseMapHash and writeMapHash extracted from explore.tsx. Keeping them in their own file means they can be unit tested without a database connection. Removes hash={true} from map.tsx to eliminate the competing hash reader. An onMove handler in explore.tsx now calls window.history.replaceState with the new hash on every map move, so URL sharing still works. Fixes zoomOut in search-map-helper.ts, which was flying to center: [0, 0], zoom: 1 (Gulf of Guinea) instead of the actual default location. Closes #706.
Pull Request Summary PR Link: github.com/openSenseMap/frontend/pull/[number — fill in after opening]
PR Description: The map's initial viewport was duplicated in four separate files with no connection between them, and one of those definitions (zoomOut in search-map-helper.ts) was actively wrong — flying to [0, 0] instead of Germany. The bigger issue was hash={true} in the base map component, which gave MapLibre its own independent copy of the startup logic that read directly from window.location.hash. On iOS Safari, stale anchor fragments from React Router navigation were tripping that reader, causing MapLibre to fall back to [0, 0] and ignore the correct initialViewState the app was passing. This PR creates a single DEFAULT_MAP_VIEWPORT constant, removes the competing hash reader, and replaces it with an explicit onMove handler that keeps the URL hash in sync.
Status: Phase 4 — Complete / Awaiting Review
Learnings & Reflections Technical Skills Gained
This was my first time tracing a bug that lived at the boundary between a React component's prop system and a third-party library's internal startup behavior. The key insight was that MapLibre's hash: true option and react-map-gl's initialViewState prop are not coordinated — the library reads window.location.hash independently, and when its parser fails, it doesn't fall back to initialViewState in every browser. Understanding that distinction required reading MapLibre source behavior rather than just the react-map-gl docs. I also learned something practical about testability: parseMapHash was buried inside explore.tsx, which imports device.server.ts, which calls initClient() and throws without a live database. Extracting pure utility functions into their own file isn't just good architecture — it's sometimes the only way to get a unit test to even load the module.
Challenges Overcome
The biggest time sink was figuring out why the iOS Safari bug happened at all, since the INITIAL_VIEW_STATE in explore.tsx was actually correct. The priority chain looked right on paper. The answer only became clear when I realized hash={true} was running a completely separate initialization path at the MapLibre level, one that React had no visibility into. The second challenge was running the tests without a database. The project's global vitest.setup.ts seeds a ToS record on startup and dies if DATABASE_URL isn't set, so the first run of the test suite failed with an unrelated-looking error. Creating a separate vitest.unit.config.ts that excludes the setup file solved it cleanly without modifying the main test infrastructure.
What I'd Do Differently Next Time
I'd check whether the component has any library-level initialization hooks — hash, initialPosition, defaultCenter props and their equivalents — before assuming the React prop system is the only thing setting state. That's what actually owned the startup behavior here, and I spent time looking in the wrong place initially. I'd also write the unit config that bypasses the DB setup earlier in the process, since the first thing that should happen before writing any tests is confirming you can even run them.
Resources Used
MapLibre GL JS source — understanding when hash: true takes over from initialViewState react-map-gl documentation — initialViewState prop behavior and the onMove event shape app/lib/location.ts in this repo — getValidMapViewport and MAP_ZOOM_LIMITS were already doing the coordinate validation I needed for parseMapHash, so I reused them rather than writing my own bounds checks Issue #706 and the reporter's note about iOS Safari 18.7.3 — having the exact reproduction environment narrowed down the browser-specific behavior immediately