feat(add-location): guided location wizard #1134 - #1135
Open
jurjendevries wants to merge 1 commit into
Open
Conversation
Add a multi-step add-location wizard at /add-location/wizard (the existing /add-location page is unchanged): - Physical yes/no branch; online-or-mobile businesses captured separately (webshops, on-site/mobile services) with an online-or-mobile Gitea label. - "Already on the map?" step: OSM/BTC Map search, select an existing merchant to update, or add a new one. Search results prefill name/category/website/ phone/opening hours from OSM (Nominatim extratags/namedetails). - Address OR location description (one required), order adapts to locale. - Friendly opening-hours editor producing OSM opening_hours syntax, incl. multiple ranges per day and 24/7. - Browser back/forward navigates steps via shallow routing. Data flow: every submission creates a Gitea issue (captcha + honeypot spam gate) with a machine-readable payload. On maintainer approval a webhook can push it via btcmap-api submit_place (instant map draft, idempotent) and/or an OSM changeset via a bot account. All push paths are env-gated and no-op without credentials, so the wizard degrades to Gitea-issue creation. New Gitea labels (name-based, auto-created): online-or-mobile, update-location, osm-approved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for btcmap ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds a guided add-location wizard with OSM-prefilled forms, opening-hours editing, structured Gitea payloads, and approval-triggered publishing to BTC Map and OSM. ChangesAdd-location wizard and metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Wizard
participant GiteaIssueAPI
participant GiteaWebhook
participant BTCMapAPI
participant OSMAPI
Wizard->>GiteaIssueAPI: Submit wizard form
GiteaIssueAPI->>GiteaIssueAPI: Store structured OSM payload
GiteaWebhook->>GiteaWebhook: Verify signature and approval
GiteaWebhook->>BTCMapAPI: Submit create payload
GiteaWebhook->>OSMAPI: Create or update OSM node
GiteaWebhook->>GiteaIssueAPI: Post result comment
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
🧹 Nitpick comments (1)
src/routes/api/gitea/issue/+server.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate the type-only import.
Proposed fix
-import { createIssueWithLabels, type GiteaRepo } from "$lib/gitea"; +import { createIssueWithLabels } from "$lib/gitea"; +import type { GiteaRepo } from "$lib/gitea";As per coding guidelines, “Do not mix type imports and value imports in the same statement.”
🤖 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 `@src/routes/api/gitea/issue/`+server.ts at line 7, Separate the GiteaRepo type import from the createIssueWithLabels value import in the import declarations, using a type-only import for GiteaRepo while preserving both imported symbols.Source: Coding guidelines
🤖 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 `@src/components/form/OpeningHoursEditor.svelte`:
- Around line 124-140: Update the raw-mode input handling near the raw editor’s
input event and the reactive parser block so lastEmitted is synchronized with
the input element’s current DOM value on every input. This prevents bind:value
keystrokes from re-entering the parser path and switching to structured mode
while editing raw text; preserve normal parsing for externally supplied values
and structured-editor writes.
In `@src/lib/btcmapApi.ts`:
- Around line 33-44: Apply one shared bounded timeout policy to all publishing
requests: update the rpc function in src/lib/btcmapApi.ts, the OSM request flow
in src/lib/osm.ts, and marker reads and writes in the webhook handler at
src/routes/api/gitea/webhook/+server.ts to use abortable requests with the same
timeout duration and consistently propagate timeout errors.
- Around line 91-94: Update the payment tag checks in the payload mapping logic
so extraFields are set to "yes" only when the corresponding tag value explicitly
indicates acceptance, not merely when it is non-empty or truthy. Apply this to
payment:onchain, payment:lightning, and payment:lightning_contactless while
preserving the existing extra-field names.
In `@src/lib/i18n/locales/en.json`:
- Around line 506-564: Add the new addLocationWizard and openingHours
translation key structures from en.json to every other locale file, preserving
identical nesting and key names. Provide locale-appropriate translations where
available, or use the established fallback convention for untranslated values,
so all locale key sets remain synchronized.
In `@src/lib/osmPayload.ts`:
- Around line 99-105: Update the action === "update" branch in the
payload-generation logic to accept only an explicitly validated "node" osmType;
route ways, relations, missing types, and unsupported values to manual handling
instead of generating an automated payload. Do not default absent or invalid
types to "node", and preserve osmId only for valid node updates.
- Around line 30-39: Update the OSM update-payload builder around setIf and
pushToOsm to emit explicit tag removals for blank fields and unchecked payment
methods, so merged tags can clear existing values. Read the wizard’s socialLinks
field, normalize it into the expected social tag keys before approval
automation, and preserve support for existing social inputs where applicable.
Ensure non-empty values remain explicit set operations and removals are
represented distinctly.
- Around line 123-132: Update the payload extraction logic around the
OSM_PAYLOAD_START and OSM_PAYLOAD_END searches to parse only one canonical
server-appended block, not the first delimiter pair found in raw form fields.
Require the block to be trailing and structurally exact, reject bodies with
forged or ambiguous delimiter matches, and preserve null for invalid input;
alternatively, validate an existing signature if the generated payload is
signed.
- Around line 134-143: Update the validation in the JSON parsing flow around
parsed and OsmPayload so tags must be a non-null, non-array object with string
keys and string values. Preserve the existing action validation and return null
for invalid payloads before returning parsed.
In `@src/routes/add-location/wizard/`+page.svelte:
- Around line 487-489: Update initDiscoverMap and the related initialization
paths around lines 562-564 and 625-646 to use an action-local cancellation flag
set during destruction. Check that flag immediately after await ensureMaplibre()
and before creating or attaching the map, returning early when cancellation
occurred; preserve normal initialization for active actions.
- Around line 334-364: Preserve PlaceSelect.osmType and osmId from
useSearchedLocation through the new-location form and submission flow instead of
discarding them. When both identify a supported OSM node, submit an update
payload targeting that existing element rather than a create payload; route ways
and relations to the existing manual-review path, while retaining create
behavior for locations without supported OSM identity.
- Around line 140-150: Persist the add-location wizard’s input values in
component state instead of relying on bind:this references, since
conditional-step unmounts reset those elements. Update the fields declared near
onlineName and the corresponding inputs in the sections around the additional
referenced ranges to use bind:value with state variables, retaining bind:this
only for DOM operations that require element references.
- Around line 404-409: Update the coordinate checks in the location payload
around pickLat and pickLong so valid zero values are preserved; test for nullish
or explicitly absent values instead of truthiness when generating lat, long, and
the OpenStreetMap URL.
In `@src/routes/api/gitea/issue/`+server.ts:
- Around line 328-334: Validate wizard payloads in the issue-creation flow
before consuming the captcha: for create requests, require valid
coordinates/ranges, the BTC Map category, payment methods, and either an address
or description; for update requests, require node identity and all required
fields. Reject invalid payloads before constructing the approval-eligible issue,
while preserving retryability and leaving valid payload handling unchanged.
In `@src/routes/api/gitea/webhook/`+server.ts:
- Around line 110-145: Make approval processing durably idempotent before any
OSM write: in src/routes/api/gitea/webhook/+server.ts lines 110-145, atomically
claim the repository/issue/payload-revision key before publishing; in
src/routes/api/gitea/webhook/+server.ts lines 47-68, fail closed when reading
the push marker and verify marker writes; in src/lib/osm.ts lines 164-180,
persist or reconcile a created node before permitting retries; and in
src/lib/osm.ts lines 202-209, distinguish cleanup failure from an unknown
node-creation outcome.
- Around line 106-139: Update the target guard and submission branches in the
webhook handler to require an effective destination for the payload action:
allow BTC Map only for create actions, while updates require OSM. Use the
existing submitToBtcMap and submitToOsm flows in the corresponding branches, and
prevent an empty result from reaching the permanent pushed-marker path.
---
Nitpick comments:
In `@src/routes/api/gitea/issue/`+server.ts:
- Line 7: Separate the GiteaRepo type import from the createIssueWithLabels
value import in the import declarations, using a type-only import for GiteaRepo
while preserving both imported symbols.
🪄 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
Run ID: 8e8c71fc-fa55-4bd1-9d0b-cb474b82252c
📒 Files selected for processing (14)
src/app.d.tssrc/components/form/AddressSearch.sveltesrc/components/form/OpeningHoursEditor.sveltesrc/lib/btcmapApi.tssrc/lib/constants.tssrc/lib/geocoding.test.tssrc/lib/geocoding.tssrc/lib/i18n/locales/en.jsonsrc/lib/osm.tssrc/lib/osmPayload.test.tssrc/lib/osmPayload.tssrc/routes/add-location/wizard/+page.sveltesrc/routes/api/gitea/issue/+server.tssrc/routes/api/gitea/webhook/+server.ts
Comment on lines
+124
to
+140
| // Parse an externally-supplied value (initial mount, prefill from OSM data). | ||
| $: if (value !== lastEmitted) { | ||
| const parsed = parse(value); | ||
| if (parsed === "24/7") { | ||
| always24 = true; | ||
| days = makeDefaultDays(); | ||
| rawMode = false; | ||
| } else if (parsed) { | ||
| always24 = false; | ||
| days = parsed; | ||
| rawMode = false; | ||
| } else { | ||
| // Unrepresentable — keep the raw string editable. | ||
| rawMode = true; | ||
| } | ||
| lastEmitted = value; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the component structure first, then inspect the relevant slices.
ast-grep outline src/components/form/OpeningHoursEditor.svelte --view expanded > /tmp/oh_outline.txt
printf '--- OUTLINE ---\n'
sed -n '1,220p' /tmp/oh_outline.txt
printf '\n--- RELEVANT LINES 1-260 ---\n'
cat -n src/components/form/OpeningHoursEditor.svelte | sed -n '1,260p'Repository: teambtcmap/btcmap.org
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for rawMode / lastEmitted / parse usage to understand state transitions.
rg -n "rawMode|lastEmitted|function parse|emit\\(|bind:value|on:input|always24|days =" src/components/form/OpeningHoursEditor.svelteRepository: teambtcmap/btcmap.org
Length of output: 1370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of the component with line numbers.
cat -n src/components/form/OpeningHoursEditor.svelte | sed -n '1,260p'Repository: teambtcmap/btcmap.org
Length of output: 10062
Keep raw-mode edits out of the parser path (src/components/form/OpeningHoursEditor.svelte:125-140, 199-205)
bind:value updates value on every keystroke, but lastEmitted only tracks structured-editor writes. Once the raw text becomes parseable, this reactive block flips back to the structured editor mid-edit. Sync lastEmitted from the raw input’s DOM value on input so raw-mode typing stays in raw mode.
🐛 Proposed fix
<input
type="text"
bind:value
+ on:input={(e) => (lastEmitted = (e.target as HTMLInputElement).value)}
{disabled}
placeholder="Mo-Fr 09:00-17:00; Sa 10:00-14:00"
class="w-full rounded-2xl border-2 border-input p-3 focus:outline-link disabled:cursor-not-allowed disabled:bg-gray-100 dark:bg-white/[0.15]"
/>🤖 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 `@src/components/form/OpeningHoursEditor.svelte` around lines 124 - 140, Update
the raw-mode input handling near the raw editor’s input event and the reactive
parser block so lastEmitted is synchronized with the input element’s current DOM
value on every input. This prevents bind:value keystrokes from re-entering the
parser path and switching to structured mode while editing raw text; preserve
normal parsing for externally supplied values and structured-editor writes.
Comment on lines
+33
to
+44
| async function rpc<T>( | ||
| method: string, | ||
| params: Record<string, unknown>, | ||
| ): Promise<T> { | ||
| const res = await fetch(`${API_BASE}/rpc`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${env.BTCMAP_API_RPC_TOKEN}`, | ||
| }, | ||
| body: JSON.stringify({ jsonrpc: "2.0", method, params, id: 1 }), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Apply one bounded timeout policy to every publishing request.
src/lib/btcmapApi.ts#L33-L44: abort stalled JSON-RPC requests.src/lib/osm.ts#L48-L57: abort stalled OSM API requests.src/routes/api/gitea/webhook/+server.ts#L51-L67: abort stalled marker reads and writes.
📍 Affects 3 files
src/lib/btcmapApi.ts#L33-L44(this comment)src/lib/osm.ts#L48-L57src/routes/api/gitea/webhook/+server.ts#L51-L67
🤖 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 `@src/lib/btcmapApi.ts` around lines 33 - 44, Apply one shared bounded timeout
policy to all publishing requests: update the rpc function in
src/lib/btcmapApi.ts, the OSM request flow in src/lib/osm.ts, and marker reads
and writes in the webhook handler at src/routes/api/gitea/webhook/+server.ts to
use abortable requests with the same timeout duration and consistently propagate
timeout errors.
Comment on lines
+91
to
+94
| if (payload.tags["payment:onchain"]) extraFields.payment_onchain = "yes"; | ||
| if (payload.tags["payment:lightning"]) extraFields.payment_lightning = "yes"; | ||
| if (payload.tags["payment:lightning_contactless"]) | ||
| extraFields.payment_lightning_contactless = "yes"; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not convert "no" payment tags into "yes".
Every non-empty value is truthy, so payment:onchain=no currently publishes affirmative acceptance.
Proposed fix
- if (payload.tags["payment:onchain"]) extraFields.payment_onchain = "yes";
- if (payload.tags["payment:lightning"]) extraFields.payment_lightning = "yes";
- if (payload.tags["payment:lightning_contactless"])
+ if (payload.tags["payment:onchain"] === "yes")
+ extraFields.payment_onchain = "yes";
+ if (payload.tags["payment:lightning"] === "yes")
+ extraFields.payment_lightning = "yes";
+ if (payload.tags["payment:lightning_contactless"] === "yes")
extraFields.payment_lightning_contactless = "yes";📝 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.
Suggested change
| if (payload.tags["payment:onchain"]) extraFields.payment_onchain = "yes"; | |
| if (payload.tags["payment:lightning"]) extraFields.payment_lightning = "yes"; | |
| if (payload.tags["payment:lightning_contactless"]) | |
| extraFields.payment_lightning_contactless = "yes"; | |
| if (payload.tags["payment:onchain"] === "yes") extraFields.payment_onchain = "yes"; | |
| if (payload.tags["payment:lightning"] === "yes") extraFields.payment_lightning = "yes"; | |
| if (payload.tags["payment:lightning_contactless"] === "yes") | |
| extraFields.payment_lightning_contactless = "yes"; |
🤖 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 `@src/lib/btcmapApi.ts` around lines 91 - 94, Update the payment tag checks in
the payload mapping logic so extraFields are set to "yes" only when the
corresponding tag value explicitly indicates acceptance, not merely when it is
non-empty or truthy. Apply this to payment:onchain, payment:lightning, and
payment:lightning_contactless while preserving the existing extra-field names.
Comment on lines
+506
to
+564
| "addLocationWizard": { | ||
| "title": "Add a Location", | ||
| "physicalQuestion": "Does the business have a physical location visitors can go to?", | ||
| "physicalHint": "A physical location is a place customers can visit in person (a shop, café, office, market stall, etc.).", | ||
| "yes": "Yes", | ||
| "no": "No", | ||
| "back": "Back", | ||
| "onlineHeading": "Online or mobile business", | ||
| "onlineIntro": "Thanks! This is for businesses without a location customers visit — online shops and webshops, and services delivered at the customer's place (for example a mobile window cleaner). We can't show these on the map yet, but we're building that. Share your details and we'll store them for the upcoming listing.", | ||
| "socialLabel": "Social media", | ||
| "socialPlaceholder": "Links to X, Instagram, Nostr, etc.", | ||
| "onlineSuccessType": "online or mobile business", | ||
| "onlineSuccessText": "Thanks for your submission! The map currently shows only businesses with a visitable location, but we'll list online and mobile bitcoin-accepting businesses in the future and your data will be used for that.", | ||
| "mapHeading": "Is your business already on the map?", | ||
| "mapIntro": "Search for your business or browse the map. If you find it, select it to view and update its details.", | ||
| "mapClickHint": "Tap an orange dot to select an existing location.", | ||
| "searchedResultLabel": "Search result", | ||
| "useSearchedLocation": "This is my business — add it", | ||
| "notOnMap": "It's not on the map yet", | ||
| "loadingPlace": "Loading location…", | ||
| "updateHeading": "Update this location", | ||
| "updateIntro": "Review the details below and correct anything that's outdated or missing.", | ||
| "updatePaymentLegend": "Accepted payment methods (update if changed)", | ||
| "newHeading": "Add a new physical location", | ||
| "newIntro": "Tell us where the business is and how it accepts bitcoin. Place a pin on the map, then give an address or a description of the location — whichever fits best.", | ||
| "locationDescriptionLabel": "Location description", | ||
| "locationDescriptionHint": "Describe how to find the place if it has no street address — landmarks, directions, floor, etc.", | ||
| "locationDescriptionPlaceholder": "e.g. 200m north of the temple gate, second stall on the left", | ||
| "addressOrDescriptionHint": "Provide a street address or a location description below — at least one is required.", | ||
| "locationTextError": "Please provide either an address or a location description.", | ||
| "newSuccessType": "location", | ||
| "newSuccessText": "Thanks! Your location has been submitted for review. Once a BTC Map editor approves it, it will appear on the map and be merged into OpenStreetMap.", | ||
| "updateSuccessType": "update", | ||
| "updateSuccessText": "Thanks! Your update has been submitted for review by a BTC Map editor." | ||
| }, | ||
| "openingHours": { | ||
| "open247": "Open 24/7", | ||
| "allDay": "All day", | ||
| "from": "Opening time", | ||
| "to": "Closing time", | ||
| "closed": "Closed", | ||
| "addRange": "Add hours", | ||
| "removeRange": "Remove this time range", | ||
| "copyToAll": "Copy first open day to all days", | ||
| "preview": "Result", | ||
| "none": "Not set", | ||
| "invalid": "This doesn't look like valid opening hours.", | ||
| "rawHint": "Editing opening hours in raw OSM format:", | ||
| "useSimpleEditor": "Switch back to the simple editor (clears current value)", | ||
| "days": { | ||
| "Mo": "Monday", | ||
| "Tu": "Tuesday", | ||
| "We": "Wednesday", | ||
| "Th": "Thursday", | ||
| "Fr": "Friday", | ||
| "Sa": "Saturday", | ||
| "Su": "Sunday" | ||
| } | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether other locale files got the new keys, and en.json's existing diacritic style.
fd . src/lib/i18n/locales -e json
for f in src/lib/i18n/locales/*.json; do
echo "== $f =="
jq 'has("addLocationWizard"), has("openingHours")' "$f"
done
rg -n '&[a-zA-Z]+;' src/lib/i18n/locales/en.json | head -20
rg -nP '[À-ÿ]' src/lib/i18n/locales/en.json | head -20Repository: teambtcmap/btcmap.org
Length of output: 977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Keys present in locale files"
for f in src/lib/i18n/locales/*.json; do
printf "\n== %s ==\n" "$f"
jq -r 'keys[]' "$f" | rg '^(addLocationWizard|openingHours)$' || true
done
echo
echo "## Non-ASCII / entity contexts in en.json"
python3 - <<'PY'
from pathlib import Path
import re
text = Path('src/lib/i18n/locales/en.json').read_text(encoding='utf-8')
for m in re.finditer(r'&[A-Za-z]+;|[^\x00-\x7F]', text):
start = max(0, m.start()-60)
end = min(len(text), m.end()+60)
snippet = text[start:end].replace('\n','\\n')
print(f"{m.start():6d}: {snippet}")
PYRepository: teambtcmap/btcmap.org
Length of output: 3244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Diacritic entities in en.json"
rg -n '&(a|e|i|o|u|A|E|I|O|U|c|C)acute;|&(a|e|i|o|u|A|E|I|O|U|c|C)grave;|&(a|e|i|o|u|A|E|I|O|U|n|N|s|S|y|Y)tilde;|&(a|e|i|o|u|A|E|I|O|U)uml;|&(a|e|i|o|u|A|E|I|O|U)circ;|&(c|C)cedil;|&(n|N)tilde;|&(o|O)slash;|&(a|A)elig;|&(o|O)elig;' src/lib/i18n/locales/en.json || true
echo
echo "## All HTML entities in en.json"
rg -n '&[A-Za-z]+;' src/lib/i18n/locales/en.json || true
echo
echo "## All literal non-ASCII characters in en.json"
python3 - <<'PY'
from pathlib import Path
import unicodedata
text = Path('src/lib/i18n/locales/en.json').read_text(encoding='utf-8')
for i, ch in enumerate(text):
if ord(ch) > 127:
name = unicodedata.name(ch, 'UNKNOWN')
print(f"{i}: {ch} U+{ord(ch):04X} {name}")
PYRepository: teambtcmap/btcmap.org
Length of output: 900
Add addLocationWizard and openingHours to the other locale files. The new keys exist only in src/lib/i18n/locales/en.json, so the locale key sets are out of sync.
🤖 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 `@src/lib/i18n/locales/en.json` around lines 506 - 564, Add the new
addLocationWizard and openingHours translation key structures from en.json to
every other locale file, preserving identical nesting and key names. Provide
locale-appropriate translations where available, or use the established fallback
convention for untranslated values, so all locale key sets remain synchronized.
Source: Coding guidelines
Comment on lines
+30
to
+39
| // Only assign a tag when the source value is non-empty, so we never write blank | ||
| // tags to OSM. | ||
| function setIf( | ||
| tags: Record<string, string>, | ||
| key: string, | ||
| value: unknown, | ||
| ): void { | ||
| const v = str(value); | ||
| if (v) tags[key] = v; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Model removals and the wizard’s actual social field in update payloads.
The update form sends socialLinks, but this builder only reads twitter, facebook, and instagram, so those edits are silently omitted. Additionally, blank fields and unchecked payment methods are dropped; because pushToOsm merges tags, existing values can never be removed.
Introduce explicit set/remove operations and normalize the social input before approval automation.
Also applies to: 53-69
🤖 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 `@src/lib/osmPayload.ts` around lines 30 - 39, Update the OSM update-payload
builder around setIf and pushToOsm to emit explicit tag removals for blank
fields and unchecked payment methods, so merged tags can clear existing values.
Read the wizard’s socialLinks field, normalize it into the expected social tag
keys before approval automation, and preserve support for existing social inputs
where applicable. Ensure non-empty values remain explicit set operations and
removals are represented distinctly.
Comment on lines
+404
to
+409
| lat: pickLat ? pickLat.toString() : "", | ||
| long: pickLong ? pickLong.toString() : "", | ||
| osm: | ||
| pickLat && pickLong | ||
| ? `https://www.openstreetmap.org/edit#map=21/${pickLat}/${pickLong}` | ||
| : "", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve valid zero-valued coordinates.
Locations on latitude or longitude 0 are treated as missing because these checks use truthiness.
Proposed fix
- lat: pickLat ? pickLat.toString() : "",
- long: pickLong ? pickLong.toString() : "",
+ lat: pickLat != null ? pickLat.toString() : "",
+ long: pickLong != null ? pickLong.toString() : "",
osm:
- pickLat && pickLong
+ pickLat != null && pickLong != null📝 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.
Suggested change
| lat: pickLat ? pickLat.toString() : "", | |
| long: pickLong ? pickLong.toString() : "", | |
| osm: | |
| pickLat && pickLong | |
| ? `https://www.openstreetmap.org/edit#map=21/${pickLat}/${pickLong}` | |
| : "", | |
| lat: pickLat != null ? pickLat.toString() : "", | |
| long: pickLong != null ? pickLong.toString() : "", | |
| osm: | |
| pickLat != null && pickLong != null | |
| ? `https://www.openstreetmap.org/edit#map=21/${pickLat}/${pickLong}` | |
| : "", |
🤖 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 `@src/routes/add-location/wizard/`+page.svelte around lines 404 - 409, Update
the coordinate checks in the location payload around pickLat and pickLong so
valid zero values are preserved; test for nullish or explicitly absent values
instead of truthiness when generating lat, long, and the OpenStreetMap URL.
Comment on lines
+487
to
+489
| async function initDiscoverMap(node: HTMLDivElement) { | ||
| const maplibre = await ensureMaplibre(); | ||
| if (!maplibre || destroyed) return; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cancel pending map initialization when an action is destroyed.
Changing steps while ensureMaplibre() is pending destroys the action but leaves destroyed false. Initialization can then create a map on a detached node and leak its WebGL resources. Add an action-local cancellation flag checked after the await.
Also applies to: 562-564, 625-646
🤖 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 `@src/routes/add-location/wizard/`+page.svelte around lines 487 - 489, Update
initDiscoverMap and the related initialization paths around lines 562-564 and
625-646 to use an action-local cancellation flag set during destruction. Check
that flag immediately after await ensureMaplibre() and before creating or
attaching the map, returning early when cancellation occurred; preserve normal
initialization for active actions.
Comment on lines
+328
to
+334
| let body = generateBody(type, data, areasText); | ||
|
|
||
| // For wizard flows that support automated OSM pushes, append a | ||
| // machine-readable payload block that api/gitea/webhook parses on approval. | ||
| if (config.osmPayload) { | ||
| body += `\n\n${buildOsmPayloadBlock(config.osmPayload, data)}`; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Validate each wizard payload before creating an approval-eligible issue.
Client checks are bypassable, and src/routes/add-location/wizard/+page.svelte Line 935 does not require the category needed by BTC Map submission. Validate create coordinates/ranges, category, payment methods, and address-or-description; validate update node identity and required fields. Do this before consuming the captcha so validation errors remain retryable.
🤖 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 `@src/routes/api/gitea/issue/`+server.ts around lines 328 - 334, Validate
wizard payloads in the issue-creation flow before consuming the captcha: for
create requests, require valid coordinates/ranges, the BTC Map category, payment
methods, and either an address or description; for update requests, require node
identity and all required fields. Reject invalid payloads before constructing
the approval-eligible issue, while preserving retryability and leaving valid
payload handling unchanged.
Comment on lines
+106
to
+139
| if (!btcmapApiConfigured() && !osmConfigured()) { | ||
| error(503, "No push target configured"); | ||
| } | ||
|
|
||
| // Guard against duplicate pushes from re-delivered events. | ||
| if (await issueHasPushedMarker(repoFullName, issue.number)) { | ||
| return json({ ok: true, skipped: "already pushed" }); | ||
| } | ||
|
|
||
| const lines: string[] = []; | ||
| try { | ||
| // 1. Instant BTC Map draft via submit_place (create only; the import | ||
| // pipeline is not an OSM-node editor, so updates skip it). | ||
| if (osmPayload.action === "create" && btcmapApiConfigured()) { | ||
| const sp = await submitPlaceFromPayload( | ||
| osmPayload, | ||
| `gitea-${issue.number}`, | ||
| ); | ||
| lines.push( | ||
| `🗺️ Added to BTC Map instantly via submit_place (id \`${sp.id}\`, \`${sp.origin}:${sp.external_id}\`).`, | ||
| ); | ||
| } | ||
|
|
||
| // 2. Automated OSM changeset (create or update) via the bot account. | ||
| if (osmConfigured()) { | ||
| const result = await pushToOsm(osmPayload); | ||
| const changesetUrl = result.url.replace( | ||
| `/node/${result.osmId}`, | ||
| `/changeset/${result.changesetId}`, | ||
| ); | ||
| lines.push( | ||
| `✅ Pushed to OSM: [${result.osmType}/${result.osmId}](${result.url}) in changeset [${result.changesetId}](${changesetUrl}).`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require an effective target for the payload action.
For an update with only btcmap-api configured, Line 106 passes, but btcmap submission is skipped and OSM is unavailable. The empty result is then marked as pushed permanently.
Proposed fix
- if (!btcmapApiConfigured() && !osmConfigured()) {
+ const submitToBtcMap =
+ osmPayload.action === "create" && btcmapApiConfigured();
+ const submitToOsm = osmConfigured();
+
+ if (!submitToBtcMap && !submitToOsm) {
error(503, "No push target configured");
}Use submitToBtcMap and submitToOsm in the corresponding branches.
📝 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.
Suggested change
| if (!btcmapApiConfigured() && !osmConfigured()) { | |
| error(503, "No push target configured"); | |
| } | |
| // Guard against duplicate pushes from re-delivered events. | |
| if (await issueHasPushedMarker(repoFullName, issue.number)) { | |
| return json({ ok: true, skipped: "already pushed" }); | |
| } | |
| const lines: string[] = []; | |
| try { | |
| // 1. Instant BTC Map draft via submit_place (create only; the import | |
| // pipeline is not an OSM-node editor, so updates skip it). | |
| if (osmPayload.action === "create" && btcmapApiConfigured()) { | |
| const sp = await submitPlaceFromPayload( | |
| osmPayload, | |
| `gitea-${issue.number}`, | |
| ); | |
| lines.push( | |
| `🗺️ Added to BTC Map instantly via submit_place (id \`${sp.id}\`, \`${sp.origin}:${sp.external_id}\`).`, | |
| ); | |
| } | |
| // 2. Automated OSM changeset (create or update) via the bot account. | |
| if (osmConfigured()) { | |
| const result = await pushToOsm(osmPayload); | |
| const changesetUrl = result.url.replace( | |
| `/node/${result.osmId}`, | |
| `/changeset/${result.changesetId}`, | |
| ); | |
| lines.push( | |
| `✅ Pushed to OSM: [${result.osmType}/${result.osmId}](${result.url}) in changeset [${result.changesetId}](${changesetUrl}).`, | |
| ); | |
| } | |
| if (!btcmapApiConfigured() && !osmConfigured()) { | |
| error(503, "No push target configured"); | |
| } | |
| const submitToBtcMap = | |
| osmPayload.action === "create" && btcmapApiConfigured(); | |
| const submitToOsm = osmConfigured(); | |
| if (!submitToBtcMap && !submitToOsm) { | |
| error(503, "No push target configured"); | |
| } | |
| // Guard against duplicate pushes from re-delivered events. | |
| if (await issueHasPushedMarker(repoFullName, issue.number)) { | |
| return json({ ok: true, skipped: "already pushed" }); | |
| } | |
| const lines: string[] = []; | |
| try { | |
| // 1. Instant BTC Map draft via submit_place (create only; the import | |
| // pipeline is not an OSM-node editor, so updates skip it). | |
| if (submitToBtcMap) { | |
| const sp = await submitPlaceFromPayload( | |
| osmPayload, | |
| `gitea-${issue.number}`, | |
| ); | |
| lines.push( | |
| `🗺️ Added to BTC Map instantly via submit_place (id \`${sp.id}\`, \`${sp.origin}:${sp.external_id}\`).`, | |
| ); | |
| } | |
| // 2. Automated OSM changeset (create or update) via the bot account. | |
| if (submitToOsm) { | |
| const result = await pushToOsm(osmPayload); | |
| const changesetUrl = result.url.replace( | |
| `/node/${result.osmId}`, | |
| `/changeset/${result.changesetId}`, | |
| ); | |
| lines.push( | |
| `✅ Pushed to OSM: [${result.osmType}/${result.osmId}](${result.url}) in changeset [${result.changesetId}](${changesetUrl}).`, | |
| ); | |
| } |
🤖 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 `@src/routes/api/gitea/webhook/`+server.ts around lines 106 - 139, Update the
target guard and submission branches in the webhook handler to require an
effective destination for the payload action: allow BTC Map only for create
actions, while updates require OSM. Use the existing submitToBtcMap and
submitToOsm flows in the corresponding branches, and prevent an empty result
from reaching the permanent pushed-marker path.
Comment on lines
+110
to
+145
| // Guard against duplicate pushes from re-delivered events. | ||
| if (await issueHasPushedMarker(repoFullName, issue.number)) { | ||
| return json({ ok: true, skipped: "already pushed" }); | ||
| } | ||
|
|
||
| const lines: string[] = []; | ||
| try { | ||
| // 1. Instant BTC Map draft via submit_place (create only; the import | ||
| // pipeline is not an OSM-node editor, so updates skip it). | ||
| if (osmPayload.action === "create" && btcmapApiConfigured()) { | ||
| const sp = await submitPlaceFromPayload( | ||
| osmPayload, | ||
| `gitea-${issue.number}`, | ||
| ); | ||
| lines.push( | ||
| `🗺️ Added to BTC Map instantly via submit_place (id \`${sp.id}\`, \`${sp.origin}:${sp.external_id}\`).`, | ||
| ); | ||
| } | ||
|
|
||
| // 2. Automated OSM changeset (create or update) via the bot account. | ||
| if (osmConfigured()) { | ||
| const result = await pushToOsm(osmPayload); | ||
| const changesetUrl = result.url.replace( | ||
| `/node/${result.osmId}`, | ||
| `/changeset/${result.changesetId}`, | ||
| ); | ||
| lines.push( | ||
| `✅ Pushed to OSM: [${result.osmType}/${result.osmId}](${result.url}) in changeset [${result.changesetId}](${changesetUrl}).`, | ||
| ); | ||
| } | ||
|
|
||
| await postComment( | ||
| repoFullName, | ||
| issue.number, | ||
| `${PUSHED_MARKER}\n${lines.join("\n")}`, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Make approval processing durably idempotent before performing OSM writes. The current comment marker cannot atomically claim work or reconcile an ambiguous OSM create result.
src/routes/api/gitea/webhook/+server.ts#L110-L145: atomically claim a repository/issue/payload-revision key before publishing.src/routes/api/gitea/webhook/+server.ts#L47-L68: fail closed on marker reads and verify marker writes.src/lib/osm.ts#L164-L180: persist or reconcile the created node before another attempt is allowed.src/lib/osm.ts#L202-L209: distinguish cleanup failure from an unknown node-creation outcome.
📍 Affects 2 files
src/routes/api/gitea/webhook/+server.ts#L110-L145(this comment)src/routes/api/gitea/webhook/+server.ts#L47-L68src/lib/osm.ts#L164-L180src/lib/osm.ts#L202-L209
🤖 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 `@src/routes/api/gitea/webhook/`+server.ts around lines 110 - 145, Make
approval processing durably idempotent before any OSM write: in
src/routes/api/gitea/webhook/+server.ts lines 110-145, atomically claim the
repository/issue/payload-revision key before publishing; in
src/routes/api/gitea/webhook/+server.ts lines 47-68, fail closed when reading
the push marker and verify marker writes; in src/lib/osm.ts lines 164-180,
persist or reconcile a created node before permitting retries; and in
src/lib/osm.ts lines 202-209, distinguish cleanup failure from an unknown
node-creation outcome.
Collaborator
|
Thanks, would you mind adding screenshots/screenrecording to make this easier to digest? |
Collaborator
|
Would it be possible to split this up into several smaller PRs? |
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.
Closes #1134
What
A guided, multi-step add-location wizard at a new route
/add-location/wizard. The existing/add-locationpage is left untouched so the two can run side by side.Flow
online-or-mobileGitea label; not placed on the map.UX details
opening_hourssyntax — multiple ranges per day (lunch breaks), 24/7, live preview + validation via the existingopening_hoursdep.extratags+namedetails).Data flow & spam control
osm-approvedlabel), a webhook (/api/gitea/webhook, HMAC-verified) can push:submit_place→ instant map draft, idempotent ongitea-<issue>;Ops prerequisites (not in this PR)
online-or-mobile,update-location,osm-approved.places_sourcetoken + import origin (BTCMAP_API_RPC_TOKEN,BTCMAP_PLACE_IMPORT_ORIGIN), and — new to the org — an OSM bot account + OAuth2 token (OSM_OAUTH_TOKEN), plusGITEA_WEBHOOK_SECRET.Notes
pnpm run format:fix && check && lint && testall green; production build passes.This is offered as a proof-of-concept for the flow and the approve-to-push architecture; feedback very welcome, especially on the OSM write path.
🤖 Generated with Claude Code
Summary by CodeRabbit