feat(ui): Saved Views #3366
feat(ui): Saved Views #3366
Conversation
jamie-rasmussen
force-pushed
the
jamie/hackweek-saved-views
branch
from
6aa510d to
2d95ee7
Compare
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=50f25014dffa9e45d2e97986f323dcdd209008a6 |
jamie-rasmussen
force-pushed
the
jamie/hackweek-saved-views
branch
from
2d95ee7 to
52ee76a
Compare
jamie-rasmussen
force-pushed
the
jamie/hackweek-saved-views
branch
3 times, most recently
from
1174218 to
7176507
Compare
jamie-rasmussen
force-pushed
the
jamie/hackweek-saved-views
branch
6 times, most recently
from
f5192c2 to
49e0fb9
Compare
|
Starting to dive into review here. Noticing a bug: save a view with a sort applied, change views, change back to the view with the saved sort - it is applied briefly, then resets and drops the sort |
|
Also, not blocking, but for me the interactions are very slow. |
| ]; | ||
|
|
||
| // Value of the object | ||
| type ViewDefinition = Record<string, any>; |
There was a problem hiding this comment.
View definition is untyped?
| } | ||
| const className = 'SavedView'; | ||
| tsClient | ||
| .objCreate({ |
There was a problem hiding this comment.
As implemented, because SavedView is not defined on the server, this has no validation. This makes me a bit nervous that this critical data structure is enforced within this component. If you defined a builtin object, you could use useCreateBuiltinObjectInstance which would allow you to simply pass the relevant data:
project_id: string;
object_id: string;
val: T;
builtin_object_class?: string;
And have val be validated in typescript and enforced on the backend. This feels like a safer approach
| tab: string; | ||
| }; | ||
|
|
||
| const CallsPageLastView = ({entity, project, tab}: CallsPageLastViewProps) => { |
There was a problem hiding this comment.
I would call this RedirectToLastView to emphasize that it is a redirect
| const tsClient = getTsClient(); | ||
| const fetchViews = useCallback(() => { | ||
| tsClient | ||
| .objsQuery({ |
There was a problem hiding this comment.
Similarly, this could be done with useBaseObjectInstances
| return <Loading />; | ||
| } | ||
| if (!projectInfo) { | ||
| return <Alert severity="error">Invalid project: {project}</Alert>; |
There was a problem hiding this comment.
Here and elsewhere, it seems like we could have a nicer landing 404 / landing page when something is not found
|
|
||
| newQuery.set('view', viewToLoad.object_id); | ||
| history.push({search: newQuery.toString()}); | ||
| props.onRecordLastView(viewToLoad.object_id); |
There was a problem hiding this comment.
will this line run in an unmounted? I am not sure if the execution order ensures this runs before the URL changes
| .then(res => { | ||
| const newQuery = new URLSearchParams(history.location.search); | ||
| newQuery.set('view', objectId); | ||
| history.push({search: newQuery.toString()}); |
There was a problem hiding this comment.
same here regarding history
| return 'Evaluations'; | ||
| const currentViewDefinition = useCurrentViewDefinition(baseView); | ||
|
|
||
| const history = useHistory(); |
There was a problem hiding this comment.
it is always a bit of a smell to me when a component uses history below the main routing component (in our case Browse3). It means that CallsPage itself is now strictly dependent on the URL and cannot be embedded elsewhere. This particular case seems challenging to separate, so maybe we can punt on this comment. However, we should acknowledge the impact of this change
| return {}; | ||
| } | ||
| }, [query.filter, entity, project, tab]); | ||
| type CallsPageLastViewProps = { |
There was a problem hiding this comment.
I would consider moving these helper components / hooks. It seems like we are putting a lot of logic in Browse3 which is highly calls-page specific
| const {search} = history.location; | ||
| const newQuery = new URLSearchParams(search); | ||
| newQuery.set('filter', JSON.stringify(filter)); | ||
| history.push({search: newQuery.toString()}); |
There was a problem hiding this comment.
I noticed in various places in this PR (and the compare anything project), that we are often doing history.push... with some locally constructed information. The impact of this is that the URL contract is no longer tightly defined, but rather implicitly defined throughout the various component implementations. The idea of weave-js/src/components/PagePanelComponents/Home/Browse3/context.tsx is to centralize all path construction such that we have a single interface definition for construction of URLs. This unification also allows the same URL to work in peek drawers as well as main views. Specifically, something like history.push(routerContext.callsUIUrl(entity, project, filter)); is "smart" enough to perform the appropriate URL update regardless of where it is rendered. It would be nice to keep all URL construction co-located for easier maintenance
jamie-rasmussen
force-pushed
the
jamie/hackweek-saved-views
branch
from
49e0fb9 to
2de401e
Compare
Description
Internal Jira: https://wandb.atlassian.net/browse/WB-21701
Adds "Saved Views" to the UI. Views are saved as Weave objects. Table settings on the Traces and Evaluations pages like filters, sorting, page size, column visibility, and column pinning are stored in the view object.
New views can be saved, views can be updated, renamed, and deleted.
Some screenshots: