Uh oh!
There was an error while loading. Please reload this page.
fix(plugin-gantt): guard reload()'s finally so a superseded run cannot clear the loading state - #7236
Merged
Conversation
…annot clear the loading state `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRRumy89BYdW9ogbcdHTho
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This was referenced Sep 2, 2026
This was referenced Sep 2, 2026
os-litant
marked this pull request as ready for review
September 2, 2026 01:46
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Sep 2, 2026
This was referenced Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#7231
The defect
ObjectGantt'sreload()sequences concurrent runs withreloadSeqRefand guards every result write withisCurrent()—setDataon all three branches,setErroron the error branch. Itsfinallycarried no guard, so a superseded reload still flippedloading/refreshingoff.Note which ordering produces the symptom: not an exotic out-of-order response, but the plain one. The stale run only has 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 before any rows arrived and the chart painted empty until the fresh response landed. (The out-of-order case — fresh finishes first — is what the pre-existing
setDataguard already covered; it is kept below as a control.)The caller measurement the card asked for
The card's "Not measured" paragraph was the first task. Enumerating
reload()'s call sites inpackages/plugin-gantt/src/ObjectGantt.tsx(line numbers on this branch)::712useEffect(() => { reload(); }, [reload])reload's identity — its deps at:707areeffectiveDataSource, resource, hasInlineData, dataProvider, dataItems, schema.filter, schema.sort, objectSchema. So this one site is the card's initial load, schema-resolution refire and view/filter change callers.:1440:1484:1585:1753onRefreshonGanttView):1820Which pairs can actually overlap:
:712re-fires against itself:objectSchemaresolving mid-flight re-keysreload, and so does a host changingschema.filter/schema.sort/ the bound object.loadingis the flag that gates the placeholder at:1596, so this is the pair that blanks the chart. Reproduced in the harness (case 1 below). It is not confined to the two-queries regime: the filter/sort/object route re-keysreloadwith no schema read involved.loading), but the stale run used to droprefreshingwhile the fresh read was still outstanding, so the toolbar'sdisabled/aria-busy(GanttView.tsx:3449,:3451) went honest-to-lying mid-flight and the button accepted another click.loading === false), and a filter/sort change or a lateobjectSchemacan re-keyreloadat any moment.if (loading) return placeholderearly return at:1596, so while a non-silent reload holdsloadingtrue there is no toolbar and no row handler mounted to fire one. Worth recording that the unguardedfinallywas itself the thing that opened this window: it releasedloadingearly and re-mounted the toolbar while the fresh query was still running.So the defect is live on three distinct pairs, only one of which passes through schema resolution — which is the card's scoping fence, measured rather than assumed.
The fix, and why it clears both flags
The obvious form — clear only the flag this run's own
silentmode set — is wrong on the third pair above. The superseded silent run correctly skips its clear; the current non-silent run clears onlyloading; nobody ever clearsrefreshing, and the refresh button stays busy for the life of the component. Clearing both is the honest form because 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. Case 3 pins that shape.Checked, per the card's warning, that the fresh run always reaches its own
finally:catchthenfinally, both modes, always.returninsidetry(hostdataprop at:658, inlinevalueitems at:663) —finallystill runs.AbortSignalis passed toeffectiveDataSource.find()anywhere in this file, so there is no cancel path whose promise never settles. Afind()that never settles would strandloading, identically before and after this change.setStateafter unmount is a no-op on this repo's React; unchanged here.No path relied on the stale
finallydoing the clearing:reloadincrementsreloadSeqRefas its first statement, so a newer sequence number always implies a newer run already inside its owntry.Tests
New file
packages/plugin-gantt/src/ObjectGantt.staleReloadFinally-7231.test.tsx. A data source hands back deferredfind()promises so two reloads can be held in flight and completed in either order;getObjectSchemaresolves immediately, which is what issues the second reload while the first is pending.refreshing.Ablation (run on the committed tree, guard reverted then restored):
import { ObjectGantt } from './ObjectGantt'), so the subject is the source module and nodist/is in the path — no rebuild leg applies.if (isCurrent()) {2 occurrences to 1, injectedif (silent) setRefreshing(false);0 to 1, blobc063fb2tocb81262.Tests 1 failed | 2 passed (3)— exactly case 1 red, both controls green.c063fb2(byte-identical to theHEADblob),git diff HEADempty, anchor counts back to 2 / 0.Gates
All run on
529d546, the final commit, with the worktree clean:pnpm exec vitest run packages/plugin-gantt/Test Files 57 passed (57)·Tests 451 passed (451)pnpm --filter @object-ui/plugin-gantt run type-checktsc --noEmit && tsc -p tsconfig.test.json— the second leg is what type-checks the new test file)check:control-bytescheck-control-bytes: OK (scanned 5998 tracked text file(s); skipped 85 binary).check:vi-mock-specifierscheck-vi-mock-specifiers: OK (4128 tracked source file(s), 2398 test-named; 533 carry a mock; 782 relative specifier(s) resolved...)check:vi-mock-inheritcheck-vi-mock-inherit: OK (... 118 call site(s) on @object-ui/react judged (118 inherit, 0 auto-mocked) ...)check:self-importNo package names itself inside its own src/.check:phantom-depsEvery in-scope import is declared by the package that publishes it.check-changeset-presence.mjs2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s): .changeset/7231-gantt-stale-reload-finally.md.check-changeset-no-major.mjsNo changeset declares a major bump.Declared narrowing — eslint. The repo-wide
pnpm lint(turbo run lintover the 42 packages that define alintscript) is CI's run. Locally eslint ran over the touched package only —eslint .insidepackages/plugin-gantt, which is exactly the unit turbo invokes for it. Three readings, so the narrowing is a measurement and not a skip:eslint.config.js(its own walk and its own ignores), not from a guess about which files count;--format json: 87 files, 0 errors, 333 warnings (the new test file contributes 10 warnings, in line with its siblings —.github/workflows/lint.ymldeliberately sets no--max-warnings, so this gate is about errors);eslint.config.jsdeclares noparserOptions, noprojectand noprojectServiceanywhere, so no rule reads type information across files — every verdict is single-file. Every config block matches**/*.{ts,tsx}only, so the added.changeset/*.mdis outside the linted set entirely. This diff therefore cannot move the verdict on any file it does not contain.Scope
Reload guard only.
packages/plugin-gantt/src/ObjectGantt.tsx(thefinallyin thereload()closure), one new test beside it, one changeset. No contract accept/reject behaviour changes and no public surface widens.Deliberately untouched, each queued or owned elsewhere: the
buildExpandFields(objectSchema?.fields)projection site in this same file (that is #7230, which remains open and is queued behind this card); the paging /$topshape of the gantt fetch (#7210 remains open — its half 2 is a maintainer decision); and the schema-gating regime (#7225 and #6482 both remain open). Per the card, this is deliberately not folded into that gating work: a gate would hide the schema-resolution instance and leave the guard broken on the toolbar-refresh and write-readback pairs measured above.🤖 Generated with Claude Code
https://claude.ai/code/session_01NRRumy89BYdW9ogbcdHTho
Generated by Claude Code
Generated by Claude Code