Only trigger changes for tables, cards and calendars if necessary (otherwise triggers auto-save)

[leon-marzahn]

Scope

What's changed:

• The query limit in a table/cards view is only updated if the QUERY_LIMIT_MAX would actually change the query limit on said table
• In a calendar view, only subscribe to datesSet after rendering, skipping the initial value (as the initial value is the same before and after rendering)
• Don't create/save preset on page change Reverted by bea5cf2, as per discussion in https://github.com/directus/directus/pull/20106/files#r1374908744
• Fixes unintended behaviour where a preset is saved even when no values were changed

Potential Risks / Drawbacks

• There shouldn't be any for table/cards views, but welcome to a sanity check
• I'm unsure about the calendar behaviour and the swap of render and subscription might cause unintended effects

Questions

• I wasn't able to investigate whether kanban and map views have similar behaviour, so might be worth looking into in the future

---

Fixes #20105

[changeset-bot]

🦋 Changeset detected

Latest commit: bea5cf2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@directus/app	Patch
@directus/api	Patch
directus	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[leon-marzahn]

JFYI i have no prior experience with vue, so please let me know if i did something wrong :)

[paescuj]

Great findings @leon-marzahn ❤️ LGTM 🚀

[leon-marzahn]

@paescuj Thanks ❤️

[br41nslug]

At least for the table layout this does not seem to be a complete fix for the original issue.

firefox_bz2RUikqto.mp4

[leon-marzahn]

At least for the table layout this does not seem to be a complete fix for the original issue.

firefox_bz2RUikqto.mp4

It might be related to the fact there weren't any presets beforehand, will investigate now!

[paescuj]

At least for the table layout this does not seem to be a complete fix for the original issue.
firefox_bz2RUikqto.mp4

Interesting catch! This happens when you switch between collections and the page in layoutQuery changes.
We could (and maybe should) filter that out, but that feels out of scope for this issue / PR since the original concern was about unnecessary creation of additional presets even though a preset already existed.

[br41nslug]

Interesting will do some further testing then 😄

[paescuj]

Interesting will do some further testing then 😄

Cool ❤️ Here's the corresponding line:

directus/app/src/composables/use-preset.ts

Line 121 in c80f8ee

updatePreset({ layout_query: assign({}, layout_query, { [layout.value]: query }) });

I wouldn't consider this as a blocker for this PR though & prefer to track this in a separate issue.

[leon-marzahn]

@paescuj @br41nslug This is a very ugly solution, but something like this would work:

function updatePreset(preset: Partial<Preset>, immediate?: boolean) {
	localPreset.value = assign({}, localPreset.value, preset);

	const hasChanged = Object.keys(preset.layout_query ?? {}).some(key => {
		const query = (preset.layout_query ?? {})[key];
		return !(Object.keys(query).length === 1 && 'page' in query);
	});

	if (!hasChanged) return;

	immediate ? savePreset() : handleChanges();
}

It just checks if the only change is the page in the layout_query and only saves the preset/bookmark if something else changed. I can either create a new issue and pr or do it in this one, whichever is preferred :)

[paescuj]

[...] I wonder if it makes more sense to just set the page to 1 if it isn't 1 already?
But even then, if you were on a different page it would save a preset even though you have just moved to a different page, that seems unintended?

This is why I prefer to create a separate issue for this, so we can discuss such things there 😄

[leon-marzahn]

[...] I wonder if it makes more sense to just set the page to 1 if it isn't 1 already?
But even then, if you were on a different page it would save a preset even though you have just moved to a different page, that seems unintended?

This is why I prefer to create a separate issue for this, so we can discuss such things there 😄

I'll create one now

[paescuj]

@br41nslug LGTY?

[leon-marzahn]

@paescuj Here is the issue with PR attached #20202

[br41nslug]

This is why I prefer to create a separate issue for this, so we can discuss such things there 😄

Why would we split this up? In my current tests the current solution does not completely solve the issue of "Don't create new presets for users when they have made no changes" 🤔 Independently of wheter there are already presets defined or not.

I wouldn't consider this as a blocker for this PR though & prefer to track this in a separate issue.

I disagree, in my opinion this is exactly what the issue is about "creating new presets without changes" 🤔 Although i could be wrong, we're missing actual "reproduction steps" on the issue, perhaps we should add some more detail to the original image to prevent this confusion.

[leon-marzahn]

This is why I prefer to create a separate issue for this, so we can discuss such things there 😄

Why would we split this up? In my current tests the current solution does not completely solve the issue of "Don't create new presets for users when they have made no changes" 🤔 Independently of wheter there are already presets defined or not.

I wouldn't consider this as a blocker for this PR though & prefer to track this in a separate issue.

I disagree, in my opinion this is exactly what the issue is about "creating new presets without changes" 🤔 Although i could be wrong, we're missing actual "reproduction steps" on the issue, perhaps we should add some more detail to the original image to prevent this confusion.

If we rephrase the issue to exactly this and add reproduction steps, would that work for you? The only issue is that this PR (and the other one) might not cover every example and we would have to keep it open until all cases are fixed

[br41nslug]

might not cover every example and we would have to keep it open until all cases are fixed

We can never be sure to have covered all the possible cases but lets at least fix the 2 situations we know about.

[leon-marzahn]

might not cover every example and we would have to keep it open until all cases are fixed

We can never be sure to have covered all the possible cases but lets at least fix the 2 situations we know about.

Okay, I will make changes to this issue and pr 👍🏻

[br41nslug]

Or lets organize that in the second PR you've created as that already looks like it includes the changes from this PR and the extra fix for switching collections.

[leon-marzahn]

Or lets organize that in the second PR you've created as that already looks like it includes the changes from this PR and the extra fix for switching collections.

Oops, already deleted the other one 😅

PR has been updated

[leon-marzahn]

@br41nslug I've updated the issue to include a reproduction and the latest changes are in this branch, how does it look?

[br41nslug]

Seems the situation i ran into is indeed resolved, will have a look at the code and test other layouts tomorrow

[br41nslug]

The used fix for preset changes was a bit too effective 😬 It prevented saving layout changes and options altogether

outdated

[leon-marzahn]

Thanks @azrikahar! I was too busy with work 😅

[azrikahar]

Thanks @azrikahar! I was too busy with work 😅

No worries! The rest of the changes LGTM 😊