Skip to content

Commit ebf6f9b

Browse files
committed
refactor(#48): address code review findings
- super-table.vue: wire filterValidColumnDisplays to drop overrides whose root field has been deleted from the collection (mirrors the existing filterValidFields / filterValidSort guards). - super-table.vue + options.vue: drop the window-event refresh path. The watch on layoutOptions.columnDisplays with flush: 'post' is sufficient for the prop round-trip from the sidebar slot, removing ~25 LOC of duplicated refresh wiring and a 250ms timer. - index.ts: dedupe availableFieldChoices by root key so a column shown in two languages collapses to one picker entry. - tests/unit/utils/adjustFieldsForDisplays.test.ts: top-level beforeEach(vi.resetModules) so the module-level store cache cannot leak between tests.
1 parent a5cf4f8 commit ebf6f9b

4 files changed

Lines changed: 30 additions & 53 deletions

File tree

index.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,18 +27,20 @@ const layoutConfig = {
2727
// components (options sidebar). Each entry uses the *root* field key as id
2828
// (translations.title rather than translations.title:de-DE) so all
2929
// language variants of the same root field share a single override entry.
30+
// Multiple language columns of the same root collapse into one picker entry.
3031
const availableFieldChoices = computed(() => {
3132
const layoutFields: string[] = props.layoutQuery?.fields ?? [];
3233
const customFieldNames: Record<string, string> = props.layoutOptions?.customFieldNames ?? {};
33-
return layoutFields.map((key: string) => {
34-
// Storage key: strip language suffix so translations roll up to a
35-
// single override entry per root field.
34+
const seen = new Map<string, { key: string; label: string }>();
35+
for (const key of layoutFields) {
3636
const rootKey = key.includes(':') ? key.split(':')[0] : key;
37+
if (seen.has(rootKey)) continue;
3738
const root = rootKey.split('.')[0];
3839
const fieldData = fieldsStore.getField(props.collection, root);
3940
const fallbackName = fieldData?.name ?? formatTitle(rootKey);
40-
return { key: rootKey, label: customFieldNames[key] ?? fallbackName };
41-
});
41+
seen.set(rootKey, { key: rootKey, label: customFieldNames[key] ?? fallbackName });
42+
}
43+
return Array.from(seen.values());
4244
});
4345

4446
return {

src/options.vue

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -83,21 +83,9 @@ const {
8383
8484
function onSet(payload: { fieldKey: string; display: ColumnDisplay }) {
8585
setOverride(payload.fieldKey, payload.display);
86-
scheduleTableRefresh();
8786
}
8887
function onRemove(fieldKey: string) {
8988
removeOverride(fieldKey);
90-
scheduleTableRefresh();
91-
}
92-
93-
// After mutating columnDisplays we need the table to refetch with the new
94-
// override-driven deep paths. Reactivity propagates through the parent layout
95-
// asynchronously, so we dispatch a window event after the next tick — super-table.vue
96-
// listens for it and calls getItems() with fresh aliasedFields.
97-
function scheduleTableRefresh() {
98-
setTimeout(() => {
99-
window.dispatchEvent(new CustomEvent('super-layout-table:refresh'));
100-
}, 0);
10189
}
10290
10391
// Issue #48: read setters from `props.layoutOptions` instead of the synced ref.

src/super-table.vue

Lines changed: 18 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -273,24 +273,14 @@
273273
</template>
274274

275275
<script lang="ts" setup>
276-
import {
277-
ref,
278-
computed,
279-
toRefs,
280-
watch,
281-
unref,
282-
nextTick,
283-
onMounted,
284-
onUnmounted,
285-
type Ref,
286-
} from 'vue';
276+
import { ref, computed, toRefs, watch, unref, onMounted, onUnmounted, type Ref } from 'vue';
287277
import { useI18n } from 'vue-i18n';
288278
import { useRouter } from 'vue-router';
289279
import { debounce } from 'lodash';
290280
import { useStores, useCollection, useSync, useApi } from '@directus/extensions-sdk';
291281
import { formatTitle } from '@directus/format-title';
292282
import { getDefaultDisplayForType } from './utils/getDefaultDisplayForType';
293-
import { filterValidFields } from './utils/fieldValidity';
283+
import { filterValidFields, filterValidColumnDisplays } from './utils/fieldValidity';
294284
import { useTableApi } from './composables/api';
295285
import { useAliasFields } from './composables/useAliasFields';
296286
import { useLanguageSelector } from './composables/useLanguageSelector';
@@ -485,8 +475,16 @@ const fieldsForAliasing = computed(() => {
485475
// Use alias fields for proper relational data handling.
486476
// Issue #48: forward the columnDisplays override map so the API query expands
487477
// override template paths (e.g. `{{ user.first_name }}`) into deep field
488-
// requests via `adjustFieldsForDisplays`.
489-
const columnDisplaysRef = computed(() => (layoutOptions.value as any)?.columnDisplays ?? {});
478+
// requests via `adjustFieldsForDisplays`. Drop entries that point at fields
479+
// the user has since deleted from the collection (mirrors filterValidFields).
480+
type ColumnDisplayShape = { template: string; display?: string };
481+
const columnDisplaysRef = computed(() =>
482+
filterValidColumnDisplays<ColumnDisplayShape>(
483+
(layoutOptions.value as any)?.columnDisplays,
484+
collection.value,
485+
fieldsStore
486+
)
487+
);
490488
491489
const { aliasedFields, aliasQuery, getFromAliasedItem } = useAliasFields(
492490
fieldsForAliasing,
@@ -941,32 +939,18 @@ async function handleQuickFilterSaved(event: any) {
941939
}
942940
}
943941
944-
// Issue #48: when columnDisplays change locally, our own reactive chain handles
945-
// the rebuild AND the watch on fieldsWithRelational fires. But when the change
946-
// originates in options.vue (a sibling slot), the prop round-trip through the
947-
// parent layout takes a few ticks. We watch our own layoutOptions for changes
948-
// to the columnDisplays map and force a refetch after nextTick so aliasedFields
949-
// has fully settled.
942+
// Issue #48: when columnDisplays change in options.vue (sibling slot), the prop
943+
// round-trip through the parent layout takes a few ticks. flush: 'post' makes
944+
// the watcher run after the DOM update so aliasedFields has fully settled
945+
// before we refetch.
950946
watch(
951947
() => (layoutOptions.value as any)?.columnDisplays,
952-
async () => {
953-
await nextTick();
948+
() => {
954949
getItems();
955950
},
956-
{ deep: true }
951+
{ deep: true, flush: 'post' }
957952
);
958953
959-
// Window-event fallback: options.vue dispatches this after setOverride/removeOverride.
960-
// Helps in cases where the prop sync hasn't propagated by the time our local
961-
// watch (above) would have fired. Both paths converge on the same getItems call;
962-
// directus' API layer dedupes in-flight requests so this isn't a double-fetch.
963-
function handleColumnDisplaysChanged() {
964-
// 250ms ≈ enough for the options.vue → parent layout → super-table.vue prop
965-
// round-trip to land. Microtask-precise nextTick is too eager here because
966-
// the round-trip spans a full event-loop turn (parent re-render).
967-
setTimeout(() => getItems(), 250);
968-
}
969-
970954
// Setup event listeners on mount
971955
onMounted(() => {
972956
// Load presets from layoutOptions (no localStorage needed)
@@ -977,12 +961,10 @@ onMounted(() => {
977961
978962
// Listen for save events from actions
979963
window.addEventListener('quick-filter-saved', handleQuickFilterSaved);
980-
window.addEventListener('super-layout-table:refresh', handleColumnDisplaysChanged);
981964
});
982965
983966
onUnmounted(() => {
984967
window.removeEventListener('quick-filter-saved', handleQuickFilterSaved);
985-
window.removeEventListener('super-layout-table:refresh', handleColumnDisplaysChanged);
986968
});
987969
988970
// Update manual filters when props.filter changes (from native filter interface)

tests/unit/utils/adjustFieldsForDisplays.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ import { vi } from 'vitest';
44
// Note: adjustFieldsForDisplays uses useStores() at runtime which we mock in
55
// tests/setup.ts. With those mocks, calling it with no overrides should return
66
// the original fields unchanged when the store has no field metadata.
7+
// Reset modules before every test so the module-level store cache in
8+
// adjustFieldsForDisplays.ts cannot leak between tests.
9+
beforeEach(() => {
10+
vi.resetModules();
11+
});
712

813
describe('adjustFieldsForDisplays', () => {
914
it('accepts overrides as an optional third parameter', async () => {

0 commit comments

Comments
 (0)