Skip to content

Fix grid caching #5200

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 4 commits into from
Dec 2, 2024
Merged

Fix grid caching #5200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af3ef35
clear stale lookers
benjaminpkane Nov 26, 2024
4075355
simplify spotlight render state, filter duplicated ids for linked rev…
benjaminpkane Nov 26, 2024
949fc2c
lint render
benjaminpkane Dec 2, 2024
f6faba4
rm noop
benjaminpkane Dec 2, 2024
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
8 changes: 3 additions & 5 deletions app/packages/core/src/components/Grid/Grid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,9 @@ function Grid() {
retainItems: true,
rowAspectRatioThreshold: threshold,
get: (next) => page(next),
render: (id, element, dimensions, soft, hide) => {
render: (id, element, dimensions, zooming) => {
if (lookerStore.has(id.description)) {
const looker = lookerStore.get(id.description);
hide ? looker?.disable() : looker?.attach(element, dimensions);

lookerStore.get(id.description)?.attach(element, dimensions);
return;
}

Expand All @@ -84,7 +82,7 @@ function Grid() {
throw new Error("bad data");
}

if (soft) {
if (zooming) {
// we are scrolling fast, skip creation
return;
}
Expand Down
1 change: 0 additions & 1 deletion app/packages/core/src/components/Grid/useRefreshers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { useRecoilValue } from "recoil";
import { gridAt, gridOffset, gridPage } from "./recoil";

const MAX_LRU_CACHE_ITEMS = 510;
const MAX_LRU_CACHE_SIZE = 1e9;

export default function useRefreshers() {
const cropToContent = useRecoilValue(fos.cropToContent(false));
Expand Down
14 changes: 13 additions & 1 deletion app/packages/core/src/components/Grid/useSelect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,25 @@ export default function useSelect(
useEffect(() => {
deferred(() => {
const fontSize = getFontSize();
const retained = new Set<string>();
spotlight?.updateItems((id) => {
store.get(id.description)?.updateOptions({
const instance = store.get(id.description);
if (!instance) {
return;
}

retained.add(id.description);
instance.updateOptions({
...options,
fontSize,
selected: selected.has(id.description),
});
});

for (const id of store.keys()) {
if (retained.has(id)) continue;
store.delete(id);
}
});
}, [deferred, getFontSize, options, selected, spotlight, store]);

Expand Down
12 changes: 2 additions & 10 deletions app/packages/spotlight/src/row.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,18 +147,10 @@ export default class Row<K, V> {

show(
element: HTMLDivElement,
hidden: boolean,
attr: typeof BOTTOM | typeof TOP,
soft: boolean,
zooming: boolean,
config: SpotlightConfig<K, V>
): void {
if (hidden !== this.#hidden) {
hidden
? this.#container.classList.add(styles.spotlightRowHidden)
: this.#container.classList.remove(styles.spotlightRowHidden);
this.#hidden = hidden;
}

if (!this.attached) {
this.#container.style[attr] = `${this.#from}px`;
this.#container.style[attr === BOTTOM ? TOP : BOTTOM] = UNSET;
Expand All @@ -171,7 +163,7 @@ export default class Row<K, V> {

for (const { element, item } of this.#row) {
const width = item.aspectRatio * this.height;
config.render(item.id, element, [width, this.height], soft, hidden);
config.render(item.id, element, [width, this.height], zooming);
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

SpotlightConfig interface needs zooming parameter in render method

The SpotlightConfig interface in app/packages/spotlight/src/types.ts needs to be updated to include the zooming parameter in its render method signature, as it's being used in row.ts.

  • app/packages/spotlight/src/types.ts: Add zooming parameter to the render method definition in SpotlightConfig interface
🔗 Analysis chain

Verify render method compatibility

The render method now uses the zooming parameter. Ensure all implementations of SpotlightConfig.render handle this parameter correctly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all render method implementations to verify they handle the zooming parameter
ast-grep --pattern 'render($_, $_, $_, $_) {
  $$$
}'

Length of output: 55

Script:

#!/bin/bash
# Search for render method implementations in TypeScript/JavaScript files
rg "render\s*\([^)]*\)" -t ts -t js -A 5

# Also search for SpotlightConfig interface/type definition
rg "SpotlightConfig" -t ts -t js -A 5

Length of output: 16575

}
}

Expand Down
19 changes: 9 additions & 10 deletions app/packages/spotlight/src/section.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ export default class Section<K, V> {
readonly #width: number;

#direction: DIRECTION;
#dirty: Set<Row<K, V>> = new Set();
#end: Edge<K, V>;
#itemIds = new Set<string>();
#nextMap: WeakMap<ID, ID> = new WeakMap();
#previousMap: WeakMap<ID, ID> = new WeakMap();
#shown: Set<Row<K, V>> = new Set();
Expand Down Expand Up @@ -129,7 +129,6 @@ export default class Section<K, V> {
target,
threshold,
top,
updater,
zooming,
}: {
config: SpotlightConfig<K, V>;
Expand Down Expand Up @@ -181,14 +180,8 @@ export default class Section<K, V> {
break;
}

if (this.#dirty.has(row) && !zooming && updater) {
row.updateItems(updater);
this.#dirty.delete(row);
}

row.show(
this.#container,
this.#dirty.has(row) && zooming,
this.#direction === DIRECTION.FORWARD ? TOP : BOTTOM,
zooming,
config
Expand Down Expand Up @@ -225,7 +218,6 @@ export default class Section<K, V> {

updateItems(updater: (id: ID) => void) {
for (const row of this.#shown) row.updateItems(updater);
for (const row of this.#rows) !this.#shown.has(row) && this.#dirty.add(row);
}

async first(
Expand Down Expand Up @@ -308,7 +300,9 @@ export default class Section<K, V> {

renderer(() => {
const { rows, remainder } = this.#tile(
[...end.remainder, ...data.items],
[...end.remainder, ...data.items].filter(
(i) => !this.#itemIds.has(i.id.description)
),
this.#height,
data.next === null,
data.focus,
Expand Down Expand Up @@ -350,6 +344,7 @@ export default class Section<K, V> {
edge: newEnd,
width: this.#width,
});

this.#end = this.#start;
this.#start = newEnd;

Expand Down Expand Up @@ -452,6 +447,10 @@ export default class Section<K, V> {
rowItems.reverse();
}

for (const i of rowItems) {
this.#itemIds.add(i.id.description);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

should these be getting removed when the row is destroyed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, rows are only deleted/destroyed when the section is deleted/destroyed. Will leave a comment

const row = new Row({
config: this.#config,
dangle:
Expand Down
3 changes: 1 addition & 2 deletions app/packages/spotlight/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ export type Render = (
id: ID,
element: HTMLDivElement,
dimensions: [number, number],
soft: boolean,
disable: boolean
zooming: boolean
) => void;

export interface Response<K, V> {
Expand Down
Loading