-
Notifications
You must be signed in to change notification settings - Fork 93
feat: create entry point for new data browsing experience VSCODE-722 #1214
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
Anemy
left a comment
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.
Left a couple comments to start the discussion. There is a lot in this pr, it would likely speed things up if we broke it into multiple, by first adding just the new webview entry, and then adding the documents list, and after that the formatting for the documents.
| }) | ||
| ); | ||
| // Wait for minimum loading duration | ||
| await new Promise((resolve) => setTimeout(resolve, 600)); |
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.
Can we avoid a constant timeout here and instead wait for the end state? Same with the setTimeouts below. There's a waitFor that some tests use.
It'll speed up all of our tests if we use that as a pattern.
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.
hm, good point...the idea was to not have loads so fast that the user isn't sure if anything happened at all. We can just use sinon.useFakeTimers(); in the unit tests.
| alignItems: 'center', | ||
| justifyContent: 'space-between', | ||
| padding: `${spacing[200]}px ${spacing[300]}px`, | ||
| borderBottom: '1px solid var(--vscode-panel-border, #444)', |
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're going to end up using a lot of these vscode variables. How do you feel about having them all in one place? Like vscode-styles and we reference this panelBorder string from there.
src/views/webview-app/app.tsx
Outdated
| // Each vscode-api.ts calls acquireVsCodeApi() at module load time, | ||
| // and acquireVsCodeApi() can only be called once per webview. | ||
| const OverviewPage = lazy(() => import('./overview-page')); | ||
| const PreviewPage = lazy(() => import('../data-browsing-app/preview-page')); |
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 haven't seen this in use before, cool.
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 actually doesn't even work without this. It loads both pages and gets all confused...
| } | ||
| type ViewType = 'tree' | 'json' | 'table'; | ||
|
|
||
| const ITEMS_PER_PAGE_OPTIONS = [10, 25, 50, 100]; |
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.
Can we add these later in another pr? This pr already has a lot in it. We likely want to save this in the extension settings as well.
| interface PreviewDocument { | ||
| [key: string]: unknown; | ||
| } | ||
| type ViewType = 'tree' | 'json' | 'table'; |
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 won't have a table view, let's remove that.
| }} | ||
| > | ||
| {/* Toolbar */} | ||
| <div className={toolbarStyles}> |
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.
@vscode-elements/react-elements has a toolbar container and toolbar buttons. Should we be using those?
https://vscode-elements.github.io/components/toolbar-container/
| const getTotalCount = (signal?: AbortSignal): Promise<number> => | ||
| element.getTotalCount(signal); | ||
|
|
||
| this._dataBrowsingController.openDataBrowser(this._context, { |
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.
Can we show the view and then show the find results after? That way things are faster for the user. I think we can have things going in parallel.
|
|
||
| return ( | ||
| <div | ||
| style={{ |
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.
Can we follow the pattern for styles that the rest of the webview does?
className={cssVariableDeclaredAtTheTop}
I understand that this will change in the next pr when we start to show the toolbar and documents, however we should avoid introducing new patterns incase someone else picks it up and assumes it's the way to go.
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.
There's already a vscode-api file in the webview-app, can we share some of this logic?
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 was on the fence whether to keep these separate or merge in one file...will update as you suggested
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.
taking a second look at it, it really feels like this should be separated per folder. Its going to become a huge mess down the line if we cram it all in one file.
src/commands/index.ts
Outdated
|
|
||
| // Commands for the data browsing upgrade. | ||
| mdbOpenCollectionPreviewFromTreeView: | ||
| 'mdb.internal.openCollectionPreviewFromTreeView', |
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 we want to .internal as a pattern, I'd expect us to do it to all the internal functions but this isn't the case right now so maybe let's stick to the old thing?
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.
roger roger
| @@ -1,6 +1,6 @@ | |||
| const FEATURE_FLAGS = { | |||
| useOldConnectionForm: | |||
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.
is this unused?
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 seems to be unused. I can't find any references to it.
| "test": "pnpm run test-webview && pnpm run test-extension", | ||
| "test-extension": "cross-env MDB_IS_TEST=true NODE_OPTIONS=--no-force-async-hooks-checks xvfb-maybe node ./out/test/runTest.js", | ||
| "test-webview": "mocha -r ts-node/register --exit --grep=\"${MOCHA_GREP}\" --file ./src/test/setup-webview.ts src/test/suite/views/webview-app/**/*.test.tsx", | ||
| "test-webview": "mocha -r ts-node/register --exit --grep=\"${MOCHA_GREP}\" --file ./src/test/setup-webview.ts src/test/suite/views/**/**/*.test.tsx", |
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 test seems to be no longer about testing the webview with this change
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'm with you on the named imports
src/views/webview-app/vscode-api.ts
Outdated
| }; | ||
|
|
||
| export default vscode; | ||
| export default getVSCodeApi; |
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.
one of the reasons why I dislike default is that this rename will have no actual effect 😢
can we turn it into a named export while we're at it?
| DEEP_LINK_ALLOWED_COMMANDS, | ||
| DEEP_LINK_DISALLOWED_COMMANDS, | ||
| } from '../../mdbExtensionController'; | ||
| import * as featureFlags from '../../featureFlags'; |
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.
can we just do named exports?
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.
Typically I would agree, but then I would need to refactor the stubbing mechanism to something much more complicated, and I don't really want to complicate things (this line: sandbox.stub(featureFlags, 'getFeatureFlag').returns(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.
Fwiw, that kind of stubbing only works because we don't work with actual ESM yet, only transpiled code. Once we do, it's going to break
I'd check whether it's possible to avoid stubbing in tests entirely and, if necessary, export something from featureFlags to temporarily set feature flags in tests.
PR Summary: VSCODE-722 – Enhanced Data Browsing Experience
Overview
Introduces a webview-based data browsing experience for viewing collection documents, controlled by a feature flag (
MDB_USE_ENHANCED_DATA_BROWSING_EXPERIIENCE).The existing tree-based document list remains the default.
Key Changes
New Data Browsing Infrastructure
DataBrowsingController
New controller managing webview panels for document browsing, with support for abort controllers to cancel in-flight requests when new ones are initiated.
ShowPreviewTreeItem
New tree item that replaces DocumentListTreeItem when the feature flag is enabled. Clicking it opens the document preview webview.
preview-page.tsx
React component for the document preview webview with loading state, dark mode support, and message handling.
webviewHelpers.ts
Extracted common webview utilities (createWebviewPanel, getWebviewHtml, getNonce) for reuse between the connection dialog and the data browser.
Tree View Updates
CollectionTreeItem now conditionally renders either:
based on the feature flag.
Added documentUtils.ts to share the CollectionType enum and formatDocCount helper between tree items.
Extension Controller Integration
mdb.internal.openCollectionPreviewFromTreeViewto handle clicking the preview tree item.Webview Controller Refactoring
Message Protocol
Test Infrastructure
Dependencies
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes