feat: add interactive map search with property clustering and search area - #153
feat: add interactive map search with property clustering and search area#153Tanya-garg10 wants to merge 6 commits into
Conversation
|
@Tanya-garg10 is attempting to deploy a commit to the Akshay Kumar's projects Team on Vercel. A member of the Team first needs to authorize it. |
akshay0611
left a comment
There was a problem hiding this comment.
Review: feat: add interactive map search with property clustering and search area
This PR introduces an AI-powered natural language search hero, an interactive Leaflet map with clustering, an NLP query parser, and tests. The scope is ambitious and the components are thoughtfully built — this is some impressive work.
What looks great ✨
- The
nlp-parser.tsis surprisingly thorough — it handles prices, ratings, locations (with both known city names and fallback preposition-based detection), amenities, types, guests, and bedrooms, all with regex. The amenity synonym mapping is clean and easy to extend. - The
AISearchHeroUX is polished — the gradient background, suggestion chips, editable filter badges with inline editing, and the "Reset Search" flow all feel premium. - The
MapComponentclustering algorithm (pixel-based proximity) is a smart lightweight alternative to bringing in a full cluster library. The dark/light tile switching viauseThemeis a thoughtful touch. - The
MapPlaceholderloading state is beautifully designed with the animated pulse markers and grid lines — great attention to detail. - You wrote tests for the NLP parser covering multiple query patterns, including edge cases. Solid.
Blockers
-
package.json—leafletand@types/leafletneed to be added as dependencies. Without them,npm installwon't install Leaflet and the build will fail with a module resolution error. -
components/MapComponent.tsx:93—property.pricePerNight * 100shows incorrect prices. In the codebase,pricePerNightis already a plain integer in INR (e.g.,120,450— seelib/dummy-data.ts:159-374). Multiplying by 100 would display ₹45,000 instead of ₹450 on the map markers. This should be justproperty.pricePerNight(orMath.round(property.pricePerNight)).
Optional suggestions (feel free to ignore)
- The static
import L from 'leaflet'at the top ofMapComponentcan cause SSR/build failures in Next.js since Leaflet accesseswindowat module level. You might want to usenext/dynamicwithssr: falsein the parent page that renders the map, or import Leaflet inside auseEffect/ dynamic import.
Verdict
REQUEST_CHANGES
These are two concrete fixes — add the Leaflet packages to package.json and remove the * 100 on the price display. The components themselves are really well-crafted and the overall direction is excellent.
Remove erroneous * 100 multiplier from pricePerNight calculations. pricePerNight is already a plain INR integer, so multiplying inflated prices 100x (e.g. Rs.450 was showing as Rs.45,000). Fixes both the price marker label and the popup card price display.
|
Thank you for the detailed review and feedback! I've addressed the requested changes: I also reviewed the SSR suggestion for Leaflet and will keep it in mind for future improvements. The requested changes have been pushed. Please take another look @akshay0611 |
akshay0611
left a comment
There was a problem hiding this comment.
Review: feat: add interactive map search with property clustering and search area
This is a well-scoped PR that delivers three solid components — an AI-powered natural language search, an interactive map with clustering, and an NLP query parser with tests. The contributor already addressed the price display issue from the previous review. The overall code quality is strong.
What looks great ✨
- The
nlp-parser.tsis remarkably thorough — it handles pricing, location (known cities + fallback regex), ratings, amenity synonyms, property types, guests, and bedrooms. The synonym mapping structure makes it easy to extend. AISearchHeroUX is polished: gradient backgrounds, suggestion chips, inline-editable filter badges, and the reset flow all feel premium. The attention to micro-interactions (hover states, transitions) shows real care.- The custom pixel-based clustering algorithm in
MapComponentis a smart choice — avoids pulling in a heavy cluster library while still handling dense marker areas cleanly. - The
MapPlaceholderloading skeleton with animated pulse markers and decorative grid lines is a delightful touch. - Tests cover the main query patterns and edge cases. Solid coverage for a first pass.
Blockers
package.json—leafletand@types/leafletmust be added as dependencies.MapComponent.tsxdoesimport L from 'leaflet'andimport 'leaflet/dist/leaflet.css', but the packages aren't listed inpackage.json. This will cause a build failure in CI and for anyone checking out the repo. Fix: runpnpm add leaflet @types/leafletand commit the updatedpackage.json+pnpm-lock.yaml.
Optional suggestions (feel free to ignore)
- The static
import L from 'leaflet'at the top ofMapComponentmay cause SSR issues since Leaflet accesseswindowat module level. You might want to usenext/dynamicwithssr: falsein the parent page, or import Leaflet inside auseEffect. The existingtypeof window === 'undefined'guards help, but the import itself could still cause a build-time failure on the server.
Verdict
REQUEST_CHANGES
Just one remaining issue — adding the Leaflet dependency. The price fix you already pushed was spot-on. You're very close — the code is clean, well-structured, and clearly tested locally. Ship that dependency update and this will be ready to land.
|
Hi! I've addressed the remaining blocker: The latest changes have been pushed. Please review when you get a chance @akshay0611 |
akshay0611
left a comment
There was a problem hiding this comment.
Review: feat: add interactive map search with property clustering and search area
This PR introduces an interactive Leaflet map with clustering, an NLP search hero, and a completely revamped properties page with a split-pane map/list layout. The scope is ambitious and the implementation is polished — impressive work, Tanya.
What looks great ✨
- The custom pixel-based clustering algorithm in
MapComponentis a smart lightweight alternative to pulling in a cluster library. It handles dense areas cleanly and the cluster zoom-on-click interaction is intuitive. - The dark/light tile switching via
useThemein the map shows real attention to detail. AISearchHeroUX is genuinely premium — the gradient hero section, suggestion chips, inline-editable filter badges, and the "Reset Search" flow all feel like a production feature.- The
nlp-parser.tsis thorough: prices, locations (with fallback preposition regex), ratings, amenity synonyms, property types, guests, and bedrooms. Good test coverage too. - The split-pane layout on the properties page with hover/selection sync between the list and map is smooth and well-executed.
- You already fixed the previous review feedback — the Leaflet dependency is in
package.json, MapComponent usesnext/dynamicwithssr: false, and the map markers show correct prices. Solid response to feedback.
Blockers
app/properties/page.tsx:222— The mobile preview card overlay still hasselectedProperty.pricePerNight * 100. Prices like ₹310 would show as ₹31,000. The map markers were fixed correctly (Math.round(property.pricePerNight).toLocaleString()), but this second instance was missed. Remove the* 100to match the same pattern used elsewhere.
Optional suggestions (feel free to ignore)
- Consider using
next/imageinstead of<img>in the mobile card overlay (app/properties/page.tsx:210). The existing codebase standard isnext/image, so this would avoid layout shift and stay consistent.
Verdict
REQUEST_CHANGES
You're very close — the codebase-wide structure is clean, the component design is thoughtful, and you've already addressed the prior review feedback well. This is just one lingering * 100 that slipped through. Fix that and this is ready to land.
|
I've addressed the requested change: ✅ Removed the unnecessary * 100 from the mobile preview card overlay in app/properties/page.tsx, so the displayed price is now consistent with the map markers and the rest of the application. The latest changes have been pushed. Please take another look @akshay0611 |
akshay0611
left a comment
There was a problem hiding this comment.
Review: feat: add interactive map search with property clustering and search area
This PR delivers a genuinely ambitious feature — a responsive Leaflet map with custom clustering, viewport-aware filtering, marker/list synchronization, and a polished Airbnb-style split layout on the properties page. The scope is large and the craftsmanship shows.
What looks great ✨
- The custom pixel-based clustering algorithm in
MapComponentis a smart, lightweight alternative to pulling in a heavy cluster library, and zoom-on-cluster-click is an intuitive interaction. - Dark/light basemap switching via
useThemeshows real attention to detail. - The "Search This Area" / "Reset Map Filter" flow with the
isInternalMapChangeguard is thoughtfully designed to avoid recursive viewport resets. - The
MapPlaceholderloading skeleton — animated pulse markers, decorative grid lines, and the river/parks mimics — is a delightful touch that keeps the empty state premium. - You resolved the earlier
* 100price bug consistently across the map markers, popups, and the mobile preview card. Clean follow-through on feedback.
Blockers
components/PropertyCard.tsx:95-98— The card's<Link>is nowhref="#"withe.preventDefault(), and it only navigates whenonViewMapis passed. ButonViewMapis never passed to anyPropertyCardin the codebase, so clicking a property card on the home page (and anywhere the card is used outside the map) now does nothing — it no longer routes to/property/[id]. This breaks existing navigation.
Suggested fix — keep the default detail navigation and only intercept when the map handler is actually present:<Link href={`/property/${property.id}`} onClick={onViewMap ? (e) => { e.preventDefault(); onViewMap(property); } : undefined} >
Optional suggestions (feel free to ignore)
- On the properties page,
selectedPropertyis looked up from the fullpropertyListrather thanfiltered, so the mobile preview card could show a listing that a filter has since removed. Consider finding it infiltered/sorted. - The map controls and card prices all use ₹ while
FilterSidebarnow labels price in$— a small currency-label pass would keep things consistent.
Verdict
REQUEST_CHANGES
You're genuinely close — the map itself is polished and the state management around viewport/reset is thoughtful. This is really one fix: keep the property card navigating to the detail page by default so the existing listing flow doesn't regress. Fix that link and this is ready to land.
|
I've addressed the requested change and raised a new PR with the fix. ✅ Restored the default The latest changes have been pushed and the PR is ready for review. Please take another look @akshay0611 |
@Tanya-garg10 Kindly keep the fix in this PR only, it helps to keep the scope clean! |
|
Okay!! |
4854019 to
974db5d
Compare
Description
This PR introduces an Interactive Map Search feature to improve the property discovery experience by allowing users to explore listings geographically.
Features Added
This feature enhances location-based property exploration and aligns with the project's roadmap for a dedicated map view.
Closes #121
Type of change
How Has This Been Tested?
pnpm dev)pnpm lint)pnpm build)Screenshots/Video
2026-07-14.14-19-17.1.1.1.mp4
Checklist: