feat(component-library, insights): added a simple datetime picker and historical fetching to the pyth feeds demo, with URL deeplinking #3351
Conversation
… historical fetching to the pyth feeds demo, with URL deeplinking
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
cprussin
left a comment
cprussin
left a comment
There was a problem hiding this comment.
Overall looks great! Few interesting discussion points but nothing that needs to change. Very nice!
| params: { symbol }, | ||
| searchParams: { datasources, startAt }, | ||
| }, | ||
| } = validatedParams; |
There was a problem hiding this comment.
total nit and not something to change but ngl I am really not enjoying reading the early return pattern any more having given it more time, I really find this to obscure the control flow of the code and really dislike it
There was a problem hiding this comment.
Perhaps we should discuss more as a team? The last thing I want is for exhaustion or frustration from anyone on our team when reviewing PRs.
Disclaimer: you've seen my preference for control flow. I find early returns or guard statements to reduce cognitive complexity. I first started using this style a few years ago when I started Golang dev at Netflix, since that is the preferred style in that language (based on the constructs it has available), and while it took some time to get used to, I now find it easier to wrap my head around.
SonarSource released a research paper a few years ago that declared metrics for Cognitive Complexity (they attempted to throw out the Cyclomatic Complexity from the 70s, since that was crafted for the FORTRAN and mainframe days), and the paper penalizes heavy use of statements that break from linear control flow.
The paper's a bit dry, so probably worth an AI summary 😂 : https://www.sonarsource.com/docs/CognitiveComplexity.pdf
Happy to chat with you and the gang more, and I apologize if this style has mentally fatigued you in any way while reviewing (though I appreciate you soldiering forward, regardless)
| <Suspense | ||
| fallback={ | ||
| <div className={classes.suspenseLoader}> | ||
| <div>{suspenseLoaderLabel}</div> | ||
| <Spinner isIndeterminate label={suspenseLoaderLabel} /> | ||
| </div> | ||
| } | ||
| > |
There was a problem hiding this comment.
Nothing that needs to change (I see you're using suspense because you're using query params in PythProAppStateProvider, but more an interesting discussion point that I'd like to discuss with you and get your thoughts on:
Personally I've found suspense to be really clunky and I just don't like the devex of using it:
- It's implicit, not explicit. The loading state does not happen where the data is loaded, so the code is not obvious, and the fact that there are suspense boundaries or understanding where the boundaries are is not easily traceable from the types or the structure of the code. Knowing how to follow the code could require jumping up many layers in the component tree.
- It doesn't use the type system for exhaustive state management. In particular, it handles loading promises, but does nothing for errored promises, requiring a separate error boundary (which is IMO a terrible devex). The reality is that anything that can suspend can also fail and not requiring devs to explicitly consider failure is a huge failing.
- It makes it harder to build skeleton loader states where the skeleton follows the structure of the loaded page, because you end up with a ton of suspense boundaries if you want to do this well and the code gets clunky and obtuse very fast.
I pretty much only use suspense nowadays when it's absolutely required, which is usually one of two cases for SSR:
- I'm trying to leverage nextjs app router states which require use of suspense
- I'm using a hook that React requires being wrapped in a suspense boundary
Instead, I've found it very pleasant to work with hooks that return data in the way that useData returns -- essentially returning a discriminated union. The app can then switch on state in a way that's explicit and typescript forces devs to acknowledge and handle errors. Additionally because the loading state happens alongside the data that's being loaded, it's very easy to trace and follow the data flow.
apps/insights/src/components/PythProDemoPriceChart/index.module.scss
Outdated
Show resolved
Hide resolved
| const searchParams = useSearchParams(); | ||
|
|
||
| /** local variables */ | ||
| const selectedReplayDate = searchParams.get(QUERY_PARAM_START_AT) ?? ""; |
There was a problem hiding this comment.
fyi that we use nuqs elsewhere for dealing with query params that are handled client-side and I've found it to be quite nice
| // data to be munged with the new data | ||
| ...initialState, | ||
| }); | ||
| let query = updateQueryString({ |
There was a problem hiding this comment.
Do we actually have to code this to use a let binding and update the query string twice in the case that !isReplaySymbol(source)?
| box-shadow: | ||
| 0 4px 6px -4px rgb(from black r g b / 10%), | ||
| 0 10px 15px -3px rgb(from black r g b / 10%); |
There was a problem hiding this comment.
This should probably live in theme.elevations right?
There was a problem hiding this comment.
replaced with @include theme.elevation("default", 1);
| const handleHidePicker = useCallback(() => { | ||
| isClosingRef.current = true; | ||
| setPopoverOpen(false); | ||
| setTimeout(() => { | ||
| isClosingRef.current = false; | ||
| }, 200); | ||
| }, []); |
There was a problem hiding this comment.
Would it be cleaner to use famer-motion to handle the close animations?
(I've been going back and forth on this and there's a lot I don't like about motion, I have strongly considered reverting back from <AnimatePresence> to the old-fashioned timeout syncing, so I'm genuinely looking for your opinion here, not making a suggestion)
…onent and used it for the feeds demo to power a mobile and desktop menu
… occur when a chart is destroyed while a pending write is still incoming
…eaks feat(insights hub): tweaked the layout and design to give more room to the chart and be better on mobile
Resolves this Linear ticket
https://linear.app/douro-labs/issue/UI-400/insights-hub-ui-integrate-with-clickhouse-for-grabbing-t-7-data-for
Summary
duckdband are now querying the ClickhouseDB, directly, for historical data. You can select any day, but any day or time combo might not have dataRationale
The previous pyth feeds demo only featured a single, limited day and time window for the historical replay symbols, which didn't make for a very compelling sales pitch.
How has this been tested?
In Action
CleanShot.2026-01-08.at.12.14.53.mp4