feat: Offline AI Plant/Wildlife Identification#1906
feat: Offline AI Plant/Wildlife Identification#1906Copilot wants to merge 12 commits intodevelopmentfrom
Conversation
1 similar comment
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
The Biome CI shows Is there something specific you'd like me to address or change? The wildlife identification feature is fully implemented and all new files pass lint checks. Let me know if you have any specific requests or feedback! |
|
@coderabbitai Please review this PR for code quality, best practices, and potential issues. |
|
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a wildlife identification feature: UI screens and tiles, persisted history atoms and hooks, offline species database and identifier, online identification service and API route using OpenAI, route wiring and i18n strings, plus feature flag gating. Changes
Sequence DiagramssequenceDiagram
participant User
participant ExpoClient as Expo Client
participant OfflineID as Offline Identifier
participant SpeciesDB as Species Database
participant APIServer as API Server
participant OpenAI as OpenAI Service
User->>ExpoClient: Select image & optional description, request identify
ExpoClient->>OfflineID: identifyFromDescription(description)
OfflineID->>SpeciesDB: searchSpecies(query)
SpeciesDB-->>OfflineID: Matching species (offline results)
alt Network Available
ExpoClient->>APIServer: POST /api/wildlife/identify (image key)
APIServer->>OpenAI: generateObject(prompt + imageUrl, schema)
OpenAI-->>APIServer: Identification results
APIServer-->>ExpoClient: Online results
else Network Error
ExpoClient-->>OfflineID: Use offline results as fallback
end
ExpoClient->>SpeciesDB: Save persisted image + add to history (wildlifeHistoryAtom)
ExpoClient->>User: Display results
sequenceDiagram
participant User
participant ExpoClient as Expo Client
participant History as Wildlife History (Jotai)
participant Detail as Species Detail Screen
participant SpeciesDB as Species Database
User->>ExpoClient: Open Wildlife screen
ExpoClient->>History: load history (wildlifeHistoryAtom)
History-->>ExpoClient: WildlifeIdentification[]
ExpoClient->>User: Render history list
User->>ExpoClient: Tap history item -> navigate /wildlife/[id]
Detail->>SpeciesDB: getSpeciesById(id)
alt Species found
SpeciesDB-->>Detail: SpeciesEntry
else Not found
Detail->>History: load matching history entry
History-->>Detail: result from saved history
end
Detail->>User: Render species details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
✅ Actions performedFull review triggered. |
|
@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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
apps/expo/features/wildlife/types.ts (1)
29-32: Make theconfidencecontract explicit.
SpeciesCardmultiplies this value by 100, so a plainnumberhere leaves every producer guessing whether the API returns0..1or0..100. Please document or narrow the range at the type boundary.💡 Suggested tightening
export interface IdentificationResult { species: SpeciesEntry; + /** Normalized confidence score in the range [0, 1]. */ confidence: number; source: 'online' | 'offline'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/wildlife/types.ts` around lines 29 - 32, The IdentificationResult.confidence field is ambiguous (0..1 vs 0..100); update the contract to explicitly represent a 0..1 probability by replacing the plain number with a constrained/Branded type (e.g., Confidence0To1 or type alias with a descriptive JSDoc) and update usages like SpeciesCard to rely on that 0..1 contract (it currently multiplies by 100). Change the IdentificationResult interface (symbol: IdentificationResult) to use the new Confidence type and add a brief JSDoc comment clarifying the range so producers and consumers know the expected 0..1 probability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/features/wildlife/atoms/wildlifeAtoms.ts`:
- Around line 6-17: The current wildlifeStorage wrapper manually calls
JSON.parse/JSON.stringify around AsyncStorage causing double-encoding and
hydration failures; replace the custom adapter by passing AsyncStorage directly
into createJSONStorage (i.e., use createJSONStorage with AsyncStorage as the
storage backend) and remove the getItem/setItem/removeItem JSON serialization
logic so createJSONStorage can handle parsing/stringifying itself; update the
wildlifeStorage declaration (symbol: wildlifeStorage) to use createJSONStorage
with AsyncStorage.
In `@apps/expo/features/wildlife/components/SpeciesCard.tsx`:
- Around line 1-3: SpeciesCard is rendering enum-backed values (source,
dangerLevel, category) raw; replace those direct uses with translated labels by
mapping each enum to an i18n key or using the app's t()/translate utility inside
SpeciesCard (or a helper map) to render user-facing strings (e.g., map
IdentificationResult.source values like "offline" to translation keys such as
"species.source.offline"); update all usages of source, dangerLevel, and
category in the SpeciesCard render (lines previously flagged ~38-59) to call the
translator or lookup map so non-English locales show localized labels.
In `@apps/expo/features/wildlife/data/speciesDatabase.ts`:
- Around line 494-502: searchSpecies currently treats blank/whitespace queries
as matches because it lowercases without checking for emptiness; modify
searchSpecies to trim the incoming query and immediately return an empty array
if the trimmed string is empty, otherwise proceed with the existing lowercase
match logic against SPECIES_DATABASE (references: searchSpecies,
SPECIES_DATABASE, commonName, scientificName, description, characteristics).
In `@apps/expo/features/wildlife/hooks/useWildlifeIdentification.ts`:
- Line 57: The offline query selection currently uses the nullish coalescing
operator so an empty description string ("" ) wins; update the logic in
useWildlifeIdentification where identifyFromDescription is called so
blank/whitespace descriptions are treated as absent—e.g., evaluate offlineQuery
with a trim check (use offlineQuery && offlineQuery.trim().length > 0) and pass
offlineQuery only when non-empty, otherwise pass selectedImage.fileName to
identifyFromDescription.
- Line 56: In useWildlifeIdentification, replace the current raw error logging
in the catch/fallback that calls console.warn('Online identification
unavailable, using offline database:', error) with a sanitized log that only
includes non-sensitive fields (e.g., error?.code and error?.message) or remove
the error payload entirely; update the console.warn invocation in that block so
it doesn't print the full Axios error object.
In `@apps/expo/features/wildlife/screens/IdentificationScreen.tsx`:
- Line 20: The selectedImage URI is being stored directly instead of a persisted
local filename; update the useImagePicker destructure to include
permanentlyPersistImageLocally, call
permanentlyPersistImageLocally(selectedImage) just before invoking
addIdentification, and pass the returned persisted filename (not
selectedImage.uri) into addIdentification; reference the useImagePicker hook,
the permanentlyPersistImageLocally function, the selectedImage object, and the
addIdentification call to locate where to change the code.
In `@apps/expo/features/wildlife/screens/SpeciesDetailScreen.tsx`:
- Around line 30-42: The fallback lookup currently finds a history entry by
history entry id (historyLoadable.data.find(h => h.id === id)) which fails when
the route id is a species id; change the fallback to search historyLoadable.data
for any entry whose results array contains a result with result.species.id ===
id (update the logic that sets historyEntry/displaySpecies accordingly), and
ensure you do not render the not-found UI until wildlife history has finished
loading (check historyLoadable.state === 'hasData' before showing the
speciesNotFound view and render a loading state while it is still pending).
In `@apps/expo/features/wildlife/screens/WildlifeScreen.tsx`:
- Line 34: The hardcoded fallback label "Unknown species" in WildlifeScreen
should be localized; replace the literal Text node with the i18n lookup used
elsewhere (call t with an appropriate key like t('wildlife.unknownSpecies')),
add that key to the translations files under the wildlife namespace in all
locales, and update the component to use t('wildlife.unknownSpecies') instead of
the hardcoded string so the fallback label is localized across languages.
- Around line 48-52: The code collapses non-'hasData' loadable states into an
empty array—update handling of wildlifeHistoryAtom/historyLoadable so you check
for historyLoadable.state === 'hasError' and surface an error UI (or message)
before falling back to empty array and ensure the same explicit error branch is
used in the rendering logic around the 84-114 conditional block; additionally
remove the hardcoded "Unknown species" string by either moving useTranslation()
into the HistoryItem component or passing the t function into HistoryItem and
replacing the literal with a translation key (e.g.,
t('wildlife.unknownSpecies')) so the fallback is localized.
In `@packages/api/src/routes/wildlife/index.ts`:
- Around line 110-114: The temp-upload deletion currently runs only after a
successful call to WildlifeIdentificationService.identifySpecies and can leak or
convert a success into a 500; refactor the handler so the model call
(service.identifySpecies(imageUrl)) runs inside a try block, move
PACKRAT_BUCKET.delete(image) into a finally block and make the delete
best-effort (catch/log any errors from PACKRAT_BUCKET.delete so they don't
change the response), ensuring the identification result (identification) is
returned or the model error is handled independently of the upload cleanup.
- Around line 142-149: The 500-response currently echoes internal error details
via c.json({ error: `Failed to identify species: ${error.message}` }, 500);
update the 500 branch in the error handler (the conditional that checks error
instanceof Error and the current return c.json(...) for 500) to return a generic
message (e.g., { error: 'Internal server error' } or { error: 'Failed to
identify species' }) without including error.message, while leaving the 400
branch (Invalid image / Unsupported image format) and existing logging/Sentry
reporting unchanged.
---
Nitpick comments:
In `@apps/expo/features/wildlife/types.ts`:
- Around line 29-32: The IdentificationResult.confidence field is ambiguous
(0..1 vs 0..100); update the contract to explicitly represent a 0..1 probability
by replacing the plain number with a constrained/Branded type (e.g.,
Confidence0To1 or type alias with a descriptive JSDoc) and update usages like
SpeciesCard to rely on that 0..1 contract (it currently multiplies by 100).
Change the IdentificationResult interface (symbol: IdentificationResult) to use
the new Confidence type and add a brief JSDoc comment clarifying the range so
producers and consumers know the expected 0..1 probability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4590d1c7-0d37-45bf-9591-7478eafc8075
📒 Files selected for processing (25)
apps/expo/app/(app)/(tabs)/(home)/index.tsxapps/expo/app/(app)/wildlife/[id].tsxapps/expo/app/(app)/wildlife/identify.tsxapps/expo/app/(app)/wildlife/index.tsxapps/expo/config.tsapps/expo/features/wildlife/atoms/wildlifeAtoms.tsapps/expo/features/wildlife/components/SpeciesCard.tsxapps/expo/features/wildlife/components/WildlifeTile.tsxapps/expo/features/wildlife/components/index.tsapps/expo/features/wildlife/data/speciesDatabase.tsapps/expo/features/wildlife/hooks/index.tsapps/expo/features/wildlife/hooks/useWildlifeHistory.tsapps/expo/features/wildlife/hooks/useWildlifeIdentification.tsapps/expo/features/wildlife/index.tsapps/expo/features/wildlife/lib/offlineIdentifier.tsapps/expo/features/wildlife/screens/IdentificationScreen.tsxapps/expo/features/wildlife/screens/SpeciesDetailScreen.tsxapps/expo/features/wildlife/screens/WildlifeScreen.tsxapps/expo/features/wildlife/screens/index.tsapps/expo/features/wildlife/types.tsapps/expo/lib/i18n/locales/en.jsonpackages/api/src/routes/index.tspackages/api/src/routes/wildlife/index.tspackages/api/src/services/index.tspackages/api/src/services/wildlifeIdentificationService.ts
- Fix searchSpecies() to also search imageDescription and habitat fields
(offline identification was skipping these purpose-built matching fields)
- Replace hardcoded 'Unknown species' with t('wildlife.unknownSpecies')
and add the key to en.json
- Remove unused wildlifeHistoryCountAtom (dead code)
- Remove redundant `as SelectedImage` cast and unused SelectedImage import
in IdentificationScreen (TypeScript narrows type in the conditional branch)
- Refactor WildlifeScreen to use useWildlifeHistory hook instead of
reading wildlifeHistoryAtom directly for consistent abstraction
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Addressed the issues in commit
All files pass |
|
@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. |
|
@copilot CodeRabbit has provided review feedback on this PR. Please review and address the comments. |
Critical: - wildlifeAtoms: fix double JSON encoding - pass AsyncStorage directly to createJSONStorage instead of manually calling JSON.parse/stringify - useWildlifeIdentification: fix empty-string ?? bug (offlineQuery='' won over filename); sanitize error log (no more raw Axios object in console) - SpeciesDetailScreen: fix history lookup to also search by species.id in h.results (was only checking history entry id, broke navigation from online identification results); add loading spinner before 'not found' - API route: move R2 delete to finally with best-effort catch so temp uploads are always cleaned up; make 500 response generic (no error.message) Major: - speciesDatabase: trim + early return in searchSpecies for blank queries - IdentificationScreen: call permanentlyPersistImageLocally() before addIdentification to persist image across cache evictions - WildlifeScreen: add explicit hasError branch (was silently showing empty state when AsyncStorage fails) - SpeciesCard: translate source/dangerLevel/category via t() Minor: - types.ts: add JSDoc documenting confidence is [0,1] - en.json: add historyLoadError, source.*, dangerLevel.*, category.* keys Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
All CodeRabbit comments addressed in commit Critical:
Major:
Minor:
All changes pushed in commit |
|
@copilot CodeRabbit has reviewed this PR. Please address the feedback. |
|
@copilot CodeRabbit has reviewed this PR. Please address the feedback. |
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@copilot CodeRabbit has reviewed this PR. Please address the feedback. |
|
@copilot There are still 22 unresolved CodeRabbit inline comments on this PR. Please address all remaining review feedback. |
|
@copilot CodeRabbit has reviewed this PR. Please address the feedback. |
|
@coderabbitai Copilot has pushed updates. Please re-review. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
apps/expo/features/wildlife/hooks/useWildlifeHistory.ts (1)
12-59:⚠️ Potential issue | 🟠 MajorAwait the hydrated history before mutating it.
baseWildlifeHistoryAtomis AsyncStorage-backed, soprevcan still be unresolved during hydration. These synchronous updaters will fail on cold start, andclearHistorycurrently dropsPromise.all(...)without waiting for the image cleanup to finish.♻️ Suggested fix
const addIdentification = useCallback( async ( imageUri: string, results: IdentificationResult[], location?: WildlifeIdentification['location'], ) => { const entry: WildlifeIdentification = { id: nanoid(), imageUri, timestamp: Date.now(), results, location, }; - setHistory((prev) => [entry, ...prev]); + setHistory(async (prev) => [entry, ...(await prev)]); return entry; }, [setHistory], ); const deleteIdentification = useCallback( (id: string) => { - setHistory((prev) => { - const entry = prev.find((e) => e.id === id); + setHistory(async (prev) => { + const history = await prev; + const entry = history.find((e) => e.id === id); if (entry?.imageUri) { - // Best-effort: delete the persisted image file - ImageCacheManager.clearImage(entry.imageUri).catch((err: unknown) => { + await ImageCacheManager.clearImage(entry.imageUri).catch((err: unknown) => { console.warn('Failed to delete wildlife image file:', err); }); } - return prev.filter((e) => e.id !== id); + return history.filter((e) => e.id !== id); }); }, [setHistory], ); const clearHistory = useCallback(() => { - setHistory((prev) => { - // Best-effort: delete all persisted image files before clearing - Promise.all( - prev.map((entry) => + setHistory(async (prev) => { + const history = await prev; + await Promise.all( + history.map((entry) => ImageCacheManager.clearImage(entry.imageUri).catch((err: unknown) => { console.warn('Failed to delete wildlife image file:', err); }), ), ); return []; }); }, [setHistory]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/wildlife/hooks/useWildlifeHistory.ts` around lines 12 - 59, The callbacks must handle the atom's potential Promise during hydration: make addIdentification, deleteIdentification and clearHistory async and, inside their setHistory updater functions, resolve prev with const resolved = await Promise.resolve(prev) before reading or mutating it; in clearHistory await Promise.all(...) for ImageCacheManager.clearImage calls on the resolved entries before returning [] so cleanup completes, and in deleteIdentification await resolving prev then filter/delete and return the new array (also include the same resolution in addIdentification before prepending the new entry).apps/expo/app/(app)/(tabs)/(home)/index.tsx (1)
133-149:⚠️ Potential issue | 🟡 MinorDon't freeze the wildlife search metadata at module load.
This entry is translated with the global
t()whiletileInfois initialized, so its title/keywords won't react to the later locale sync fromuseTranslation(). Search can still miss the translated wildlife label until the module reloads. Build this metadata fromDashboardScreeninstead of at module scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/app/`(app)/(tabs)/(home)/index.tsx around lines 133 - 149, The wildlife tile metadata (the "wildlife" entry inside tileInfo) is built at module load using the global t(), so its title/keywords won't update when locale changes; move construction of that metadata into DashboardScreen (or create a helper factory like buildTileInfo(t)) and generate the wildlife entry after calling useTranslation() so its title/keywords use the local t() and react to locale sync; reference the wildlife entry, tileInfo, WildlifeTile, DashboardScreen and useTranslation/useTranslation().apps/expo/features/wildlife/screens/IdentificationScreen.tsx (1)
63-84:⚠️ Potential issue | 🟠 MajorThe stale-request guard still doesn't protect against a photo change.
The comparison on Line 73 only reads the same closed-over
selectedImagecaptured when this request started, so it never sees a later selection. An in-flight result can still be written intosavedResults/history after the user switches to a different photo.Suggested fix
-import { useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; @@ const [descriptionText, setDescriptionText] = useState(''); const [savedResults, setSavedResults] = useState<IdentificationResult[] | null>(null); + const latestSelectedImageUriRef = useRef<string | null>(null); + + useEffect(() => { + latestSelectedImageUriRef.current = selectedImage?.uri ?? null; + }, [selectedImage?.uri]); @@ { onSuccess: async (identificationResults) => { - if (!selectedImage || selectedImage.uri !== imageUriAtStart) return; + if (latestSelectedImageUriRef.current !== imageUriAtStart) return; setSavedResults(identificationResults);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/wildlife/screens/IdentificationScreen.tsx` around lines 63 - 84, The stale-request guard in handleIdentify is comparing imageUriAtStart against the closed-over selectedImage, so it never detects a later selection; fix by comparing imageUriAtStart to the current selected image at callback time (not the captured one) — e.g., create and maintain a ref (selectedImageRef) that is updated whenever selectedImage changes, then in identify's onSuccess check if selectedImageRef.current?.uri !== imageUriAtStart and return early if so before calling setSavedResults/addIdentification; update any related code paths to use selectedImageRef for the live value.
🧹 Nitpick comments (2)
apps/expo/features/wildlife/hooks/useWildlifeIdentification.ts (1)
12-23: Consider handling the case whereuploadImagereturnsundefinedmore gracefully.
uploadImagecan returnundefinedwithout throwing whenfileNameis empty (see snippet fromuploadImage.tslines 5-7). The current check at line 14-16 handles this, but the error message could be more specific about why the upload failed.💡 Optional: More descriptive error message
const image = await uploadImage(selectedImage.fileName, selectedImage.uri); if (!image) { - throw new Error("Couldn't upload image"); + throw new Error("Couldn't upload image: fileName may be empty or upload failed"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/wildlife/hooks/useWildlifeIdentification.ts` around lines 12 - 23, The identifyOnline function should handle uploadImage returning undefined with a clearer error: when uploadImage(selectedImage.fileName, selectedImage.uri) yields no image, throw an Error that includes why (e.g., missing/empty fileName or failed upload) and relevant context (referencing selectedImage.fileName and selectedImage.uri) so callers can debug; update the error thrown in identifyOnline to include those details and/or a hint that uploadImage may return undefined when fileName is empty.apps/expo/features/wildlife/data/speciesDatabase.ts (1)
486-487: TypecategoryasSpeciesCategory.This exported helper currently accepts any string, so typoed callers silently return
[]instead of failing at compile time. Reusing the existing union keeps the API tighter.Suggested fix
-import type { SpeciesEntry } from '../types'; +import type { SpeciesCategory, SpeciesEntry } from '../types'; @@ -export function getSpeciesByCategory(category: string): SpeciesEntry[] { +export function getSpeciesByCategory(category: SpeciesCategory): SpeciesEntry[] { return SPECIES_DATABASE.filter((s) => s.category === category); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/wildlife/data/speciesDatabase.ts` around lines 486 - 487, The getSpeciesByCategory function accepts a plain string; change its parameter type to the existing union type SpeciesCategory so callers must pass a valid category at compile time (e.g., update the signature export function getSpeciesByCategory(category: SpeciesCategory): SpeciesEntry[]). Ensure SpeciesCategory is in scope (import or reference the file where it's defined) and leave the implementation using SPECIES_DATABASE.filter((s) => s.category === category) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/app/`(app)/wildlife/index.tsx:
- Around line 3-4: The WildlifeRoute currently renders WildlifeScreen
unconditionally; add a feature-flag guard that checks
enableWildlifeIdentification and prevents deep-link access when false. Update
the WildlifeRoute function (or add a directory _layout.tsx) to import and
evaluate enableWildlifeIdentification and, if disabled, redirect to a safe route
or return a fallback (e.g., navigate to home or render NotFound/Forbidden)
instead of rendering WildlifeScreen; ensure the check runs on initial render and
on navigation so deep links are blocked.
In `@apps/expo/features/wildlife/lib/offlineIdentifier.ts`:
- Around line 12-15: The identifyFromDescription function currently
short-circuits on an empty description before calling searchSpecies, bypassing
the blank-query fallback; change it to trim the input into a variable (e.g.,
const query = description.trim()) and pass that query into searchSpecies(query)
instead of returning early so searchSpecies can apply its blank-query fallback;
keep any existing handling of the returned IdentificationResult[] as before.
In `@apps/expo/features/wildlife/screens/IdentificationScreen.tsx`:
- Around line 39-50: The handler for image selection calls reset() and
setSavedResults(null) before knowing if the user completed takePhoto() or
pickImage(), which causes Cancel to clear results; move the calls so they run
only after a successful image was obtained—i.e., call takePhoto() or
pickImage(), check their returned result (or catch a cancelled error), and only
then invoke reset() and setSavedResults(null) (or call reset()/setSavedResults
inside the success branch after await takePhoto()/pickImage()). Update the async
selectedIndex handler that calls takePhoto(), pickImage(), reset(), and
setSavedResults to perform reset/setSavedResults only on success.
In `@packages/api/src/routes/wildlife/index.ts`:
- Around line 127-131: The temp-upload deletion is being fired-and-forgotten in
the finally block via PACKRAT_BUCKET.delete(image).catch(...); change this to
await the delete before sending the HTTP response by moving an awaited call to
await PACKRAT_BUCKET.delete(image) (or wrapped in try/catch to log errors) to
just before the final return c.json(...) so the deletion completes within the
request lifecycle; reference the existing PACKRAT_BUCKET.delete(image) call, the
image identifier variable, and the final c.json(...) return to locate where to
move/await the operation.
In `@packages/api/src/services/wildlifeIdentificationService.ts`:
- Around line 70-91: The generateObject call in wildlifeIdentificationService.ts
is using the `prompt` option with multimodal content (text + image) which only
accepts strings; replace the `prompt` property with `messages` and pass the
existing array (role: 'user', content: [{type:'text', text:...}, {type:'image',
image: imageUrl}]) so the call to generateObject (the invocation that includes
model: openai(DEFAULT_MODELS.OPENAI_CHAT), schema: identificationResponseSchema,
system: SPECIES_IDENTIFICATION_SYSTEM_PROMPT, temperature: 0.2) uses `messages`
instead of `prompt` to correctly send the image payload.
---
Duplicate comments:
In `@apps/expo/app/`(app)/(tabs)/(home)/index.tsx:
- Around line 133-149: The wildlife tile metadata (the "wildlife" entry inside
tileInfo) is built at module load using the global t(), so its title/keywords
won't update when locale changes; move construction of that metadata into
DashboardScreen (or create a helper factory like buildTileInfo(t)) and generate
the wildlife entry after calling useTranslation() so its title/keywords use the
local t() and react to locale sync; reference the wildlife entry, tileInfo,
WildlifeTile, DashboardScreen and useTranslation/useTranslation().
In `@apps/expo/features/wildlife/hooks/useWildlifeHistory.ts`:
- Around line 12-59: The callbacks must handle the atom's potential Promise
during hydration: make addIdentification, deleteIdentification and clearHistory
async and, inside their setHistory updater functions, resolve prev with const
resolved = await Promise.resolve(prev) before reading or mutating it; in
clearHistory await Promise.all(...) for ImageCacheManager.clearImage calls on
the resolved entries before returning [] so cleanup completes, and in
deleteIdentification await resolving prev then filter/delete and return the new
array (also include the same resolution in addIdentification before prepending
the new entry).
In `@apps/expo/features/wildlife/screens/IdentificationScreen.tsx`:
- Around line 63-84: The stale-request guard in handleIdentify is comparing
imageUriAtStart against the closed-over selectedImage, so it never detects a
later selection; fix by comparing imageUriAtStart to the current selected image
at callback time (not the captured one) — e.g., create and maintain a ref
(selectedImageRef) that is updated whenever selectedImage changes, then in
identify's onSuccess check if selectedImageRef.current?.uri !== imageUriAtStart
and return early if so before calling setSavedResults/addIdentification; update
any related code paths to use selectedImageRef for the live value.
---
Nitpick comments:
In `@apps/expo/features/wildlife/data/speciesDatabase.ts`:
- Around line 486-487: The getSpeciesByCategory function accepts a plain string;
change its parameter type to the existing union type SpeciesCategory so callers
must pass a valid category at compile time (e.g., update the signature export
function getSpeciesByCategory(category: SpeciesCategory): SpeciesEntry[]).
Ensure SpeciesCategory is in scope (import or reference the file where it's
defined) and leave the implementation using SPECIES_DATABASE.filter((s) =>
s.category === category) unchanged.
In `@apps/expo/features/wildlife/hooks/useWildlifeIdentification.ts`:
- Around line 12-23: The identifyOnline function should handle uploadImage
returning undefined with a clearer error: when
uploadImage(selectedImage.fileName, selectedImage.uri) yields no image, throw an
Error that includes why (e.g., missing/empty fileName or failed upload) and
relevant context (referencing selectedImage.fileName and selectedImage.uri) so
callers can debug; update the error thrown in identifyOnline to include those
details and/or a hint that uploadImage may return undefined when fileName is
empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4103d4cb-beb5-449d-95a0-3e935c45ccc0
📒 Files selected for processing (25)
apps/expo/app/(app)/(tabs)/(home)/index.tsxapps/expo/app/(app)/wildlife/[id].tsxapps/expo/app/(app)/wildlife/identify.tsxapps/expo/app/(app)/wildlife/index.tsxapps/expo/config.tsapps/expo/features/wildlife/atoms/wildlifeAtoms.tsapps/expo/features/wildlife/components/SpeciesCard.tsxapps/expo/features/wildlife/components/WildlifeTile.tsxapps/expo/features/wildlife/components/index.tsapps/expo/features/wildlife/data/speciesDatabase.tsapps/expo/features/wildlife/hooks/index.tsapps/expo/features/wildlife/hooks/useWildlifeHistory.tsapps/expo/features/wildlife/hooks/useWildlifeIdentification.tsapps/expo/features/wildlife/index.tsapps/expo/features/wildlife/lib/offlineIdentifier.tsapps/expo/features/wildlife/screens/IdentificationScreen.tsxapps/expo/features/wildlife/screens/SpeciesDetailScreen.tsxapps/expo/features/wildlife/screens/WildlifeScreen.tsxapps/expo/features/wildlife/screens/index.tsapps/expo/features/wildlife/types.tsapps/expo/lib/i18n/locales/en.jsonpackages/api/src/routes/index.tspackages/api/src/routes/wildlife/index.tspackages/api/src/services/index.tspackages/api/src/services/wildlifeIdentificationService.ts
| export function identifyFromDescription(description: string): IdentificationResult[] { | ||
| if (!description.trim()) return []; | ||
|
|
||
| const matches = searchSpecies(description); |
There was a problem hiding this comment.
Don't bypass the blank-query fallback here.
Line 13 returns early before searchSpecies() can apply the blank-query fallback added in this PR, so empty offline searches still produce no candidates. Pass the trimmed string through instead of short-circuiting.
Proposed fix
export function identifyFromDescription(description: string): IdentificationResult[] {
- if (!description.trim()) return [];
-
- const matches = searchSpecies(description);
+ const matches = searchSpecies(description.trim());
return matches.slice(0, 5).map((species, index) => ({
species,
confidence: Math.max(MIN_CONFIDENCE, BASE_CONFIDENCE - index * CONFIDENCE_DECAY),
source: 'offline' as const,
}));📝 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.
| export function identifyFromDescription(description: string): IdentificationResult[] { | |
| if (!description.trim()) return []; | |
| const matches = searchSpecies(description); | |
| export function identifyFromDescription(description: string): IdentificationResult[] { | |
| const matches = searchSpecies(description.trim()); | |
| return matches.slice(0, 5).map((species, index) => ({ | |
| species, | |
| confidence: Math.max(MIN_CONFIDENCE, BASE_CONFIDENCE - index * CONFIDENCE_DECAY), | |
| source: 'offline' as const, | |
| })); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/wildlife/lib/offlineIdentifier.ts` around lines 12 - 15,
The identifyFromDescription function currently short-circuits on an empty
description before calling searchSpecies, bypassing the blank-query fallback;
change it to trim the input into a variable (e.g., const query =
description.trim()) and pass that query into searchSpecies(query) instead of
returning early so searchSpecies can apply its blank-query fallback; keep any
existing handling of the returned IdentificationResult[] as before.
| } finally { | ||
| // Best-effort cleanup: delete temp upload regardless of identification outcome. | ||
| PACKRAT_BUCKET.delete(image).catch((err: unknown) => { | ||
| console.error('Failed to delete temp upload from R2:', err); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/api/src/routes/wildlife/index.ts | head -150Repository: PackRat-AI/PackRat
Length of output: 5734
🏁 Script executed:
# Search for PACKRAT_BUCKET definition
rg "PACKRAT_BUCKET" -A 3 -B 3 packages/api/srcRepository: PackRat-AI/PackRat
Length of output: 8534
🏁 Script executed:
# Check if this is a Cloudflare Workers environment
rg "waitUntil|CloudflareWorkers|ExecutionContext" packages/api/src -A 2Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
# Look for how other routes handle cleanup or promises
rg "finally|catch.*err" packages/api/src/routes -A 2 -B 1 | head -100Repository: PackRat-AI/PackRat
Length of output: 6528
🏁 Script executed:
# Check the full return path from finally block
cat -n packages/api/src/routes/wildlife/index.ts | sed -n '110,180p'Repository: PackRat-AI/PackRat
Length of output: 2331
🏁 Script executed:
# Check Hono/Cloudflare integration for waitUntil support
rg "ExecutionContext|waitUntil" packages/api/src -A 2Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
# Check the Env and Variables types to see if context has waitUntil
rg "type Env|type Variables" packages/api/src/types -A 10Repository: PackRat-AI/PackRat
Length of output: 369
Await the temp-upload deletion before returning the response.
PACKRAT_BUCKET.delete(image).catch(...) in the finally block is fire-and-forget; on Cloudflare Workers, this promise can be dropped after the response is sent. Other routes in this codebase (packs/analyzeImage.ts, packs/items.ts) correctly await delete operations synchronously. Move the await to line 167 before the final return c.json() to ensure the cleanup completes within the request lifecycle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/routes/wildlife/index.ts` around lines 127 - 131, The
temp-upload deletion is being fired-and-forgotten in the finally block via
PACKRAT_BUCKET.delete(image).catch(...); change this to await the delete before
sending the HTTP response by moving an awaited call to await
PACKRAT_BUCKET.delete(image) (or wrapped in try/catch to log errors) to just
before the final return c.json(...) so the deletion completes within the request
lifecycle; reference the existing PACKRAT_BUCKET.delete(image) call, the image
identifier variable, and the final c.json(...) return to locate where to
move/await the operation.
|
@copilot CodeRabbit completed another review pass and now has 27 inline comments. Please address all outstanding review feedback. |
|
@coderabbitai Copilot has pushed updates. Please re-review. |
|
✅ Actions performedReview triggered.
|
Critical: - Fix AI SDK generateObject call: use `messages` with ImagePart instead of `prompt` (which is text-only) for image-based species identification - Wrap imageUrl in `new URL()` for proper ImagePart type Major: - Add feature flag gate to all wildlife route pages (index, identify, [id]) redirecting to home when enableWildlifeIdentification is disabled - Pass undefined instead of empty string for offlineQuery so blank descriptions don't bypass the fileName fallback - Don't wipe identification results when user cancels image picker; only clear on successful new image selection - Improve isNetworkError: check Axios error codes (ERR_NETWORK, ECONNABORTED, etc.) and tighten message matching to avoid misclassifying server-side timeouts as network errors
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@copilot CodeRabbit has reviewed this PR. Please address the feedback. |
…, R2 cleanup, i18n, types Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
All addressed in commit
All 7 items addressed in commit |
There was a problem hiding this comment.
Pull request overview
This PR implements the Offline AI Plant/Wildlife Identification feature (Issue #1809). It adds a new Wildlife feature module to the Expo app with online AI identification (via the Cloudflare Workers API using OpenAI's vision model) and an offline fallback that performs keyword matching against a bundled local species database. The feature is gated behind a enableWildlifeIdentification feature flag (disabled by default).
Changes:
- New API endpoint
POST /api/wildlife/identifybacked by an OpenAI vision model, with R2 temp image management - New
features/wildlifemodule in the Expo app: atoms, hooks, screens (WildlifeScreen,IdentificationScreen,SpeciesDetailScreen), components, local species database, and offline identifier - i18n strings for all new wildlife UI, feature flag integration, and a new
WildlifeTileon the home dashboard
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
packages/api/src/services/wildlifeIdentificationService.ts |
New service wrapping OpenAI generateObject for species identification |
packages/api/src/services/index.ts |
Re-exports the new wildlife service |
packages/api/src/routes/wildlife/index.ts |
New POST /identify route with R2 cleanup and Zod-validated response |
packages/api/src/routes/index.ts |
Mounts wildlife routes under protectedRoutes |
apps/expo/lib/i18n/locales/en.json |
Adds all wildlife-related translation keys |
apps/expo/features/wildlife/types.ts |
Core types: SpeciesEntry, IdentificationResult, WildlifeIdentification |
apps/expo/features/wildlife/atoms/wildlifeAtoms.ts |
Jotai atomWithStorage + loadable for history persistence |
apps/expo/features/wildlife/data/speciesDatabase.ts |
Bundled North American species database with search utilities |
apps/expo/features/wildlife/lib/offlineIdentifier.ts |
Offline keyword matching against species database |
apps/expo/features/wildlife/hooks/useWildlifeIdentification.ts |
Mutation hook with online/offline fallback logic |
apps/expo/features/wildlife/hooks/useWildlifeHistory.ts |
History management hook with image file cleanup |
apps/expo/features/wildlife/hooks/index.ts |
Barrel export for hooks |
apps/expo/features/wildlife/components/SpeciesCard.tsx |
Card component for displaying a single identification result |
apps/expo/features/wildlife/components/WildlifeTile.tsx |
Home dashboard tile for the wildlife feature |
apps/expo/features/wildlife/components/index.ts |
Barrel export for components |
apps/expo/features/wildlife/screens/WildlifeScreen.tsx |
History list screen with loading/error/empty states |
apps/expo/features/wildlife/screens/SpeciesDetailScreen.tsx |
Detail view for a species from history or database |
apps/expo/features/wildlife/screens/IdentificationScreen.tsx |
Photo capture + identification flow screen |
apps/expo/features/wildlife/screens/index.ts |
Barrel export for screens |
apps/expo/features/wildlife/index.ts |
Top-level barrel export for the wildlife feature |
apps/expo/app/(app)/wildlife/index.tsx |
Route guarded by feature flag |
apps/expo/app/(app)/wildlife/identify.tsx |
Route guarded by feature flag |
apps/expo/app/(app)/wildlife/[id].tsx |
Route guarded by feature flag |
apps/expo/app/(app)/(tabs)/(home)/index.tsx |
Adds WildlifeTile to the dashboard |
apps/expo/config.ts |
Adds enableWildlifeIdentification: false feature flag |
You can also share your feedback on Copilot code review. Take the survey.
| message: error instanceof Error ? error.message : undefined, | ||
| }); | ||
| const trimmed = offlineQuery?.trim(); |
There was a problem hiding this comment.
When the online identification fails and the offline fallback is used, if offlineQuery is empty/not provided, the code falls back to selectedImage.fileName as the query (line 83-84). File names from the camera (camera_1234567890.jpg) or photo library (image_picker_xxxxxx.jpg) are unlikely to match any species in the local database, making the offline fallback return empty results when no description is provided.
A better fallback would be to return [] (no results) when no description is available for offline matching, which is more honest than a false attempt at matching with a filename. The PR checklist notes "Fix useWildlifeIdentification.ts: improve upload error message" as an unchecked item.
| // Clean up temp upload before responding on error | ||
| await PACKRAT_BUCKET.delete(image).catch((err: unknown) => { | ||
| console.error('Failed to delete temp upload from R2:', err); | ||
| }); | ||
|
|
||
| if (error instanceof Error) { | ||
| if ( |
There was a problem hiding this comment.
The 400 error response at line 122 returns the raw error.message directly to the client. The WildlifeIdentificationService wraps AI SDK errors as "Wildlife identification failed: <original message>", so the leaked message includes an internal AI SDK error string. While this is for specific cases ("Invalid image", "Unsupported image format"), exposing internal error messages from AI SDK libraries to API consumers is a security concern as it can reveal implementation details.
Consider returning a generic user-friendly message like "Invalid image format" for 400 responses instead of the raw wrapped AI error message.
| const history = await prev; | ||
| const entry = history.find((e) => e.id === id); | ||
| if (entry?.imageUri) { | ||
| await ImageCacheManager.clearImage(entry.imageUri).catch((err: unknown) => { | ||
| console.warn('Failed to delete wildlife image file:', err); | ||
| }); | ||
| } | ||
| return history.filter((e) => e.id !== id); | ||
| }); | ||
| }, | ||
| [setHistory], | ||
| ); | ||
|
|
There was a problem hiding this comment.
Similar to clearHistory, the deleteIdentification callback also performs async side effects (ImageCacheManager.clearImage(...)) inside the setHistory atom updater function. An atom updater should be a pure synchronous reducer. Triggering async operations from within it is an anti-pattern. The image cleanup should happen outside the atom updater: read the current entry first (or capture it in a local variable outside setHistory), then call setHistory to remove it, and finally call ImageCacheManager.clearImage() outside the atom setter.
| return c.json({ error: error.message }, 400); | ||
| } | ||
| } | ||
|
|
||
| return c.json({ error: 'Failed to identify species' }, 500); | ||
| } |
There was a problem hiding this comment.
The finally block fires a fire-and-forget R2 delete (PACKRAT_BUCKET.delete(image).catch(...)) without awaiting it. In Cloudflare Workers, unresolved promises may be abandoned as soon as the response is returned. This means the temporary upload may not be deleted.
The existing analyzeImage.ts (line 99) follows the correct pattern of await PACKRAT_BUCKET.delete(image) in the success path. The cleanup should be awaited. The finally block approach creates a fire-and-forget promise that the Workers runtime may cut short.
To reliably delete the temp upload, either await the delete directly (best for success cases) or use c.executionCtx.waitUntil(PACKRAT_BUCKET.delete(image)) to register the cleanup promise with the Workers runtime for best-effort scenarios.
| // Capture image URI at request-start time to guard against stale completions | ||
| // if the user changes image before this request resolves. | ||
| const imageUriAtStart = selectedImage.uri; | ||
| identify( |
There was a problem hiding this comment.
The stale-request guard at line 77 (selectedImage.uri !== imageUriAtStart) doesn't actually work as intended because selectedImage is captured as a stale closure at the time handleIdentify is invoked. If the user changes the image mid-flight, React re-renders the component with the new selectedImage, but the onSuccess callback was closed over the old render's selectedImage — so inside onSuccess, selectedImage.uri will always equal imageUriAtStart, making the check always false and never skipping the stale result.
To fix this, use a useRef to track the current image URI: declare const currentImageUriRef = useRef<string | null>(null), update it to selectedImage.uri in handleIdentify before calling identify, set it to null when the user selects a new image, and compare currentImageUriRef.current !== imageUriAtStart inside onSuccess. This is noted as an unchecked item in the PR description.
| wildlife: { | ||
| // Placeholder: title/keywords are overridden with locale-reactive values inside DashboardScreen | ||
| title: 'Wildlife', | ||
| keywords: [ | ||
| 'wildlife', | ||
| 'identify', | ||
| 'plant', | ||
| 'flower', | ||
| 'tree', | ||
| 'bird', | ||
| 'mammal', | ||
| 'species', | ||
| 'nature', | ||
| 'animal', | ||
| 'offline', | ||
| ], | ||
| component: WildlifeTile, |
There was a problem hiding this comment.
The tileInfo object is defined at module level and the wildlife entry calls the standalone t() function (imported from expo-app/lib/i18n, not useTranslation) at module initialization time. This means the translated strings for title and keywords are evaluated once when the module is first imported, before the component mounts.
This differs from all other entries in tileInfo, which use hardcoded English string literals and thus don't depend on i18n initialization timing. While this works for the English locale (since the i18n instance is initialized at the top of apps/expo/lib/i18n/index.ts), it creates an inconsistency with the rest of the file and will not react to locale changes at runtime.
The PR checklist notes this as an unresolved item: "Fix home/index.tsx: move wildlife tile title/keywords inside DashboardScreen with useMemo".
| history.map((entry) => | ||
| ImageCacheManager.clearImage(entry.imageUri).catch((err: unknown) => { | ||
| console.warn('Failed to delete wildlife image file:', err); | ||
| }), | ||
| ), | ||
| ); | ||
| return []; | ||
| }); | ||
| }, [setHistory]); | ||
|
|
||
| return { | ||
| historyState, | ||
| addIdentification, |
There was a problem hiding this comment.
The Promise.all inside the setHistory updater function is not awaited and its reference is discarded. The updater function passed to setHistory is a synchronous reducer and runs synchronously during Jotai atom update. Launching async side effects (file deletions) from inside an atom updater is problematic: the promises are fire-and-forget and there is no guarantee they'll be captured by the runtime before the function returns. Additionally, starting async work inside an atom updater (which is supposed to be a pure reducer) is an anti-pattern.
This should be restructured so that the image URIs are collected first (by reading from the current atom state outside the updater), then setHistory([]) is called synchronously, and the cleanup of image files happens separately outside the atom updater.
| export function getSpeciesByCategory(category: SpeciesCategory): SpeciesEntry[] { | ||
| return SPECIES_DATABASE.filter((s) => s.category === category); | ||
| } |
There was a problem hiding this comment.
The getSpeciesByCategory function parameter is typed as string instead of SpeciesCategory (which is defined in ../types.ts). Using the more specific type would provide compile-time safety and prevent passing invalid category strings. The PR description lists this as an unchecked item: "Fix speciesDatabase.ts: type getSpeciesByCategory parameter as SpeciesCategory".
searchSpeciesto includeimageDescriptionandhabitatin searcht('wildlife.unknownSpecies')wildlifeHistoryCountAtomas SelectedImagecastWildlifeScreento useuseWildlifeHistoryhookwildlifeAtoms.tssearchSpecies??bug inuseWildlifeIdentificationoffline pathuseWildlifeIdentificationpermanentlyPersistImageLocallyinIdentificationScreenSpeciesDetailScreenhistory lookup + loading statehasErrorbranch inWildlifeScreenfinallyin API routesource,dangerLevel,categoryinSpeciesCardandSpeciesDetailScreenconfidenceintypes.tsen.jsonenableWildlifeIdentification: false(ship dark by default)deleteIdentification/clearHistoryisNetworkError(remove over-broadreturn truefallback)scientificNamefallback in API species.id slugpermanentlyPersistImageLocallyin try/catch inIdentificationScreenhasErrorbranch inSpeciesDetailScreen(surfaces history load error instead of "not found"){ getOnInit: true }toatomWithStoragesoloadablecorrectly enters loading state on cold startIdentificationScreenonSuccess(ignore results if user changed image mid-flight)t()for wildlife tile title and category keywords inhome/index.tsxgenerateObjectinwildlifeIdentificationService.tsWildlifeScreenofflineIdentifier.ts: remove early-return, pass trimmed query tosearchSpeciesIdentificationScreen.tsx: useuseRefto track current image URI for accurate stale-request guardfinallyblock)useWildlifeHistory.ts: async updaters for AsyncStorage-backed atom to handle hydrationhome/index.tsx: move wildlife tiletitle/keywordsinsideDashboardScreenwithuseMemofor locale reactivityspeciesDatabase.ts: typegetSpeciesByCategoryparameter asSpeciesCategoryuseWildlifeIdentification.ts: improve upload error message with filename contextOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by CodeRabbit