fix(ui): Large arguments are downloaded as files instead of rendered#5268
fix(ui): Large arguments are downloaded as files instead of rendered#5268
Conversation
|
What is taking time here, rendering or making the request to download everything. If its rendering taking time maybe we can paginate it on the front-end so we only ever render a subset of the data |
The request to download is what takes the most amount of time.
The problem with rendering is that displaying huge amounts of information on the screen is not very useful. I think pagination also wouldn't be super useful, since users are often are looking for a very specific target in their list of targets - they wouldn't want to go through many pages until they find it. Being able to just download the whole contents in a file gives you the most amount of flexibility. |
e22354f to
209c16c
Compare
209c16c to
d585a5d
Compare
Okay then pagination would not help.
I mean as soon as we have the data and do pagination etc you could always support filtering, like a fuzzy search. Still IMHO we should still do pagination with filtering and if requests are too big we could only fetch the data when you click a button like you have here. |
jharvey10
left a comment
There was a problem hiding this comment.
Just a few optional suggestions and questions, but overall looks good to me!
| useEffect(() => { | ||
| let cancelled = false; | ||
|
|
||
| // Use setTimeout to defer blocking work to a macrotask, allowing React to | ||
| // commit and paint the loading state first | ||
| const timeoutId = setTimeout(() => { | ||
| if (cancelled) return; | ||
|
|
||
| try { | ||
| const result = alloyStringify(value); | ||
| if (!cancelled) { | ||
| setState({ status: 'ready', result }); | ||
| } | ||
| } catch { | ||
| if (!cancelled) { | ||
| setState({ status: 'ready', result: '[Error stringifying value]' }); | ||
| } | ||
| } | ||
| }, 0); |
There was a problem hiding this comment.
This will definitely work, but if you like, there's actually a react built-in that solves for this exact use case: useDeferredValue. (Also combined with useMemo for expensive value calculation caching). Its purpose is to treat a particular value as lower priority (in terms of rendering) to have react automatically process it in the background. So here, you'd instead have something like:
const AsyncLargeValue = ({ value }: { value: Value }) => {
const deferredValue = useDeferredValue(value);
const isPending = value !== deferredValue;
const result = useMemo(() => {
try {
return alloyStringify(deferredValue);
} catch {
return '[Error stringifying value]';
}
}, [deferredValue]);
return <DownloadButton result={result} disabled={isPending} />;
};There was a problem hiding this comment.
I'm going to give up on implementing this change because Cursor disagrees with it, and after researching it for a bit I'm still not sure how exactly to make it better. I could probably iterate on it a bit and test it, but if it ultimately looks the same to the end user there's probably not enough benefit to change it.
There was a problem hiding this comment.
Interesting! I guess there's some subtleties with useDeferredValue I don't know about. Practically speaking, when I've used it the UI tends to "feel" more responsive. The non-deferred render pieces happen immediately, which makes the UI "feel" snappy, and then when things are idle, the deferred value recomputes and is typically "ready" by the time it's actually needed (or an intermediate fallback state is put in to bridge the gap).
The point about the equality check is a fair one though. That's probably just my pseudo logic breaking down from not properly thinking through the initial render case.
Either way, I don't think there's anything wrong with your approach!
|
@kalleep Agree that there could be better on-demand requesting, and probably a revamp of the entire UX of this page is prudent. I think there's some pre-existing page load logic that makes doing smarter loading at the moment a bit more involved of a change, but I'm certain that we can refactor those parts as well to make even more improvements. |
3123228 to
1496348
Compare
…hem in the browser
1496348 to
ee2b326
Compare
Pull Request Details
The UI is not really usable when large amounts of text are displayed. An earlier changed mitigated this partially by disabling json highlighting, but in practice if a blob of text is large enough it'd still cause the UI to freeze. It's also hard to copy paste it and it's not possible to view it in full.
This PR will change the component-specific UI pages to display a download link for arguments whose values contain more than 50 000 characters.
Screen.Recording.2026-01-15.at.11.47.45.mov
Issue(s) fixed by this Pull Request
Fixes #1121
Related to #515
Notes to the reviewer
TBH there is a minor issue with the button. It goes blue before it's actually clickable 😅 I couldn't fix that, but anyway it's a big improvement over the current behaviour.
I know very little about UI development, so I had to rely on Cursor to generate the code for me. I hope it didn't do something silly. On a high level it make sense to me.
PR Checklist