From 529d54680c02d92344f332820e68c633f666d461 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 01:19:31 +0000 Subject: [PATCH] fix(plugin-gantt): guard `reload()`'s `finally` so a superseded run cannot clear the loading state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `reload()` sequences concurrent runs with `reloadSeqRef` and guards every result write with `isCurrent()`, but its `finally` carried no guard. A superseded reload therefore still flipped `loading` / `refreshing` off: the stale run only had to finish first — the ordinary case whenever a second reload is issued while the first is still in flight — and the placeholder was released before any rows had arrived, painting an empty chart until the fresh response landed. The `finally` now clears the flags only when the run reaching it is still current, and clears BOTH of them rather than only the one its own `silent` mode set: being current at that point means nothing is in flight any more, so clearing only its own mode would strand the other flag whenever the superseded run used the other mode (a silent toolbar refresh overtaken by a filter-change reload would have left `refreshing` on for the life of the component). New test file pins three orderings: the stale run finishing first (the defect — red on the unguarded tree), the fresh run finishing first (control — green either way, covering the pre-existing `setData` guard), and a silent run superseded by a non-silent one (pins the clear-both shape). Reload-guard only: which queries are issued, how they are projected and how they page are all untouched. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01NRRumy89BYdW9ogbcdHTho --- .changeset/7231-gantt-stale-reload-finally.md | 22 ++ ...jectGantt.staleReloadFinally-7231.test.tsx | 231 ++++++++++++++++++ packages/plugin-gantt/src/ObjectGantt.tsx | 20 +- 3 files changed, 271 insertions(+), 2 deletions(-) create mode 100644 .changeset/7231-gantt-stale-reload-finally.md create mode 100644 packages/plugin-gantt/src/ObjectGantt.staleReloadFinally-7231.test.tsx diff --git a/.changeset/7231-gantt-stale-reload-finally.md b/.changeset/7231-gantt-stale-reload-finally.md new file mode 100644 index 0000000000..e1221f261d --- /dev/null +++ b/.changeset/7231-gantt-stale-reload-finally.md @@ -0,0 +1,22 @@ +--- +'@object-ui/plugin-gantt': patch +--- + +`ObjectGantt` no longer blanks the chart when one reload supersedes another + +`reload()` already sequenced concurrent runs with `reloadSeqRef` and guarded every +result write with `isCurrent()`, but its `finally` was unguarded — so a **superseded** +reload still flipped `loading` / `refreshing` off. The stale run only had to finish +first, which is the ordinary case whenever a second reload is issued while the first +is still in flight: the placeholder was released, no rows had arrived, and the user +saw an empty chart until the fresh response landed. + +The `finally` now clears the flags only when the run reaching it is still the current +one. It clears **both** flags rather than only the one its own `silent` mode set: +being current at that point means nothing is in flight any more, so clearing only its +own mode would strand the other flag whenever the superseded run used the other mode +— a silent toolbar refresh overtaken by a filter-change reload would have left the +refresh button busy for the life of the component. + +This is the reload guard alone. Nothing about which queries are issued, how they are +projected or how they page changes. diff --git a/packages/plugin-gantt/src/ObjectGantt.staleReloadFinally-7231.test.tsx b/packages/plugin-gantt/src/ObjectGantt.staleReloadFinally-7231.test.tsx new file mode 100644 index 0000000000..9fc250af4e --- /dev/null +++ b/packages/plugin-gantt/src/ObjectGantt.staleReloadFinally-7231.test.tsx @@ -0,0 +1,231 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#7231 — `reload()`'s `finally` must belong to the CURRENT reload. + * + * `reload()` sequences concurrent runs with `reloadSeqRef` and guards every + * result write with `isCurrent()` (`setData` on three branches, `setError` on + * the error branch). The `finally` used to carry no guard, so a SUPERSEDED + * reload still flipped `loading` / `refreshing` off — clearing the loading + * placeholder while the fresh query was still in flight. The user saw an + * empty chart: placeholder gone, no rows arrived yet. + * + * Note which ordering produces it: NOT an exotic out-of-order response, but + * the plain in-issue-order one. The stale reload merely has to FINISH FIRST, + * which is the ordinary case whenever a second reload is issued while the + * first is still in flight. The out-of-order case (fresh finishes first) is + * the one the pre-existing `setData` guard already covered, and it is kept + * below as the control. + * + * The guard shape matters, hence the third case. The flags are per-MODE + * (`silent` → `refreshing`, otherwise `loading`), so a `finally` that clears + * only its own mode's flag when current leaks the other one: a silent reload + * superseded by a non-silent one would never clear `refreshing`, leaving the + * toolbar's refresh button stuck busy for the life of the component. What + * makes clearing BOTH correct is that "I am current AND I am finishing" + * means nothing is in flight any more — a newer reload would have made this + * one stale, and an older one has no claim on the flags. + * + * Scope: this is the reload guard only. The overlapping-reload pairs it + * covers include the toolbar refresh and the write-readback paths, where two + * reloads legitimately overlap and no schema gating is involved — see the + * card for why this must not be folded into the gating work. + */ + +import React from 'react'; +import { render, screen, waitFor, fireEvent, act } from '@testing-library/react'; +import { describe, it, expect, vi } from 'vitest'; +import { ObjectGantt } from './ObjectGantt'; +import type { DataSource } from '@object-ui/types'; + +// Probe stand-in: the real chart is irrelevant here, but `refreshing` is not — +// case 3 reads it back off the DOM. +vi.mock('./GanttView', () => ({ + GanttView: ({ tasks, onRefresh, refreshing }: any) => ( +
+ {tasks.map((t: any) => ( +
{t.title}
+ ))} + +
+ ), +})); + +const PLACEHOLDER = 'Loading Gantt chart...'; + +const ROWS_A = [ + { id: '1', name: 'From the stale query', start_date: '2024-01-01', end_date: '2024-01-05' }, +]; +const ROWS_B = [ + { id: '2', name: 'From the fresh query', start_date: '2024-02-01', end_date: '2024-02-05' }, +]; + +const OBJECT_SCHEMA = { + fields: { + name: { type: 'text' }, + start_date: { type: 'date' }, + end_date: { type: 'date' }, + }, +}; + +const GANTT_CONFIG = { + titleField: 'name', + startDateField: 'start_date', + endDateField: 'end_date', +}; + +function schemaWith(filter?: unknown): any { + return { + type: 'gantt', + gantt: GANTT_CONFIG, + data: { provider: 'object', object: 'tasks' }, + ...(filter === undefined ? {} : { filter }), + }; +} + +interface Deferred { + promise: Promise; + resolve: (value: T) => void; +} + +function deferred(): Deferred { + let resolve!: (value: T) => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + +/** + * A data source whose every `find()` hands back a promise the test resolves + * by hand, so reload N and reload N+1 can be held in flight together and + * completed in either order. `getObjectSchema` resolves immediately — that is + * what issues the second reload (`objectSchema` is a `reload` dependency) + * while the first `find()` is still pending. + */ +function makeDeferredDataSource() { + const finds: Deferred[] = []; + const dataSource = { + find: vi.fn(() => { + const d = deferred(); + finds.push(d); + return d.promise; + }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn().mockResolvedValue({}), + delete: vi.fn(), + getObjectSchema: vi.fn().mockResolvedValue(OBJECT_SCHEMA), + } as unknown as DataSource; + return { dataSource, finds }; +} + +/** Settle one held `find()` and let React flush the resulting commits. */ +async function settle(d: Deferred, rows: unknown[]) { + await act(async () => { + d.resolve({ data: rows }); + await Promise.resolve(); + }); +} + +/** Let pending microtasks/effects run without resolving anything. */ +async function flush() { + await act(async () => { + await Promise.resolve(); + }); +} + +describe('ObjectGantt — a superseded reload must not clear the loading state (objectui#7231)', () => { + it('keeps the placeholder up when the STALE reload finishes first and the fresh one is still in flight', async () => { + const { dataSource, finds } = makeDeferredDataSource(); + + render(); + + // Reload #1 (mount) is in flight; the object schema resolves and re-keys + // `reload`, issuing reload #2 before #1 has answered. + await waitFor(() => expect((dataSource.find as any).mock.calls.length).toBe(2)); + expect(screen.getByText(PLACEHOLDER)).toBeTruthy(); + + // The superseded reload #1 answers first — the ordinary ordering. + await settle(finds[0], ROWS_A); + + // Its `finally` must NOT clear `loading`: the fresh query has not answered, + // so releasing the placeholder here paints an empty chart. + expect(screen.getByText(PLACEHOLDER)).toBeTruthy(); + expect(screen.queryByTestId('gantt-view')).toBeNull(); + + // The current reload #2 answers and owns the transition out of loading. + await settle(finds[1], ROWS_B); + + await waitFor(() => expect(screen.getByTestId('gantt-view')).toBeTruthy()); + expect(screen.getByText('From the fresh query')).toBeTruthy(); + expect(screen.queryByText('From the stale query')).toBeNull(); + }); + + it('control — the fresh reload finishing FIRST paints its rows, and the late stale answer changes nothing', async () => { + const { dataSource, finds } = makeDeferredDataSource(); + + render(); + + await waitFor(() => expect((dataSource.find as any).mock.calls.length).toBe(2)); + + // Out-of-order: the current reload #2 answers before the superseded #1. + await settle(finds[1], ROWS_B); + + await waitFor(() => expect(screen.getByTestId('gantt-view')).toBeTruthy()); + expect(screen.getByText('From the fresh query')).toBeTruthy(); + + // The late stale answer must neither clobber the data (the pre-existing + // `setData` guard) nor put the placeholder back. + await settle(finds[0], ROWS_A); + await flush(); + + expect(screen.getByTestId('gantt-view')).toBeTruthy(); + expect(screen.getByText('From the fresh query')).toBeTruthy(); + expect(screen.queryByText('From the stale query')).toBeNull(); + expect(screen.queryByText(PLACEHOLDER)).toBeNull(); + }); + + it('does not strand `refreshing` when a SILENT reload is superseded by a non-silent one', async () => { + const { dataSource, finds } = makeDeferredDataSource(); + + const { rerender } = render(); + + await waitFor(() => expect((dataSource.find as any).mock.calls.length).toBe(2)); + await settle(finds[0], ROWS_A); + await settle(finds[1], ROWS_A); + await waitFor(() => expect(screen.getByTestId('gantt-view')).toBeTruthy()); + expect(screen.getByTestId('gantt-view').getAttribute('data-refreshing')).toBe('false'); + + // Toolbar refresh → reload #3, silent: it owns `refreshing`, not `loading`. + fireEvent.click(screen.getByTestId('gv-refresh')); + await waitFor(() => + expect(screen.getByTestId('gantt-view').getAttribute('data-refreshing')).toBe('true'), + ); + + // A filter change re-keys `reload` → reload #4, non-silent, superseding the + // silent one while it is still in flight. Different flag, same sequence. + rerender(); + await waitFor(() => expect((dataSource.find as any).mock.calls.length).toBe(4)); + expect(screen.getByText(PLACEHOLDER)).toBeTruthy(); + + // The superseded silent reload answers: it must touch neither flag. + await settle(finds[2], ROWS_A); + expect(screen.getByText(PLACEHOLDER)).toBeTruthy(); + + // The current reload answers. Nothing is in flight any more, so BOTH flags + // must be honest — a guard that only cleared `loading` here would leave the + // refresh button spinning forever. + await settle(finds[3], ROWS_B); + + await waitFor(() => expect(screen.getByTestId('gantt-view')).toBeTruthy()); + expect(screen.getByTestId('gantt-view').getAttribute('data-refreshing')).toBe('false'); + expect(screen.getByText('From the fresh query')).toBeTruthy(); + }); +}); diff --git a/packages/plugin-gantt/src/ObjectGantt.tsx b/packages/plugin-gantt/src/ObjectGantt.tsx index dc20b2f032..c063fb29e9 100644 --- a/packages/plugin-gantt/src/ObjectGantt.tsx +++ b/packages/plugin-gantt/src/ObjectGantt.tsx @@ -686,8 +686,24 @@ export const ObjectGantt: React.FC = ({ setError(err as Error); } } finally { - if (silent) setRefreshing(false); - else setLoading(false); + // Only the NEWEST reload owns the loading flags, for the same reason + // the result writes above are guarded. An unguarded clear here let a + // SUPERSEDED reload release the placeholder while the fresh query was + // still in flight, so the chart painted empty in between + // (objectui#7231). + // + // The current reload clears BOTH flags, not just the one its own + // `silent` mode set: reaching this point as the current run means + // nothing is in flight any more — a newer reload would have made this + // one stale, and an older one has no claim on the flags. Clearing only + // this run's own mode would strand the other one whenever the + // superseded reload ran in the OTHER mode (a silent toolbar refresh + // overtaken by a filter-change reload would leave `refreshing` on for + // the life of the component). + if (isCurrent()) { + setRefreshing(false); + setLoading(false); + } } // eslint-disable-next-line react-hooks/exhaustive-deps -- (rest as any).data intentionally untracked, matching the original effect }, [effectiveDataSource, resource, hasInlineData, dataProvider, dataItems, schema.filter, schema.sort, objectSchema]);