Skip to content

feat(query-bar): allow users to set the default sort to be recent first COMPASS-6706 #6663

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.

Sign up for GitHub

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

Merged
merged 24 commits into from
Feb 4, 2025
Merged

feat(query-bar): allow users to set the default sort to be recent first COMPASS-6706 #6663

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
f3174bc
changes
djechlin-mongodb Jan 28, 2025
1d13dbd
changes
djechlin-mongodb Jan 28, 2025
987e78d
Merge branch 'main' into COMPASS-6706
djechlin-mongodb Jan 28, 2025
59e0b25
remove stray comments
djechlin-mongodb Jan 28, 2025
b4a3945
copy
djechlin-mongodb Jan 28, 2025
7a56074
changes
djechlin-mongodb Jan 30, 2025
b2d9240
changes
djechlin-mongodb Jan 30, 2025
af1f596
changes
djechlin-mongodb Jan 30, 2025
879cca9
changes
djechlin-mongodb Jan 30, 2025
82207d6
c
djechlin-mongodb Jan 30, 2025
650a774
this works still
djechlin-mongodb Jan 30, 2025
4e4dd30
changes
djechlin-mongodb Jan 30, 2025
72f0697
c
djechlin-mongodb Jan 30, 2025
ddcab6f
c
djechlin-mongodb Jan 30, 2025
60668d2
changes
djechlin-mongodb Jan 30, 2025
b7d6a8c
c
djechlin-mongodb Jan 30, 2025
0489bcc
crud test
djechlin-mongodb Jan 30, 2025
df8dc6c
leafygreen
djechlin-mongodb Jan 30, 2025
c1ed3d9
longer description
djechlin-mongodb Jan 30, 2025
d22bea9
remove selectable values code
djechlin-mongodb Feb 1, 2025
db27b01
remove a selectable values ref
djechlin-mongodb Feb 3, 2025
7114687
value : string not value: string | undefined
djechlin-mongodb Feb 3, 2025
ebbfca0
unit test fix in settings
djechlin-mongodb Feb 3, 2025
69daf52
fix unused var
djechlin-mongodb Feb 4, 2025
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
34 changes: 31 additions & 3 deletions packages/compass-crud/src/stores/crud-store.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1663,7 +1663,8 @@ describe('store', function () {
const plugin = activatePlugin();
store = plugin.store;
deactivate = () => plugin.deactivate();
await dataService.insertOne('compass-crud.test', { name: 'testing' });
await dataService.insertOne('compass-crud.test', { name: 'testing1' });
await dataService.insertOne('compass-crud.test', { name: 'testing2' });
});

afterEach(function () {
Expand All @@ -1680,9 +1681,36 @@ describe('store', function () {

(state) => {
expect(state.error).to.equal(null);
expect(state.docs).to.have.length(1);
expect(state.docs).to.have.length(2);
expect(state.docs[0].doc.name).to.equal('testing1');
expect(state.debouncingLoad).to.equal(false);
expect(state.count).to.equal(1);
expect(state.count).to.equal(2);
expect(state.start).to.equal(1);
expect(state.shardKeys).to.deep.equal({});
},
]);

void store.refreshDocuments();

await listener;
});

it('uses the sort order from preferences', async function () {
await preferences.savePreferences({
defaultSortOrder: '{ _id: -1 }',
});
const listener = waitForStates(store, [
(state) => {
expect(state.debouncingLoad).to.equal(true);
expect(state.count).to.equal(null);
},

(state) => {
expect(state.error).to.equal(null);
expect(state.docs).to.have.length(2);
expect(state.docs[0].doc.name).to.equal('testing2');
expect(state.debouncingLoad).to.equal(false);
expect(state.count).to.equal(2);
expect(state.start).to.equal(1);
expect(state.shardKeys).to.deep.equal({});
},
Expand Down
11 changes: 10 additions & 1 deletion packages/compass-crud/src/stores/crud-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import semver from 'semver';
import StateMixin from '@mongodb-js/reflux-state-mixin';
import type { Element } from 'hadron-document';
import { Document } from 'hadron-document';
import { validate } from 'mongodb-query-parser';
import HadronDocument from 'hadron-document';
import _parseShellBSON, { ParseMode } from '@mongodb-js/shell-bson-parser';
import type { PreferencesAccess } from 'compass-preferences-model/provider';
Expand Down Expand Up @@ -1615,8 +1616,16 @@ class CrudStoreImpl
countOptions.hint = '_id_';
}

let sort = query.sort;
if (!sort && this.preferences.getPreferences().defaultSortOrder) {
sort = validate(
'sort',
this.preferences.getPreferences().defaultSortOrder
);
}
Comment on lines +1619 to +1625
Copy link
Collaborator

@gribnoysup gribnoysup Jan 31, 2025

Choose a reason for hiding this comment

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

This changes the behavior only for CRUD, but query bar is also used in the schema tab, I think we might want to move this logic to query bar itself so that it consistently applies every time no matter where current query from query bar is accessed

Copy link
Contributor Author

@djechlin-mongodb djechlin-mongodb Feb 3, 2025

Choose a reason for hiding this comment

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

It's debatable. I'd actually lean toward putting it in the "crud" layer not the "query bar" layer.

Firstly the product question -- if I understand correctly, the schema tab basically ignores sort and we should actually get rid of that field because it's not helping users at all. If sort were somehow used, I would say no don't use defaults -- for schema users, it's very important they transparently see the inputs to the validation, so silently joining in a default becomes un-helpful to them, whereas it's a non-misleading nice-to-have in document list.

From there, I actually would view this as a CRUD/document-list feature not a query bar feature. Users are basically complaining the docs are in the wrong order, they're not complaining that anything is wrong with their queries. In particular, the feature request is to not change recent queries or saved queries at all -- users are trying to not think about the sort being applied for them, they want the docs to just appear in the right order. So that suggests a document-list feature.

If you move it to query-bar you get a bit of complexity as we now have to manage the double concept of "what the user typed" and "what the user typed after defaults are applied." CRUD would then execute the latter. Recent/saved queries only care about user input; if you believe my above argument then schema only cares about user input; doc list cares about the defaults. To me it just looks like this is something doc list wants to do, and we shouldn't assume it readily generalizes to other uses or that we get any leverage out of moving it to query bar.

LMK what you think. If we do want it used in schema then yeah it needs to be more general. Can you think of any other reasons query bar or its other users would need to care about the defaults -- maybe logging or something gets affected, noting that we specifically don't want the sorts showing up in recent/favorites? Any other value to putting it in the more general location of query bar I'm missing? At this point it just looks a little lower complexity/less stateful concepts to manage to put it in CRUD.

Copy link
Collaborator

@gribnoysup gribnoysup Feb 3, 2025

Choose a reason for hiding this comment

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

We already use maxTimeMS from preferences in query bar to validate user input and in other cases provide default substitutions from query bar defined constants when user input is empty when returning a parsed query, so I don't think it's that far off from query bar responsibilities. It is also true though that maxTimeMS itself while used for validation, is not returned as a default value so I agree that at the current moment we can go both ways about it. I have my preference, but I think your arguments are reasonable too 🙂

If you move it to query-bar you get a bit of complexity as we now have to manage the double concept of "what the user typed" and "what the user typed after defaults are applied."

I want to touch on this a bit because I think it's an important part of the state management story. I think there is no extra complexity involved here compared to running this logic elsewhere, just actual states (query bar user input, current preferences values, etc) and a derived state of a parsed query that takes multiple of those sources into account. So if it's only technical complexity that stops you from doing this, I don't think it should. But as I mentioned, your other points make sense, so feel free to resolve this if it still makes more sense to you to keep it in CRUD only.

Copy link
Contributor Author

@djechlin-mongodb djechlin-mongodb Feb 3, 2025

Choose a reason for hiding this comment

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

Chatted offline a bit -- confirmed schema tab actually ignores the sort part of the query. From here I think that tilts the argument to the CRUD component because we know the other possible user, schema tab, will actually ignore this data if we send it (and Sergey signed off on my reasoning).


const findOptions = {
sort: query.sort,
sort,
projection: query.project,
skip: query.skip,
limit: docsPerPage,
Expand Down
51 changes: 50 additions & 1 deletion packages/compass-preferences-model/src/preferences-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ import {
export const THEMES_VALUES = ['DARK', 'LIGHT', 'OS_THEME'] as const;
export type THEMES = typeof THEMES_VALUES[number];

export const SORT_ORDER_VALUES = [
'',
'{ $natural: -1 }',
'{ _id: 1 }',
'{ _id: -1 }',
] as const;
export type SORT_ORDERS = typeof SORT_ORDER_VALUES[number];

export type PermanentFeatureFlags = {
showDevFeatureFlags?: boolean;
enableDebugUseCsfleSchemaMap?: boolean;
Expand Down Expand Up @@ -69,6 +77,7 @@ export type UserConfigurablePreferences = PermanentFeatureFlags &
enableGenAISampleDocumentPassing: boolean;
enablePerformanceAdvisorBanner: boolean;
maximumNumberOfActiveConnections?: number;
defaultSortOrder: SORT_ORDERS;
enableShowDialogOnQuit: boolean;
enableCreatingNewConnections: boolean;
enableProxySupport: boolean;
Expand Down Expand Up @@ -187,7 +196,13 @@ type PreferenceDefinition<K extends keyof AllPreferences> = {
/** A description used for the --help text and the Settings UI */
description: K extends keyof InternalUserPreferences
? null
: { short: string; long?: string };
: {
short: string;
long?: string;
options?: AllPreferences[K] extends string
? { [k in AllPreferences[K]]: { label: string; description: string } }
: never;
};
/** A method for deriving the current semantic value of this option, even if it differs from the stored value */
deriveValue?: DeriveValueFunction<AllPreferences[K]>;
/** A method for cleaning up/normalizing input from the command line or global config file */
Expand All @@ -199,6 +214,7 @@ type PreferenceDefinition<K extends keyof AllPreferences> = {
? boolean
: false
: boolean;

validator: z.Schema<
AllPreferences[K],
z.ZodTypeDef,
Expand Down Expand Up @@ -537,6 +553,39 @@ export const storedUserPreferencesProps: Required<{
validator: z.boolean().default(false),
type: 'boolean',
},
/**
* Set the default sort.
*/
defaultSortOrder: {
ui: true,
cli: true,
global: true,
description: {
short: 'Default Sort for Query Bar',
long: "All queries executed from the query bar will apply the sort order '$natural: -1'.",
options: {
'': {
label: '$natural: 1 (MongoDB server default)',
description: 'in natural order of documents',
},
'{ $natural: -1 }': {
label: '$natural: -1',
description: 'in reverse natural order of documents',
},
'{ _id: 1 }': {
label: '_id: 1',
description: 'in ascending order by id',
},
'{ _id: -1 }': {
label: '_id: -1',
description: 'in descending order by id',
},
},
},
validator: z.enum(SORT_ORDER_VALUES).default(''),
type: 'string',
},

/**
* Switch to enable DevTools in Electron.
*/
Expand Down
4 changes: 2 additions & 2 deletions packages/compass-preferences-model/src/provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ export {
} from './utils';
export { capMaxTimeMSAtPreferenceLimit } from './maxtimems';
export { featureFlags } from './feature-flags';
export { getSettingDescription } from './preferences-schema';
export type { AllPreferences } from './preferences-schema';
export { getSettingDescription, SORT_ORDER_VALUES } from './preferences-schema';
export type { AllPreferences, SORT_ORDERS } from './preferences-schema';
export type { DevtoolsProxyOptions } from '@mongodb-js/devtools-proxy-support';
10 changes: 10 additions & 0 deletions packages/compass-settings/src/components/settings/general.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,16 @@ describe('GeneralSettings', function () {
});
});

it('renders defaultSortOrder', function () {
expect(within(container).getByTestId('defaultSortOrder')).to.exist;
});

it('changes defaultSortOrder value when selecting an option', function () {
const select = within(container).getByTestId('defaultSortOrder');
userEvent.selectOptions(select, '_id: 1 (in ascending order by creation)');
Copy link
Collaborator

@gribnoysup gribnoysup Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This test fails for me locally (and in CI):

1 failing

  1) GeneralSettings
       changes defaultSortOrder value when selecting an option:
     TestingLibraryElementError: Value "_id: 1 (in ascending order by creation)" not found in options

I'm pretty sure selectOptions works only with browser select and you'll have to "manually" click around things to select something in a leafygreen component. Does it work for you?

expect(getSettings()).to.have.property('defaultSortOrder', '{ _id: 1 }');
});

['maxTimeMS'].forEach((option) => {
it(`renders ${option}`, function () {
expect(within(container).getByTestId(option)).to.exist;
Expand Down
1 change: 1 addition & 0 deletions packages/compass-settings/src/components/settings/general.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const generalFields = [
'readOnly',
'enableShell',
'protectConnectionStrings',
'defaultSortOrder',
'showKerberosPasswordField',
'maxTimeMS',
'enableDevTools',
Expand Down
69 changes: 63 additions & 6 deletions packages/compass-settings/src/components/settings/settings-list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
getSettingDescription,
featureFlags,
} from 'compass-preferences-model/provider';
import { SORT_ORDER_VALUES } from 'compass-preferences-model/provider';
import { settingStateLabels } from './state-labels';
import {
Checkbox,
Expand All @@ -12,6 +13,8 @@ import {
css,
spacing,
TextInput,
Select,
Option,
FormFieldContainer,
Badge,
} from '@mongodb-js/compass-components';
Expand Down Expand Up @@ -157,6 +160,55 @@ function NumericSetting<PreferenceName extends NumericPreferences>({
);
}

function DefaultSortOrderSetting<PreferenceName extends 'defaultSortOrder'>({
name,
onChange,
value,
disabled,
}: {
name: PreferenceName;
onChange: HandleChange<PreferenceName>;
value: string | undefined;
Copy link
Collaborator

@gribnoysup gribnoysup Feb 3, 2025

Choose a reason for hiding this comment

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

I don't think undefined is ever possible here and you're getting a compilation error that's related to that: leafygreen Select tries to do some infers to smartly detect uncontrolled component and shows some optional props as required (you can see those errors in CI):

@mongodb-js/compass-settings: src/components/settings/settings-list.tsx(185,8): error TS2322: Type '{ children: Element[]; className: string; allowDeselect: false; "aria-labelledby": string; id: PreferenceName; name: PreferenceName; "data-testid": PreferenceName; value: string | undefined; onChange: (value: string) => void; disabled: boolean; }' is not assignable to type 'IntrinsicAttributes & ((Omit<BaseSelectProps & Omit<LabelProp, "aria-label" | "aria-labelledby" | "label"> & Required<...> & Partial<...> & { ...; } & { ...; }, "ref"> | ... 7 more ... | Omit<...>) & RefAttributes<...>)'.
@mongodb-js/compass-settings:   Type '{ children: Element[]; className: string; allowDeselect: false; "aria-labelledby": string; id: PreferenceName; name: PreferenceName; "data-testid": PreferenceName; value: string | undefined; onChange: (value: string) => void; disabled: boolean; }' is missing the following properties from type 'Omit<BaseSelectProps & Omit<LabelProp, "aria-label" | "aria-labelledby" | "label"> & Required<Pick<LabelProp, "label">> & Partial<...> & { ...; } & { ...; }, "ref">': label, readOnly

Generally speaking controlled react component should always get the value, so this error is legit, it just shows itself in a weird way at the moment. If somehow we expect the value here to not always be provided it needs to be handled when component is rendered

Copy link
Contributor Author

@djechlin-mongodb djechlin-mongodb Feb 3, 2025

Choose a reason for hiding this comment

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

fixed.

disabled: boolean;
}) {
const optionDescriptions = getSettingDescription(name).description.options;
const onChangeCallback = useCallback(
(value: string) => {
onChange(name, value as UserConfigurablePreferences[PreferenceName]);
},
[name, onChange]
);

return (
<>
<SettingLabel name={name} />
<Select
className={inputStyles}
allowDeselect={false}
aria-labelledby={`${name}-label`}
id={name}
name={name}
data-testid={name}
value={value}
onChange={onChangeCallback}
disabled={disabled}
>
{SORT_ORDER_VALUES.map((option) => (
<Option
key={option}
value={option}
description={
optionDescriptions && optionDescriptions[option].description
}
>
{optionDescriptions && optionDescriptions[option].label}
</Option>
))}
</Select>
</>
);
}

function StringSetting<PreferenceName extends StringPreferences>({
name,
onChange,
Expand Down Expand Up @@ -263,9 +315,16 @@ function SettingsInput({
disabled={!!disabled}
/>
);
}

if (type === 'number') {
} else if (type === 'string' && name === 'defaultSortOrder') {
input = (
<DefaultSortOrderSetting
name={name}
onChange={onChange}
value={value as string}
disabled={!!disabled}
/>
);
} else if (type === 'number') {
input = (
<NumericSetting
name={name}
Expand All @@ -275,9 +334,7 @@ function SettingsInput({
disabled={!!disabled}
/>
);
}

if (type === 'string') {
} else if (type === 'string') {
input = (
<StringSetting
name={name}
Expand Down
Loading