Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DX-1307: New databrowser design #100

Closed
wants to merge 86 commits into from
Closed

DX-1307: New databrowser design #100

wants to merge 86 commits into from

Conversation

ytkimirti
Copy link
Contributor

No description provided.

Copy link

linear bot commented Oct 14, 2024

Copy link

vercel bot commented Oct 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 12, 2024 8:07am

query.refetch()
}, [query, resetCache])

const keys = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We assume key will be created, and use this to optimistically add the key, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there is no optimistic updates. The logic inside use-add-key is just to make the newly added key appear on top

const keys = useMemo(() => {
const keys = query.data?.pages.flatMap((page) => page.keys) ?? []

const keysSet = new Set(keys.map(([key, _]) => key))
Copy link
Contributor

Choose a reason for hiding this comment

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

can a key exist twice, why do we take the set here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because I modify the cache when adding a new key, to make it appear at the top. But if the user scrolls down and sees that key himself, there might be duplicates.
And I am not sure if redis can give duplicates if keys get deleted etc. while scanning but it's a general safeguard

Comment on lines +14 to +26
return useQuery({
queryKey: [FETCH_SIMPLE_KEY_QUERY_KEY, dataKey],
queryFn: async () => {
let result
if (type === "string") result = (await redis.get(dataKey)) as string | null
else if (type === "json") result = (await redis.json.get(dataKey)) as string | null
else throw new Error(`Invalid type when fetching simple key: ${type}`)

if (result === null) deleteKeyCache(dataKey)

return result
},
})
Copy link
Contributor

Choose a reason for hiding this comment

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

We should maybe invalidate this query with an interval

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was like that in the old version, i think it's a bit unexpected from some users so I removed all queries with intervals
https://linear.app/upstash/issue/DX-1303/data-browser-keeps-sending-requests-while-monitoring

@fahreddinozcan
Copy link
Contributor

image
This is a small design thing, but when you hover or click on a key, the background now includes the bottom between-border, bot not the one on the top. So it looks a bit bit bit weird when you zoom a bit

@fahreddinozcan
Copy link
Contributor

image
here, when you click on delete icon and then cancel the operation, it still directs you to edit. so it behaves as if you've clicked on the row. maybe we should stop propagation?

@fahreddinozcan
Copy link
Contributor

when the key is longer then the row width, we just cut the display sharply. we can maybe add gradient shadow etc. at the end of row, indicating the value is actually longer

image

@fahreddinozcan
Copy link
Contributor

image
I think we're missing refetch button somewhere around here to refetch all keys

@fahreddinozcan
Copy link
Contributor

image
Do we need this bottom padding after the infinite scroll? I think the last item should be closer to the bottom border

@fahreddinozcan
Copy link
Contributor

I can't seem to add/edit items in streams and there's no warning or info about it

@fahreddinozcan
Copy link
Contributor

image

@fahreddinozcan
Copy link
Contributor

image
list and stream icons are identical

@fahreddinozcan
Copy link
Contributor

When a structure is empty, we should show some information about this maybe

image

@fahreddinozcan
Copy link
Contributor

image
when structure is empty, size remains at loading state

@fahreddinozcan
Copy link
Contributor

Also, my keys keep disappearing. I create a hash, remove all entries. switch tabs or something and boom it's gone from sidebar list

@fahreddinozcan
Copy link
Contributor

image
wdyt about having a table with field and value tags here? might be confusing rn

@fahreddinozcan
Copy link
Contributor

When no selected keys, the right side remains at loading state

image

@ytkimirti
Copy link
Contributor Author

Also, my keys keep disappearing. I create a hash, remove all entries. switch tabs or something and boom it's gone from sidebar list

This is a bit more nuanced, to show the newly created key on the top, I modify the react query cache. But when you refresh the keys it does the usual ordering and the newly added key is pushed down to its normal place.

One fix would be to store newly created keys in the global store and always show them on top but I am not sure I we would want that

@ytkimirti
Copy link
Contributor Author

image Do we need this bottom padding after the infinite scroll? I think the last item should be closer to the bottom border

I added it to show the loading spinner, if I remove it the scroll would look glitched when showing and hiding spinner

@ytkimirti
Copy link
Contributor Author

Closed since we are moving databrowser to it's own repo

@ytkimirti ytkimirti closed this Nov 12, 2024
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.

3 participants