UI: Alloc FS: File Viewer #6048

DingoEatingFuzz · 2019-07-31T08:54:16Z

Completes the feature set of the allocation explorer by making files viewable. File viewing currently supports:

Text files (logs, json, code, etc.)
Images (jpg, png, svg, webp, gif, etc.)

If a file is not supported by the viewer, there will still be a link to open the API endpoint in a new tab for convenience. Depending on the file, the browser will either embed it (e.g., pdf, video) or download it for you (arbitrary binary).

A text file shown in the file cat view

Tailing a file in the file viewer

An embedded image from within an allocation

An unsupported file type

Based on the task-log component. Commonalities will be refactored.

The new streaming-file component takes an arbitrary logger component along with some mode flags and handles things like polling, DOM updates, and scroll position.

backspace

Nice thorough tests! I found some UX problems that I added GIFs for in various comments, though I didn’t dig in to know where the problems they indicate actually are. But this is looking good, close to ready, IMO.

backspace · 2019-08-01T19:58:39Z

ui/app/templates/components/image-file.hbs

+  <span class="image-file-caption-primary">
+    <strong data-test-file-name>{{fileName}}</strong>
+    {{#if (and width height)}}
+      <span data-test-file-stats>({{width}}px &times; {{height}}{{#if size}}px, {{format-bytes size}}{{/if}})</span>


The height px probably belongs outside the #if?

backspace · 2019-08-01T20:01:45Z

ui/app/styles/components/image-file.scss

+    display: block;
+    color: $grey-light;
+    font-style: italic;
+  }


I don’t see this style used anywhere, is it an orphan from a previous approach?

Indeed. Will delete.

ui/app/components/streaming-file.js

backspace · 2019-08-01T20:13:58Z

ui/app/components/task-file.js

+
+  isLarge: computed('stat.Size', function() {
+    return this.stat.Size > 50000;
+  }),


Not a big thing but could use gt?

backspace · 2019-08-01T20:15:36Z

ui/app/templates/components/task-file.hbs

+  {{else}}
+    <div data-test-unsupported-type class="empty-message is-hollow">
+      <h3 class="empty-message-headline">Unsupported File Type</h3>
+      <p class="empty-message-body message">The Nomad UI could not render this file, but you can still call view the file directly.</p>


Maybe call is a relic of a previous wording?

Um, how it got there is lost to time, but I'll remove it 🔪

ui/app/templates/components/task-file.hbs

backspace · 2019-08-01T21:11:21Z

ui/tests/unit/utils/stream-frames-test.js

+        offset: 4,
+        message: 'One fish, Two fish. Red fish, Blue fish.',
+      },
+    },


Nice unit test!

ui/tests/integration/image-file-test.js

backspace · 2019-08-02T13:01:37Z

ui/tests/unit/adapters/node-test.js

@@ -20,7 +20,6 @@ module('Unit | Adapter | Node', function(hooks) {
    this.server.create('allocation', { id: 'node-1-2', nodeId: 'node-1' });
    this.server.create('allocation', { id: 'node-2-1', nodeId: 'node-2' });
    this.server.create('allocation', { id: 'node-2-2', nodeId: 'node-2' });
-    this.server.logging = true;


I was wondering about this!

This deletion was special enough to get its own commit 🤓

It was already in the code (as evident by the diff) and I just happened to notice it spewing log lines in the test runner.

If you aren't familiar with this.server.logging = true, it logs every HTTP request that goes through Pretender. Great for debugging, super noisy though.

yeah, what do you think of this kind of solution that lets you add a query parameter to turn on logging? I like it more than having to edit code 😆

ooOOOoo, that's quite neat indeed. I like that. And nothing about the qunit runner removes that query param in an annoying manner?

I haven’t had any trouble with it so far! 🎉

+1 to doing this. We do this in consul lots, but we use document.cookie instead of query params. Using cookies means settings stick around when the URL changes, which is handy. Then we use Web Inspector to edit the cookies, which turns the Web Inspector into kind of a semi 'dev preferences' panel.

Actually, we didn't ever add a setting to turn HTTP logging on and off (we use it more to control mocks/doubles), but I think I'd add that also as we have some noisy logging going on aswell which I don't mind but other folks might.

backspace · 2019-08-02T14:42:51Z

oh yes, one other thing I meant to include, is that I think the spacing in the breadcrumbs looks a bit off when viewing a file vs a directory

I’m not sure what’s preferable: make the container the same height for both to avoid jumping when browsing from a directory into a file, or have the text move down when it’s a file…??

johncowen · 2019-08-02T14:51:50Z

Hey folks the other thing I've noticed here (thanks for the grabs @backspace !), there's a lot of testing for emojis going on here, which I believe are URL safe (yet multibyte).

Are there tests for paths/filenames with things like spaces and question marks, I can't see anything here? I also had a quick look in the other PRs related to this but I didn't see anything (but I might have missed it).

Is this something being tested for elsewhere?

John

…s a fallback

This was causing elements to flow off the page, since the element was assuming 100% but also had a 250px margin for the left column. This had previously been "fixed" by setting overflow-x: auto, but that resulted in tooltips from being clipped. This is a better solution to the same problem.

DingoEatingFuzz · 2019-08-08T22:46:44Z

Ooookay, all comments addressed!

Everything is in new commits, so that's probably the easiest way to re-review, but I'll go over some of the bigger things.

Errant No Connection error: This was due to the allocation's client not being loaded in time, which I fixed and added a test for.
File cat can go off screen: This was some CSS that was oddly related to the clipped tooltip overflow-x:auto bug from a couple months ago. I set the width while accounting for the margin and it's good now.
Breadcrumbs have awkward bottom-margin when viewing a file: This was due to the button height, which was no secret. I fixed it with an existing style that I forget existed: is-compact. This modifier also needed to be tweaked since this is the first time it's being used for multiple buttons in a bar.
Missing test coverage for tricky characters like "?": I added a new set of possible characters to the mirage factory. Everything is still working fine!

johncowen · 2019-08-09T13:25:34Z

Hey @DingoEatingFuzz ,

I gave the URL chars a try, some work, but I could definitely get things to go awry with %'s at least. I could reasonably consistently get a 'URI malformed' to appear.

Don't let this hold you up though, if you want to move on with this and look at that later.

John

backspace

Thanks for all the changes, my only remaining concerns relate to flapping tests; I’m not sure why the PR build has the failure in the “viewing a file” test and the push build doesn’t, but they both have log messages about “Decycle max depth reached at 6” and significant accompanying noise, maybe it’s related?

When running the tests locally, I occasionally get this failure (in addition to getting the “viewing a file” one once):

It’s rarely happening but still worrisome 😳

ETA: I keep running with the fs filter and I got this failure too:

ui/app/components/task-file.js

ui/app/templates/allocations/allocation/task/fs.hbs

ui/tests/acceptance/task-fs-test.js

DingoEatingFuzz · 2019-08-14T03:21:18Z

One last re-review? 🤞

I addressed the character encoding issue and fixed the flapping tests (which were due to illegal characters I added to further test the encoding issue).

backspace

Looking good, I think ui/out is a mistaken inclusion, but otherwise, congratulations on seeing this through!

backspace · 2019-08-14T11:37:12Z

ui/out

@@ -0,0 +1,1317 @@
+Environment: test


Looks like a stray inclusion of this file?

Lol, I can't win.

backspace

yay 🎉

github-actions · 2023-02-04T02:15:23Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

DingoEatingFuzz added 28 commits July 30, 2019 17:22

Move common stream frame decoding to a util

a21ae52

Use the stream decode util and never opt to use the plain query param

07f1c89

Tweak log window math

871fe10

New task-file component

a993a30

Based on the task-log component. Commonalities will be refactored.

Extract a streaming-file component from the task-log component

caf48ee

The new streaming-file component takes an arbitrary logger component along with some mode flags and handles things like polling, DOM updates, and scroll position.

image-file component for showing an image and image metadata

45c8f37

Styles for the image-file component

8966bc0

Address WindowResizable refactor

0698e11

Markup for the image-file component

6ec7faf

Refactored and image support of the task-file component

8403cba

Add a plainText mode

a609606

Custom Log instance to deal with API quirks

c335ace

Always escape < and > to avoid inadvertently rendering html

ccbe31f

Integrate the task-file component with the fs explorer pages

fea3731

Fix a bug where tail calls for files weren't getting the correct params

bd9d73b

Mirage factory for file system fixtures

c4dd61d

Use the alloc file factory for the fs stat and fs ls end points

97c4509

cat, stream, and readat mocks for alloc fs

33f1da9

Add hollow variation to empty-message

4b037bd

Add unsupported file type state

55349dc

Refactor existing fs tests to use new mirage factories

8f496e8

Integration tests for the image-file component

7294f38

Test coverage for streaming file component

6bd54f7

Test coverage for task-file component

c451615

Add file mocks to every mirage scenario

0471f28

fixup-integrate-file-component

a465046

Remove errant server logging line

be5c88f

Update factory-based fs tests to sort properly

e277cc5

DingoEatingFuzz added the theme/ui label Jul 31, 2019

DingoEatingFuzz mentioned this pull request Jul 31, 2019

UI: Allocation file system explorer #5871

Merged

4 tasks

backspace suggested changes Aug 2, 2019

View reviewed changes

DingoEatingFuzz added 9 commits August 7, 2019 15:08

Use a data-uri instead of an image for the image-file-test

e97c911

Minor fixes from code review

55039b6

Make a dedicated fs-breadcrumbs component

26e74fe

Add additional troublesome characters to the alloc-file name factory

55d8ff4

Include all client fs endpoints in the hosts block

186a620

Always preload the alloc node so the client can be dialed first

038fc27

Test that the client is contacted correctly, and the server is used a…

ed55a7b

…s a fallback

Prevent a change in height when switching from a dir to a file

a728ed1

DingoEatingFuzz requested a review from backspace August 8, 2019 22:46

backspace reviewed Aug 9, 2019

View reviewed changes

ui/app/components/task-file.js Show resolved Hide resolved

ui/app/templates/allocations/allocation/task/fs.hbs Show resolved Hide resolved

ui/tests/acceptance/task-fs-test.js Show resolved Hide resolved

backspace reviewed Aug 9, 2019

View reviewed changes

ui/tests/acceptance/task-fs-test.js Outdated Show resolved Hide resolved

backspace mentioned this pull request Aug 9, 2019

Add ability to control Mirage logging via query #6105

Merged

DingoEatingFuzz force-pushed the f-ui/alloc-fs-files branch from ba8305d to d36bdf3 Compare August 12, 2019 21:35

DingoEatingFuzz requested a review from backspace August 14, 2019 03:20

backspace reviewed Aug 14, 2019

View reviewed changes

Encode characters in file paths to ensure proper URIs

a321145

DingoEatingFuzz force-pushed the f-ui/alloc-fs-files branch from d36bdf3 to a321145 Compare August 14, 2019 19:54

DingoEatingFuzz requested a review from backspace August 19, 2019 17:31

backspace approved these changes Aug 19, 2019

View reviewed changes

DingoEatingFuzz merged commit 354da0f into f-ui/alloc-fs Aug 19, 2019

DingoEatingFuzz deleted the f-ui/alloc-fs-files branch August 19, 2019 17:36

github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Alloc FS: File Viewer #6048

UI: Alloc FS: File Viewer #6048

DingoEatingFuzz commented Jul 31, 2019

backspace left a comment •

edited

Loading

backspace Aug 1, 2019

DingoEatingFuzz Aug 5, 2019

backspace Aug 1, 2019

DingoEatingFuzz Aug 5, 2019

backspace Aug 1, 2019

backspace Aug 1, 2019

DingoEatingFuzz Aug 5, 2019

backspace Aug 1, 2019

backspace Aug 2, 2019

DingoEatingFuzz Aug 5, 2019

backspace Aug 6, 2019

DingoEatingFuzz Aug 6, 2019

backspace Aug 6, 2019

johncowen Aug 7, 2019 •

edited

Loading

backspace commented Aug 2, 2019

johncowen commented Aug 2, 2019

DingoEatingFuzz commented Aug 8, 2019

johncowen commented Aug 9, 2019

backspace left a comment •

edited

Loading

DingoEatingFuzz commented Aug 14, 2019

backspace left a comment

backspace Aug 14, 2019

DingoEatingFuzz Aug 14, 2019

backspace left a comment

github-actions bot commented Feb 4, 2023

UI: Alloc FS: File Viewer #6048

UI: Alloc FS: File Viewer #6048

Conversation

DingoEatingFuzz commented Jul 31, 2019

backspace left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncowen Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

backspace commented Aug 2, 2019

johncowen commented Aug 2, 2019

DingoEatingFuzz commented Aug 8, 2019

johncowen commented Aug 9, 2019

backspace left a comment • edited Loading

Choose a reason for hiding this comment

DingoEatingFuzz commented Aug 14, 2019

backspace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backspace left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2023

backspace left a comment •

edited

Loading

johncowen Aug 7, 2019 •

edited

Loading

backspace left a comment •

edited

Loading