exposing the setView, getView, and listSavedViews api [NOT FOR MERGE YET]#177
exposing the setView, getView, and listSavedViews api [NOT FOR MERGE YET]#177
Conversation
|
|
Please provide more details as to what you are trying to achieve. Some questions that would be good to answer in your PR description:
Also, make sure to look at some of our already written logic for creating view states off of saved views here: |
| * Lists all saved views accessible via the client supplied to {@link initViewOperations}. | ||
| * Returns an array of `{ id, displayName }` objects, or an empty array if no client is set. | ||
| */ | ||
| export async function listSavedViews(): Promise<{ id: string; displayName: string }[]> { |
There was a problem hiding this comment.
I believe listSavedViews can be done from the backend with IModelDb.Views..
See queryViewDefinitionProps and getViewStateProps.
We only need to query the frontend when we want the current state of the viewport, but the saved views are stored in the iModel, which is on the backend.
The way it's done now: backend calls frontend -> frontend calls backend -> backend receives backend value
There was a problem hiding this comment.
There are 2 types of "views" that are used in applications:
- iModel Views: Contained in the iModel as you explain
- Saved Views: Created dynamically and stored in the saved views API
Using IModelDb.Views only would result in the first one.
Hi, this PR is an experimental implementation to give access to Bentley Copilot to the saved view widget. We need an API to be able to interact programmatically with saved views from the copilot. And we want that API to reside in the same repo as the widget for 2 reasons
We have still a bit of work to do before we communicate with you our exact plans. |
There was a problem hiding this comment.
Pull request overview
This PR adds a small “view operations” API surface to @itwin/saved-views-react to support code-mode usage in OS+, allowing callers to read the active view, set the active view (from either a saved view id or ViewStateProps), and list saved views (via an injected client).
Changes:
- Added
viewOperations.tswithinitViewOperations,getActiveView,setView, andlistSavedViews. - Re-exported the new APIs (and
ViewOperationsClienttype) from the package entrypoint.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/saved-views-react/src/viewOperations.ts | Introduces new public API for reading/setting the active viewport view and listing saved views via an injected client. |
| packages/saved-views-react/src/index.ts | Exposes the new view operations API from the main package export. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export interface ViewOperationsClient { | ||
| getSavedViewDataById: (args: { savedViewId: string }) => Promise<SavedViewData>; | ||
| /** Optional: list all saved views as `{ id, displayName }` pairs for name-based lookup. */ | ||
| listSavedViews?: () => Promise<{ id: string; displayName: string }[]>; | ||
| } |
There was a problem hiding this comment.
listSavedViews returns { id, displayName }, but this package consistently uses savedViewId (see SavedView.savedViewId). This inconsistency forces consumers to special-case this API and increases the chance of mixing up different kinds of ids. Consider returning { savedViewId, displayName } (e.g., Pick<SavedView, "savedViewId" | "displayName">[]) and update the JSDoc to match.
| const className = props.viewDefinitionProps.classFullName.toLowerCase(); | ||
| if (className.includes("drawing")) | ||
| return DrawingViewState.createFromProps(props, vp.iModel); | ||
| if (className.includes("sheet")) | ||
| return SheetViewState.createFromProps(props, vp.iModel); |
There was a problem hiding this comment.
View type detection via classFullName.toLowerCase().includes("drawing"|"sheet") is brittle and can misclassify custom/derived view classes whose names contain those substrings. Prefer exact checks against DrawingViewState.classFullName / SheetViewState.classFullName / SpatialViewState.classFullName (and fall back to SpatialViewState if unknown) to make this deterministic.
| const className = props.viewDefinitionProps.classFullName.toLowerCase(); | |
| if (className.includes("drawing")) | |
| return DrawingViewState.createFromProps(props, vp.iModel); | |
| if (className.includes("sheet")) | |
| return SheetViewState.createFromProps(props, vp.iModel); | |
| const className = props.viewDefinitionProps.classFullName; | |
| if (className === DrawingViewState.classFullName) | |
| return DrawingViewState.createFromProps(props, vp.iModel); | |
| if (className === SheetViewState.classFullName) | |
| return SheetViewState.createFromProps(props, vp.iModel); | |
| if (className === SpatialViewState.classFullName) | |
| return SpatialViewState.createFromProps(props, vp.iModel); |
| export async function setView(viewIdOrProps: string | ViewStateProps): Promise<boolean> { | ||
| const vp = getActiveViewport(); | ||
| if (!vp) | ||
| return false; | ||
|
|
There was a problem hiding this comment.
setView collapses multiple failure modes (no active viewport, missing client, client fetch failure, invalid props, view load failure, etc.) into a single false return and also swallows the underlying error. This makes it hard for callers to diagnose/configure failures. Consider either throwing (consistent with other exported APIs like applySavedView/createViewState) or returning a discriminated result that includes an error/reason code.
| /** | ||
| * Lists all saved views accessible via the client supplied to {@link initViewOperations}. | ||
| * Returns an array of `{ id, displayName }` objects, or an empty array if no client is set. | ||
| */ | ||
| export async function listSavedViews(): Promise<{ id: string; displayName: string }[]> { | ||
| return _client?.listSavedViews?.() ?? []; | ||
| } |
There was a problem hiding this comment.
JSDoc says listSavedViews returns an empty array only "if no client is set", but the implementation also returns [] when a client is set without listSavedViews implemented (_client?.listSavedViews?.() ?? []). Please either update the doc to reflect this behavior or change the implementation to fail loudly when listSavedViews is unavailable.
I want to clarify that this package is our public saved views UI components and clients for the Saved Views API, and it's not a place for experimental or internal code. |
exposing the setView, getView, and listSavedViews api for code mode in os+