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

Data source / vaults navigation #6745

Merged
merged 6 commits into from
Aug 13, 2024
Merged

Data source / vaults navigation #6745

merged 6 commits into from
Aug 13, 2024

Conversation

flvndvd
Copy link
Contributor

@flvndvd flvndvd commented Aug 12, 2024

Description

See figma here.

This PR introduces a new "Data Sources" tab in the menu, which becomes visible when the feature flag is enabled. This tab will be accessible to all users, with the content displayed according to their permissions. It also includes the implementation of side navigation, which is synchronized with the router to ensure the side navigation and the page on the right remain consistent. Currently, only the global and regular vaults will be displayed, with the system vault to be added in a subsequent PR. I also added skeleton pages so we can start parallelizing the work.

The navigation leverages the Layout Pattern from NextJS to avoid re-rendering the navigation every time. The breadcrumb (not yet implemented will also be included in this VaultLayout.

vaultNavigation

Risk

Low risk, everything is gated behind a FF.

Deploy Plan

@flvndvd flvndvd marked this pull request as ready for review August 12, 2024 16:26
<Item.List>
{vaults.map((vault) => (
<Fragment key={`vault-${vault.sId}`}>
<Item.SectionHeader
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 not have one section header per vault , there's one single private section with all private vaults inside
I also think the section headers order is defined by the ui (system, shared then private ), it should not rely on the api return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I was gonna change the API to return in the right order, but let me do it on the UI side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍
Still we have one section header per vault, instead of a single "private" section :
Screenshot 2024-08-12 at 22 00 44

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. Should be fixed.
image

@flvndvd flvndvd merged commit 9f9d889 into main Aug 13, 2024
3 checks passed
@flvndvd flvndvd deleted the flav/ds-nav branch August 13, 2024 07:25
@@ -0,0 +1,81 @@
import type { DataSourceViewType, UserType, VaultType } from "@dust-tt/types";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we stuck with [dsvId] out of consistency across the entire codebase 🙏

Same for the other endpoint?

Copy link
Contributor

Choose a reason for hiding this comment

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

In general we do [xId]/index.tsx I believe? but less important imho

albandum pushed a commit that referenced this pull request Aug 28, 2024
* Bump sparkle to 0.2.204

* Add feature flag + data sources tab

* Add data sources / vaults navigation

* Unfold ancestor items

* Address comments from review

* Address comments from review
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