feat: add species lists with fetch, custom lists, and detection filtering#6
feat: add species lists with fetch, custom lists, and detection filtering#6
Conversation
…ring Add a Species tab that lets users fetch expected bird species for a location and week from birda CLI (eBird data), create custom species lists from the label service, and filter detections by species list. - New database tables: species_lists, species_list_entries (migration 2) - birda CLI species command wrapper using buffered JSON - Species page with two-panel layout, fetch modal, custom list modal - Species list dropdown filter on DetectionsPage - Portal fix for CoordinateInput map picker in nested modals - Use $state.snapshot() to avoid Proxy serialization over IPC Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds a species-list management feature: translations, DB migrations and CRUD, birda CLI integration, IPC handlers, renderer IPC wrappers, sidebar/tab, a SpeciesPage UI, and detection filtering by species list. Changes
Sequence DiagramssequenceDiagram
actor User
participant Renderer as SpeciesPage (Renderer)
participant IPC as Main (IPC)
participant Birda as Birda CLI
participant DB as Database
User->>Renderer: Open Fetch Modal & submit (lat, lon, week, threshold)
Renderer->>IPC: invoke species:fetch (lat, lon, week, threshold)
IPC->>Birda: exec birda --lat --lon --week [--threshold]
Birda-->>IPC: JSON envelope with payload.species
IPC-->>Renderer: BirdaSpeciesResponse
User->>Renderer: Save fetched list (name)
Renderer->>IPC: invoke species:save-list (name, response)
IPC->>DB: INSERT INTO species_lists
IPC->>DB: INSERT INTO species_list_entries (transaction)
DB-->>IPC: created SpeciesList
IPC-->>Renderer: SpeciesList
sequenceDiagram
actor User
participant Renderer as DetectionsPage (Renderer)
participant IPC as Main (IPC)
participant DB as Database
User->>Renderer: Select species list in dropdown
Renderer->>IPC: getDetections(filter with species_list_id)
IPC->>DB: SELECT ... WHERE scientific_name IN (SELECT scientific_name FROM species_list_entries WHERE list_id = ?)
DB-->>IPC: filtered detections
IPC-->>Renderer: detections array
Renderer->>Renderer: update table
sequenceDiagram
actor User
participant Renderer as SpeciesPage (Renderer)
participant IPC as Main (IPC)
participant Labels as Labels Service
participant DB as Database
User->>Renderer: Open Custom Modal, enter search term
Renderer->>IPC: labels:search-by-common-name (query)
IPC->>Labels: search
Labels-->>IPC: matching scientific names
IPC-->>Renderer: results
User->>Renderer: select species and create list (name)
Renderer->>IPC: species:create-custom-list (name, scientificNames, description)
IPC->>DB: INSERT INTO species_lists
IPC->>DB: INSERT INTO species_list_entries
DB-->>IPC: created SpeciesList
IPC-->>Renderer: SpeciesList
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @tphakala, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's bird species management capabilities by introducing a new 'Species' section. Users can now generate species lists based on geographical location and time of year, create personalized custom lists, and seamlessly apply these lists to filter their detection data. This feature set provides a powerful tool for ornithological data analysis and organization, improving the overall utility for bird enthusiasts and researchers. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature for managing species lists, including fetching from an external source, creating custom lists, and filtering detections. The implementation is comprehensive, covering backend logic, database migrations, IPC communication, and a new UI page. The code is well-structured, particularly the new Svelte components which make good use of Svelte 5 runes for state management. My review focuses on improving error handling and robustness in a few areas, such as replacing unsafe type assertions with safer checks, and ensuring consistency in how IPC calls are made from the renderer. Overall, this is a solid contribution that adds valuable functionality.
| try { | ||
| const envelope = JSON.parse(stdout) as BirdaJsonEnvelope; | ||
| resolve(envelope.payload as unknown as BirdaSpeciesResponse); | ||
| } catch { | ||
| reject(new Error(`Failed to parse birda species output: ${stdout.slice(0, 200)}`)); | ||
| } |
There was a problem hiding this comment.
The type assertion as unknown as BirdaSpeciesResponse is unsafe and can lead to runtime errors if the birda CLI payload format changes. A simple property check would make this more robust. Additionally, the catch block for JSON parsing should capture the error object for better diagnostics.
try {
const envelope = JSON.parse(stdout) as BirdaJsonEnvelope;
const payload = envelope.payload;
if (payload && typeof payload === 'object' && 'species' in payload) {
resolve(payload as BirdaSpeciesResponse);
} else {
reject(new Error('Unexpected payload format from birda species command.'));
}
} catch (e) {
const errorDetails = e instanceof Error ? e.message : String(e);
reject(new Error(`Failed to parse birda species output: ${errorDetails}. Output: ${stdout.slice(0, 200)}`));
}
src/main/db/species-lists.ts
Outdated
| stmt.run(listId, s.scientific_name, s.common_name, s.frequency); | ||
| } | ||
|
|
||
| return getSpeciesListById(listId)!; |
There was a problem hiding this comment.
The non-null assertion ! can hide potential bugs. If getSpeciesListById returns undefined for some reason, this will cause a runtime crash. It's safer to explicitly check for the new list and throw a descriptive error if it's not found.
const newList = getSpeciesListById(listId);
if (!newList) {
throw new Error(`Failed to retrieve newly created species list with id ${listId}`);
}
return newList;
src/main/db/species-lists.ts
Outdated
| stmt.run(listId, sn); | ||
| } | ||
|
|
||
| return getSpeciesListById(listId)!; |
There was a problem hiding this comment.
The non-null assertion ! can hide potential bugs. If getSpeciesListById returns undefined for some reason, this will cause a runtime crash. It's safer to explicitly check for the new list and throw a descriptive error if it's not found.
const newList = getSpeciesListById(listId);
if (!newList) {
throw new Error(`Failed to retrieve newly created custom species list with id ${listId}`);
}
return newList;| const scientificNames = (await window.birda.invoke( | ||
| 'labels:search-by-common-name', | ||
| customSearchQuery, | ||
| )) as string[]; | ||
| // Resolve common names | ||
| const nameMap = (await window.birda.invoke('labels:resolve-all', scientificNames)) as Record<string, string>; |
There was a problem hiding this comment.
These direct calls to window.birda.invoke are inconsistent with the practice of using typed wrappers from src/renderer/src/lib/utils/ipc.ts. Creating wrappers for these functions would improve type safety and maintainability.
You should add the following wrappers to ipc.ts:
// In src/renderer/src/lib/utils/ipc.ts
export function searchByCommonName(query: string): Promise<string[]> {
return window.birda.invoke('labels:search-by-common-name', query) as Promise<string[]>;
}
export function resolveAllLabels(scientificNames: string[]): Promise<Record<string, string>> {
return window.birda.invoke('labels:resolve-all', scientificNames) as Promise<Record<string, string>>;
}Then, you can import and use them here.
const scientificNames = await searchByCommonName(customSearchQuery);
// Resolve common names
const nameMap = await resolveAllLabels(scientificNames);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/main/db/species-lists.ts`:
- Around line 52-55: The CI failure is due to unused exported functions: remove
the unnecessary export modifier from getSpeciesListById (it’s used only
internally so make it a plain function) and either delete
getSpeciesListScientificNames entirely or remove its export if you intend to
keep it for future internal use; update the declarations for getSpeciesListById
and getSpeciesListScientificNames accordingly so no unused exports remain.
- Line 43: Replace the forbidden non-null assertion on
getSpeciesListById(listId)! by checking the result at runtime: call const list =
await getSpeciesListById(listId) (or just const list =
getSpeciesListById(listId) if synchronous), if (!list) throw a descriptive Error
(e.g., `Species list not found after insert for id ${listId}`) and then return
list; apply the exact same pattern for the second occurrence at the other call
site (the call that was flagged at line 100) to satisfy the linter and provide a
defensive guard.
In `@src/renderer/src/pages/DetectionsPage.svelte`:
- Around line 161-174: When handling the cross-tab intent in the effect that
watches appState.selectedSpeciesListId (variables: prevSpeciesListIntent,
speciesListFilterId, offset, loadRunDetections), refresh or re-fetch the
speciesLists before applying the intent so the dropdown includes the newly
created list (e.g., call the same loader used in onMount or update speciesLists
if the id is missing), then apply speciesListFilterId, reset offset, and call
loadRunDetections; to allow re-triggering the same-list intent also clear the
intent after consuming it by setting appState.selectedSpeciesListId = null (this
requires a matching change in SpeciesPage.handleUseAsFilter to set the id again
when used).
In `@src/renderer/src/pages/SpeciesPage.svelte`:
- Around line 96-102: The handleUseAsFilter function currently sets
appState.selectedSpeciesListId to selectedList.id, which is a no-op if the value
is already equal and thus won't retrigger the DetectionsPage $effect; modify
handleUseAsFilter to force a change before setting the intended id — for example
set appState.selectedSpeciesListId = null (or decrement/increment a separate
appState.speciesListFilterVersion counter) then assign
appState.selectedSpeciesListId = selectedList.id so the downstream watcher
(prevSpeciesListIntent in DetectionsPage) always observes a change and reloads
the filter when the same list is clicked again.
🧹 Nitpick comments (6)
src/renderer/src/lib/components/CoordinateInput.svelte (1)
27-34: Portal action looks good for the stated purpose.This is a well-known Svelte pattern for escaping parent stacking contexts. Consider extracting it to a shared utility (e.g.,
$lib/actions/portal.ts) if other components need the same behavior.src/main/birda/species.ts (1)
34-47: Consider validating thatenvelope.payloadexists before casting.If the CLI returns a valid JSON envelope but with a missing or null
payload, the cast on line 42 would silently produce an object missing all expected fields, leading to confusing downstream errors.🛡️ Suggested defensive check
try { const envelope = JSON.parse(stdout) as BirdaJsonEnvelope; + if (!envelope.payload) { + reject(new Error('birda species response missing payload')); + return; + } resolve(envelope.payload as unknown as BirdaSpeciesResponse); } catch {src/main/db/database.ts (1)
98-99: Redundant delete — CASCADE handles this automatically.With
foreign_keys = ONandON DELETE CASCADEonspecies_list_entries, deleting fromspecies_listswould automatically cascade-delete entries. The explicitDELETE FROM species_list_entriesis harmless but unnecessary.src/renderer/src/pages/SpeciesPage.svelte (3)
28-34: Use$derived.byinstead of$derivedfor computed values.
$derived(() => { ... })produces a derived value that is the arrow function itself, not its return value. On line 391, you callfilteredEntries()which works, but Svelte can't track when the derived value changes (the function reference is stable). This means the filter logic reruns on every unrelated template re-render instead of only whenentriesorentryFilterchange.Proposed fix
- const filteredEntries = $derived(() => { + const filteredEntries = $derived.by(() => { if (!entryFilter) return entries; const q = entryFilter.toLowerCase(); return entries.filter( (e) => e.resolved_common_name.toLowerCase().includes(q) || e.scientific_name.toLowerCase().includes(q), ); });Then on line 391, use
filteredEntrieswithout the call:- {`#each` filteredEntries() as entry (entry.id)} + {`#each` filteredEntries as entry (entry.id)}
304-308: Destructive delete action has no confirmation prompt.
handleListDeleteimmediately deletes the species list without a confirmation dialog. Since lists can contain fetched data that takes time to reproduce, consider adding a confirmation step.
168-188: Add typed wrappers for label search and resolution IPC calls.The
doCustomSearch()function uses rawwindow.birda.invokecalls forlabels:search-by-common-nameandlabels:resolve-all, while the rest of this file uses typed wrappers from$lib/utils/ipc(e.g.,fetchSpeciesList,getSpeciesLists). These two channels lack wrapper functions inipc.ts, creating an inconsistency in type safety and code style.Add typed wrappers in
$lib/utils/ipcfor these two channels to maintain consistency with the rest of the codebase.
- Remove unused exports (getSpeciesListById, getSpeciesListScientificNames) to fix knip CI failure - Replace non-null assertions with runtime checks in species-lists.ts - Add payload validation in birda species CLI wrapper - Use $derived.by instead of $derived for filteredEntries in SpeciesPage - Add typed IPC wrappers (searchByCommonName, resolveAllLabels) in ipc.ts - Fix cross-tab intent: refresh species lists dropdown and allow re-triggering same list via "Use as Detection Filter" - Add missing i18n keys for hardcoded English strings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The type checker requires casting through unknown when converting Record<string, unknown> to BirdaSpeciesResponse. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/birda/species.ts`:
- Around line 40-51: The code currently uses "'species' in payload" which throws
when envelope.payload is undefined; update the parsing logic in the try block
(around envelope and payload variables in this module) to first verify payload
exists and is an object (e.g., if (!payload || typeof payload !== 'object' ||
!('species' in payload)) ) and then reject with a clear "Malformed birda
envelope: missing payload or species" (or similar) error instead of letting the
in-operator throw; ensure you still resolve with payload cast to
BirdaSpeciesResponse when valid and keep the existing catch for genuine JSON
parse errors.
In `@src/renderer/src/pages/SpeciesPage.svelte`:
- Around line 202-217: The handler handleCreateCustomList silently logs errors;
update it to set a user-visible error state (e.g., create a new variable like
customCreateError or reuse an existing fetchError) inside the catch block and
keep showCustomModal true so the modal stays open; ensure the modal
component/template displays that error (inline text or toast) when
customCreateError is set and clear the error on successful creation or when the
modal is closed; reference handleCreateCustomList, createCustomSpeciesList,
showCustomModal, and the modal's error display logic to implement this.
🧹 Nitpick comments (5)
src/main/birda/species.ts (1)
12-32: Consider validating numeric inputs before spawning the process.If
NaNorInfinityleaks in (e.g. from a UI parsing bug),String(NaN)is sent to the CLI, producing a confusing downstream error. A quick guard at the top would surface the problem earlier.🛡️ Suggested guard
): Promise<BirdaSpeciesResponse> { + if (!Number.isFinite(latitude) || !Number.isFinite(longitude) || !Number.isFinite(week)) { + throw new Error('latitude, longitude, and week must be finite numbers'); + } const birdaPath = await findBirda();src/renderer/src/pages/SpeciesPage.svelte (3)
170-185: Resolve labels only for the displayed slice, not the entire result set.
resolveAllLabelsis called with all results fromsearchByCommonName(potentially hundreds), but only the first 50 are shown. Slice first, then resolve, to avoid unnecessary IPC overhead.♻️ Proposed fix
async function doCustomSearch() { if (!customSearchQuery.trim()) { customSearchResults = []; return; } try { const scientificNames = await searchByCommonName(customSearchQuery); - const nameMap = await resolveAllLabels(scientificNames); - customSearchResults = scientificNames.slice(0, 50).map((sn) => ({ + const sliced = scientificNames.slice(0, 50); + const nameMap = await resolveAllLabels(sliced); + customSearchResults = sliced.map((sn) => ({ scientific_name: sn, common_name: nameMap[sn] ?? sn, })); } catch { customSearchResults = []; } }
85-96: List deletion has no confirmation prompt — accidental deletes are irrecoverable.
handleListDeleteimmediately deletes without any user confirmation. A fetched list (which required network + CLI invocation) cannot be easily recreated. Consider adding a simple confirmation step (e.g.,window.confirmor a small inline prompt).
98-104: Synchronous null-then-set may be batched in Svelte 5.In Svelte 5, effects run after the current microtask. Setting
selectedSpeciesListId = nullthen immediately= selectedList.idin the same synchronous call means the effect only observes the final value. This works for the normal flow (because DetectionsPage nullifies after consuming), but won't re-trigger if the user clicks "Use as Filter" very rapidly before the effect runs.This is an unlikely edge case and was addressed per prior review feedback, so flagging only for awareness.
src/renderer/src/pages/DetectionsPage.svelte (1)
235-250: Species list dropdown is stale if user creates a list without using "Use as Filter".The dropdown is populated on mount and refreshed only via the cross-tab intent. If the user navigates to Species, creates/deletes lists, then manually navigates back to Detections (without clicking "Use as Filter"), the dropdown will show stale data until the next page mount.
A lightweight fix would be to refresh the list when the detections tab becomes active:
♻️ Sketch: refresh on tab activation
Add to an existing or new
$effect:$effect(() => { if (appState.activeTab === 'detections') { getSpeciesLists() .then((l) => (speciesLists = l)) .catch(() => {}); } });This could be merged into the existing cross-tab intent effect or kept separate depending on preference.
- Guard against undefined envelope.payload in birda species CLI wrapper - Add user-visible error feedback for custom species list creation failure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
speciescommand) and saving as named listsNew files
src/main/birda/species.tssrc/main/db/species-lists.tssrc/main/ipc/species.tssrc/renderer/src/pages/SpeciesPage.svelteModified files
shared/types.tssrc/main/db/database.tssrc/main/db/detections.tssrc/main/ipc/handlers.tssrc/preload/index.tssrc/renderer/.../ipc.tssrc/renderer/.../app.svelte.tssrc/renderer/.../Sidebar.sveltesrc/renderer/.../App.sveltesrc/renderer/.../DetectionsPage.sveltesrc/renderer/.../CoordinateInput.sveltesrc/main/index.tsmessages/en.jsonTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit