fix(admin): the dialog gate's ui getter now reads through, as its comment already claimed (#404) - #414
Merged
Conversation
…omment already claimed (#404) A review pass of the change that added it found the claim and the code disagreeing, after it had merged. `gateDialogs` defines `ui` as a getter rather than a frozen value, and says why: pi's own `ui` getter THROWS once its context is stale, and pi can rebind the ui context mid-session, so the one property this wrapper replaces has to preserve on that property what the descriptor copy preserves on every other. The getter did not do that. It closed over the wrapper built at construction and never consulted `ctx.ui` again -- measured at zero reads of the original across three reads of the gated one -- which is exactly the behaviour of the frozen value the comment says it fixes. And the test pinned the DEFECT: it asserted the reads did not happen. It reads through now, rebuilding the wrapper only when the underlying `ui` is a different object, so a hot path that touches `ctx.ui` repeatedly pays an identity comparison rather than a rebuild. The test says what the rule is instead of what the bug was: each read reaches pi's own getter, a rebind is FOLLOWED rather than remembered, and a stale context still throws through the gate. Mutating the read back to a one-shot turns two tests red. Latent rather than live: nothing in this extension triggers a session replacement today, which is why a wrong comment survived a merge. It is the class of defect this round kept finding in its own prose, and the fix is to make the code true rather than to soften the sentence. No version moves. Refs #404 Signed-off-by: Rob Boerman <robboerman@live.nl>
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.
A review pass of the change that added it found the claim and the code disagreeing, after it had merged.
gateDialogsdefinesuias a getter rather than a frozen value, and says why: pi's ownuigetter THROWSonce its context is stale, and pi can rebind the ui context mid-session, so the one property this wrapper
replaces has to preserve on that property what the descriptor copy preserves on every other.
The getter did not do that. It closed over the wrapper built at construction and never consulted
ctx.uiagain -- measured at zero reads of the original across three reads of the gated one -- which is exactly the
behaviour of the frozen value the comment says it fixes. And the test pinned the DEFECT: it asserted the
reads did not happen.
It reads through now, rebuilding the wrapper only when the underlying
uiis a different object, so a hotpath that touches
ctx.uirepeatedly pays an identity comparison rather than a rebuild.The test says what the rule is instead of what the bug was: each read reaches pi's own getter, a rebind is
FOLLOWED rather than remembered, and a stale context still throws through the gate. Mutating the read back
to a one-shot turns two tests red.
Latent rather than live: nothing in this extension triggers a session replacement today, which is why a
wrong comment survived a merge. It is the class of defect this round kept finding in its own prose, and the
fix is to make the code true rather than to soften the sentence.
No version moves.
Refs #404