Skip to content

Audit log#2860

Open
david-crespo wants to merge 65 commits into
mainfrom
audit-log
Open

Audit log#2860
david-crespo wants to merge 65 commits into
mainfrom
audit-log

Conversation

@david-crespo

@david-crespo david-crespo commented Jul 23, 2025

Copy link
Copy Markdown
Collaborator

Copied from #2849, which I accidentally merged and couldn't reopen even after I fixed main.


Still very rough, but has been a helpful exercise in working through some of the design due to the sheer amount of information and differing layout from other pages.

!https://github.com/user-attachments/assets/84cbb9f6-d22b-4e64-9b1d-e39d74d22902

Stubbing out based on oxidecomputer/omicron#7339.

Uses Tanstack Virtual. On testing with > 500 lines without virtualisation it starts to get a bit chunky especially if you're interacting with the page (e.g. opening the row).

Hoping that silo name and actor display name can be plumbed through so those are hard-coded for now.

Still needs:

  • Error state
  • Loading/placeholder state
  • Copy JSON to clipboard
  • Equivalent CLI/API command
  • Fix giant footer spacing
  • Arrow key selected item navigation
  • Hide overflowing columns
  • Improved focus visible look
  • Timestamp hover
  • Fix gradient on light mode
  • Syntax highlighting

@vercel

vercel Bot commented Jul 23, 2025

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
console Ready Ready Preview Jul 3, 2026 3:22pm

Request Review

@david-crespo

Copy link
Copy Markdown
Collaborator Author

Can't repro locally, but this happened twice in CI so it's probably not a fluke.

image

Comment thread app/pages/system/AuditLog.tsx Outdated
Comment thread app/pages/system/AuditLog.tsx Outdated

Copy link
Copy Markdown
Contributor

Yeah I tweaked to move the selected item popover over the header row. We lose the pt-3 since we want to match the height of the popover header and the header row. But I think it works better this way.

Copy link
Copy Markdown
Contributor

I've also removed the auto scroll on click, it's just if you use the arrow keys or the up/down within the popover. Less jumping around for the user.

@fakemonster fakemonster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

couple visual issues that don't necessarily fall on a single line:

  1. the non-datepicker popovers (docs info, refresh interval picker, range picker) fall behind the header because it's sticky. you can add a higher z-index to them and get away with it but it's pretty screwy. better to lower the sticky header to z-10, and then add an isolate class to the entire table to put it in a different stacking context (so it doesn't cover the header, which i assume is the original purpose of the z-20)
  2. this isn't exactly using a side modal, but z-index-wise, it needs some sort of... sidemodality. right now tooltips render behind the detail view

Comment thread app/pages/system/AuditLog.tsx Outdated
Comment thread app/hooks/use-window-size.ts Outdated
Comment thread app/ui/lib/DatePicker.tsx
className={cn(
state.isOpen && 'z-10 ring-2',
'text-sans-md border-default hover:border-raise bg-default relative flex h-11 items-center rounded-l-md rounded-r-md border focus-within:ring-2 focus:z-10',
'text-sans-md border-default hover:border-raise bg-default relative flex h-10 items-center rounded-l-md rounded-r-md border focus-within:ring-2 focus:z-10',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i'm starting to suspect there's a common visual component between ListBox and these pickers that could perhaps even do something a little more graceful with rounding. will think about this for a followup

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That'd be great, we're also repeating some of this in a few places that's more brittle than a shared class or component.

Comment thread app/pages/system/AuditLog.tsx Outdated
Comment thread app/pages/system/AuditLog.tsx Outdated
Comment thread app/pages/system/AuditLog.tsx Outdated
transform: `translateY(${start - scrollMargin}px)`,
}}
>
<div

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this really should be a button. i'd expect to hit space and not have my page scroll

Comment thread app/pages/system/AuditLog.tsx Outdated
<Badge color="neutral">{log.operationId.split('_').join(' ')}</Badge>
</div>
<div className={cn('text-secondary', hideActorId && 'hidden')}>
{userId ? (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

user and silo ids feel like kind of a dead end. is there anything i can actually do with them?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm lobbying to have these resolved in the API as silo and user names. Or an object that contains both.

>
<div className="bg-raise border-secondary flex h-10 items-center justify-between border-b px-2">
<div className="flex items-center">
<button

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it's a bit of a small nicety, but i like putting an autoFocus on the close button when there is one. webaim suggests this as a requirement for a11y, though that can get a little iffy when you're somewhere between a dialog and an "expanded view". my rule of thumb has generally been "if there's an X, just focus it". would be great to do the inverse too (refocus the source row on close)

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.

4 participants