Add Interactive Leaflet Map Visualizing Global Fundable Streams - #576
Add Interactive Leaflet Map Visualizing Global Fundable Streams#576RemmyAcee wants to merge 2 commits into
Conversation
|
@RemmyAcee Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Done, Close: #567 |
📝 WalkthroughWalkthroughAdds an interactive Leaflet map for fundable streams with typed data, grid clustering, filtering, popups, loading and error states, dark styling, public exports, and Vitest/React Testing Library coverage. ChangesFundable map feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FundableMap
participant FundableMapView
participant Leaflet
participant PopupItem
FundableMap->>FundableMapView: pass filtered streams
FundableMapView->>Leaflet: render tiles and cluster markers
FundableMapView->>Leaflet: fit bounds to stream coordinates
Leaflet->>PopupItem: display cluster stream actions
PopupItem->>FundableMap: select a stream
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 3
🧹 Nitpick comments (2)
apps/web/src/components/map/FundableMap.test.tsx (1)
1-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for popup-item selection (click/keyboard).
None of the 54 tests exercise clicking or pressing Enter/Space on a
PopupItemto verifyonStreamSelectfires — this is one of the PR's stated accessibility features ("keyboard-accessible popup items"). Consider rendering a single-stream cluster's popup content and firingclick/keyDownon the resulting button, asserting the selection callback is invoked.🤖 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 `@apps/web/src/components/map/FundableMap.test.tsx` around lines 1 - 529, Add coverage in the FundableMapView rendering tests for PopupItem selection: render a single-stream cluster with an onStreamSelect callback, then trigger click and Enter/Space keyDown events on the popup item’s accessible button and assert the callback receives the stream. Use the existing mock stream data and testing-library queries, preserving the current rendering and accessibility assertions.apps/web/src/components/map/FundableMapView.tsx (1)
218-226: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRedundant
isMountedeffect gate (also flagged by static analysis).
FundableMapViewis only ever mounted client-side vianext/dynamic(() => import("./FundableMapView"), { ssr: false })inFundableMap.tsx. TheuseEffect+setIsMountedgate here is therefore redundant for that path, and callingsetStatesynchronously in an effect on mount triggers an extra render (brief flash tonull) as well as the ESLintreact-hooks/set-state-in-effecterror.♻️ Suggested simplification
- const [isMounted, setIsMounted] = useState(false); - - useEffect(() => { - setIsMounted(true); - }, []); - - const clusters = useMemo(() => clusterStreams(streams), [streams]); - - if (!isMounted) return null; + const clusters = useMemo(() => clusterStreams(streams), [streams]);If this component may be rendered directly (bypassing the
ssr:falsewrapper) elsewhere, keep the guard but avoid the effect (e.g. lazyuseStateinitializer checkingtypeof window).🤖 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 `@apps/web/src/components/map/FundableMapView.tsx` around lines 218 - 226, Remove the redundant isMounted state, its useEffect, and the early null return from FundableMapView, since the component is loaded through the ssr:false dynamic wrapper. Preserve the clusters useMemo and normal render path; only retain an equivalent non-effect client check if direct server-rendered usage exists elsewhere.Source: Linters/SAST tools
🤖 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 `@apps/web/src/components/map/FundableMap.test.tsx`:
- Around line 516-528: Update the “error boundary” test so the defined
ThrowingComponent is actually rendered through the FundableMap error-boundary
path, then assert the expected fallback UI rather than the normal application
role. Keep the test focused on verifying that a render error is caught and the
fallback is displayed.
In `@apps/web/src/components/map/FundableMap.tsx`:
- Around line 108-151: Update FundableMap so filteredStreams uses activeFilters
rather than the raw filters prop, ensuring internal filter state affects
rendered markers. Compute statusCounts from filteredStreams so the legend
matches visible results. Locate the intended search/status/category filter UI
and render or connect it within FundableMap so handleFilterChange is reachable;
if it belongs in a parent, document and wire that ownership through the existing
props without adding duplicate controls.
In `@apps/web/src/components/map/FundableMapView.tsx`:
- Around line 181-196: Remove the ineffective accessibility props from the
CircleMarker in the cluster-marker rendering path, or replace them with a
genuine focusable overlay that handles Tab and Enter/Space activation. Do not
leave role="button", tabIndex, or aria-label on CircleMarker unless keyboard
focus and activation are implemented on an actual DOM element.
---
Nitpick comments:
In `@apps/web/src/components/map/FundableMap.test.tsx`:
- Around line 1-529: Add coverage in the FundableMapView rendering tests for
PopupItem selection: render a single-stream cluster with an onStreamSelect
callback, then trigger click and Enter/Space keyDown events on the popup item’s
accessible button and assert the callback receives the stream. Use the existing
mock stream data and testing-library queries, preserving the current rendering
and accessibility assertions.
In `@apps/web/src/components/map/FundableMapView.tsx`:
- Around line 218-226: Remove the redundant isMounted state, its useEffect, and
the early null return from FundableMapView, since the component is loaded
through the ssr:false dynamic wrapper. Preserve the clusters useMemo and normal
render path; only retain an equivalent non-effect client check if direct
server-rendered usage exists elsewhere.
🪄 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 Plus
Run ID: 22b480a2-7f82-46cb-b83a-f9006b40bd1e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/web/package.jsonapps/web/src/app/globals.cssapps/web/src/components/map/FundableMap.test.tsxapps/web/src/components/map/FundableMap.tsxapps/web/src/components/map/FundableMapView.tsxapps/web/src/components/map/cluster-utils.tsapps/web/src/components/map/index.tsapps/web/src/components/map/types.ts
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
2 similar comments
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
definitely |
|
kindly merge |
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)
apps/web/package.json (1)
37-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
ioredisforapps/web/src/lib/redis.ts.
apps/web/src/lib/redis.tsstill importsRedis from "ioredis", whileapps/web/package.jsonno longer provides the package outside the lockfile references. Move the Redis support elsewhere or addioredisback toapps/webdependencies.🤖 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 `@apps/web/package.json` around lines 37 - 43, Restore the missing ioredis dependency for the Redis import in apps/web/src/lib/redis.ts by adding ioredis to the dependencies in apps/web/package.json, preserving the existing Redis implementation and aligning the package manifest with the import.
🤖 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.
Outside diff comments:
In `@apps/web/package.json`:
- Around line 37-43: Restore the missing ioredis dependency for the Redis import
in apps/web/src/lib/redis.ts by adding ioredis to the dependencies in
apps/web/package.json, preserving the existing Redis implementation and aligning
the package manifest with the import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df070c1a-a8f5-420f-a291-322bc76474c9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
apps/web/package.jsonapps/web/src/app/globals.css
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/globals.css
3 participants
Close: #522
All done. Here's a summary of the changes made:
Files Modified (6)
Files Created (1)
Key Features
Summary by CodeRabbit
New Features
Tests