Feat(vscode): Add the Table Diff view in the extension#4917
Feat(vscode): Add the Table Diff view in the extension#4917themisvaltinos merged 4 commits intomainfrom
Conversation
benfdking
left a comment
There was a problem hiding this comment.
I think there are a few things to fix.
| interface AllModelsResponse extends BaseResponse { | ||
| models: string[] | ||
| model_completions: ModelCompletion[] | ||
| keywords: string[] | ||
| macros: MacroCompletion[] | ||
| } | ||
|
|
||
| interface ModelCompletion { | ||
| name: string | ||
| description?: string | ||
| } | ||
|
|
||
| interface MacroCompletion { | ||
| name: string | ||
| description?: string | ||
| } |
There was a problem hiding this comment.
The reason for this not being updated, is that it is legacy and so it really should just be dropped.
There was a problem hiding this comment.
removed these and created a new get_models_feature to use
| const panelType = (window as any).__SQLMESH_PANEL_TYPE__ || 'lineage' | ||
| return panelType === 'tablediff' ? <TableDiffPage /> : <LineagePage /> |
There was a problem hiding this comment.
I avoided this problem initially, but I am not sure this is quite the way we want to do it. To me it's about loading the specific page and we can break it up as much as possible for the performance.
There was a problem hiding this comment.
I agree, check if what I added makes more sense
vscode/react/src/pages/tablediff.tsx
Outdated
| const [selectedModel, setSelectedModel] = useState<string | undefined>( | ||
| undefined, | ||
| ) | ||
| const { on } = useEventBus() | ||
| const queryClient = useQueryClient() | ||
|
|
||
| const { | ||
| data: models, | ||
| isLoading: isLoadingModels, | ||
| error: modelsError, | ||
| } = useApiModels() | ||
| const rpc = useRpc() | ||
|
|
||
| React.useEffect(() => { | ||
| const fetchFirstTimeModelIfNotSet = async ( | ||
| models: Model[], | ||
| ): Promise<string | undefined> => { | ||
| if (!Array.isArray(models)) { | ||
| return undefined | ||
| } | ||
| const activeFile = await rpc('get_active_file', {}) | ||
| // @ts-ignore | ||
| if (!activeFile.fileUri) { | ||
| return models[0].name | ||
| } | ||
| // @ts-ignore | ||
| const fileUri: string = activeFile.fileUri | ||
| const filePath = URI.file(fileUri).path | ||
| const model = models.find( | ||
| (m: Model) => URI.file(m.full_path).path === filePath, | ||
| ) | ||
| if (model) { | ||
| return model.name | ||
| } | ||
| return undefined | ||
| } | ||
| if (selectedModel === undefined && Array.isArray(models)) { | ||
| fetchFirstTimeModelIfNotSet(models).then(modelName => { | ||
| if (modelName && selectedModel === undefined) { | ||
| setSelectedModel(modelName) | ||
| } else { | ||
| setSelectedModel(models[0].name) | ||
| } | ||
| }) | ||
| } | ||
| }, [models, selectedModel]) | ||
|
|
||
| const modelsRecord = | ||
| Array.isArray(models) && | ||
| models.reduce( | ||
| (acc, model) => { | ||
| acc[model.name] = model | ||
| return acc | ||
| }, | ||
| {} as Record<string, Model>, | ||
| ) | ||
|
|
||
| React.useEffect(() => { | ||
| const handleChangeFocusedFile = (fileUri: { fileUri: string }) => { | ||
| const full_path = URI.parse(fileUri.fileUri).path | ||
| const model = Object.values(modelsRecord).find( | ||
| m => URI.file(m.full_path).path === full_path, | ||
| ) | ||
| if (model) { | ||
| setSelectedModel(model.name) | ||
| } | ||
| } | ||
|
|
||
| const handleSavedFile = () => { | ||
| queryClient.invalidateQueries() | ||
| } | ||
|
|
||
| const offChangeFocusedFile = on( | ||
| 'changeFocusedFile', | ||
| handleChangeFocusedFile, | ||
| ) | ||
| const offSavedFile = on('savedFile', handleSavedFile) | ||
|
|
||
| // If your event bus returns an "off" function, call it on cleanup | ||
| return () => { | ||
| if (offChangeFocusedFile) offChangeFocusedFile() | ||
| if (offSavedFile) offSavedFile() | ||
| } | ||
| }, [on, queryClient, modelsRecord]) | ||
|
|
||
| if (modelsError) { | ||
| return <div>Error: {modelsError.message}</div> | ||
| } | ||
|
|
||
| if ( | ||
| isLoadingModels || | ||
| models === undefined || | ||
| modelsRecord === false || | ||
| selectedModel === undefined | ||
| ) { | ||
| return <div>Loading models...</div> | ||
| } | ||
| if (!Array.isArray(models)) { | ||
| return <div>Error: Models data is not in the expected format</div> | ||
| } |
There was a problem hiding this comment.
Why do you need any of this?
There was a problem hiding this comment.
excellent point removed all of it
vscode/react/src/pages/tablediff.tsx
Outdated
| } | ||
|
|
||
| return ( | ||
| <div className="h-[100vh] w-[100vw]"> |
There was a problem hiding this comment.
Let's put this initial styling in the component.
| } | ||
| result: { | ||
| ok: boolean | ||
| data?: any |
There was a problem hiding this comment.
yep revised all these anys
sqlmesh/lsp/main.py
Outdated
| for env in context.context.state_reader.get_environments(): | ||
| environments[env.name] = EnvironmentInfo( | ||
| name=env.name, | ||
| snapshots=[s.fingerprint.to_identifier() for s in env.snapshots], | ||
| start_at=str(to_timestamp(env.start_at)), | ||
| plan_id=env.plan_id or "", | ||
| ) | ||
|
|
||
| # Add prod if not present (mirroring web/server/api/endpoints/environments.py) | ||
| if c.PROD not in environments: | ||
| environments[c.PROD] = EnvironmentInfo( | ||
| name=c.PROD, | ||
| snapshots=[], | ||
| start_at=str(to_timestamp(c.EPOCH)), | ||
| plan_id="", | ||
| ) | ||
|
|
||
| # Add default target environment if not present | ||
| if context.context.config.default_target_environment not in environments: | ||
| environments[context.context.config.default_target_environment] = EnvironmentInfo( | ||
| name=context.context.config.default_target_environment, | ||
| snapshots=[], | ||
| start_at=str(to_timestamp(c.EPOCH)), | ||
| plan_id="", | ||
| ) |
There was a problem hiding this comment.
These are pretty big assumptions that aren't really listed in the "endpoint" definition.
There was a problem hiding this comment.
yeah mirroring the web version wasn't wise or needed, removed these
| // No active editor, show a list of all models | ||
| const allModelsResult = await lspClient.call_custom_method( | ||
| 'sqlmesh/all_models', | ||
| { textDocument: { uri: '' } }, |
There was a problem hiding this comment.
I am not certain we want to be using the all_models endpoint which was a legacy endpoint used for autocomplete.
|
|
||
| // Get the current active editor | ||
| const activeEditor = vscode.window.activeTextEditor | ||
| let selectedModelInfo: any = null |
| target: string | ||
| stats: Record<string, number> | ||
| sample: Record<string, any> | ||
| joined_sample: Record<string, any> |
There was a problem hiding this comment.
We shouldn't really haven anys
2267d46 to
8d93e96
Compare
e33f31a to
e4d79de
Compare
| <div | ||
| className="relative h-2 rounded overflow-hidden" | ||
| style={{ | ||
| backgroundColor: 'var(--vscode-progressBar-background, #333)', |
There was a problem hiding this comment.
Why do we have backup colors here?
| </div> | ||
| <div | ||
| className="relative h-2 rounded overflow-hidden" | ||
| style={{ |
There was a problem hiding this comment.
Use tailwind styling sinsteamd
| onMouseEnter={e => | ||
| (e.currentTarget.style.color = | ||
| 'var(--vscode-textLink-activeForeground)') | ||
| } | ||
| onMouseLeave={e => | ||
| (e.currentTarget.style.color = themeColors.accent) | ||
| } |
There was a problem hiding this comment.
Unnecessary JS. Prefer CSS where you can do things in CSS
https://tailwindcss.com/docs/hover-focus-and-other-states#hover-focus-and-active
| style={{ | ||
| color: themeColors.info, | ||
| backgroundColor: 'var(--vscode-input-background)', | ||
| border: `1px solid ${themeColors.border}`, | ||
| }} |
There was a problem hiding this comment.
Prefer tailwind over style.
| </span> | ||
| </div> | ||
| ))} | ||
| {Object.entries(schema_diff.modified).map(([col, type]) => ( |
There was a problem hiding this comment.
Generally speaking turn a Row object into a component.
| color: 'var(--vscode-editor-foreground)', | ||
| }} | ||
| > | ||
| {/* Header */} |
There was a problem hiding this comment.
If it's so big you need to break it apart with comments, just break it apart with a component.
cbf8639 to
4699819
Compare
4699819 to
5b6f318
Compare
This adds support for table diff in the VSCode extension