Skip to content

fix(map): include start-geopoint in Project → Data → Map UI DEV-2240#7136

Open
magicznyleszek wants to merge 8 commits into
mainfrom
leszek/dev-2240-map-start-geopoint-fix
Open

fix(map): include start-geopoint in Project → Data → Map UI DEV-2240#7136
magicznyleszek wants to merge 8 commits into
mainfrom
leszek/dev-2240-map-start-geopoint-fix

Conversation

@magicznyleszek

@magicznyleszek magicznyleszek commented Jun 9, 2026

Copy link
Copy Markdown
Member

🗒️ Checklist

  1. run linter locally
  2. update developer docs (API, README, inline, etc.), if any
  3. for user-facing doc changes create a Zulip thread at #Support Docs Updates, if any
  4. draft PR with a title <type>(<scope>)<!>: <title> DEV-1234
  5. assign yourself, tag PR: at least Front end and/or Back end or workflow
  6. fill in the template below and delete template comments
  7. review thyself: read the diff and repro the preview as written
  8. open PR & confirm that CI passes & request reviewers, if needed
  9. act on any greptile review below a 5/5 score or leave comment explaining why you won't
  10. delete this checklist section from the final squash commit before merging

📣 Summary

Map now shows submissions that only have start-geopoint data. Before this, the map stayed empty if your project collected locations via start-geopoint but didn't have any geopoint questions.

💭 Notes

Changes here:

constants.ts

  • Added MAP_DISPLAYABLE_GEOPOINT_TYPES array that includes both 'geopoint' and 'start-geopoint' question types
  • Added isMapDisplayableGeopointType() type guard function to check if a question type can be shown on the map
  • This gives us a single place to define which question types the map should recognize (easier to maintain if we add more geo types later, like 'background-geopoint')

utils.ts

  • Updated findFirstGeopoint() to use the new type guard instead of only checking for 'geopoint'
  • Now finds either regular geopoint questions or start-geopoint

components/map/index.tsx (the main map component)

  • Constructor now uses isMapDisplayableGeopointType() to check if the project has any map-compatible questions
  • This fixes the "no geopoint responses" message showing up when start-geopoint data actually exists

components/map/FormMapWrapper.stories.tsx:

  • New Storybook stories with play tests that verify:
    • Map renders for forms with only start-geopoint
    • Map Settings shows both geopoint and start-geopoint when both exist

.storybook/preview-head.html

  • Mock window.t() before modules load (fixes "t is not defined" errors when components use t() at module scope), as preview.tsx t() definition wasn't working

components/map/MapSettings.tsx (the map settings dialog)

  • Updated to include start-geopoint questions in the question selector dropdown
  • Without this, users couldn't manually switch to start-geopoint even if it had data

storybookUtils.tsx

  • New withMinHeightWrapper() decorator for stories that need vertical space (maps, menus, modals)
  • Updated Menu.stories.tsx, FormLanguagesManager.stories.tsx, and FormMapWrapper.stories.tsx to use shared decorator instead of duplicated inline styles

components/common/modal.tsx (common Modal component):

  • Added role="dialog" and aria-label to the deprecated Modal component (because maps are tricky enough without fighting the test harness 🗺️)
    • jsapp/js/components/map/MapSettings.tsx - Added role="tab" and aria-selected to tab buttons

The root cause was simple: three different places in the code were checking type === 'geopoint' and ignoring 'start-geopoint'. Now they all call the same function that knows about both types. No more copy-paste checks that can drift apart.

👀 Preview steps

I wasn't able to have actual start-geopoint responses in submissions (doesn't work in browser, even though it asks for location permissions). Added storybook tests to cover that instead of fighting with emulating collection through mobile device (tried this few months ago and lost tons of time)

Test with start-geopoint only:

  1. ℹ️ Create a project that doesn't have an explicit geopoint question but does collect start-geopoint metadata
  2. Submit a few responses from a mobile device (so start-geopoint gets captured)
  3. Go to Project → Data → Map
  4. 🔴 [on main] See "This project does not include geographical data" message
  5. 🟢 [on PR] Map renders with submission markers at the locations where the form was started

Test with both types:
6. ℹ️ Add an explicit geopoint question to the form (like "Where are you?")
7. Submit responses that have both start-geopoint and the explicit geopoint question answered
8. Go to Project → Data → Map and click the Map Settings button (gear icon)
9. Switch to the "geopoint question" tab
10. 🟢 [on PR] See both questions listed: "Where are you?" and "start-geopoint"
11. 🟢 Switch between them to see different marker positions (form start location vs. explicit answer location)

@magicznyleszek magicznyleszek requested a review from duvld June 9, 2026 06:51
@magicznyleszek magicznyleszek self-assigned this Jun 9, 2026
@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a bug where the Project → Data → Map view would show "no geographical data" for forms that collected location via start-geopoint metadata but had no explicit geopoint question. The root cause — three separate === 'geopoint' checks that ignored start-geopoint — is consolidated into a single isMapDisplayableGeopointType() helper in constants.ts, used across the constructor of FormMap, MapSettings, and findFirstGeopoint in utils.ts.

  • MAP_DISPLAYABLE_GEOPOINT_TYPES and isMapDisplayableGeopointType() are added to constants.ts as the single source of truth for which question types the map should recognize, with a proper TypeScript type predicate return annotation.
  • MapSettings.tsx now lists start-geopoint in the question-selector dropdown so users can switch to it manually; role="tab" and aria-selected are added to tab buttons for test-harness accessibility.
  • Storybook infrastructure is improved: window.t() is moved to preview-head.html for module-level safety, storybookUtils.ts is renamed to .tsx to gain a shared withMinHeightWrapper decorator, and two new play-tested stories cover the start-geopoint-only and both-geopoint-types scenarios.

Confidence Score: 5/5

Safe to merge — the change is a narrowly scoped bug fix that replaces three independent hardcoded string comparisons with a single well-typed helper; no data paths are altered.

The fix is minimal and self-consistent: the new helper is a proper TypeScript type predicate, all three call sites are updated, the existing parseLatLng logic already handles space-separated coordinate strings (the format start-geopoint emits), and the title prop passed to aria-label in modal.tsx is typed as string, so no runtime mismatch is possible. New Storybook play tests cover both the start-geopoint-only and mixed scenarios.

No files require special attention.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[FormMap constructor] --> B{survey.some isMapDisplayableGeopointType?}
    B -->|true| C[hasGeoPoint = true\nMap renders normally]
    B -->|false| D[hasGeoPoint = false\nShow 'no geo data' message]

    C --> E[createDataQuery]
    E --> F{overridenStyles.selectedQuestion?}
    F -->|yes| G[Use overridden question]
    F -->|no| H{asset.map_styles.selectedQuestion?}
    H -->|yes| I[Use saved question]
    H -->|no| J[findFirstGeopoint survey]
    J --> K{isMapDisplayableGeopointType match?}
    K -->|geopoint| L[selectedQuestion = geopoint name]
    K -->|start-geopoint| L
    K -->|none| M[selectedQuestion = null]

    L --> N[parseLatLng submission\nselectedQuestion]
    N --> O[Render markers on map]

    style K fill:#90EE90
    style B fill:#90EE90
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[FormMap constructor] --> B{survey.some isMapDisplayableGeopointType?}
    B -->|true| C[hasGeoPoint = true\nMap renders normally]
    B -->|false| D[hasGeoPoint = false\nShow 'no geo data' message]

    C --> E[createDataQuery]
    E --> F{overridenStyles.selectedQuestion?}
    F -->|yes| G[Use overridden question]
    F -->|no| H{asset.map_styles.selectedQuestion?}
    H -->|yes| I[Use saved question]
    H -->|no| J[findFirstGeopoint survey]
    J --> K{isMapDisplayableGeopointType match?}
    K -->|geopoint| L[selectedQuestion = geopoint name]
    K -->|start-geopoint| L
    K -->|none| M[selectedQuestion = null]

    L --> N[parseLatLng submission\nselectedQuestion]
    N --> O[Render markers on map]

    style K fill:#90EE90
    style B fill:#90EE90
Loading

Reviews (7): Last reviewed commit: "Merge branch 'main' into leszek/dev-2240..." | Re-trigger Greptile

Comment thread jsapp/js/constants.ts
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@tiritea

tiritea commented Jun 9, 2026

Copy link
Copy Markdown

Before this, the map stayed empty if your project collected locations via start-geopoint but didn't have any geopoint questions.

Are you sure about this? 😕 I thought I had tested this scenario a while back, and recall that start-geopoints were showing on the map, just that you couldnt then flip to an actual geopoint question. Or is it a separate 'filter' such that unless you had an actual explicit geopoint question, the map would consider the submission non-georeferenced to begin with (so you never got the chance to pick another...)

I'm just confused because the submission JSON suggests that the submission was already getting picked up as georeferenced (so why wasnt the map using this location...); ie the _geolocation was correctly populated

@magicznyleszek

Copy link
Copy Markdown
Member Author

@copilot fix biome error

Copilot AI commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

@copilot fix biome error

Fixed in c807da3. Biome now passes (npm run lint:biome) with the formatting-adjusted isMapDisplayableGeopointType signature in jsapp/js/constants.ts.

@magicznyleszek

Copy link
Copy Markdown
Member Author

Before this, the map stayed empty if your project collected locations via start-geopoint but didn't have any geopoint questions.

Are you sure about this? 😕 I thought I had tested this scenario a while back, and recall that start-geopoints were showing on the map, just that you couldnt then flip to an actual geopoint question. Or is it a separate 'filter' such that unless you had an actual explicit geopoint question, the map would consider the submission non-georeferenced to begin with (so you never got the chance to pick another...)

I'm just confused because the submission JSON suggests that the submission was already getting picked up as georeferenced (so why wasnt the map using this location...); ie the _geolocation was correctly populated

You're right that _geolocation gets populated correctly in the submission JSON (backend handles that). The issue is that the frontend map component was making its decision before ever looking at the submission data. What was happening:

Before the fix (form has only start-geopoint, no regular geopoint):

  1. Constructor sets hasGeoPoint = false because it only checked for type === 'geopoint'.
  2. createDataQuery() calls findFirstGeopoint() which returns null.
  3. Map decides "no geo questions exist" and shows the empty blocked map.
  4. The map never even tries to fetch or display the submission data.

So the _geolocation field was correctly populated in the submission JSON, but the map component quit early after checking the form definition, not the submission data.

After the fix:

  1. hasGeoPoint = true (because frontend code now checks for both geopoint types).
  2. findFirstGeopoint() finds the start-geopoint.
  3. Map says "we have geo questions!" and proceeds to display the data.
  4. The points show up using start-geopoint as the selected question

The map doesn't use _geolocation directly anymore (see #6679). It needs to know which specific question field to read from the submission JSON, and it picks that by scanning the form definition for geopoint-type questions. Now it checks two of the types instead of just one.

@magicznyleszek magicznyleszek marked this pull request as ready for review June 9, 2026 15:42
@tiritea

tiritea commented Jun 10, 2026

Copy link
Copy Markdown

The map doesn't use _geolocation directly anymore (see #6679).

That makes sense, thanks for the explanation! So is it possible that before (when we were using _geolocation?) that it might have been picking up start-geopoints? Or is it the case that start-geopoints have never been being displayed? Just that a vaguely recall these start-geopoints were showing up in the past...

It needs to know which specific question field to read from the submission JSON, and it picks that by scanning the form definition for geopoint-type questions.

Perhaps a stupid question but... then what purpose does _geolocation now provide, if its not actually being used to display anything on maps?

@magicznyleszek

Copy link
Copy Markdown
Member Author

The map doesn't use _geolocation directly anymore (see #6679).

That makes sense, thanks for the explanation! So is it possible that before (when we were using _geolocation?) that it might have been picking up start-geopoints? Or is it the case that start-geopoints have never been being displayed? Just that a vaguely recall these start-geopoints were showing up in the past...

It needs to know which specific question field to read from the submission JSON, and it picks that by scanning the form definition for geopoint-type questions.

Perhaps a stupid question but... then what purpose does _geolocation now provide, if its not actually being used to display anything on maps?

I think it is possible that start-geopoint worked at some point. We touched map code with multiple PRs recently, upgrading parts of it.

As to _geolocation, AFAIK it is "bad" and we want to stop using it. John wrote something about it here #6679 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants