Build Simple UI to be able to visually test new API for Incidents #262
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
conservationtimothy
changed the title
Ok this is ready : ) |
IamJeffG
left a comment
IamJeffG
left a comment
There was a problem hiding this comment.
Generally very good code. I am very concerned about the length of AlertsDashboard.vue but would not block merge solely on that.
I am marking "request changes" but what I actually want is more context from you to help me review server/database/annotatedCollections.ts. I did not really review that file, am not clear what's in-scope or what's going on there.
| SELECT * FROM ${sql.identifier(entry.source_table)} WHERE _id = ${entry.source_id} OR id = ${entry.source_id} LIMIT 1 | ||
| // Fetch source data from warehouse database (source tables use _id as primary key) | ||
| const sourceResult = await warehouseDb.execute(sql` | ||
| SELECT * FROM ${sql.identifier(entry.source_table)} WHERE _id = ${entry.source_id} LIMIT 1 |
There was a problem hiding this comment.
Do you know why the OR id=... was there before? I agree that it seems risky to have had it, but maybe there was a reason it was there?
Or is this more related to changing the database and that's why it's justified? Please help me understand the changes in this file -- and is it actually related to the new UI or should it be its own PR?
There was a problem hiding this comment.
Initially, assumed id was the primary key for all warehouse tables. During testing, I found that most tables (e.g., fake_alerts, bcmform_responses) use _id as the primary key, while mapeo_data uses id. We have now updated the code to select the column by table name: id for mapeo_data, _id for all others. This ensures correct queries when fetching source data for collection entries. This fix belongs in the same PR as the annotated collections migration because it’s part of the same feature (querying warehouse data when creating incidents), the issue only surfaces when creating incidents with mapeo features, and it’s a small, related correctness improvement.
There was a problem hiding this comment.
@rudokemper does this match your understanding & intent?
| {{ | ||
| showIncidentsSidebar | ||
| ? "Hide incidents sidebar" | ||
| : "View saved incidents and create new ones" |
There was a problem hiding this comment.
I'm ok leaving i18n out of this PR. 👍
There was a problem hiding this comment.
Yes, but let's add a sub-issue to the epic to handle i18n.
| }); | ||
| incidents.value = response.incidents; | ||
| } catch (error) { | ||
| console.error("Error fetching incidents:", error); |
There was a problem hiding this comment.
probably want to surface these API failures to the user, but I'm ok keeping that as a TODO. Same for in createIncident
|
Great work, Osa! Some feedback on the UI, not code review: When I am in the process of selecting records to be part of a new incident, the sidebar UI is a little confusing. I think that this state should no longer show "Saved Incidents". Maybe it is enough to remove that, and keep the button to add a new one, like so: 
I think there should be a "Clear" control that deselects all of the records that I have selected until now. I didn't see a way to deselect anything outside of the "Clear All" button in the Incident sidebar - I intuited that I might be able to by ctrl-clicking, or drawing a bounding box over, already selected features, but that doesn't work. I didn't see a way to "activate", open, or otherwise interact with existing saved incidents. Just making sure that this was out of scope for this PR, but forthcoming in another one? |
Yes it is out of scope but will be added in a new PR. otherwise great feedback and will implement. |
…incidents when selecting
|
I have updated the incidents UI:
|
Goal
Add a simple UI to test the annotated collections/incidents API endpoints in the Alerts Dashboard. Closes #190
Screenshots
Bounding box 👆🏿
What I changed and why
Added a simple UI to AlertsDashboard.vue for testing the annotated collections/incidents API. Includes three control buttons (View Incidents, Multi-select, Bounding Box) at 50% screen height, an IncidentsSidebar component for viewing/creating incidents, and multi-select (Ctrl+click) and bounding box which is WIP (Ctrl (Command)+drag) functionality to add map features to collections.
Also Merged main into feat/annotated-collections-feature-branch and ran database migrations to ensure tables exist.
What I'm not doing here
This is a minimal testing UI so stuff can be more comprehensive
LLM use disclosure
Used LLM (Auto/Cursor) for code generation of the incidents UI components minor things like placing 50% of the screen as well as bounding box research