Conversation
- Add enableOfflineMaps feature flag to config.ts - Create features/offline-maps module with types, store, hooks, components, screens - Add OfflineMapsTile to home dashboard - Add offline-maps route - Add offlineMaps translations to en.json Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
@coderabbitai Please review this PR for code quality, best practices, and potential issues. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 921 🏁 Script executed: Length of output: 36524
Review:
|
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🔴 Bug | OfflineMapsScreen.tsx |
Hook lifecycle — cancelRefs lost on modal unmount |
| 2 | 🔴 Bug | OfflineMapsScreen.tsx |
No cancel UI despite hook exporting cancelDownload |
| 3 | 🔴 Bug | useDownloadMapRegion.ts |
Cancelled downloads leave orphaned 'idle' entries |
| 4 | 🔴 Bug | useDownloadMapRegion.ts |
No duplicate download guard |
| 5 | 🟠 Perf | OfflineMapsScreen.tsx |
Multiple MapView in ScrollView — memory/jank risk |
| 6 | 🟡 Quality | useDownloadMapRegion.ts / useDeleteMapRegion.ts |
OFFLINE_MAPS_DIR duplicated |
| 7 | 🟡 Quality | useDownloadMapRegion.ts |
Unused _regionId param |
| 8 | 🟡 Quality | OfflineMapsScreen.tsx |
Redundant double-null guard in ViewRegionModal |
| 9 | 🟡 Quality | OfflineMapsScreen.tsx |
Hardcoded "#14b8a6" hex color |
| 10 | 🟡 Quality | OfflineMapsScreen.tsx |
Size category labels not i18n-ized |
| 11 | 🟡 Quality | useDownloadMapRegion.ts |
Silent error swallowing in catch |
| 12 | 🟡 Quality | OfflineMapsScreen.tsx |
handleDownload errors unhandled |
| 13 | 🟡 Quality | useDownloadMapRegion.ts |
No storage quota check |
| 14 | 🟡 UX | OfflineMapsScreen.tsx |
"View Map" is live/online, not offline tiles |
| 15 | 🟡 A11y | OfflineMapsScreen.tsx |
Icon-only buttons missing accessibilityLabel |
|
@copilot CodeRabbit has provided review feedback on this PR. Please address the comments and suggestions. Once you've made the necessary changes, I'll ask CodeRabbit to re-review. |
- Fix hook lifecycle bug: move useDownloadMapRegion to screen level - Fix duplicate check delimiter to use region.id + '-' prefix - Add cancel button wired to cancelDownload in RegionCard - cancelDownload now deletes store entry (no orphaned idle entries) - downloadRegion now fire-and-forget; throws on validation failures - AddRegionModal shows inline error and closes only on success - No failed store entry for insufficient storage (throw instead) - Replace MapView in ScrollView cards with lightweight RegionBoundsPreview - Fix double-null guard in ViewRegionModal (single guard) - Extract OFFLINE_MAPS_DIR, REGION_TEAL_COLOR, REGION_FILL_COLOR to constants.ts - Add i18n keys for size categories, errors, accessibility labels - Update viewMapOnlineNote to clarify preview vs offline map - Add accessibilityLabel and accessibilityRole to all icon-only Pressables - Add storage quota check with error thrown on insufficient space - Log errors in catch blocks Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
All 15 issues from CodeRabbit's review have been addressed in commit 🔴 Critical bugs fixed:
🟠 Performance fixed:
🟡 Code quality fixed:
|
|
@copilot CodeRabbit has reviewed this PR and provided feedback. Please address the review comments and push updates. Once you've made the changes, the monitor will ask CodeRabbit to re-review. |
There was a problem hiding this comment.
Pull request overview
Adds an Offline Maps feature to the Expo app that lets users download predefined map regions for offline use, track download progress, and manage stored regions behind a feature flag.
Changes:
- Introduces an
offline-mapsfeature module (types, constants, SQLite-backed Legend State store, region presets + size estimation, and download/delete hooks). - Adds Offline Maps UI: home dashboard tile, Offline Maps screen with region download modal, storage banner, and region management actions.
- Adds route + feature flag + English translations for Offline Maps.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/expo/lib/i18n/locales/en.json | Adds Offline Maps translation strings. |
| apps/expo/features/offline-maps/utils/regions.ts | Defines predefined regions and size/tile-count estimation helpers. |
| apps/expo/features/offline-maps/types.ts | Adds types for regions and download status. |
| apps/expo/features/offline-maps/store/offlineMaps.ts | Adds Legend State + SQLite persisted store for offline map regions. |
| apps/expo/features/offline-maps/screens/index.ts | Re-exports the Offline Maps screen. |
| apps/expo/features/offline-maps/screens/OfflineMapsScreen.tsx | Implements the Offline Maps screen UI (list, progress, modals, MapView preview). |
| apps/expo/features/offline-maps/index.ts | Exports feature hooks/types. |
| apps/expo/features/offline-maps/hooks/useOfflineMapsStorageInfo.ts | Computes aggregate storage + counts for the feature. |
| apps/expo/features/offline-maps/hooks/useOfflineMapRegions.ts | Reads and sorts regions from the store. |
| apps/expo/features/offline-maps/hooks/useDownloadMapRegion.ts | Starts/cancels background “download” simulation with storage checks + duplicate guard. |
| apps/expo/features/offline-maps/hooks/useDeleteMapRegion.ts | Deletes a region from filesystem + store. |
| apps/expo/features/offline-maps/hooks/index.ts | Barrel export for Offline Maps hooks. |
| apps/expo/features/offline-maps/constants.ts | Centralizes offline maps directory + shared colors. |
| apps/expo/features/offline-maps/components/OfflineMapsTile.tsx | Adds dashboard tile entrypoint for Offline Maps. |
| apps/expo/config.ts | Enables enableOfflineMaps feature flag. |
| apps/expo/app/(app)/offline-maps.tsx | Adds the Offline Maps route. |
| apps/expo/app/(app)/(tabs)/(home)/index.tsx | Registers the Offline Maps tile in dashboard layout and tile metadata. |
You can also share your feedback on Copilot code review. Take the survey.
| <Text variant="footnote" className="text-muted-foreground mt-1"> | ||
| {completedCount} {t('offlineMaps.completedRegions')} | ||
| {downloadingCount > 0 | ||
| ? ` · ${downloadingCount} ${t('offlineMaps.downloading', { count: downloadingCount })}` | ||
| : ''} |
There was a problem hiding this comment.
StorageBanner is double-counting the downloading regions: the string prepends downloadingCount and also calls t('offlineMaps.downloading', { count }) where the translation already includes {{count}}. This will render like "· 2 2 downloading". Also, completedCount can render as "1 downloaded regions" (plural).
Suggestion: either remove the leading ${downloadingCount} and let i18n handle the count (and/or add proper pluralized keys for "downloaded region(s)").
| // Guard: prevent duplicate downloads for the same region | ||
| const existingEntries = Object.values(offlineMapsStore.get()); | ||
| const alreadyActive = existingEntries.some( | ||
| (entry) => entry.id.startsWith(`${region.id}-`) && entry.status === 'downloading', | ||
| ); | ||
| if (alreadyActive) return; | ||
|
|
There was a problem hiding this comment.
The duplicate-download guard silently returns when a matching region is already downloading. In AddRegionModal, handleDownload treats a resolved onStartDownload as success and closes the modal, so the user gets no feedback and no download starts.
Suggestion: make downloadRegion signal this case (e.g., throw a specific error or return a boolean) so the modal can keep open and display a message (you already added offlineMaps.duplicateDownloadTitle/message strings that currently aren’t used).
| for (let tick = 1; tick <= TOTAL_TICKS; tick++) { | ||
| if (signal.cancelled) return; | ||
| await new Promise<void>((resolve) => setTimeout(resolve, TICK_MS)); | ||
| const progress = Math.round((tick / TOTAL_TICKS) * 100); | ||
| const downloadedSize = Math.round((tick / TOTAL_TICKS) * estimatedSize); | ||
| offlineMapsStore[id].set((prev) => ({ | ||
| ...prev, | ||
| progress, | ||
| downloadedSize, | ||
| status: 'downloading', | ||
| updatedAt: new Date().toISOString(), | ||
| })); |
There was a problem hiding this comment.
runDownloadInBackground only checks signal.cancelled before the sleep. If cancelDownload is called after that check but before the store update, the next tick will still write progress and can re-create the store entry after it was deleted, leaving a cancelled region visible again (and potentially persisted).
Suggestion: add a second if (signal.cancelled) return; immediately after the await setTimeout(...) (or before writing to offlineMapsStore[id]) to prevent any post-cancel writes.
| /** Returns total storage used by all completed offline map regions */ | ||
| export function useOfflineMapsStorageInfo() { | ||
| return use$(() => { | ||
| const regions = Object.values(offlineMapsStore.get()); | ||
| const totalSize = regions.reduce((sum, r) => sum + r.downloadedSize, 0); | ||
| const completedCount = regions.filter((r) => r.status === 'completed').length; | ||
| const downloadingCount = regions.filter((r) => r.status === 'downloading').length; | ||
| return { totalSize, completedCount, downloadingCount, totalCount: regions.length }; |
There was a problem hiding this comment.
The doc comment says this hook returns storage used by completed regions, but totalSize is computed by summing downloadedSize for all regions (including downloading/failed). Either filter to status === 'completed' when computing totalSize, or update the comment (and any UI labels) to reflect that partial downloads are included.
apps/expo/lib/i18n/locales/en.json
Outdated
| "duplicateDownloadTitle": "Already Downloading", | ||
| "duplicateDownloadMessage": "This region is already being downloaded.", | ||
| "insufficientStorageTitle": "Not Enough Storage", | ||
| "insufficientStorageMessage": "There is not enough free space on your device to download this region.", |
There was a problem hiding this comment.
offlineMaps.duplicateDownloadTitle, offlineMaps.duplicateDownloadMessage, and offlineMaps.insufficientStorageTitle are added but not referenced anywhere in the code (search in apps/expo shows no usages). Either wire these into the UI (e.g., Alert/toast) or remove them to avoid dead translation strings.
|
@copilot The |
|
@copilot In addition to fixing the check-types CI failure, please also address the 5 CodeRabbit inline review comments. |
- Fix TS2322: replace invalid 'download' icon with 'inbox-arrow-down' in OfflineMapsTile - Fix TS2532: add @ts-ignore for Legend-State Proxy pattern in useDeleteMapRegion and useDownloadMapRegion - Fix TS2322: wrap Icon with View for absolute positioning (style prop not on Icon) - Fix StorageBanner double-count: use t('completedRegions', {count}) with _one/_other plural keys - Fix duplicate download: throw ERR_DUPLICATE_DOWNLOAD instead of silently returning - Fix runDownloadInBackground: add second signal.cancelled check after setTimeout sleep - Fix useOfflineMapsStorageInfo: totalSize now sums only completed regions - Extract ERR_DUPLICATE_DOWNLOAD and ERR_INSUFFICIENT_STORAGE to constants.ts - Wire duplicateDownloadMessage into AddRegionModal error handler - Remove unused duplicateDownloadTitle and insufficientStorageTitle i18n keys Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Fixed in commit
Also addressed the Copilot PR reviewer comments in the same commit: fixed
All Copilot PR reviewer feedback addressed in commit
|
Both the TypeScript fixes:
5 CodeRabbit comments:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
enableOfflineMapsfeature flag toapps/expo/config.tsapps/expo/features/offline-maps/types.tsapps/expo/features/offline-maps/store/offlineMaps.ts(Legend State + SQLite)apps/expo/features/offline-maps/constants.ts(shared OFFLINE_MAPS_DIR, REGION_TEAL_COLOR, REGION_FILL_COLOR, ERR_DUPLICATE_DOWNLOAD, ERR_INSUFFICIENT_STORAGE)apps/expo/features/offline-maps/utils/regions.ts(predefined regions, size estimation)useOfflineMapRegions,useDownloadMapRegion,useDeleteMapRegion,useOfflineMapsStorageInfoOfflineMapsTile.tsxdashboard tile componentapps/expo/features/offline-maps/screens/OfflineMapsScreen.tsxapps/expo/app/(app)/offline-maps.tsxrouteOfflineMapsTilein home dashboard (index.tsx)offlineMapstranslations toen.json"download"icon with"inbox-arrow-down"inOfflineMapsTile@ts-ignore(Legend-State Proxy pattern) on dynamic store property access<Icon>in<View>(styleprop not available onIcon)StorageBannerdouble-count — use i18n plural keys (completedRegions_one/other) instead of manual${count} + t(key)downloadRegionthrowsERR_DUPLICATE_DOWNLOADinstead of silently returning; modal shows inline errorrunDownloadInBackground— secondsignal.cancelledcheck aftersetTimeoutsleep prevents post-cancel store writesuseOfflineMapsStorageInfo—totalSizenow sums onlycompletedregionsduplicateDownloadTitle/insufficientStorageTitlei18n strings;duplicateDownloadMessagewired into error handlerconstants.ts(no magic string matching)Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.