Skip to content
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

Harden views #3845

Merged
merged 2 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmp/viewmanager/SaveAsDialogModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export class SaveAsDialogModel extends HoistModel {
private resolveOpen: (value: View) => void;

get type(): string {
return this.parent.viewType;
return this.parent.type;
}

get typeDisplayName(): string {
Expand Down
29 changes: 22 additions & 7 deletions cmp/viewmanager/View.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,20 @@ export class View<T extends PlainObject = PlainObject> {
*/
readonly value: Partial<T> = null;

private readonly model: ViewManagerModel;

get name(): string {
return this.info?.name ?? 'Default';
}

get token(): string {
return this.info?.token ?? null;
}

get type(): string {
return this.model.type;
}

get isDefault(): boolean {
return !this.info;
}
Expand All @@ -33,24 +47,25 @@ export class View<T extends PlainObject = PlainObject> {
return this.info?.lastUpdated ?? null;
}

get token(): string {
return this.info?.token ?? null;
get typedName(): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if it would make sense to have a getTypedName helper that takes a view and a model or view and a type rather than having it be a top-level property on view itself

return `${this.model.typeDisplayName} '${this.name}'`;
}

static fromBlob<T>(blob: JsonBlob, model: ViewManagerModel): View<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment in constructor re: generic ViewManagerModel type

return new View(new ViewInfo(blob, model), blob.value);
return new View(new ViewInfo(blob, model), blob.value, model);
}

static createDefault<T>(): View<T> {
return new View(null, {});
static createDefault<T>(model: ViewManagerModel): View<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment in constructor re: generic ViewManagerModel type

return new View(null, {}, model);
}

withUpdatedValue(value: Partial<T>): View<T> {
return new View(this.info, value);
return new View(this.info, value, this.model);
}

constructor(info: ViewInfo, value: Partial<T>) {
constructor(info: ViewInfo, value: Partial<T>, model: ViewManagerModel) {
Copy link
Contributor

@ghsolomon ghsolomon Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Believe this should be model: ViewManagerModel<T>, but also curious why the backpointer to the model. Looks like just for the type and typedName.

this.info = info;
this.value = value;
this.model = model;
}
}
2 changes: 1 addition & 1 deletion cmp/viewmanager/ViewInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export class ViewInfo {
/** Unique Id */
readonly token: string;

/** App-defined type discriminator, as per {@link ViewManagerConfig.viewType}. */
/** App-defined type discriminator, as per {@link ViewManagerConfig.type}. */
readonly type: string;

/** User-supplied descriptive name. */
Expand Down
26 changes: 13 additions & 13 deletions cmp/viewmanager/ViewManagerModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,11 @@ export interface ViewManagerConfig {
* different viewManagers to be added to your app in the future - e.g. `portfolioGridView` or
* `tradeBlotterDashboard`.
*/
viewType: string;
type: string;

/**
* Optional user-facing display name for the view type, displayed in the ViewManager menu
* and associated management dialogs and prompts. Defaulted from `viewType` if not provided.
* and associated management dialogs and prompts. Defaulted from `type` if not provided.
*/
typeDisplayName?: string;

Expand Down Expand Up @@ -141,7 +141,7 @@ export class ViewManagerModel<T = PlainObject> extends HoistModel {

/** Immutable configuration for this model. */
declare persistWith: ViewManagerPersistOptions;
readonly viewType: string;
readonly type: string;
readonly typeDisplayName: string;
readonly globalDisplayName: string;
readonly enableAutoSave: boolean;
Expand Down Expand Up @@ -250,7 +250,7 @@ export class ViewManagerModel<T = PlainObject> extends HoistModel {
* initial load before binding to persistable components.
*/
private constructor({
viewType,
type,
persistWith,
typeDisplayName,
globalDisplayName = 'global',
Expand All @@ -269,8 +269,8 @@ export class ViewManagerModel<T = PlainObject> extends HoistModel {
"ViewManagerModel requires 'initialViewSpec' if `enableDefault` is false."
);

this.viewType = viewType;
this.typeDisplayName = lowerCase(typeDisplayName ?? genDisplayName(viewType));
this.type = type;
this.typeDisplayName = lowerCase(typeDisplayName ?? genDisplayName(type));
this.globalDisplayName = globalDisplayName;
this.persistWith = persistWith;
this.manageGlobal = executeIfFunction(manageGlobal) ?? false;
Expand Down Expand Up @@ -332,18 +332,18 @@ export class ViewManagerModel<T = PlainObject> extends HoistModel {
}
const {pendingValue, view, api} = this;
try {
if (!(await this.maybeConfirmSaveAsync(view.info, pendingValue))) {
if (!(await this.maybeConfirmSaveAsync(view, pendingValue))) {
return;
}
const updated = await api
.updateViewValueAsync(view, pendingValue.value)
.linkTo(this.saveTask);

this.setAsView(updated);
this.noteSuccess(`Saved ${view.info.typedName}`);
this.noteSuccess(`Saved ${view.typedName}`);
} catch (e) {
this.handleException(e, {
message: `Failed to save ${view.info.typedName}. If this persists consider \`Save As...\`.`
message: `Failed to save ${view.typedName}. If this persists consider \`Save As...\`.`
});
}
this.refreshAsync();
Expand All @@ -353,7 +353,7 @@ export class ViewManagerModel<T = PlainObject> extends HoistModel {
const view = (await this.saveAsDialogModel.openAsync()) as View<T>;
if (view) {
this.setAsView(view);
this.noteSuccess(`Saved ${view.info.typedName}`);
this.noteSuccess(`Saved ${view.typedName}`);
}
this.refreshAsync();
}
Expand Down Expand Up @@ -542,16 +542,16 @@ export class ViewManagerModel<T = PlainObject> extends HoistModel {
});
}

private async maybeConfirmSaveAsync(info: ViewInfo, pendingValue: PendingValue<T>) {
private async maybeConfirmSaveAsync(view: View, pendingValue: PendingValue<T>) {
// Get latest from server for reference
const latest = await this.api.fetchViewAsync(info),
const latest = await this.api.fetchViewAsync(view.info),
isGlobal = latest.isGlobal,
isStale = latest.lastUpdated > pendingValue.baseUpdated;
if (!isStale && !isGlobal) return true;

const latestInfo = latest.info,
{typeDisplayName, globalDisplayName} = this,
msgs: ReactNode[] = [`Save ${info.typedName}?`];
msgs: ReactNode[] = [`Save ${view.typedName}?`];
if (isGlobal) {
msgs.push(
span(
Expand Down
31 changes: 24 additions & 7 deletions cmp/viewmanager/ViewToBlobApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class ViewToBlobApi<T> {
const {owner} = this;
try {
const blobs = await XH.jsonBlobService.listAsync({
type: owner.viewType,
type: owner.type,
includeValue: false
});
return blobs.map(b => new ViewInfo(b, owner));
Expand All @@ -43,7 +43,7 @@ export class ViewToBlobApi<T> {
}

async fetchViewAsync(info: ViewInfo): Promise<View<T>> {
if (!info) return View.createDefault();
if (!info) return View.createDefault(this.owner);
try {
const blob = await XH.jsonBlobService.getAsync(info.token);
return View.fromBlob(blob, this.owner);
Expand All @@ -59,12 +59,14 @@ export class ViewToBlobApi<T> {
const {owner} = this;
try {
const blob = await XH.jsonBlobService.createAsync({
type: owner.viewType,
type: owner.type,
name: name.trim(),
description: description?.trim(),
value
});
return View.fromBlob(blob, owner);
const ret = View.fromBlob(blob, owner);
this.trackChange('Created View', ret);
return ret;
} catch (e) {
throw XH.exception({message: `Unable to create ${owner.typeDisplayName}`, cause: e});
}
Expand All @@ -82,7 +84,9 @@ export class ViewToBlobApi<T> {
description: description?.trim(),
acl: isGlobal ? '*' : null
});
return View.fromBlob(blob, this.owner);
const ret = View.fromBlob(blob, this.owner);
this.trackChange('Updated View Info', ret);
return ret;
} catch (e) {
throw XH.exception({message: `Unable to update ${view.typedName}`, cause: e});
}
Expand All @@ -91,10 +95,14 @@ export class ViewToBlobApi<T> {
async updateViewValueAsync(view: View<T>, value: Partial<T>): Promise<View<T>> {
try {
const blob = await XH.jsonBlobService.updateAsync(view.token, {value});
return View.fromBlob(blob, this.owner);
const ret = View.fromBlob(blob, this.owner);
if (ret.isGlobal) {
this.trackChange('Updated Global View definition', ret);
}
return ret;
} catch (e) {
throw XH.exception({
message: `Unable to update value for ${view.info.typedName}`,
message: `Unable to update value for ${view.typedName}`,
cause: e
});
}
Expand All @@ -103,8 +111,17 @@ export class ViewToBlobApi<T> {
async deleteViewAsync(view: ViewInfo) {
try {
await XH.jsonBlobService.archiveAsync(view.token);
this.trackChange('Deleted View', view);
} catch (e) {
throw XH.exception({message: `Unable to delete ${view.typedName}`, cause: e});
}
}

private trackChange(message: string, v: View | ViewInfo) {
XH.track({
message,
category: 'Views',
data: {name: v.name, token: v.token, isGlobal: v.isGlobal, type: v.type}
});
}
}
2 changes: 1 addition & 1 deletion desktop/cmp/viewmanager/ViewManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ const menuButton = hoistCmp.factory<ViewManagerModel>({
const {view, typeDisplayName, isLoading} = model;
return button({
className: 'xh-view-manager__menu-button',
text: view.info?.name ?? `Default ${startCase(typeDisplayName)}`,
text: view.isDefault ? `Default ${startCase(typeDisplayName)}` : view.name,
icon: !isLoading
? Icon.bookmark()
: box({
Expand Down
2 changes: 1 addition & 1 deletion desktop/cmp/viewmanager/ViewMenu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export const viewMenu = hoistCmp.factory<ViewManagerProps>({
...favoriteViews.map(info => {
return menuItem({
key: `${info.token}-favorite`,
icon: view.info?.token === info.token ? Icon.check() : Icon.placeholder(),
icon: view.token === info.token ? Icon.check() : Icon.placeholder(),
text: textAndFaveToggle({info}),
onClick: () => model.selectViewAsync(info),
title: info.description
Expand Down
Loading