[History] Favorites - #292
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughAdds cross-platform Favorites with durable telemetry ranges, protected deletion, curated media storage, native and TypeScript APIs, trim previews, Favorite history navigation, and updated History UI flows. Legacy ride-media filename storage is removed, with migrations, tests, showcases, and E2E coverage added. ChangesFavorites and telemetry persistence
History and trimming
Supporting changes
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant HistoryUI
participant FavoriteStore
participant NativeBridge
participant TelemetryRepository
participant FavoriteMediaStore
HistoryUI->>FavoriteStore: create or update Favorite
FavoriteStore->>NativeBridge: call Favorite API
NativeBridge->>TelemetryRepository: persist range and summary
HistoryUI->>NativeBridge: import Favorite Media
NativeBridge->>FavoriteMediaStore: copy, hash, and reconcile media
FavoriteMediaStore-->>HistoryUI: return manifest-backed media
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
# Conflicts: # modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDatabase.kt # modules/vescape-core/ios/telemetry/TelemetryDatabase.swift # src/screens/main/map/MainMapLayers.tsx # src/screens/main/overlays/MainOverlays.tsx
# Conflicts: # src/screens/main/MainScreen.tsx # src/screens/main/history/HistoryTelemetryPanel.tsx # src/screens/main/overlays/MainOverlays.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRepository.kt (1)
647-663: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSanitize-and-copy block is now duplicated three times in this file.
Lines 649-657 are identical to
rebuildBuckets(684-692) andflushNow(816-824). Extracting aprivate fun sanitizedBucketPoints(points): List<BucketTelemetryPoint>would keep the three paths from drifting when a new exclusion flag is added.As per coding guidelines, "remove unused code and avoid duplicate code".
🤖 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 `@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRepository.kt` around lines 647 - 663, Extract the duplicated sanitization-and-copy logic from favoriteSummary, rebuildBuckets, and flushNow into a private sanitizedBucketPoints helper accepting telemetry points and returning updated points with all sanitizer exclusion flags applied. Replace each inline block with this helper while preserving the existing bucket-building behavior.Source: Coding guidelines
modules/vescape-core/android/src/test/java/expo/modules/vescapecore/telemetry/TelemetryRangeSubtractionTest.kt (1)
6-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the unbounded-range case that
clearAllactually uses.
clearAllsubtracts favorites fromTelemetryTimeRange(Long.MIN_VALUE, Long.MAX_VALUE), and a protected range ending atLong.MAX_VALUEis the one input whereendMs + 1can wrap. Neither this suite nor the Swift peer covers it.🤖 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 `@modules/vescape-core/android/src/test/java/expo/modules/vescapecore/telemetry/TelemetryRangeSubtractionTest.kt` around lines 6 - 74, Extend TelemetryRangeSubtractionTest with a clearAll-style unbounded request using TelemetryTimeRange(Long.MIN_VALUE, Long.MAX_VALUE), including a protected range whose endMs is Long.MAX_VALUE. Assert the subtraction returns the correct remaining range without overflow, and add the equivalent boundary case to the Swift peer test suite.modules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.kt (1)
555-557: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign rejection codes with iOS for
createFavorite/importFavoriteMedia.
renameFavoritegets an explicitERR_RENAME_FAVORITE, but the other two letrequire/check/errorescape as plain Kotlin exceptions, so JS sees an Expo-generic code where iOS rejects withERR_CREATE_FAVORITE/ERR_IMPORT_FAVORITE_MEDIA. Wrapping them keeps the bridge contract identical on both platforms.As per coding guidelines, "keep APIs, events, payloads, errors, lifecycle, threading, persistence, and unsupported-platform behavior aligned".
Also applies to: 568-570
🤖 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 `@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.kt` around lines 555 - 557, Update the AsyncFunction handlers for createFavorite and importFavoriteMedia to catch failures from TelemetryRepository and reject with the iOS-compatible ERR_CREATE_FAVORITE and ERR_IMPORT_FAVORITE_MEDIA codes. Preserve their existing options and repository calls, while keeping renameFavorite’s ERR_RENAME_FAVORITE behavior unchanged.Source: Coding guidelines
src/modules/history/components/FavoriteList.tsx (1)
83-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove ride duration/distance formatting into a shared history formatter.
HistorySessionSheetandFavoriteListboth define identicalformatDuration(ms)/formatDistance(distanceM: number | null)helpers, withHistoryStatsBarimplementing its own stat-bar variant with different output. Extract a shared ride-level formatter or module-level utility instead of duplicating this formatting in each component.🤖 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/modules/history/components/FavoriteList.tsx` around lines 83 - 94, Extract the duplicated formatDuration and formatDistance helpers from FavoriteList and HistorySessionSheet into a shared history formatter utility, then update both components to import and reuse them. Preserve the existing ride-level output and null-distance handling; leave HistoryStatsBar’s distinct stat-bar formatting unchanged unless it can reuse the shared behavior without changing its output.src/screens/main/history/HistoryOverlay.tsx (1)
135-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "disabled"
HistoryControlsblock.Both the favorites-list branch and the history-empty branch render an identical
HistoryControlsconfiguration (all-disabled props, identical no-op callbacks). Consider extracting a shareddisabledHistoryControlsPropsobject or a small wrapper to avoid the two copies drifting apart later.♻️ Suggested consolidation
+const NOOP_HISTORY_CONTROLS_PROPS = { + canRemove: false, + canFavorite: false, + favorited: false, + trimming: false, + saving: false, + onRemove: () => undefined, + onToggleFavorite: () => undefined, + onCancelTrim: () => undefined, + onSaveTrim: () => undefined, +} as constThen spread
{...NOOP_HISTORY_CONTROLS_PROPS}at both call sites alongsideloading,tab,onSelectTab,onBack.As per coding guidelines,
**/*.{js,jsx,ts,tsx,swift,kt}: "remove unused code and avoid duplicate code."Also applies to: 167-181
🤖 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/screens/main/history/HistoryOverlay.tsx` around lines 135 - 149, Consolidate the duplicated disabled HistoryControls configuration in HistoryOverlay by extracting the shared all-disabled props and no-op callbacks into a reusable object or wrapper, then reuse it in both the favorites-list and history-empty branches while keeping loading, tab, onSelectTab, and onBack branch-specific.Source: Coding guidelines
src/screens/main/history/useHistoryFavorites.ts (1)
139-162: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReturned object is a fresh reference every render, defeating memoization downstream.
Wrapping this in
useMemo(deps = every returned field) would let consumers depend onhistoryFavoritesas a whole without re-creating callbacks/effects on every render. Currently,useMainScreenController.ts'sexitHistory,enterHistoryMode, and theuseFocusEffectback-handler callback all depend on the wholehistoryFavoritesobject, so they (and theBackHandlerlistener itself) are torn down/rebuilt on every render of the controller rather than only when favorites state changes.♻️ Suggested fix
+ return useMemo( + () => ({ historyTab, selectHistoryTab, favorites, favoritesLoading, favoritesSaving, favoritesError, selectedSessionFavorite, trimming, trimSeed, beginTrimFavorite, updateTrimRange, cancelTrim, saveTrim, openFavorite, showFavorite, hideFavorite, renameOpenFavorite, removeOpenFavorite, removeFavorite, loadFavorites, resetHistoryFavorites, + }), + [ + historyTab, selectHistoryTab, favorites, favoritesLoading, favoritesSaving, + favoritesError, selectedSessionFavorite, trimming, trimSeed, beginTrimFavorite, + updateTrimRange, cancelTrim, saveTrim, openFavorite, showFavorite, hideFavorite, + renameOpenFavorite, removeOpenFavorite, removeFavorite, loadFavorites, resetHistoryFavorites, + ], + ) - return { ... }🤖 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/screens/main/history/useHistoryFavorites.ts` around lines 139 - 162, Memoize the returned object in the history favorites hook using useMemo, with dependencies covering every returned field from the object, including state values and callbacks such as historyTab, selectHistoryTab, saveTrim, and resetHistoryFavorites. Preserve the existing object contents while ensuring its reference changes only when one of those dependencies changes.src/screens/main/map/MainMapLayers.tsx (1)
295-332: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull linear scan of
rideGpsSampleson every trim-drag frame.The
coordinatesloop scans the entirerideGpsSamplesarray on everytrimRangeupdate while dragging. For long rides (thousands of GPS points sampled over an hour+) this recomputation on every drag frame can add up to visible jank. Since samples are time-sorted, consider binary-searching thelo/hiboundaries instead of a full scan with earlybreak.🤖 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/screens/main/map/MainMapLayers.tsx` around lines 295 - 332, Update TrimRouteHighlight to use binary searches over the time-sorted rideGpsSamples array to find the inclusive lo and hi boundaries, then build coordinates only from that subrange instead of scanning every sample on each trimRange update. Preserve the current reversed-range handling, endpoint inclusion, and null result when fewer than two coordinates remain.src/modules/history/store/historyStore.ts (1)
360-360: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHard 500 cap on reload can truncate already-loaded history (and the cap logic is duplicated).
reloadLimitis capped at 500 regardless of how many blocks were already loaded via repeatedloadMore(). If a user pages past 500 blocks and then deletes a session or clears history, the reload only fetches the newest 500, silently dropping older sessions that were previously visible, andhasMoreis derived from this truncated set rather than the pre-action pagination depth. The sameMath.min(500, Math.max(PAGE_SIZE, get().blocks.length))expression is also copy-pasted betweenremoveSelectedSessionandclearHistory.♻️ Suggested fix
+function getReloadLimit(currentBlockCount: number) { + return Math.max(PAGE_SIZE, currentBlockCount) +}Use
getReloadLimit(get().blocks.length)in both places, and reconsider whether an unbounded reload is acceptable here (this path only runs after an explicit delete/clear, not on every render) or whether pagination should be replayed vialoadMore()calls instead of a single oversized request.Also applies to: 370-371, 393-393, 406-406, 410-410, 428-428
🤖 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/modules/history/store/historyStore.ts` at line 360, Replace the duplicated hard-coded reload-limit expressions in removeSelectedSession and clearHistory with the shared getReloadLimit(get().blocks.length) helper. Ensure both reload paths preserve the previously loaded pagination depth, including histories exceeding 500 blocks, and keep hasMore derived from the complete reloaded result.src/modules/history/lib/favoritePreview.test.ts (1)
60-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name contradicts its assertions.
This case asserts the gap cap suppresses integration (both totals
0), yet is titled "integrates pack energy across time, splitting used and regen" — the behavior actually covered by the next test. Renaming to something like'skips energy integration across a long recording gap'avoids misreading a failure later.🤖 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/modules/history/lib/favoritePreview.test.ts` around lines 60 - 70, Rename the test containing the one-hour sample gap and zero energy assertions to describe skipping integration across a long recording gap. Keep the test setup and assertions unchanged; the existing integration-and-splitting test name belongs to the next test.src/components/charts/TelemetryLineChart.tsx (1)
564-574: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTrim domain ignores
windowMs, unlike the drawn line.The line and exclusion bands are positioned via
getXPosition(..., windowMs), which anchors x on the trailing window whenwindowMsis set, while the trim domain maps first→last sample onto[0, chartWidth]. With bothtrimandwindowMssupplied, the handles and dim regions would not line up with the visible curve. Current callers appear to avoid that combination, so a short comment or dev-time guard is enough to keep it from regressing.🤖 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/charts/TelemetryLineChart.tsx` around lines 564 - 574, Update the trim setup around useChartTrim and getXPosition so the trim domain uses the same trailing windowMs-based time domain as the drawn line, keeping handles and exclusion regions aligned with the visible curve. If trim and windowMs are intentionally unsupported together, add a concise comment or development-time guard documenting and enforcing that constraint.
🤖 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
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRepository.kt`:
- Around line 751-773: Update promoteProtectedRangeStarts to avoid unbounded
frame loading: add and use a DAO query that returns distinct device IDs for the
requested time window, and replace the Int.MAX_VALUE sample-state limit with a
small bounded limit sufficient to include the preceding keyframe while
retrieving the first sample. Preserve the existing deviceId-specific path and
promotion behavior.
In `@modules/vescape-core/ios/telemetry/TelemetryRepository.swift`:
- Around line 254-256: Update the Favorite creation logic around startMs and
endMs to reject missing or invalid telemetryLong values instead of defaulting
them to 0. Require both parsed timestamps before applying the endMs >= startMs
validation, and return the existing ERR_CREATE_FAVORITE failure path so iOS
matches Android behavior.
In `@src/components/charts/TelemetryChartTrim.tsx`:
- Around line 101-105: The trim seed-sync effect in TelemetryChartTrim.tsx must
depend on trim?.startMs and trim?.endMs and return early while
activeHandle.value is non-null, preventing active gestures from being
overwritten; update the effect using the existing setSharedValue and handle
refs. In HistoryRideDetail.tsx, memoize the trim object using history.trimSeed
and the stable callbacks so it is not recreated on every render.
In `@src/modules/history/components/FavoriteList.tsx`:
- Around line 71-77: Update the favorite row’s trash-button handler in
FavoriteList, reusing the confirmation flow and “Delete Favorite” ConfirmModal
pattern from HistoryRideDetail.tsx before invoking onRemove(favorite). Ensure
the modal allows cancellation and only removes the favorite after explicit
confirmation.
In `@src/modules/history/hooks/useMediaHistory.ts`:
- Around line 94-108: Update the add callback so setStored refreshes from
getFavoriteMedia after attempting all picked assets, even when an individual
importFavoriteMedia call fails. Preserve the existing error reporting and
loading cleanup, while ensuring successful partial imports appear immediately in
the gallery.
In `@src/modules/history/lib/favoritePreview.ts`:
- Around line 3-8: The documentation for the TypeScript Favorite summary
preview, including summarizeFavoriteRange, must link to both native summary
implementations with `@parity` references. Add links identifying the Android
FavoriteSummaryBuilder and the corresponding iOS implementation; if the
JS/native behavior intentionally differs, use `@platform-diff` with the reason
instead.
In `@src/modules/history/store/favoriteStore.ts`:
- Around line 35-44: Update the load() method to track the load start/version
and validate it before applying the getFavorites() result, so any mutation
completed after loading began prevents the stale snapshot from updating
favorites. Preserve the existing loading, error, and finally state handling
while ensuring saveTrim/add mutations invalidate or supersede in-flight loads.
In `@src/modules/history/store/historyStore.ts`:
- Around line 406-414: Update clearHistory to increment liveRefreshVersion
before starting its asynchronous clear-and-refetch workflow, matching
removeSelectedSession. Ensure the existing refreshLive version check observes
this invalidation so in-flight refreshes cannot restore cleared blocks or
liveBlocks.
In `@src/screens/main/history/HistoryControls.tsx`:
- Around line 64-112: Add descriptive accessibilityLabel props to each icon-only
IconButton in the trimming and favorite branches of HistoryControls: identify
the actions as cancel trim, save trim, go back, rename favorite, and delete
favorite. Keep the existing handlers, loading, disabled, styling, and testID
props unchanged.
In `@src/screens/main/mainScreenStore.ts`:
- Around line 102-116: Update setHistoryTab and openFavorite in the history
store to also clear trimRange when navigating between tabs or opening a
favorite. Preserve the existing state transitions while ensuring trim mode ends
whenever the current ride context changes.
---
Nitpick comments:
In
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRepository.kt`:
- Around line 647-663: Extract the duplicated sanitization-and-copy logic from
favoriteSummary, rebuildBuckets, and flushNow into a private
sanitizedBucketPoints helper accepting telemetry points and returning updated
points with all sanitizer exclusion flags applied. Replace each inline block
with this helper while preserving the existing bucket-building behavior.
In
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.kt`:
- Around line 555-557: Update the AsyncFunction handlers for createFavorite and
importFavoriteMedia to catch failures from TelemetryRepository and reject with
the iOS-compatible ERR_CREATE_FAVORITE and ERR_IMPORT_FAVORITE_MEDIA codes.
Preserve their existing options and repository calls, while keeping
renameFavorite’s ERR_RENAME_FAVORITE behavior unchanged.
In
`@modules/vescape-core/android/src/test/java/expo/modules/vescapecore/telemetry/TelemetryRangeSubtractionTest.kt`:
- Around line 6-74: Extend TelemetryRangeSubtractionTest with a clearAll-style
unbounded request using TelemetryTimeRange(Long.MIN_VALUE, Long.MAX_VALUE),
including a protected range whose endMs is Long.MAX_VALUE. Assert the
subtraction returns the correct remaining range without overflow, and add the
equivalent boundary case to the Swift peer test suite.
In `@src/components/charts/TelemetryLineChart.tsx`:
- Around line 564-574: Update the trim setup around useChartTrim and
getXPosition so the trim domain uses the same trailing windowMs-based time
domain as the drawn line, keeping handles and exclusion regions aligned with the
visible curve. If trim and windowMs are intentionally unsupported together, add
a concise comment or development-time guard documenting and enforcing that
constraint.
In `@src/modules/history/components/FavoriteList.tsx`:
- Around line 83-94: Extract the duplicated formatDuration and formatDistance
helpers from FavoriteList and HistorySessionSheet into a shared history
formatter utility, then update both components to import and reuse them.
Preserve the existing ride-level output and null-distance handling; leave
HistoryStatsBar’s distinct stat-bar formatting unchanged unless it can reuse the
shared behavior without changing its output.
In `@src/modules/history/lib/favoritePreview.test.ts`:
- Around line 60-70: Rename the test containing the one-hour sample gap and zero
energy assertions to describe skipping integration across a long recording gap.
Keep the test setup and assertions unchanged; the existing
integration-and-splitting test name belongs to the next test.
In `@src/modules/history/store/historyStore.ts`:
- Line 360: Replace the duplicated hard-coded reload-limit expressions in
removeSelectedSession and clearHistory with the shared
getReloadLimit(get().blocks.length) helper. Ensure both reload paths preserve
the previously loaded pagination depth, including histories exceeding 500
blocks, and keep hasMore derived from the complete reloaded result.
In `@src/screens/main/history/HistoryOverlay.tsx`:
- Around line 135-149: Consolidate the duplicated disabled HistoryControls
configuration in HistoryOverlay by extracting the shared all-disabled props and
no-op callbacks into a reusable object or wrapper, then reuse it in both the
favorites-list and history-empty branches while keeping loading, tab,
onSelectTab, and onBack branch-specific.
In `@src/screens/main/history/useHistoryFavorites.ts`:
- Around line 139-162: Memoize the returned object in the history favorites hook
using useMemo, with dependencies covering every returned field from the object,
including state values and callbacks such as historyTab, selectHistoryTab,
saveTrim, and resetHistoryFavorites. Preserve the existing object contents while
ensuring its reference changes only when one of those dependencies changes.
In `@src/screens/main/map/MainMapLayers.tsx`:
- Around line 295-332: Update TrimRouteHighlight to use binary searches over the
time-sorted rideGpsSamples array to find the inclusive lo and hi boundaries,
then build coordinates only from that subrange instead of scanning every sample
on each trimRange update. Preserve the current reversed-range handling, endpoint
inclusion, and null result when fewer than two coordinates remain.
🪄 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: d1b99583-bdab-43f5-bf02-1ea2dc7e9718
📒 Files selected for processing (60)
CONTEXT.mddocs/adr/0014-media-history-is-a-local-derived-view.mddocs/adr/0029-favorites-pin-telemetry-ranges.mddocs/adr/0030-favorite-media-is-curated-copied-storage.mdmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/FavoriteMediaStore.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/FavoriteSummaryBuilder.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDao.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDatabase.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryEntities.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRangeSubtraction.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRepository.ktmodules/vescape-core/android/src/test/java/expo/modules/vescapecore/telemetry/FavoriteMediaTest.ktmodules/vescape-core/android/src/test/java/expo/modules/vescapecore/telemetry/FavoriteSummaryBuilderTest.ktmodules/vescape-core/android/src/test/java/expo/modules/vescapecore/telemetry/TelemetryRangeSubtractionTest.ktmodules/vescape-core/ios/VescapeCoreModule.swiftmodules/vescape-core/ios/telemetry/FavoriteMediaStore.swiftmodules/vescape-core/ios/telemetry/FavoriteMediaStoreTests.swiftmodules/vescape-core/ios/telemetry/FavoriteStore.swiftmodules/vescape-core/ios/telemetry/FavoriteStoreTests.swiftmodules/vescape-core/ios/telemetry/TelemetryDatabase.swiftmodules/vescape-core/ios/telemetry/TelemetryMigrationTests.swiftmodules/vescape-core/ios/telemetry/TelemetryRangeSubtraction.swiftmodules/vescape-core/ios/telemetry/TelemetryRangeSubtractionTests.swiftmodules/vescape-core/ios/telemetry/TelemetryRepository.swiftmodules/vescape-core/src/index.tssrc/app/settings/components/charts.tsxsrc/app/settings/components/modals.tsxsrc/components/charts/TelemetryChartTrim.tsxsrc/components/charts/TelemetryLineChart.tsxsrc/components/modals/TextPromptModal.tsxsrc/modules/history/components/FavoriteList.tsxsrc/modules/history/components/HistoryPanelNav.tsxsrc/modules/history/components/HistoryRideMediaDrawer.tsxsrc/modules/history/components/HistorySessionSheet.tsxsrc/modules/history/components/MediaHistoryGallery.tsxsrc/modules/history/hooks/useMediaHistory.tssrc/modules/history/lib/favoritePreview.test.tssrc/modules/history/lib/favoritePreview.tssrc/modules/history/lib/favorites.test.tssrc/modules/history/lib/favorites.tssrc/modules/history/lib/mediaHistory.test.tssrc/modules/history/lib/mediaHistory.tssrc/modules/history/store/favoriteStore.test.tssrc/modules/history/store/favoriteStore.tssrc/modules/history/store/historyStore.test.tssrc/modules/history/store/historyStore.tssrc/modules/history/store/rideMediaFiles.tssrc/screens/main/MainScreen.tsxsrc/screens/main/history/HistoryControls.tsxsrc/screens/main/history/HistoryMapLoading.tsxsrc/screens/main/history/HistoryOverlay.tsxsrc/screens/main/history/HistoryRideDetail.tsxsrc/screens/main/history/HistoryTelemetryPanel.tsxsrc/screens/main/history/TrimStatsBar.tsxsrc/screens/main/history/useHistoryFavorites.tssrc/screens/main/mainScreenStore.tssrc/screens/main/map/MainMapLayers.tsxsrc/screens/main/overlays/MainOverlays.tsxsrc/screens/main/useMainScreenController.ts
💤 Files with no reviewable changes (3)
- src/modules/history/store/rideMediaFiles.ts
- src/modules/history/lib/mediaHistory.test.ts
- src/modules/history/lib/mediaHistory.ts
| private suspend fun promoteProtectedRangeStarts( | ||
| protected: Collection<TelemetryTimeRange>, | ||
| deviceId: String?, | ||
| ) { | ||
| for (range in protected) { | ||
| val devices = if (deviceId != null) { | ||
| listOf(deviceId) | ||
| } else { | ||
| dao.getFrames(range.startMs, range.endMs, null, Int.MAX_VALUE) | ||
| .map { it.deviceId } | ||
| .distinct() | ||
| } | ||
| for (protectedDeviceId in devices) { | ||
| val first = getSampleStates( | ||
| range.startMs, | ||
| range.endMs, | ||
| protectedDeviceId, | ||
| Int.MAX_VALUE, | ||
| ).firstOrNull() ?: continue | ||
| dao.updateFrame(first.state.toFrame(previous = null, keyframe = true).copy(id = first.id)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
promoteProtectedRangeStarts loads the whole protected range into memory to find one frame.
Both queries are unbounded (Int.MAX_VALUE) but only the first sample and the distinct device ids are needed. For clearAll (deviceId = null) this materializes every frame of every favorited range twice — a one-hour favorite at 2 Hz is ~7k delta frames per device, and getSampleStates additionally rebuilds full state for all of them just to call firstOrNull().
Add a DAO query for the distinct device ids in a window, and cap the sample decode at the first frame (limit = 1 still needs the preceding keyframe, so a small bounded limit is enough).
🤖 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
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRepository.kt`
around lines 751 - 773, Update promoteProtectedRangeStarts to avoid unbounded
frame loading: add and use a DAO query that returns distinct device IDs for the
requested time window, and replace the Int.MAX_VALUE sample-state limit with a
small bounded limit sufficient to include the preceding keyframe while
retrieving the first sample. Preserve the existing deviceId-specific path and
promotion behavior.
| <IconButton | ||
| icon={TrashIcon} | ||
| destructive | ||
| testID={`favorite-remove-${favorite.id}`} | ||
| onPress={() => onRemove(favorite)} | ||
| /> | ||
| </Pressable> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Deleting a Favorite from the list skips confirmation, unlike the detail view.
HistoryRideDetail.tsx requires confirming ("Delete Favorite" ConfirmModal) before removing an open Favorite, but this list row's trash button fires onRemove immediately on tap. A single mis-tap here permanently removes the Favorite (and its associated media) with no chance to cancel.
🛡️ Suggested fix: reuse the confirm pattern from HistoryRideDetail
+import { useState } from 'react'
+import { ConfirmModal } from '`@/components/modals/ConfirmModal`'
...
export function FavoriteList({ favorites, loading, onOpen, onRemove }: FavoriteListProps) {
const insets = useSafeAreaInsets()
+ const [pendingRemoval, setPendingRemoval] = useState<Favorite | null>(null)
...
<IconButton
icon={TrashIcon}
destructive
testID={`favorite-remove-${favorite.id}`}
- onPress={() => onRemove(favorite)}
+ onPress={() => setPendingRemoval(favorite)}
/>
))}
</ScrollView>
+ <ConfirmModal
+ visible={pendingRemoval != null}
+ title="Delete Favorite"
+ message="The Favorite is removed. Its telemetry stays in history and becomes deletable again."
+ confirmLabel="Delete"
+ cancelLabel="Keep"
+ destructive
+ onConfirm={() => {
+ if (pendingRemoval) onRemove(pendingRemoval)
+ setPendingRemoval(null)
+ }}
+ onCancel={() => setPendingRemoval(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.
| <IconButton | |
| icon={TrashIcon} | |
| destructive | |
| testID={`favorite-remove-${favorite.id}`} | |
| onPress={() => onRemove(favorite)} | |
| /> | |
| </Pressable> | |
| import { useState } from 'react' | |
| import { ConfirmModal } from '`@/components/modals/ConfirmModal`' | |
| ... | |
| export function FavoriteList({ favorites, loading, onOpen, onRemove }: FavoriteListProps) { | |
| const insets = useSafeAreaInsets() | |
| const [pendingRemoval, setPendingRemoval] = useState<Favorite | null>(null) | |
| ... | |
| <IconButton | |
| icon={TrashIcon} | |
| destructive | |
| testID={`favorite-remove-${favorite.id}`} | |
| onPress={() => setPendingRemoval(favorite)} | |
| /> | |
| </Pressable> | |
| ... | |
| <ConfirmModal | |
| visible={pendingRemoval != null} | |
| title="Delete Favorite" | |
| message="The Favorite is removed. Its telemetry stays in history and becomes deletable again." | |
| confirmLabel="Delete" | |
| cancelLabel="Keep" | |
| destructive | |
| onConfirm={() => { | |
| if (pendingRemoval) onRemove(pendingRemoval) | |
| setPendingRemoval(null) | |
| }} | |
| onCancel={() => setPendingRemoval(null)} | |
| /> |
🤖 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/modules/history/components/FavoriteList.tsx` around lines 71 - 77, Update
the favorite row’s trash-button handler in FavoriteList, reusing the
confirmation flow and “Delete Favorite” ConfirmModal pattern from
HistoryRideDetail.tsx before invoking onRemove(favorite). Ensure the modal
allows cancellation and only removes the favorite after explicit confirmation.
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/screens/main/history/useHistoryFavorites.ts (1)
197-223: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winHook's returned object is a new reference every render, defeating
useCallbackmemoization for its consumers.
historyFavoritesis a plain object literal rebuilt on every call touseHistoryFavorites. SinceuseMainScreenController(the caller) re-renders frequently for unrelated reasons (e.g. live BLE telemetry updates), this object gets a new identity far more often than its contents actually change. Downstream,exitHistoryandenterHistoryModeinsrc/screens/main/useMainScreenController.tsboth list the wholehistoryFavoritesobject in theiruseCallbackdeps (lines 258, 285), and the hardware-backuseFocusEffect(useCallback(..., [exitHistory, ...]))(line 396) transitively depends on it — so theBackHandlerlistener ends up being removed/re-added on nearly every render instead of only when relevant state changes.Prefer depending on the specific stable functions you actually need (e.g.
historyFavorites.resetHistoryFavorites,historyFavorites.loadFavorites) at the call sites, and/or memoize this hook's return value so its identity is stable when its underlying values haven't changed.♻️ Example direction
- }, [enterTelemetry, historyFavorites, mapRef, selectSession]) + }, [enterTelemetry, historyFavorites.resetHistoryFavorites, mapRef, selectSession])As per coding guidelines, "Prefer clear architecture over compatibility shortcuts or hidden assumptions" for `**/*.{js,jsx,ts,tsx,swift,kt}".
🤖 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/screens/main/history/useHistoryFavorites.ts` around lines 197 - 223, Memoize the object returned by useHistoryFavorites so its identity only changes when its exposed values change, including the derived canPreviousFavorite and canNextFavorite values. Update the return construction around the hook’s returned symbols, preserving all existing fields and callbacks, so useMainScreenController’s historyFavorites-dependent callbacks do not churn on unrelated renders.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.
Outside diff comments:
In `@src/screens/main/history/useHistoryFavorites.ts`:
- Around line 197-223: Memoize the object returned by useHistoryFavorites so its
identity only changes when its exposed values change, including the derived
canPreviousFavorite and canNextFavorite values. Update the return construction
around the hook’s returned symbols, preserving all existing fields and
callbacks, so useMainScreenController’s historyFavorites-dependent callbacks do
not churn on unrelated renders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff7366e9-99fa-4434-bc2a-9df7c06d7e26
📒 Files selected for processing (9)
e2e/flows/history.yamlsrc/modules/history/components/HistoryPanelNav.tsxsrc/screens/main/MainScreen.tsxsrc/screens/main/history/HistoryControls.tsxsrc/screens/main/history/HistoryOverlay.tsxsrc/screens/main/history/HistoryRideDetail.tsxsrc/screens/main/history/HistoryTelemetryPanel.tsxsrc/screens/main/history/useHistoryFavorites.tssrc/screens/main/useMainScreenController.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/main/useMainScreenController.ts (1)
253-259: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
historyFavoritesobject as auseCallbackdependency churnsexitHistory/enterHistoryMode— and the BackHandler listener — on every render.
historyFavoritesis a new object literal on every call touseHistoryFavorites(no memoization on its return value), butexitHistory(Line 259) andenterHistoryMode(Line 286) list the whole object as a dependency even though each only calls one independently-stable method (resetHistoryFavorites,loadFavorites). This makes both callbacks unstable across renders, which then makes theuseFocusEffectcallback at Line 397 (which depends onexitHistory) unstable too — so the hardwareBackHandlerlistener is torn down and re-registered on every render, not just on real navigation changes. NotecancelHistoryTrim(Line 122) already applies the correct pattern forhistoryFavorites.cancelTrim; the same should apply here.🛠️ Proposed fix
const exitHistory = useCallback(() => { setOpenMediaAssetId(null) historyFavorites.resetHistoryFavorites() void selectSession(null) enterTelemetry() requestAnimationFrame(() => mapRef.current?.recenterLive({ resetPadding: true, animationDuration: 0 }), ) - }, [enterTelemetry, historyFavorites, mapRef, selectSession]) + }, [enterTelemetry, historyFavorites.resetHistoryFavorites, mapRef, selectSession])const enterHistoryMode = useCallback(async () => { enterHistory() void historyFavorites.loadFavorites() await loadInitial() await loadOlderHistoryPages() if (useMainScreenStore.getState().mode !== 'history') return const latest = getLatestSession(useHistoryStore.getState().sessions) if (latest) { await selectSession(latest) } - }, [enterHistory, historyFavorites, loadInitial, loadOlderHistoryPages, selectSession]) + }, [ + enterHistory, + historyFavorites.loadFavorites, + loadInitial, + loadOlderHistoryPages, + selectSession, + ])Also applies to: 278-286, 366-369, 397-397
🤖 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/screens/main/useMainScreenController.ts` around lines 253 - 259, Update the useCallback dependency arrays for exitHistory and enterHistoryMode to depend on the independently stable historyFavorites.resetHistoryFavorites and historyFavorites.loadFavorites methods rather than the whole historyFavorites object, matching the cancelHistoryTrim pattern. Preserve the existing callback behavior so the useFocusEffect and BackHandler listener remain stable across unrelated renders.
🧹 Nitpick comments (3)
modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRepository.kt (1)
774-799: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
promoteProtectedRangeStartsruns for every favorite on everydeleteRangecall, even when unrelated to the requested range.The fix for the prior unbounded-materialization issue (bounded
getSampleStateswindow viadao.getFirstFrameInRange) looks solid. However, indeleteRange(line 532) this is invoked for the fullprotectedcollection regardless of whetherrequestedoverlaps or abuts each favorite's range.clearAlllegitimately needs to promote every favorite (it deletes everything outside protected ranges), but a targeteddeleteRangecall only needs to promote favorites whose keyframe lead-in could be affected by the specific requested interval.Consider filtering
protectedto ranges near/overlappingrequested(e.g.range.startMs - KEYFRAME_INTERVAL_MS <= requested.endMs && range.endMs >= requested.startMs) before promoting, to avoid unnecessary DB round-trips per favorite when many favorites exist.♻️ Proposed fix
suspend fun deleteRange(options: Map<String, Any?>): Int = withContext(Dispatchers.IO) { val query = RangeMutationOptions.from(options) flushNow() val requested = TelemetryTimeRange(query.fromMs, query.toMs) val protected = favoriteTelemetryRanges() - promoteProtectedRangeStarts(protected, query.deviceId) + val affected = protected.filter { + it.startMs - KEYFRAME_INTERVAL_MS <= requested.endMs && it.endMs >= requested.startMs + } + promoteProtectedRangeStarts(affected, query.deviceId) val deleted = subtractProtectedTelemetryRanges(requested, protected).sumOf { range -> dao.deleteRange(range.startMs, range.endMs, query.deviceId) } deleted }🤖 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 `@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRepository.kt` around lines 774 - 799, Update the targeted deleteRange flow before calling promoteProtectedRangeStarts so it only passes protected ranges whose keyframe lead-in can overlap or abut the requested interval, using KEYFRAME_INTERVAL_MS and the requested start/end bounds; preserve clearAll’s behavior of promoting every protected range.modules/vescape-core/ios/telemetry/FavoriteStore.swift (1)
116-167: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winiOS
updateFavoritedoes a full-table scan to find one Favorite by id.FavoriteStorehas no indexed single-row fetch, soTelemetryRepository.updateFavoriteloads and decodes every favorite row just to check existence — unlike Android's indexeddao.getFavorite(id).
modules/vescape-core/ios/telemetry/FavoriteStore.swift#L116-L167: add afunc get(_ id: String) -> Favorite?backed bySELECT * FROM favorites WHERE id = ?(primary-key lookup), alongsidelist().modules/vescape-core/ios/telemetry/TelemetryRepository.swift#L320-L323: replaceFavoriteStore.shared.list().first(where: { $0.id == id })with the newFavoriteStore.shared.get(id).🤖 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 `@modules/vescape-core/ios/telemetry/FavoriteStore.swift` around lines 116 - 167, FavoriteStore.swift lines 116-167: add a get(_ id: String) -> Favorite? method beside list() using the resolved writer and a parameterized primary-key query, returning nil when unavailable or not found. TelemetryRepository.swift lines 320-323: replace the full list().first(where:) lookup with FavoriteStore.shared.get(id); no other changes are needed there.src/screens/main/history/HistoryControls.tsx (1)
61-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCancel label says "edit" even when creating a new Favorite.
accessibilityLabel="Cancel Favorite edit"is static, but this same trimming header is used both when creating a brand-new Favorite (star toggle from a ride,favoriteisundefined) and when editing an existing one (favoritepresent). Screen-reader users creating a new Favorite will hear "Cancel Favorite edit," which is inaccurate.🛠️ Proposed fix
- <IconButton - icon={XIcon} - onPress={onCancelTrim} - disabled={saving} - testID="trim-cancel" - accessibilityLabel="Cancel Favorite edit" - /> + <IconButton + icon={XIcon} + onPress={onCancelTrim} + disabled={saving} + testID="trim-cancel" + accessibilityLabel={favorite ? 'Cancel Favorite edit' : 'Cancel Favorite creation'} + />🤖 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/screens/main/history/HistoryControls.tsx` around lines 61 - 95, Update the cancel IconButton accessibilityLabel in the trimming header to distinguish creation from editing: use a create-appropriate label when favorite is undefined and retain the existing edit label when favorite is present. Keep the onCancelTrim behavior unchanged.
🤖 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
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.kt`:
- Around line 558-561: Update the updateFavorite AsyncFunction to handle invalid
ranges where endMs is less than startMs before the existing null guard, ensuring
the operation rejects with CodedException code ERR_UPDATE_FAVORITE rather than
leaking IllegalArgumentException. Either validate and return the same controlled
error or catch the repository exception and rethrow it as ERR_UPDATE_FAVORITE,
while preserving the existing missing-favorite behavior.
---
Outside diff comments:
In `@src/screens/main/useMainScreenController.ts`:
- Around line 253-259: Update the useCallback dependency arrays for exitHistory
and enterHistoryMode to depend on the independently stable
historyFavorites.resetHistoryFavorites and historyFavorites.loadFavorites
methods rather than the whole historyFavorites object, matching the
cancelHistoryTrim pattern. Preserve the existing callback behavior so the
useFocusEffect and BackHandler listener remain stable across unrelated renders.
---
Nitpick comments:
In
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRepository.kt`:
- Around line 774-799: Update the targeted deleteRange flow before calling
promoteProtectedRangeStarts so it only passes protected ranges whose keyframe
lead-in can overlap or abut the requested interval, using KEYFRAME_INTERVAL_MS
and the requested start/end bounds; preserve clearAll’s behavior of promoting
every protected range.
In `@modules/vescape-core/ios/telemetry/FavoriteStore.swift`:
- Around line 116-167: FavoriteStore.swift lines 116-167: add a get(_ id:
String) -> Favorite? method beside list() using the resolved writer and a
parameterized primary-key query, returning nil when unavailable or not found.
TelemetryRepository.swift lines 320-323: replace the full list().first(where:)
lookup with FavoriteStore.shared.get(id); no other changes are needed there.
In `@src/screens/main/history/HistoryControls.tsx`:
- Around line 61-95: Update the cancel IconButton accessibilityLabel in the
trimming header to distinguish creation from editing: use a create-appropriate
label when favorite is undefined and retain the existing edit label when
favorite is present. Keep the onCancelTrim behavior unchanged.
🪄 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: 075bca4a-a5ea-4ede-9903-61cde8e95d3c
📒 Files selected for processing (45)
docs/adr/0029-favorites-pin-telemetry-ranges.mde2e/flows/edge-drawer-focus.yamle2e/flows/history.yamlmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/FavoriteSummaryBuilder.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDao.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryEntities.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryRepository.ktmodules/vescape-core/ios/VescapeCoreModule.swiftmodules/vescape-core/ios/telemetry/FavoriteStore.swiftmodules/vescape-core/ios/telemetry/FavoriteStoreTests.swiftmodules/vescape-core/ios/telemetry/TelemetryRepository.swiftmodules/vescape-core/src/index.tssrc/app/settings/components/charts.tsxsrc/app/settings/components/modals.tsxsrc/components/charts/TelemetryChartTrim.tsxsrc/components/charts/TelemetryLineChart.tsxsrc/components/charts/chartMath.test.tssrc/components/charts/chartMath.tssrc/components/charts/telemetryChartTrimMath.test.tssrc/components/charts/telemetryChartTrimMath.tssrc/components/forms/Input.tsxsrc/components/overlays/AnchoredSheet.tsxsrc/modules/history/components/HistoryPanelNav.tsxsrc/modules/history/components/HistoryRideLabel.tsxsrc/modules/history/components/HistorySessionSheet.tsxsrc/modules/history/hooks/useMediaHistory.tssrc/modules/history/lib/favoritePreview.tssrc/modules/history/lib/favorites.test.tssrc/modules/history/lib/favorites.tssrc/modules/history/lib/rideFormat.test.tssrc/modules/history/lib/rideFormat.tssrc/modules/history/store/favoriteStore.test.tssrc/modules/history/store/favoriteStore.tssrc/modules/history/store/historyStore.test.tssrc/modules/history/store/historyStore.tssrc/screens/main/MainScreen.tsxsrc/screens/main/history/HistoryControls.tsxsrc/screens/main/history/HistoryOverlay.tsxsrc/screens/main/history/HistoryRideDetail.tsxsrc/screens/main/history/HistoryTelemetryPanel.tsxsrc/screens/main/history/useHistoryFavorites.tssrc/screens/main/mainScreenStore.test.tssrc/screens/main/mainScreenStore.tssrc/screens/main/useMainScreenController.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryEntities.kt
- docs/adr/0029-favorites-pin-telemetry-ranges.md
| AsyncFunction("updateFavorite") Coroutine { id: String, options: Map<String, Any?> -> | ||
| TelemetryRepository.get(context.applicationContext).updateFavorite(id, options) | ||
| ?: throw CodedException("ERR_UPDATE_FAVORITE", "favorite does not exist or could not be stored", null) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'modules/vescape-core/(android|ios)/|docs/board-warnings.md|TelemetryRepository' || true
echo "== kotlin updateFavorite sites =="
rg -n "fun updateFavorite|updateFavorite|require\(endMs >= startMs\)|ERR_UPDATE_FAVORITE|updateFavorite" modules/vescape-core -S
echo "== target file excerpt =="
sed -n '530,575p' modules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.kt 2>/dev/null || true
echo "== android telemetry repo excerpt if exists =="
f=$(git ls-files | rg 'TelemetryRepository\.kt$|VescapeCoreModule\.kt$')
for file in $f; do
case "$file" in
*TelemetryRepository.kt)
echo "--- $file"
wc -l "$file"
rg -n "updateFavorite|require|endMs|startMs" "$file" -A8 -B8
;;
esac
doneRepository: KacperKozak/vescape
Length of output: 41092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== iOS updateFavorite implementation =="
sed -n '300,360p' modules/vescape-core/ios/telemetry/TelemetryRepository.swift
sed -n '628,648p' modules/vescape-core/ios/VescapeCoreModule.swift
echo "== Android code that maps Java exceptions to JS errors =="
f=$(git ls-files | rg 'modules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.kt$|ErrorReporting|Exception')
rg -n "CodedException|IllegalArgumentException|java\\.lang\\.IllegalArgumentException|onCodedException|ErrorReporting|MapString" modules/vescape-core/android/src/main/java/expo/modules/vescapecore -S
echo "== Kotlin optional conversion for map? =="
sed -n '550,565p' modules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.kt
echo "== read-only semantics probe for Kotlin-like Kotlin require ==
python3 - <<'PY'
def update_favorite(id_, existing, options):
if existing is None:
return None
start_ms = options["startMs"]
end_ms = options["endMs"]
if not (end_ms >= start_ms):
raise ValueError("endMs must be greater than or equal to startMs")
if options.get("exists") is False:
return None
return {"id": id_}
for name, args in [
("valid", ("/", {"exists": True, "startMs": 10, "endMs": 20})),
("invalid_range", ("/", {"exists": True, "startMs": 20, "endMs": 10})),
("not_exist", ("/", {"exists": False, "startMs": 10, "endMs": 20})),
]:
try:
result = update_favorite(*args)
print(f"{name}: result={result}, null_guard_taken={result is None}")
except Exception as e:
print(f"{name}: throws={type(e).__name__}: {e}")
PYRepository: KacperKozak/vescape
Length of output: 9709
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Java exception mapping for this module =="
sed -n '890,915p' modules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.kt
echo "== Android updateFavorite parity call context =="
sed -n '555,563p' modules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.kt
echo "== Android parseLong behavior probe in sandbox =="
java -Xmx64m -version >/dev/null 2>&1 || true
if command -v java >/dev/null 2>&1; then
cat > /tmp/RequireProbe.java <<'JAVA'
public class RequireProbe {
public static void main(String[] args) {
for (String[] pair : new String[][]{{"10","20"},{"20","10"}}) {
long startMs = Long.parseLong(pair[0]);
long endMs = Long.parseLong(pair[1]);
try {
assertTrue(endMs >= startMs, "endMs must be greater than or equal to startMs");
System.out.println(pair[0]+" -> "+pair[1]+" result=return value");
} catch (IllegalArgumentException e) {
System.out.println(pair[0]+" -> "+pair[1]+" throws=IllegalArgumentException: "+e.getCause());
}
}
}
static void assertTrue(boolean condition, String msg) { if (!condition) throw new IllegalArgumentException(msg); }
}
JAVA
javac /tmp/RequireProbe.java
java -cp /tmp RequireProbe
fiRepository: KacperKozak/vescape
Length of output: 1817
Handle invalid favorite ranges as ERR_UPDATE_FAVORITE before the null guard.
TelemetryRepository.updateFavorite already calls require(endMs >= start_ms) before returning a null favorite, so endMs < startMs throws IllegalArgumentException instead of returning null. Align Android with iOS by either validating the range and returning null, or catching and re-throwing it as the controlled ERR_UPDATE_FAVORITE rejection.
🤖 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
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/VescapeCoreModule.kt`
around lines 558 - 561, Update the updateFavorite AsyncFunction to handle
invalid ranges where endMs is less than startMs before the existing null guard,
ensuring the operation rejects with CodedException code ERR_UPDATE_FAVORITE
rather than leaking IllegalArgumentException. Either validate and return the
same controlled error or catch the repository exception and rethrow it as
ERR_UPDATE_FAVORITE, while preserving the existing missing-favorite behavior.
Source: Coding guidelines
1 participant
This PR implements Favorites: durable, optionally named time ranges over Ride History with a
↻ History | ⭐ Favoritestoggle on the history screen.What this lands:
@parity), with summary stats denormalized at creation from raw samplesNavigation:
All implementation work is tracked in the issues above and can merge back into this branch.
Issues
Implementation notes
[History] 5 - Favorite media #291: System pickers do not always expose a reliable capture time. The native manifest keeps
captured_atnullable for those assets; they stay visible in the Favorite gallery but are never given a fabricated map pin.[History] 3 - Pin favorited telemetry #289: Delete protection is minute-bucket granular: every bucket touched by a Favorite and all its raw samples stay together, avoiding any database-wide rebuild. Android also promotes each retained island’s first delta frame to a keyframe before deleting its predecessor.
[History] 2 - Trim range selection #288: Trim is JS-only; live stats are a best-effort preview (pack energy integrated from V·I, distance from GPS deltas) while the saved Favorite's durable stats stay native at creation. Handles are bounded to the chart's visible span (Moving Window ± display padding), so a trim cannot reach idle beyond that padding.
[History] 1 - Create and list Favorites #287: Favorite summary sums per-bucket odometer deltas, so the hop across a minute-bucket boundary is not counted — the same arithmetic history session rows already use.
[History] 1 - Create and list Favorites #287: GPS-distance fallback for rides with no odometer is Android-only (
@platform-diff): iOS's bucket writer storesgps_distance_cm = 0, so iOS favorites report no distance for GPS-only rides, exactly like iOS history rows.[History] 1 - Create and list Favorites #287: Android has no Room/SQLite harness in JVM unit tests, so DAO round-trip is covered on iOS (real GRDB in-memory DB) while Android covers the pure summary builder, the migration SQL, and the bridge map. A real Android round-trip needs an instrumentation target that does not exist yet.
[History] 1 - Create and list Favorites #287:
e2eFakehas no favorites stub — inEXPO_PUBLIC_E2E=1builds the Favorites tab reads the real native table, which is empty because E2E history is faked in JS.[History] 4 - Favorite detail and rename #290: Favorite detail is the history detail path fed a favorite-backed
HistorySession(favoriteToSession), so chart, map route and stats are shared with history mode instead of duplicated; the pinned summary wins over anything derivable from minute buckets, and only geography/bucket ids/device come from the buckets.[History] 4 - Favorite detail and rename #290: Media in favorite mode still runs through the session-id-keyed media path (now keyed by the
favorite:<id>synthetic session), which [History] 5 - Favorite media #291 replaces with favorite-owned storage and the native manifest.[History] 4 - Favorite detail and rename #290: A Favorite older than the loaded history pages has no overlapping buckets, so its detail opens with no bucket-derived geography and the map fits from the GPS samples the range read returns.
Summary by CodeRabbit