chore: editor sidebar tree view #33250
Conversation
| </DropdownMenu> | ||
| )} | ||
| {itemSideAction && | ||
| itemSideAction(item) !== undefined && |
There was a problem hiding this comment.
Not sure about the best pattern here without blowing up any defaults but I needed something where I could override sideaction through icon
| @@ -293,7 +293,7 @@ const LemonTreeNode = forwardRef<HTMLDivElement, LemonTreeNodeProps>( | |||
| <div className={cn('flex flex-col gap-y-px list-none m-0 p-0 h-full w-full', className)}> | |||
There was a problem hiding this comment.
@adamleithp @mariusandra some slight changes here to core tree component. No strong opinions here/happy to take suggestions if there's a built in way to do what I'm doing here. The plus button on sources needs to be a direct link and not trigger a dropdown
There was a problem hiding this comment.
I'll have a look tonight!
📸 UI snapshots have been updated7 snapshot changes in total. 0 added, 7 modified, 0 deleted:
Triggered by this commit. |
… into editor-sidebar-tree-view
There was a problem hiding this comment.
PR Summary
Implements a new tree view UI for the SQL editor sidebar with client-side search functionality and improved database table organization.
- Upgraded LemonTree component in
frontend/src/lib/lemon-ui/LemonTree/LemonTree.tsxwith consolidated action button prop and improved folder detection logic - Added fuzzy search with Fuse.js in new
queryDatabaseLogic.tsx, replacing old schema management implementation - Set fixed height (39px) in
frontend/src/scenes/data-warehouse/editor/EditorScene.scssto match tree view nav bar - Cleaned up button styling by removing unnecessary transition classes and improving type safety
- TODO items remain: feature flag implementation and API setup for filesystem
11 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -1,6 +1,8 @@ | |||
| .EditorScene { | |||
| .LemonTabs__bar { | |||
| margin-bottom: 1px; | |||
| // height is hardcoded to match implicit height from tree view nav bar˝ | |||
There was a problem hiding this comment.
syntax: There's a rogue unicode character '˝' at the end of this comment line that should be removed
| // height is hardcoded to match implicit height from tree view nav bar˝ | |
| // height is hardcoded to match implicit height from tree view nav bar |
| </DropdownMenuTrigger> | ||
|
|
||
| {/* The Dropdown content menu */} | ||
| {!!itemSideAction(item) && ( |
There was a problem hiding this comment.
style: Extra !! operator unnecessary since itemSideAction(item) is already checked
| {!!itemSideAction(item) && ( | |
| {itemSideAction(item) && ( |
| const { treeRef } = useValues(queryDatabaseLogic) | ||
| const { searchTerm } = useValues(queryDatabaseLogic) | ||
| const { setSearchTerm, clearSearch } = useActions(queryDatabaseLogic) |
There was a problem hiding this comment.
style: Consider destructuring values in a single useValues call for better performance
| => { | ||
| // Copy column name when clicking on a column | ||
| if (item && item.record?.type === 'column') { | ||
| void copyToClipboard(item.record.columnName, item.record.columnName) |
There was a problem hiding this comment.
logic: Duplicate parameter passed to copyToClipboard - columnName used twice
| void copyToClipboard(item.record.columnName, item.record.columnName) | |
| void copyToClipboard(item.record.columnName, 'Column name') |
| return [] | ||
| } | ||
|
|
||
| const relevantJoins = joins.filter((join) => join.source_table_name === table!.name) |
There was a problem hiding this comment.
style: Unnecessary non-null assertion (!). The table variable is already checked to be non-null in the condition above
| const relevantJoins = joins.filter((join) => join.source_table_name === table!.name) | |
| const relevantJoins = joins.filter((join) => join.source_table_name === table.name) |
| const FUSE_OPTIONS: Fuse.IFuseOptions<any> = { | ||
| keys: [{ name: 'name', weight: 2 }], | ||
| threshold: 0.3, | ||
| ignoreLocation: true, | ||
| includeMatches: true, | ||
| } |
There was a problem hiding this comment.
style: Consider moving FUSE_OPTIONS to a constants file for better maintainability
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
… into editor-sidebar-tree-view
| ) | ||
|
|
||
| Object.entries(tablesBySourceType).forEach(([sourceType, tablesWithMatches]) => { | ||
| expandedIds.push(`search-${sourceType}`) |
There was a problem hiding this comment.
There's an inconsistency between the ID formats used for search expansion. In createSourceFolderNode(), the ID is normalized with sourceType === 'PostHog' ? 'posthog' : sourceType, but here the raw sourceType is used without normalization. This could prevent auto-expansion from working correctly for some source types, particularly "PostHog". Consider using the same normalization logic in both places:
expandedIds.push(`search-${sourceType === 'PostHog' ? 'posthog' : sourceType}`)| expandedIds.push(`search-${sourceType}`) | |
| expandedIds.push(`search-${sourceType === 'PostHog' ? 'posthog' : sourceType}`) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
Size Change: +44 B (0%) Total Size: 1.83 MB ℹ️ View Unchanged
|
| const viewId = item.id.startsWith('search-view-') | ||
| ? item.id.replace('search-view-', '') | ||
| : item.id.replace('view-', '') |
There was a problem hiding this comment.
The ID extraction logic may not handle edge cases where the view ID itself contains the substring view-. Consider using a more robust approach such as a regex with capture groups or splitting the string and taking the appropriate part. For example:
// More robust approach
const viewId = item.id.startsWith('search-view-')
? item.id.substring('search-view-'.length)
: item.id.substring('view-'.length)This ensures correct extraction regardless of the ID's content.
| const viewId = item.id.startsWith('search-view-') | |
| ? item.id.replace('search-view-', '') | |
| : item.id.replace('view-', '') | |
| const viewId = item.id.startsWith('search-view-') | |
| ? item.id.substring('search-view-'.length) | |
| : item.id.substring('view-'.length) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted: Caution Detected flapping snapshotsThese snapshots have auto-updated more than once since the last human commit:
The flippy-flappies are deadly and must be fixed ASAP. They're productivity killers.
Triggered by this commit. |
Problem
Changes
TODO