Conversation
Search improvements: - Silos: fix multi-page search - Silo Images: add search - Projects: fix multi-page search - Project Instances: fix multi-page search - Project VPCs: fix multi-page search - Project IP Pools: fix multi-page search - Project Floating IPs: add search - Project Images: add search Add buttons to cmd-k: - Project Affinity: "New Group" - Project Access: "Add user or group" - Project Disks: "New disk" - Project Floating IPs: "New Floating IP" - Project Images: "Upload Image" - Project Snapshots: "New Snapshot" - Project VPC: "New VPC" - Silo Access: "Add user or Group" - Silo Images: Add "Promote image"
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I will review in more detail, but it all looks solid at first glance. We were already tracking some of these issues in #1352, so thanks for fixing them! |
| [] | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh and it's janky if you click the background because that only affects the form, so you close the form but not the ctrl-k popup. Out of scope for this PR, just noting it.
| })), | ||
| ], | ||
| [navigate, silos] | ||
| [navigate, allSilos] |
There was a problem hiding this comment.
We need to make sure this hook behaves correctly when the silos request comes back (the old one would have been prefetched). I'm pretty sure it does — that's the point of this remove callback. On every re-run of the effect, we remove whatever the last one added before adding the new items.
I would leave this one not-prefetched, as you've done. Since quick actions are hidden until the user opens the menu, it's the perfect thing to defer.
console/app/hooks/use-quick-actions.tsx
Lines 91 to 106 in b43b995
| </PageHeader> | ||
| <TableActions> | ||
| <CreateLink to={pb.projectsNew()}>New Project</CreateLink> | ||
| <CreateLink to={pb.projectsNew()}>New project</CreateLink> |
There was a problem hiding this comment.
I did these out of perfectionism, but it doesn't matter in practice because the button text get all caps from CSS anyway.
| [navigate, project, allDisks] | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
I tested the menu with 1000 disks at http://localhost:4000/projects/other-project/disks and filtering and scrolling were instantaneous. My computer is fast, but I think the menu is also just fast. It's just a big ul.
#3129 inspired me to add a couple more features to the quick actions menu, which in turn inspired a big refactor to avoid problems with possible duplicate entries. I also added a bunch of e2e tests that we probably should have had before. ### New features - `ctrl+n`/`ctrl+p` keyboard navigation (in addition to arrow keys) - "Go up" actions derived from breadcrumbs, so you can quickly navigate to parent pages - Links are real links, which means you can middle click them if you insist https://github.com/user-attachments/assets/d40d4d80-88b4-41e6-a41b-e7274520fa11 ### Refactor The old store held a flat list of `QuickActionItem`. https://github.com/oxidecomputer/console/blob/751a80e64b32bacbfc22379e70c24383685cb853/app/ui/lib/ActionMenu.tsx#L22-L28 `value` there is the text label displayed in the UI and (problematically) is also meant to uniquely identify the item. When calling pages (e.g., the project layout and the instance list page) called `useQuickActions` with a list of items, `useQuickActions` called `addActions`, which deduped the list of items based on `value`. The problem is that items with the same `value` in different groups could collide — removing by value would remove items you didn't meant to remove. We had an `invariant` call making sure the values were unique, but this is actually pretty bad in hindsight because it's probably possible for a user to name something to create a duplicate. (In practice we probably avoided this because all user-settable names are lower-case and all our nav items have upper case letters.) In any case, the new breadcrumbs feature added a new source of items that could potentially be a duplicate with something else in the list. That is what prompted me to get rid of the deduping mechanism altogether. The new store is a `Map<string, QuickActionItem[]>` keyed by an ID for each calling page, so registration is just `map.set(id, items)` and cleanup is `map.delete(id)`. Deduping is unnecessary because each source component owns its own slot. In theory each source is responsible for deduping its items, but the way the item lists are constructed, this is never really a problem. And if a duplicate sneaks in, it doesn't actually matter. Finally, since I was already messing everything up, I changed `QuickActionItem` to take both link and callback items, like we do for menu items. They're almost all links — only a few modal-opening actions are not.
oxidecomputer/console@ac30d2d...d697520 * [d697520c](oxidecomputer/console@d697520c) oxidecomputer/console#3135 * [c7362f2a](oxidecomputer/console@c7362f2a) oxidecomputer/console#3132 * [bde9b1d2](oxidecomputer/console@bde9b1d2) oxidecomputer/console#3131 * [f57bb8ae](oxidecomputer/console@f57bb8ae) oxidecomputer/console#3134 * [8cabc2de](oxidecomputer/console@8cabc2de) oxidecomputer/console#3133 * [751a80e6](oxidecomputer/console@751a80e6) oxidecomputer/console#3129

I was initially excited about using
cmd-kto navigate the UI with the keyboard but immediately ran into some limitations and so took a stab at improving them.This is my first PR to oxidecomputer/console and was Claude assisted.
I have tested locally and verified these changes seem to work as expected. (see images in #3128)
If this is the wrong approach / redundant, feel free to summarily close.