Locate Features: improve tutorial GeoJSON import and preview rendering#81
Merged
Conversation
…dering - Order preview sub-grid references by task partition index so colors stay aligned with their cells after reload - Reject uploads with duplicate screens - Warn when scenario screen numbers are not serial (1, 2, 3, …) - Show error when project's sub-grid size is not resolved before importing instead of silently creating a single task per feature - Default missing sub-grid references to 0 and clip extra references to the sub-grid size, warning in both cases - Add --size-tile-locate (2x --size-tile) and use it for the Locate preview
tnagorra
requested changes
Jun 3, 2026
Comment on lines
+225
to
+237
| const groupedScreens = listToGroupList( | ||
| screens, | ||
| (screen) => screen, | ||
| (screen) => screen, | ||
| ); | ||
|
|
||
| const duplicateScreens = Object.values(groupedScreens) | ||
| .filter((group) => group.length > 1) | ||
| .map((group) => group[0]); | ||
|
|
||
| if (duplicateScreens.length === 0) { | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
You can use getDuplicates from togglecorp/fujs
| } | ||
|
|
||
| const numSubGrids = (2 ** subGridValue) ** 2; | ||
| const numSubGrids = (2 ** subgridSizeToValueMap[subgridSize]) ** 2; |
Contributor
There was a problem hiding this comment.
What if the expression is undefined? Maybe check that beforehand?
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `Locate scenario screens are expected to be serial (1, 2, 3, …). Found: ${sortedScreens.join(', ')}.`, | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
I think the numbering is repacked from indices
| // author can fix the scenario numbering. | ||
| const sortedScreens = result.features | ||
| .map((feature) => feature.properties.screen) | ||
| .toSorted((a, b) => compareNumber(a, b)); |
Contributor
There was a problem hiding this comment.
Suggested change
| .toSorted((a, b) => compareNumber(a, b)); | |
| .toSorted(compareNumber); |
Comment on lines
+908
to
+922
| result.features.forEach((feature) => { | ||
| if (feature.properties.references.length > numSubGrids) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `Feature on screen ${feature.properties.screen} has ${feature.properties.references.length} references but the sub-grid only has ${numSubGrids} cells; extra references were ignored.`, | ||
| ); | ||
| } | ||
|
|
||
| setFieldValue(scenarioPages, 'scenarios'); | ||
| if (feature.properties.references.length < numSubGrids) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `Feature on screen ${feature.properties.screen} has ${feature.properties.references.length} references but the sub-grid needs ${numSubGrids} cells; missing references were filled in.`, | ||
| ); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
We should just throw error in this scenario IMO
| // Screens are unique (enforced by the schema above) and should be | ||
| // serial (1, 2, 3, …). Warn -- but don't block -- on gaps so the | ||
| // author can fix the scenario numbering. | ||
| const sortedScreens = result.features |
Contributor
There was a problem hiding this comment.
We also need to add these checks for other project types.
| Array.from(new Array(numSubGrids).keys()).map((index) => ({ | ||
| clientId: ulid(), | ||
| // FIXME(frozenhelium): maybe the default value should be first option? | ||
| reference: feature.properties.references[index] ?? 0, |
Contributor
There was a problem hiding this comment.
We should be strict and just throw if there are scenarios where references[index] is undefined
- extract per-project-type GeoJSON transforms
- enforce serial screens (all types),
- unique screens via getDuplicates,
- exact sub-grid reference counts, and reference values against
- project custom options / tile options
- add unit tests for the tutorial transforms
- fix vitest config (node env)
tnagorra
reviewed
Jun 5, 2026
| )), | ||
| })); | ||
| if (isNotDefined(result)) { | ||
| return; |
Contributor
There was a problem hiding this comment.
If result is not defined, it should be an error.
tnagorra
approved these changes
Jun 5, 2026
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.
Uh oh!
There was an error while loading. Please reload this page.