fix(relation): retry menu options after a failed query instead of caching the failure - #7987
Draft
martinjagodic wants to merge 3 commits into
Draft
fix(relation): retry menu options after a failed query instead of caching the failure#7987martinjagodic wants to merge 3 commits into
martinjagodic wants to merge 3 commits into
Conversation
Caught by CI's `lint:format` gate, which runs prettier separately from eslint — eslint was clean, which is why this was missed locally. Two line-wrapping changes in the test file, no behaviour.
✅ Deploy Preview for decap-cms ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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
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.
Summary
A relation dropdown could show "No options" until you typed something, and stay that way for the life of the page — even once the backend had recovered. Typing worked because it used a different cache key.
Reported downstream at Mestna-obcina-Celje/moc-www#740, where it affected every relation field on the site and several editors at once.
Root cause
Three defects compounding, all on
main:querythunk never rejects — on failure it resolves with the actionqueryFailurereturns, which carriespayload.errorwhere a success carriespayload.hits.RelationControlreadresult.payload.hits || [], so a failed request became the claim that the collection has no entries.RelationCachethen memoised that claim for the life of the page. It only stores resolved values, so making failures reject is enough to fix it — but nothing was rejecting.defaultOptions={true}loads exactly once, in a mount effect with no dependencies, and there is no way to ask it to try again. Even with 1 and 2 fixed, the menu would have stayed empty.The change
hitsFromQueryResultthrows on a failed query, so the cache never stores it and the next attempt genuinely retries.menuOptionsstate) and passes it as an array rather than letting react-select load it once.onMenuOpenretries when the list has never loaded.menuOptions === undefined("never loaded") is deliberately distinct from[]("loaded, genuinely empty") — only the former retries, so a truly empty collection is not re-queried on every open.Two consequential fixes:
shouldComponentUpdateignored state entirely, so nosetStatecould repaint the menu on its own — it only ever rendered becausequeryHitshappened to change in the same tick. It now considers state when it is given it: the coreWidgetborrows this method off the control instance and calls it withnextPropsalone (EditorControlPane/Widget.js:92), sonextStateis genuinely absent about half the time and must not be dereferenced.getSelectedOptionsreturnsnullfor a field with no value, previously unreachable because the initial load was gated on the field already having one.isLoadingis now passed explicitly, since react-select had been deriving it fromdefaultOptions={true}. Without it a slow load renders as "No options" — the exact confusion being fixed.Request count is unchanged or slightly lower: the mount-time empty-term query used to come from react-select and now comes from the control, and the redundant second call for fields that already have a value is gone.
Testing
shouldComponentUpdatewith a single argument the wayWidget.jsdoes.tsc --noEmitclean, prettier and eslint clean.Draft
Opened as a draft: this touches a widget every Decap user has, and I would like a second pair of eyes on the
shouldComponentUpdatecontract in particular before it lands.