fix: defer Spreadsheet.selectCellRange until sheet widget layout is ready#9381
Open
DiegoCardoso wants to merge 5 commits into
Open
fix: defer Spreadsheet.selectCellRange until sheet widget layout is ready#9381DiegoCardoso wants to merge 5 commits into
DiegoCardoso wants to merge 5 commits into
Conversation
… internals #9333 guarded setSelectedCellAndRange by reading `this.api.spreadsheetWidget.sheetWidget.definedRowHeights`. Those are private GWT fields: their names survive only in `pretty` GWT output (local/dev), so the check worked there. CI compiles the client with `-Dgwt.style=OBF`, which obfuscates the field names, so `this.api.spreadsheetWidget` is undefined and the guard threw `Cannot read properties of undefined (reading 'sheetWidget')`. That single throw dropped the selection and tripped the `*_noErrors`/ `*_noConsoleErrors` console-log assertions, producing the ~20 spreadsheet IT failures seen only on CI for the 24.10/25.0/25.1 cherry-picks (and untouched by #9373, which threw on the same line). Drop the private-field probe. The api's `setSelectedCellAndRange` is a public, @JsType-exported method whose name is stable under obfuscation; call it and, if the deferred initial relayout hasn't run yet (it throws inside `$scrollAreaIntoView`), keep the latest selection and retry until it applies. Reproduced locally by compiling the client with -Dgwt.style=OBF: the old probe fails NavigationIT, this fix passes. Follow-up to #9333, #9373. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DiegoCardoso
force-pushed
the
fix/spreadsheet-selection-obf-safe
branch
from
0a4ca6e to
bb82001
Compare
Move the layout-readiness handling from the connector into the widget that owns the operation: SpreadsheetWidget.selectCellRange now checks SheetWidget.isLoaded() and, if false, stores the latest args and schedules itself via Scheduler.scheduleDeferred until layout state is populated (bounded at 100 hops as a safety net). The connector's setSelectedCellAndRange goes back to a one-liner — no try/catch, no pending state, no constants. Why this is the right layer: the bug is "scrollAreaIntoView dereferences a null definedRowHeights when called before the deferred init in resetFromModel completes". The fix belongs next to scrollAreaIntoView's caller, not in the JS connector reaching across an obfuscated boundary. GWT's own scheduler is the natural deferral primitive (the layout init uses it too); FIFO ordering means typical cases resolve in zero or one hops. Only selectCellRange is deferred — it's the only RPC that touches row/column metrics. Other RPCs (cellsUpdated, refreshCellStyles, popup ops, etc.) don't need this and didn't fail in the original CI runs. If another method ever surfaces the same race, the same pattern can be replicated. Locally validated with -Dgwt.style=OBF (matching CI): - Positive (new GWT + simple JS): NavigationIT 26/0, SpreadsheetDialogIT 1/0. - Control (no GWT fix + simple JS): SpreadsheetDialogIT fails with the underlying NPE — Uncaught TypeError: Cannot read properties of undefined (reading 'length') in FlowClient bundle, the actual definedRowHeights dereference. Proves the GWT-side deferral is what's load-bearing. Follow-up to #9333. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DiegoCardoso
changed the title
- Move static constant + 3 instance fields to top-of-class field block (java:S1213 — field declaration order convention). - Replace lambdas with method references (java:S1612).
|
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Follow-up to #9333. When a Spreadsheet inside a closed Dialog is opened, Flow
flushes the property writes and the
setSelectedCellAndRangeRPC in the sametask. The api receives the call before the deferred init scheduled in
SheetWidget.resetFromModelhas run — soSheetWidget.getRowHeightreads anuninitialized
definedRowHeightsand throwsThat throw drops the selection and trips the
*_noErrors/*_noConsoleErrorsconsole-log assertions.
#9333's connector-side guard masked this by probing
definedRowHeightsthrough the api's private GWT fields — names that only survive
-Dgwt.style=pretty(local/dev). CI compiles the GWT client with-Dgwt.style=OBF, where those private names are renamed andapi.spreadsheetWidgetis
undefined. The guard itself then threw, producing the same SEVERE log andthe same ~20 spreadsheet IT failures on the cherry-pick PRs.
Move the layout-readiness handling into the widget that owns the operation:
SheetWidget.isLoaded()(package-private) — exposes the already-existingloadedflag that flips at the end ofresetFromModel's deferred command.SpreadsheetWidget.selectCellRange— if!sheetWidget.isLoaded(), store thelatest args and
Scheduler.scheduleDeferred(this::applyPendingSelectCellRange).The deferred re-checks, applies, or re-schedules. Latest-wins (rapid successive
calls collapse to the final selection). Bounded at 100 hops as a safety net;
in the typical case the layout init has already been scheduled before us, so
GWT's FIFO scheduler resolves us in 0–1 hops.
vaadin-spreadsheet.js—setSelectedCellAndRangegoes back to a one-liner(
this._flush(); this.api.setSelectedCellAndRange(...args)). No try/catch,no pending state, no constants.
Only
selectCellRangeis deferred — it's the only RPC that touches row/columnmetrics. Other RPCs (
cellsUpdated, popup ops, etc.) don't need this anddidn't fail in the original CI runs.
Reproduce locally by compiling the client with
-Dgwt.style=OBF(matching CI)and running the merged ITs in production mode:
underlying
Cannot read properties of undefined (reading 'length')inFlowClient-…js— the actualdefinedRowHeightsNPE.Follow-up to #9333.
🤖 Generated with Claude Code