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

Refactor type/expandable/preventSelection computation #10051

Open
philipperolet opened this issue Jan 17, 2025 · 10 comments
Open

Refactor type/expandable/preventSelection computation #10051

philipperolet opened this issue Jan 17, 2025 · 10 comments
Assignees

Comments

@philipperolet
Copy link
Contributor

See discussion in #10043

First step is design: can we make it so those are not computed bluntly from mimetype in a principled manner?

@aubin-tchoi
Copy link
Contributor

I'm ok with moving forward with using has_children as you suggested and logging the diffs with the current behavior (knowing that we are probably fine with a certain amount of approximation, the current behavior is somewhat over-engineered)

@aubin-tchoi
Copy link
Contributor

seems like we have to add the mime type to ContentNode to expose it to the front-end

@aubin-tchoi
Copy link
Contributor

I'm looking at getVisualForContentNode here, if we have the mime type we can rewrite the function to rely on the mime type and once tested and shipped we could start squeezing out the ContentNodeType as we know it
all of this works on the assumption that this switch is easy to test (by displaying the two icons side to side in dev for instance), we can't add a log there

@aubin-tchoi
Copy link
Contributor

  • doesn't make much sense to add the mime types in connectors's /content-nodes for shadowing

@aubin-tchoi
Copy link
Contributor

aubin-tchoi commented Jan 17, 2025

Listing out where the type is used:

  • In ContentNodeTree, AssistantActionsSection, DataSourceSelectionSection and TrackerDataSourceSelectedTree to display the DataSourceViewDocumentModal on click only for files.
  • In DataSourceViewPermissionTree to override the preventSelection property and prevent selection of things other than databases when viewType === "tables".
  • In getTableIdForContentNode: related to TableQuery configuration.
  • In DocumentOrTableDeleteDialog to choose the appropriate word.

I may have missed a few locations but overall choosing correctly between database and the other ones seems quite important, and seems hard to do without relying on the mime type.

@philipperolet
Copy link
Contributor Author

What about doing Node table = ContentNode database, then adapting using mimetype where necessary? IMO to sort it out we should clarify semantics. What does it mean that a node is of type table => it means it have an entry in tables and is table-queryable. It should be the same for content node database. In cases in which we use the database type for something else, then we should not use the type at all.

doesn't make much sense to add the mime types in connectors's /content-nodes for shadowing

yeah no need to add it (just ignore in the diff)

@aubin-tchoi
Copy link
Contributor

I think that'd be best indeed, it's much easier to understand and better rationalized.
There are a few occurrences where we use the "database" type for stuff that are not actually table queryable (GitHub issues, Intercom Help Centers), will track down more precisely why we do that, if it's only for choosing an icon i'll just change it at the location where we choose the icon

@aubin-tchoi
Copy link
Contributor

thanks!

@philipperolet
Copy link
Contributor Author

@philipperolet
Copy link
Contributor Author

PreventSelection + expandable have been taken care of

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

No branches or pull requests

2 participants