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

Merge release/v1.1.0 to develop #5205

Merged
merged 15 commits into from
Dec 4, 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
3 changes: 2 additions & 1 deletion app/packages/core/src/components/Common/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,14 @@ export const getFormatter = (fieldType: string, timeZone: string, bounds) => {
);
}

return numeral(v).format(
const str = numeral(v).format(
[INT_FIELD, FRAME_NUMBER_FIELD, FRAME_SUPPORT_FIELD].includes(fieldType)
? "0a"
: bounds[1] - bounds[0] < 0.1
? "0.0000a"
: "0.00a"
);
return str === "NaN" ? v.toString() : str;
},
};
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,12 @@ const RangeSlider = ({
const one = useRecoilValue(state.oneBound({ path, modal }));
const timeZone = useRecoilValue(fos.timeZone);
const hasBounds = useRecoilValue(state.hasBounds({ path, modal }));
const nonfinitesText = useRecoilValue(state.nonfinitesText({ path, modal }));

if (!hasBounds) {
return <Box text="No results" />;
return (
<Box text={nonfinitesText ? `${nonfinitesText} present` : "No results"} />
);
}

const showSlider = hasBounds && !(excluded && defaultRange);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Nonfinite } from "@fiftyone/state";
import { boundsAtom, nonfiniteAtom, rangeAtom } from "@fiftyone/state";
import { boundsAtom, nonfiniteData, rangeAtom } from "@fiftyone/state";
import { selectorFamily } from "recoil";

export const FLOAT_NONFINITES: Nonfinite[] = ["inf", "ninf", "nan"];
Expand All @@ -25,14 +25,17 @@ export const hasDefaultRange = selectorFamily({
},
});

export const hasNonfinites = selectorFamily({
key: "hasNonfinites",
export const nonfinitesText = selectorFamily({
key: "nonfinitesText",
get:
(params: { path: string; modal: boolean }) =>
({ get }) => {
return FLOAT_NONFINITES.every((key) =>
get(nonfiniteAtom({ key, ...params }))
const data = get(nonfiniteData({ ...params, extended: false }));
const result = Object.entries(data).filter(
([k, v]) => k !== "none" && Boolean(v)
);

return result.length ? result.map(([key]) => key).join(", ") : null;
},
});

Expand Down
8 changes: 3 additions & 5 deletions app/packages/core/src/components/Grid/Grid.tsx
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ export default function TableView(props: ViewPropsType) {
background: isSelected
? selectedCellColor
: "unset",
fontSize: "1rem",
}}
onClick={() => {
handleCellClick(rowIndex, columnIndex);
Expand All @@ -220,6 +221,7 @@ export default function TableView(props: ViewPropsType) {
background: isRowSelected
? selectedCellColor
: "unset",
fontSize: "1rem",
}}
>
{currentRowHasActions ? (
Expand Down
12 changes: 2 additions & 10 deletions app/packages/spotlight/src/row.ts
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);
}
}

Expand Down
19 changes: 9 additions & 10 deletions app/packages/spotlight/src/section.ts
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>();
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure #itemIds is cleared upon destruction to prevent memory leaks

The #itemIds set is not cleared in the destroy method. Over time, this could lead to memory leaks if old item IDs are retained. Please consider clearing the #itemIds set when the section is destroyed.

Apply this diff to clear the #itemIds set in the destroy method:

  destroy(destroyItems = false) {
    this.#section.remove();
    for (const row of this.#rows) row.destroy(destroyItems);
    this.#rows = [];
+   this.#itemIds.clear();
  }

Committable suggestion skipped: line range outside the PR's diff.

#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);
}

Comment on lines +450 to +453
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add null/undefined check for item ID description

The code assumes i.id.description is always defined. Consider adding a null check to prevent potential runtime errors.

       for (const i of rowItems) {
-        this.#itemIds.add(i.id.description);
+        if (i.id?.description) {
+          this.#itemIds.add(i.id.description);
+        }
       }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

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

const row = new Row({
config: this.#config,
dangle:
Expand Down
3 changes: 1 addition & 2 deletions app/packages/spotlight/src/types.ts
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
Loading