quiltdata / quilt

Quilt is a data mesh for connecting people with actionable data
https://quiltdata.com
Apache License 2.0
1.33k stars 90 forks source link

Athena: refactoring and minor UI/UX improvements (alphabetize lists, better loading/error visuals) #4208

Closed fiskus closed 1 week ago

fiskus commented 2 weeks ago

The main ideas for refactoring are

  1. replacing

    const data = useData()
    return <>{data.case{Ok: () => <Element />, Err: () => <Alert /> }}</>

    with

    const data = useData()
    if (isError(data)) return <Alert />
    if (isOk(data)) return <Element />
  2. use state Provider instead of passing data as props


Besides refactoring, there are numerous changes and improvements:


I suggest to start reading the code from Athena.tsx:

  <Model.Provider preferences={ui.athena}>
    <AthenaContainer />
  </Model.Provider>

and then into Model.Providerapp/containers/Buckets/Athena/model/state.tsx


image

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 68.47091% with 233 lines in your changes missing coverage. Please review.

Project coverage is 39.02%. Comparing base (5e7ce5d) to head (eabaec7). Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...og/app/containers/Bucket/Queries/Athena/Athena.tsx 0.00% 66 Missing and 6 partials :warning:
...containers/Bucket/Queries/Athena/model/requests.ts 87.40% 51 Missing :warning:
...g/app/containers/Bucket/Queries/Athena/History.tsx 0.00% 45 Missing and 2 partials :warning:
...p/containers/Bucket/Queries/Athena/QueryEditor.tsx 0.00% 21 Missing and 5 partials :warning:
...pp/containers/Bucket/Queries/Athena/Workgroups.tsx 0.00% 15 Missing and 3 partials :warning:
.../app/containers/Bucket/Queries/Athena/Database.tsx 82.05% 7 Missing :warning:
...pp/containers/Bucket/Queries/Athena/Components.tsx 0.00% 3 Missing :warning:
...pp/containers/Bucket/Queries/Athena/model/utils.ts 90.32% 3 Missing :warning:
...p/containers/Bucket/Queries/Athena/model/state.tsx 94.73% 2 Missing :warning:
...containers/Bucket/Queries/Athena/CreatePackage.tsx 0.00% 1 Missing :warning:
... and 3 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4208 +/- ## ========================================== + Coverage 37.68% 39.02% +1.33% ========================================== Files 768 772 +4 Lines 35321 35540 +219 Branches 5021 5311 +290 ========================================== + Hits 13312 13870 +558 + Misses 21402 20444 -958 - Partials 607 1226 +619 ``` | [Flag](https://app.codecov.io/gh/quiltdata/quilt/pull/4208/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | Coverage Δ | | |---|---|---| | [api-python](https://app.codecov.io/gh/quiltdata/quilt/pull/4208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | `91.09% <ø> (ø)` | | | [catalog](https://app.codecov.io/gh/quiltdata/quilt/pull/4208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | `14.30% <68.47%> (+2.22%)` | :arrow_up: | | [lambda](https://app.codecov.io/gh/quiltdata/quilt/pull/4208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | `87.95% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fiskus commented 2 weeks ago

1.blocking pop-ups on errors encountered while trying to run a query (try "list tables" or some other invalid sql) feel quite odd. i'd much prefer a persistent error message that doesn't block the ui and doesn't disappear

I know how to fix this. And it, probably, even makes the state/data more consistent. Added TODO notes, and going to implement this, if I have time.

  1. getting an error (can't access property 0, rows[0] is undefined)

Thanks! I thought, I fixed that. Fixed now

  1. "load more" button...loads the same data again

Fixed

  1. workgroup looks a off ... use a stock control

Done

  1. "query executions" heading seems too small (especially considering that it gets larger when navigated to)

That's intentional, but probably overthought. Index page has the main element "Query body" and this header is the biggest, other headers are equal sized, including "Query executions". The results page has two main elements, "Query body" and results with breadcrumbs (including "Query executions"). I think, I'm going to do 2 things:

  1. make all headers equal
  2. make "headers" for selects as part of Select, as for Catalog/Database, as you proposed

Done

  1. it feels somewhat confusing that in query executions table only the completion date is clickable (quite easy to miss imo)

Noted and understood. But, I think, it is more or less familiar pattern from social media. I'll make the whole row clickable, if I have time.

  1. query execution ui jumps when expanded / collapsed

Fixed

  1. ux suggestion: a button to insert the query into the editor, not just copy it

I'm not sure about this. The copy and paste is, probably, the main use case, so it can be replaced by insertion. But I don't know what icon I can use ("edit" icon, I guess). And it is somewhat off, that I click here, but UI is changed in a different place. Also, user can go to execution query (click on "Date completed" link), and edit it there. I need to experiment with this.

  1. layout jumps when switching to / from individual executions

Fixed

nl0 commented 2 weeks ago

Also, user can go to execution query (click on "Date completed" link), and edit it there. I need to experiment with this.

probably that's good enough (i only noticed that "feature" after posting the review)

fiskus commented 2 weeks ago

Persistent query error - done.

The whole execution row clickable - done.

fiskus commented 2 weeks ago

Re-tested manually, and wrote some more tests.

fiskus commented 1 week ago

Fixed two bugs:

  1. when there are no results (but there are no errors either), I showed only one row with the current execution. Now I show the message saying there are no results.
  2. the execution contains the lowercased catalog and database name, so I'm comparing lowercased values now

Also, added a couple of tests more