-
Notifications
You must be signed in to change notification settings - Fork 238
Feat(vscode): Add the Table Diff view in the extension #4917
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.
I think there are a few things to fix.
vscode/extension/src/lsp/custom.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need any of this?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put this initial styling in the component.
} | ||
result: { | ||
ok: boolean | ||
data?: any |
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 can't really have any
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are pretty big assumptions that aren't really listed in the "endpoint" definition.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
no any.
target: string | ||
stats: Record<string, number> | ||
sample: Record<string, any> | ||
joined_sample: Record<string, any> |
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 shouldn't really haven anys
2267d46
to
8d93e96
Compare
This adds support for table diff in the VSCode extension