-
Notifications
You must be signed in to change notification settings - Fork 288
feat(lineage): vscode updated lineage #5495
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite nervous about these changes because they undo a lot of work that went into making the lineage more up to date and safer with better type safety.
vscode/react/src/pages/help.ts
Outdated
|
||
export function getNodeTypeColor(nodeType: NodeType) { | ||
return { | ||
sql: 'bg-lineage-node-type-background-sql', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it might be good for the library to expose these strings so that they are tied together correctly. If you change the library you'll break these downstream without any link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those colors (for ModelType) are specific to this particular (vscode ext) version of a lineage
const { | ||
data: columnLineageData, | ||
refetch: getColumnLineage, | ||
isFetching: isColumnLineageFetching, | ||
error: columnLineageError, | ||
} = useApiColumnLineage(nodeId, name, { models_only: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These api calls should not leak into the different parts and should be passed down with the correct abstractions.
import { ModelNodeColumn } from './ModelNodeColumn' | ||
import type { ModelName as ModelNameType } from '@/domain/models' | ||
|
||
export const ModelNode = React.memo(function ModelNode({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we tried the react compiler? I think it should do a much better job than us implementing memos by hand?
57e35b0
to
21f636c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of comments some may be duplicate to what I said before so make reasonable judgements ...
- we should add tests
a few other comments on styling

^ can we remove the border around the view and also there's some horizontal space that is being lost to padding.
- Can we also not have the lock nodes button, does it do anything?
- When you hover over a column, it's not obvious that it is clickable, can we change the cursor on it
- If you click a column it is painfully slow to show the column lineage, like multiple seconds slow and sometimes doesn't even show the lineage
- Do we need this
current
tag, I feel like the yellow visualisation does a good enough job of highlighting that, it's the selected node? - If you put the tags on one line, I feel like you would gain quite a bit of real estate

- When you select/deselect a column it's pretty unpleasant movement between the two


vscode/bus/src/brand.ts
Outdated
/** | ||
* Constraint that only accepts branded string types | ||
*/ | ||
export type BrandedString = string & Brand<string> | ||
|
||
/** | ||
* BrandedRecord is a type that creates a branded Record type with strict key checking. | ||
* This ensures that Record<BrandedKey1, V> is NOT assignable to Record<BrandedKey2, V> | ||
* | ||
* @example | ||
* type ModelFQN = Branded<string, 'ModelFQN'> | ||
* type ModelName = Branded<string, 'ModelName'> | ||
* | ||
* type FQNMap = BrandedRecord<ModelFQN, string> | ||
* type NameMap = BrandedRecord<ModelName, string> | ||
* | ||
* const fqnMap: FQNMap = {} | ||
* const nameMap: NameMap = fqnMap // TypeScript error! | ||
*/ | ||
export type BrandedRecord<K extends BrandedString, V> = Record<K, V> & { | ||
readonly __recordKeyBrand: K | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: For me, I am less convinced about these types. They are an additional thing to learn that actually doesn't stop you from writing any less code. Every time you write one of these, it's another thing for someone new to the codebase to jump to and understand.
BrandedString<"ThingA">
versus
Branded<String, "ThingA">
BrandedRecord<"ThingA", int>
versus
Record<Branded<"ThingA", string>, int>
vscode/react/src/pages/lineage.tsx
Outdated
const rpc = useRpc() | ||
React.useEffect(() => { | ||
const fetchFirstTimeModelIfNotSet = async ( | ||
const fetchFirstTimeModelIfNotSet = async <T extends BrandedString>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the indirection of the generic? In this case, we always want this to return just a model fqn.
vscode/react/src/pages/help.ts
Outdated
@@ -0,0 +1,36 @@ | |||
import type { ModelType } from '@/api/client' | |||
|
|||
type NodeType = ModelType | 'cte/subquery' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use cte/subquery
, then you need to invent a new concept for ModelFQN. A subquery's id is not a model fqn. You need to define a new concept for it.
const [showNodeColumns, setShowNodeColumns] = React.useState(showColumns) | ||
const [isHovered, setIsHovered] = React.useState(false) | ||
|
||
const nodeId = id as ModelNodeId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: To me, this is the first thing you should and we should make a note why we do it.
selectedNodeId, | ||
selectedNodes, | ||
showColumns, | ||
fetchingColumns, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that we built this dynamically in the past, but I took this away. There are two big problems with this:
- The context changes over time; to make sure we don't ever show bad graphs, I reset the graph on every context reload.
- The performance of this dynamic fetching makes the lineage unusable if you are using a remote machine, which is true for anyone using Codespaces
const { | ||
columns, | ||
selectedColumns: modelSelectedColumns, | ||
columnNames, | ||
} = useColumns<ModelFQN, ModelColumnName, Column, ModelColumnID>( | ||
selectedColumns, | ||
data.name, | ||
data.columns, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be used and again cannot be just gotten from state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdym, gotten from state ?
it is calculating for each node
) | ||
|
||
const hasSelectedColumns = selectedColumns.intersection(columnNames).size > 0 | ||
const hasFetchingColumns = fetchingColumns.intersection(columnNames).size > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my comments above, I don't think we should have dynamic data graphs.
The graph should either be loading fully or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is fetching data for CLL, we cant make send it upfront, yet
> | ||
<NodeAppendix | ||
position="top" | ||
className="bg-lineage-node-appendix-background" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have really no problem with these but if they are coming from the common it might be worth just making them constants.
className="overflow-visible" | ||
onMouseEnter={() => setIsHovered(true)} | ||
onMouseLeave={() => setIsHovered(false)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am always going to fight to not have visual stuff be JS dependent because it's slow 😛 but I don't have a good view of whether it is possible. Might be a question for Claude 😛
type={type} | ||
description={description} | ||
className={cn( | ||
'ModelNodeColumn', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we use kebab casing for css ModelNodeColumn
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do, but for component class i prefer component name as css class to find it faster if needed
deb8585
to
40f65ad
Compare
5d345c5
to
59c4c2f
Compare
No description provided.