Merged
Conversation
…PI call - Added new function that updates localforage cache with provided Place data - Eliminates redundant API call when we already have server-fetched data - Type-safe implementation with proper error handling
…data copying - Use updatePlaceInCache with server data instead of redundant API call - Replace initializeData data copying with $: reactive declarations - Simplifies code and leverages Svelte's reactivity properly - Removes unnecessary local variable duplications
✅ Deploy Preview for btcmap ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
Review Summary by QodoOptimize merchant page with reactive declarations and cache updates
WalkthroughsDescription• Add updatePlaceInCache function to update cache with server data without API call • Replace manual data copying with Svelte reactive declarations in merchant page • Eliminate redundant API call by using pre-fetched server data directly • Simplify component logic and leverage Svelte's reactivity system Diagramflowchart LR
A["Server Data"] -->|"updatePlaceInCache"| B["LocalForage Cache"]
A -->|"Reactive Declarations"| C["Component Variables"]
D["initializeData"] -->|"Simplified"| C
E["updateSinglePlace"] -->|"Replaced with"| F["updatePlaceInCache"]
File Changes1. src/routes/merchant/[id]/+page.svelte
|
Contributor
Code Review by Qodo
1. Undeclared reactive variables
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an exported cache-update function for places, refactors the merchant page to use reactive bindings and dynamically call the new cache updater, introduces place→OSM tag transformation utilities, and adds an in-module verified-date cache in the map setup. Changes
Sequence DiagramsequenceDiagram
participant Merchant as Merchant Page
participant Cache as LocalForage (places_v4)
participant Main as Main Thread
participant Store as Places Store
Merchant->>Cache: read `places_v4`
Cache-->>Merchant: return array
alt place.deleted_at set
Merchant->>Merchant: remove place from array
else place exists
Merchant->>Merchant: update existing place entry
else new place
Merchant->>Merchant: append place to array
end
Merchant->>Cache: write updated `places_v4`
Cache-->>Merchant: confirm write
Merchant->>Main: yieldToMain()
Main-->>Merchant: resume
Merchant->>Store: places.set(updated array)
Store-->>Merchant: store updated
Merchant-->>Merchant: return updated Place | null
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/sync/places.ts (1)
452-493: Reduce duplicated cache-mutation logic between single-place update paths.
updatePlaceInCachenow duplicates most ofupdateSinglePlace’s delete/upsert/persist/store-update flow. This increases drift risk when one path changes and the other doesn’t.♻️ Suggested refactor (shared helper)
+const applyPlaceToCache = async (incomingPlace: Place): Promise<Place | null> => { + const cachedPlaces = await localforage.getItem<Place[]>("places_v4"); + if (!cachedPlaces) return null; + + if (incomingPlace.deleted_at) { + const updatedPlaces = cachedPlaces.filter((p) => p.id !== incomingPlace.id); + if (updatedPlaces.length !== cachedPlaces.length) { + await localforage.setItem("places_v4", updatedPlaces); + await yieldToMain(); + places.set(updatedPlaces); + } + return null; + } + + const placeIndex = cachedPlaces.findIndex((p) => p.id === incomingPlace.id); + const updatedPlaces = + placeIndex !== -1 + ? [...cachedPlaces.slice(0, placeIndex), incomingPlace, ...cachedPlaces.slice(placeIndex + 1)] + : [...cachedPlaces, incomingPlace]; + + await localforage.setItem("places_v4", updatedPlaces); + await yieldToMain(); + places.set(updatedPlaces); + return incomingPlace; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sync/places.ts` around lines 452 - 493, updatePlaceInCache duplicates the cache mutation flow found in updateSinglePlace (delete vs upsert, persist to localforage, await yieldToMain, and places.set), so extract that shared behavior into a single helper (e.g., upsertOrRemovePlaceInCache or syncPlaceToCache) that accepts the Place and cachedPlaces array and performs: 1) remove when place.deleted_at is set (filter, persist, yieldToMain, places.set) 2) upsert otherwise (replace or append, persist, yieldToMain, places.set) and returns the resulting Place or null; then replace the duplicated blocks in updatePlaceInCache and updateSinglePlace to call this helper and keep their try/catch flow intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/merchant/`[id]/+page.svelte:
- Line 143: The current const assignment "const name = data.name" will not
update when the page's "data" object changes; change it to a reactive
declaration so the variable tracks updates (e.g., replace the const with a
reactive binding such as using the Svelte reactive label to set "name" from
"data.name"); update the declaration that sets "name" to mirror the pattern used
for other fields in this component so headings/meta text and i18n values stay in
sync with "data".
---
Nitpick comments:
In `@src/lib/sync/places.ts`:
- Around line 452-493: updatePlaceInCache duplicates the cache mutation flow
found in updateSinglePlace (delete vs upsert, persist to localforage, await
yieldToMain, and places.set), so extract that shared behavior into a single
helper (e.g., upsertOrRemovePlaceInCache or syncPlaceToCache) that accepts the
Place and cachedPlaces array and performs: 1) remove when place.deleted_at is
set (filter, persist, yieldToMain, places.set) 2) upsert otherwise (replace or
append, persist, yieldToMain, places.set) and returns the resulting Place or
null; then replace the duplicated blocks in updatePlaceInCache and
updateSinglePlace to call this helper and keep their try/catch flow intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f58a8321-f5c4-4107-8c47-47c1e20f86cc
📒 Files selected for processing (2)
src/lib/sync/places.tssrc/routes/merchant/[id]/+page.svelte
- Create /transforms/place.ts with reusable functions: - mapPayment(): Maps payment fields to PayMerchant type - getBoosted(): Calculates boosted status - getContactFields(): Gets contact with fallback to osm:contact fields - getPaymentMethod(): Concatenates payment method strings - buildOsmTags(): Builds OSM tags object for tag modal - Update merchant detail page server to use new functions - Reduces code duplication and improves maintainability
- Add verifiedCache WeakMap to memoize verifiedArr results - WeakMap allows garbage collection when Place objects are no longer referenced - Avoids re-computing verified dates on every render in components
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/transforms/place.ts (1)
3-10: Usetypeinstead ofinterfaceforContactFields.Per coding guidelines, prefer
typeoverinterfacefor new type definitions in TypeScript.Suggested fix
-export interface ContactFields { - phone?: string; - website?: string; - email?: string; - twitter?: string; - instagram?: string; - facebook?: string; -} +export type ContactFields = { + phone?: string; + website?: string; + email?: string; + twitter?: string; + instagram?: string; + facebook?: string; +};As per coding guidelines: "Prefer
typeoverinterfacefor new type definitions in TypeScript; useinterfaceonly when needed (declaration merging or class implementation)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/transforms/place.ts` around lines 3 - 10, Replace the exported interface ContactFields with an exported type alias that preserves the same optional string properties (phone, website, email, twitter, instagram, facebook); update any references to ContactFields to continue using the same name (no API change) and ensure the exported declaration reads "export type ContactFields = { ... }" with the same property signatures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/transforms/place.ts`:
- Around line 3-10: Replace the exported interface ContactFields with an
exported type alias that preserves the same optional string properties (phone,
website, email, twitter, instagram, facebook); update any references to
ContactFields to continue using the same name (no API change) and ensure the
exported declaration reads "export type ContactFields = { ... }" with the same
property signatures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98d9378c-4642-4318-ad12-7f11b917c895
📒 Files selected for processing (3)
src/lib/map/setup.tssrc/lib/transforms/place.tssrc/routes/merchant/[id]/+page.server.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/map/setup.ts (1)
589-620:⚠️ Potential issue | 🟠 MajorCache key is too coarse; can serve stale verification dates and leak memory over time.
Line 594 returns cached data by
place.idonly. If fresh API data for the same place has updated verification fields, this still returns old values. Also, this module-scope cache has no eviction bound.Proposed fix (cache by content signature, not only id)
-// Cache verification arrays per place id; entries are overwritten whenever fresh Place data arrives via stores -const verifiedCache = new Map<number, string[]>(); +type VerifiedCacheEntry = { signature: string; values: string[] }; +const verifiedCache = new Map<number, VerifiedCacheEntry>(); + +const verifiedSignature = (place: Place): string => + [ + place["osm:survey:date"] ?? "", + place["osm:check_date"] ?? "", + place["osm:check_date:currency:XBT"] ?? "", + ].join("|"); export const verifiedArr = (place: Place): string[] => { const cacheKey = place.id; - if (verifiedCache.has(cacheKey)) { - return verifiedCache.get(cacheKey)!; - } + const signature = verifiedSignature(place); + const cached = verifiedCache.get(cacheKey); + if (cached && cached.signature === signature) return cached.values; const verified: string[] = []; @@ - verifiedCache.set(cacheKey, verified); + verifiedCache.set(cacheKey, { signature, values: verified }); return verified; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/map/setup.ts` around lines 589 - 620, The current verifiedCache (module-scope Map) keyed only by place.id causes stale results and unbounded memory growth; update verifiedArr to compute a content signature from the relevant verification fields (e.g., concatenate or hash place["osm:survey:date"], place["osm:check_date"], place["osm:check_date:currency:XBT"] and/or a place.lastUpdated field) and use a composite cache key like `${place.id}:${signature}` when reading/writing verifiedCache, so new API data with changed verification fields produces a different cache entry; also add a simple eviction bound on verifiedCache (e.g., cap size and delete oldest entries or implement an LRU policy) to prevent memory leaks and ensure stale entries are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/map/setup.ts`:
- Around line 589-620: The current verifiedCache (module-scope Map) keyed only
by place.id causes stale results and unbounded memory growth; update verifiedArr
to compute a content signature from the relevant verification fields (e.g.,
concatenate or hash place["osm:survey:date"], place["osm:check_date"],
place["osm:check_date:currency:XBT"] and/or a place.lastUpdated field) and use a
composite cache key like `${place.id}:${signature}` when reading/writing
verifiedCache, so new API data with changed verification fields produces a
different cache entry; also add a simple eviction bound on verifiedCache (e.g.,
cap size and delete oldest entries or implement an LRU policy) to prevent memory
leaks and ensure stale entries are removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3514b136-671f-4a2c-892c-13f9bb29da1e
📒 Files selected for processing (2)
src/lib/map/setup.tssrc/lib/transforms/place.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/transforms/place.ts
Collaborator
|
I opened a few merchants from the map view and the performance was clearly better than before, near instant in many cases, but when I tried jumping to a few random places directly by typing their ids into an address bar, most of them failed to load. This link works: https://btcmap.org/merchant/7361 While the same preview link doesn't: https://deploy-preview-790--btcmap.netlify.app/merchant/7361 Not sure if related to this RR or Netlify preview env quirks 
|
- Add LRU cache size management (MAX_CACHE_SIZE=100) to verifiedArr to prevent unbounded memory growth - Add SSR browser guard to updatePlaceInCache for runtime safety - Convert updatePlaceInCache to dynamic import in onMount for explicit client-only loading - Fix mapPayment return type to PayMerchant | undefined - Remove unused updatePlaceInCache import 🤖 Generated with [opencode](https://opencode.ai)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving the merchant detail view’s performance and data handling by moving repeated place-field derivations into shared utilities, introducing a cache-aware place update helper, and adjusting merchant page data bindings.
Changes:
- Refactors merchant server-load processing to use new
$lib/transforms/placehelpers (OSM tags, contact fields, payment mapping, boosted state). - Adds
updatePlaceInCache(place)to update LocalForage + in-memoryplacesstore using already-fetchedPlacedata. - Adds per-place caching for
verifiedArrcomputations and updates LeafletMaptype imports to avoid name conflicts.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/merchant/[id]/+page.svelte | Switches merchant page to reactive bindings and uses the new cache update helper on mount. |
| src/routes/merchant/[id]/+page.server.ts | Refactors place-derived fields (payment/contact/osmTags/boosted) into shared transform utilities. |
| src/lib/transforms/place.ts | New utility module for deriving contact/payment/boosted state and building OSM-style tags from Place. |
| src/lib/sync/places.ts | Adds updatePlaceInCache for updating LocalForage and the places store using an existing Place object. |
| src/lib/map/setup.ts | Renames Leaflet Map import to LeafletMap and introduces caching for verifiedArr. |
…Tags Using the existing OSMTags type directly removes the explicit-any usage and ensures the intermediate accumulator is consistent with the return type. 🤖 Generated with [opencode](https://opencode.ai)
Reactive $: assignments require prior let declarations in Svelte/TypeScript. Restores explicit types for all fields derived from server data and brings back safe [] defaults for merchantEvents and filteredCommunities so downstream slice/iteration cannot throw on first render. Also converts const name to a reactive declaration so it tracks data updates. 🤖 Generated with [opencode](https://opencode.ai)
… entries A Place object with the same id but newer verification dates would previously return the first-computed array forever. Keying on id + updated_at means any update to the Place naturally produces a cache miss and a fresh computation. Falls back to id-only when updated_at is absent. 🤖 Generated with [opencode](https://opencode.ai)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/transforms/place.ts`:
- Around line 23-48: mapPayment and getPaymentMethod only read osm:payment:*
keys and miss canonical payment:* fields; update both to check canonical keys
first and fall back to osm:payment:* equivalents (e.g., check
place["payment:uri"] then place["osm:payment:uri"], same for payment:pouch and
payment:coinos) so payment links/icons aren’t lost, and extend getPaymentMethod
to include "payment:onchain", "payment:lightning", and
"payment:lightning_contactless" (checking canonical then osm-prefixed keys) when
resolving the method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: babdd0da-49ed-40d1-b3de-6bd8b8e25c5b
📒 Files selected for processing (4)
src/lib/map/setup.tssrc/lib/sync/places.tssrc/lib/transforms/place.tssrc/routes/merchant/[id]/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/sync/places.ts
Deleted places return only {id} without include_deleted=true, causing
a 500 when the server load tries to access lat, lon, icon and other
fields on the minimal response object.
🤖 Generated with [opencode](https://opencode.ai)
DOMPurify requires a DOM environment and crashes during server-side rendering. Skip sanitization on the server — the input is OSM opening_hours data, not arbitrary user HTML, so this is safe. Fixes 500 error when viewing deleted merchants (and any merchant with opening_hours during SSR). 🤖 Generated with [opencode](https://opencode.ai)
Verifies that deleted merchants return 200 (not 500), display the removal notice banner, show the merchant name with (Deleted) suffix, and still render the map container. 🤖 Generated with [opencode](https://opencode.ai)
- formatOpeningHours now uses simple escapeHtml instead of DOMPurify, which is SSR-safe and prevents XSS vulnerabilities - escapeHtml function refactored to use simple string replace instead of document.createElement (which breaks on SSR) - Removed unused DOMPurify import This fixes the XSS vulnerability where malicious opening_hours data containing </span><script>... could be executed during SSR. 🤖 Generated with [opencode](https://opencode.ai)
bubelov
added a commit
that referenced
this pull request
Mar 5, 2026
Removed 8 fields from Place type: - line: unused OSM field - payment:uri, payment:pouch, payment:coinos: derived fields (created in buildOsmTags) - payment:lightning, payment:onchain, payment:lightning_contactless: derived fields - osm:payment:lightning:requires_companion_app: never requested or used These fields were either never populated by the API or were derived fields that shouldn't have been in the Place type. 🤖 Generated with [opencode](https://opencode.ai)
The e2e tests depend on merchant ID 7361 remaining in the API with deleted_at set. If this merchant gets purged in the future, the tests will need a different deleted merchant ID. 🤖 Generated with [opencode](https://opencode.ai)
escapedcat
pushed a commit
that referenced
this pull request
Mar 6, 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
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.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor