Implement upload support

[jelly]

This implements #378

• Wait for cockpit release with send-acks support and refreshed images for creating tests
• Implement conflict dialog mockup design from @garrett
• Pull cockpit-lib-update for cockpit.format_bytes typescript shenanigans
• Design tweaks
• Write tests
  • Test upload success
    • Test upload cancel
    • Test upload permission issues
    • Test upload disk full
    • Test multi upload
    • Test progress popover
    • Test file conflict
      • Skip
        • Replace
        • Cancel
        • Skip all
        • Replace all

---

Conflict handling

[jelly]

@garrett for debugging the popover I created this patch which mimmicks an upload scenario:

0001-WiP-debug-popover.txt

[jelly]

Suggestion from @mvollmer:

await Dialogs.show() and let Dialogs.close(arg) take an argument to resolve the promise.

[garrett]

I have an error trying to build this PR:

✘ [ERROR] Could not resolve "cockpit-upload-helper"

    src/upload-button.tsx:37:23:
      37 │ import { upload } from "cockpit-upload-helper";
         ╵                        ~~~~~~~~~~~~~~~~~~~~~~~

  You can mark the path "cockpit-upload-helper" as external to exclude
  it from the bundle, which will remove this error and leave the
  unresolved path in the bundle.

I'd ping you on Matrix, but it seems down at the moment.

[garrett]

Replace dialog.

Single file:

When there are multiple files:

Notes:

1. Title should wrap if it's too long
2. Changes are listed side by side, using the width of the modal
3. (Not shown) ARIA would have labels "Size" and "Modified" for the respective values for new and original files for screenreader support
4. Replace is a warning button
5. Skip is now "Keep original" to make it more explicit and tie in with the "original" language above
6. New and original and replace and keep original are the consistently the same from left to right (so actions mirror details and vice versa)
7. Date-Time is in the locale setting
8. Filename is in the title and is slightly bolder
9. The "also" action of "Apply this action to all conflicting files" specifically says conflicting files instead of just "all files", as it wouldn't replace files that don't conflict.
10. There's a little more spacing between the "Apply this action..." opt-in and the details above, as the description and details are considered one section and the actions are another (so there's a little more space to add "breathing room" to separate the sections)
11. Cancel is a link styled button (as it should be)

[garrett]

Perhaps we should include the filename in the description like this, complete with monospace ("it's from a computer" <samp>) styling?

applied

[allisonkarlitskaya]

Random UI idea: since we compute the "will replace" check based on local information, and since we do it in one go before starting any uploads, I wonder if we'd better show a single dialog with the names of all files that would be replaced, instead of an endless series of "and this?" "and this?" "and this?"

[allisonkarlitskaya]

I feel like that could be restructured like

if (resolution && applyToAll) {
  if (resolution.skip)
    continue;
  push();
} else {
  ... ask ...
}

or so?

[allisonkarlitskaya]

The way fsinfo works is that you get a dictionary mapping file names to their attributes. We should keep that data around so that we can just do a dictionary lookup instead of this quadratic looping business...

[jelly]

Agreed!

[jelly]

As this requires re-architecting the whole state (which I did in a separate PR, let's do it there). This at the moment has more prio.

[garrett]

Two things:

• Adjust the conflict dialog to match the mockups
• Set a width limiter on the modal and make sure it wraps as expected (we also have mockups)

Otherwise, the functionality looked correct for this first-pass PR when I checked yesterday.

Thanks! Please ping me again when the two things have been implemented. Looking forward to landing this soon; we're getting close!

[garrett]

I'm working on the sizing of the popover. It's not as straightforward as one might think. (But you know that. 😉 Just confirming what you said in Matrix.)

[martinpitt]

Thanks! Great to see this unblocked 👏 I found a bunch of minor issues.

[martinpitt]

Completely optional: can be shortened to handleClick = ref.current?.click and thus the helper function eliminated altogether.

[jelly]

And this is now racy... So maybe we should pass files again.

[jelly]

Rawhide builds are broken due to https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/J3HMPMH3THO34J2WK4XEAVIY6VPMMK3C/

[allisonkarlitskaya]

This is starting to look really great now, thanks!

Please see one comment from me. It's possible that I just don't understand this code properly and an explanation is all that's required.

[allisonkarlitskaya]

LGTM! 🎉

[martinpitt]

Thanks! Some new/remaining issues, and my previous review still has a few outstanding issues. All of them except the handleClick() elimination are a blocker for me.

[jelly]

Thanks! Some new/remaining issues, and my previous review still has a few outstanding issues. All of them except the handleClick() elimination are a blocker for me.

Ah, I fixed those but didn't mark as resolve as you might have expected?

[martinpitt]

Right, thanks for your latest push -- that resolves both of my previous reviews, I closed the threads.

Good enough for me -- if you need to push again (like, for Garrett's review or so?), please strengthen the test as below, but not important.

Cheers! 🎉

[cockpituous]

These 2 added lines are not executed by any test.

[cockpituous]

This added line is not executed by any test.