Skip to content

Conversation

@david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented Dec 17, 2025

This change adds disk detail side modals to the disk list page and the instance disks tab. The one on the disk list gets its own route, but since the one on instances is already nested under the storage tab, I decided it's probably not worth giving it a path. I experimented with ?disk=xyz in the query string, but it feels a bit pointless and complicates the code.

The motivation for this is that while working on local disks in #2984, I talked to @jmpesp and we agreed it is hard to make sense of the error that you can't attach a local disk associated with one sled to an instance with local disks on another sled. To help the user understand and deal with this, we want to put sled ID in the local disk response. Without this detail modal, there would be no place to display that sled ID.

The contents of the modal will have to be updated in #2984. I just wanted to keep the bulk of this change outside of that PR.

While working on this I noticed that the AGENTS.md was too long and also out of date in some ways, so I cleaned that up too.

Screenshot 2025-12-17 at 3 49 21 PM Screenshot 2025-12-17 at 3 49 10 PM

@vercel
Copy link

vercel bot commented Dec 17, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
console Ready Ready Preview Dec 18, 2025 11:42pm

isOpen
onDismiss={onDismiss}
animate={animate}
title="Disk details"
Copy link
Collaborator Author

@david-crespo david-crespo Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to call this Disk, but it looks stupid. The other modals like this (project image, silo image, identity provider) are all naturally two-word titles. @benjaminleonard @charliepark if you think it looks fine as Disk, I'm fine changing it.

Image

await page.getByRole('table').click() // focus the content pane
await page.mouse.wheel(0, 200)
// scroll so the dropdown menu isn't behind the pagination bar
await row.scrollIntoViewIfNeeded()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude figured out this test was breaking with an unwanted nav to disk detail because the click on the table was landing right in the middle of the table, on a disk link cell!

@david-crespo
Copy link
Collaborator Author

Back to the usual Firefox flake. Nature is healing.

return <span className="text-default">{data.data.name}</span>
return (
<LinkCell to={pb.disk({ project, disk: data.data.name })}>{data.data.name}</LinkCell>
)
Copy link
Collaborator Author

@david-crespo david-crespo Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the details view is just a side modal, we could theoretically just open it right on the snapshots page and it would work fine. The only problem with that is that it wouldn't be able to have a URL, which is probably also fine. I feel a little weird about there being two places to see disk details. On the other hand the current interaction is kind of jarring. It feels like a long way to go to nav to the disks page with a side modal open.

@benjaminleonard
Copy link
Contributor

It's more or less okay, but I want to prod at the design a little. Solo properties table looks funky. Are you in a rush to merge?

@david-crespo
Copy link
Collaborator Author

Not a big rush, would be nice if it was today. I want to get it into #2984.

@david-crespo
Copy link
Collaborator Author

david-crespo commented Dec 18, 2025

This got much bigger. I pulled out a read-only side modal form component. And used it in all of the read-only forms, which includes SSH key edit, Gateway edit, Image edit, and Identity Provider edit. This is nice because it makes these behave more consistently: they all get a footer with a close button and they all use the animate prop correctly to avoid animating in on a fresh pageload when the modal is supposed to already be open.

I also decided to make the disk detail modal for snapshots open inline on the snapshots page instead of navigating to the disks list page with the side modal open. It feels way better.

2025-12-18-snapshot-disk-modal.mp4

<form
id={id}
className="ox-form is-side-modal"
className="ox-form"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The is-side-modal styling mechanism was removed in #2815

@david-crespo david-crespo enabled auto-merge (squash) December 18, 2025 23:46
@david-crespo david-crespo merged commit b1fba78 into main Dec 18, 2025
7 checks passed
@david-crespo david-crespo deleted the disk-detail-modal branch December 18, 2025 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants