fix(builder,plugin-page-builder): a rename outlives the panel it started in - #1857
Conversation
…ted in Two Codex findings on merged #1367, both reachable through the panel switch and both decided from state that switch destroys. The PENDING NAME is released only by the attempt that is still the live one. Cleanup ran unconditionally, so an earlier rename finishing under a later one released the name for both — and the panel judges the next edit against the name a class is heading for, so with it gone an author typing the original back is read as "nothing changed" and dropped, while the later rename persists a different name. The SUPERSESSION IDENTITY moves to the host. Kept in a ref inside the field, it was destroyed by the rail unmounting the manager and started again at zero, so the first answer to arrive after a switch matched the newer attempt's number: a refusal for a rename already replaced was raised from the shell, naming an edit that no longer existed. One identity serves both, because they are the same question asked twice — which of these attempts is the one still being awaited. The host takes it inside `onRename`, and the panel reads it straight after that call; a REQUIRED prop rather than an optional one, so no consumer can silently keep the version that does not survive. The third finding on that PR — a synchronous refusal being dropped before it could be reported — is already fixed on main, with a docblock explaining why the synchronous path needs no supersession check. Verified rather than re-fixed. One test was written and then removed rather than kept: a remount case asserted from the panel's own DOM passes with the defect reinstated, because an unmounted field's `setRefused` reaches nothing either way. The refusal goes to the notice sink, so the case lives in `builder-notices.test.tsx` where one is rendered, and this file says so.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c01870628d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const mine = (renameAttempts.current[classId] ?? 0) + 1; | ||
| renameAttempts.current[classId] = mine; |
There was a problem hiding this comment.
When a valid named class has the ID __proto__ (isUsableNamedClass accepts any string ID up to 128 characters), this reads the inherited prototype accessor instead of 0; mine consequently becomes a string, and the assignment invokes the prototype setter rather than storing a counter. endRename can then never match the attempt, leaving the pending slug permanently set, so after a refused save retrying the same slug is discarded as a no-op. Use a Map or null-prototype record for these counters.
AGENTS.md reference: AGENTS.md:L303-L306
Useful? React with 👍 / 👎.
github-actions
Bot
added
type: docs
|
The post-merge Codex finding on this PR (rename-attempt counters on a plain record, so a class id of |
1 participant
Three Codex findings on merged #1367. Two fixed, one already fixed on main —
verified rather than re-fixed.
What an author sees today
The class manager lives in a rail that unmounts it on every panel switch, and
that switch is exactly what makes two renames of one class overlap: rename, move
away, come back, rename again, and the first write is still on the network.
A revert is silently dropped. The panel judges an edit against the name a
class is heading for. Cleanup released that name unconditionally, so an earlier
rename finishing under a later one released it for both — and typing the original
back then reads as "nothing changed" against the name still on screen. Nothing is
written, and the later rename goes on to persist a different name.
A stale refusal is raised for an edit that no longer exists. Whether an answer
still describes the rename being awaited was decided by a counter kept inside the
field. The unmount destroys it, so on the way back it starts again at zero and the
first answer to arrive matches the newer attempt's number.
The fix
One identity, owned by the host, serving both — they are the same question asked
twice: which of these attempts is the one still being awaited. The host takes it
inside
onRename, and the panel reads it straight after that call.currentRenameAttemptis a required prop rather than an optional one, so noconsumer can silently keep the version that does not survive a remount. That is a
boundary rather than a note, which is what
AGENTS.mdasks for where the correctpath and the easy path differ.
The third finding
Already fixed on main: the synchronous-refusal branch reports it, with a docblock
explaining why that path needs no supersession check. Re-verified against today's
code rather than taken from the filing, which predates it.
Evidence
Two mutants, each killing exactly its own test:
keeps the pending name when an EARLIER rename settles under a later one;does not raise a refusal the author has already renamed past.The first test needed a two-stage release to build the window at all: a single
release lets both renames finish, and then no pending name is owed to anyone and
the no-op is correct. The first version passed for that reason.
One test was written and then removed. A remount case asserted from the
panel's own DOM passes with the defect reinstated — an unmounted field's
setRefusedreaches nothing either way, so the defect and the fix look identicalthere. The refusal goes to the notice sink, so the case lives in
builder-notices.test.tsxwhere one is rendered, and the panel suite says sowhere the next reader will look.
The shared test harness also gained a real counting identity. A constant stub
makes every answer look current and quietly removes the supersession these tests
are about — it broke the existing out-of-order test the moment it was introduced,
which is how it was caught.
Gates:
check-types0, lint 0, builder 3216 passed, page-builder 1086passed,
fallowpass with all four introduced counts at 0.