Arvados

(This would have been more helpful to say earlier, but...)

Space is already tight in the top title/button bar where the "cancel" button and "running" badge are. I can see this getting awkward when the (entirely user-defined) service label gets longer.

Could this go in its own tab? It wouldn't even need to be a dropdown, we could just show the entire list. (Admittedly there are already lots of tabs.)

Could it go in the Overview tab? Even here it might not need to be a dropdown.

The UI is good, thanks. I happened to notice one bug that would be good to fix if possible:

1. I have All Processes open in a tab.
2. I submit a service container request via CLI.
3. In my open tab, I see the new process and open it.
4. When I open the process page, the "Connect to service" button is available (good).
5. But if I just sit on that process page, after about 5-10 seconds the button disappears (bad). If I refresh the page it reappears and stays there. I'm guessing there's some state transition early in the process lifecycle that clears the button that shouldn't.

I noticed this with both single-port and multi-port containers.

Changes at arvados|cfcd70a4b097bf258553c3c45ffe0328baf3bf65 branch 23026-container-services-ui
Tests developer-run-tests-services-workbench2: #1614

• All agreed upon points are implemented / addressed.
  • Added service button which shows whenever status=running and published_ports.length
  • Styled buttons to always be in a 2-row format, with the stop/status stretching to match the service button width
  • Added tooltip to single service button for long names
  • Added error toasts for when token is missing (for private services) or empty/malformed URL
  • Added published_ports field to containerRequestFieldsNoMounts, fixes the bug Brett noticed due to some requests excluding published_ports causing it to disappear
  • Added unit tests for the URL token injection - I could not find a case where the URL constructor does not return a search+hash that does not match the end of the href, but that case is checked just to be safe
  • Added component test for single/multi service with public/private url that verifies correct URL opened
  • Added integration test to test basic service button and URL opened (I only did a basic test for the integration since the component / unit tests already cover all the edge cases)
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • Follow up ticket #23161 to add confirmation dialog to cancel button to prevent misclicks
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • Passes all tests, added fairly comprehensive new tests
• Documentation has been updated.
  • n/a
• Behaves appropriately at the intended scale (describe intended scale).
  • Intended scale to be ~10ish services
• Considered backwards and forwards compatibility issues between client and server.
  • There is only 1 concern, if WB is updated before the server knows about published_ports, then processes won't load. This is probably unlikely to happen since I assume the only time anyone updates WB is when they're updating arvados.
• Follows our coding standards and GUI style guidelines.
  • yes

• On my screen, the ServiceMenu button cuts off with ellipses for no real reason (as in, it's not saving a significant amount of space and looks off). Removing the flexGrow, flexbasis, and minWidth properties on the buttonContainer frees this up and looks better in my opinion. The button will still overflow to ellipses when occluded by the toolbar, so I can't see a reason not to.
• To me, it looks better if the ServiceMenu button is below the Run/Cancel button and status badge rather than above (something about the order of the colors I think), but I don't feel too strongly about it either way.
• I'm also not a fan of the ServiceMenu with multiple ports looking like an HTML <select> but behaving like a button with a popover. Perhaps setting the width of the popover to the same as the width of the button and word-wrapping the list items? Or changing it to an MUI Select (https://mui.com/material-ui/react-select/#placeholder) maybe? Again, I don't feel too strongly about this.

• injectTokenParam() is very generic and might be useful elsewhere, so it should probably be in its own file in /common
• in process.cy.js L465, Cypress best practices recommends using data-* for test selectors, because sometimes UI libraries (like MUI) inject their own id prop.

Changes at arvados|fd4d2f302031a433d9b2c99b545dc549fea1ec96
Tests developer-run-tests-services-workbench2: #1622

On my screen, the ServiceMenu button cuts off with ellipses for no real reason

So I actually spent several days trying to get this to work nicely - with the suggested changes, the service button does expand to take up extra space, but that happens at the expense of space for the title which is noticeable when the title is long. It seems flexbox is unable to operate in a mode where the title is 100% prioritized before giving extra space to the button cluster, since flexbox allocates space in a proportional mode. So when the process title and service name are both long, space is split proportionally between them causing the service button to get a lot of space and causing the title to wrap.

Ideally, the title would be given as much space as possible minus the minimum space required for the buttons (their min-width) and then the service button expanded as needed, but I don't think it's possible to do that without JS. I had attempted to make this happen by making the flex-basis of the title to be content and the flex-basis of the button to be it's min-width, but flex-basis doesn't seem to work as expected for content that wraps (I think the basis ends up being the wrapped content). And then there's the problem that you must pick a static proportion for allocating extra space or shrinking the basis if the title is too big. Removing flex-grow and flex-shrink really just defaults it 0 and 1, so the areas would be equally shrunk as necessary instead of shrinking the button cluster before the title so you end up with a wrapped title and the button cluster awkwardly taking up half the space.

(basically this can happen and because the allocation is proportional to flex-basis, there isn't a good way to prevent it)

I would be open to a solution but I don't think it's easy to make it behave nicely with longer process titles, and I think that may be a common case so it seems safer to just stick to a static button cluster width.

To me, it looks better if the ServiceMenu button is below the Run/Cancel button

Done

I'm also not a fan of the ServiceMenu with multiple ports looking like an HTML <select>

I was actually modeling it after the Menu component (https://mui.com/material-ui/react-menu/#customization). I would push back against using Select since it's unexpected for a select input to perform an action when something is selected, and I think the popover menu being a different width is fine since it's anchored on one corner and that's the normal behavior in the MUI example

injectTokenParam() is very generic and might be useful elsewhere

Moved to common/url

in process.cy.js L465

Fixed, I kept the ID since it's used for aria-labeledby but the test uses data-cy

LGTM