Give the create flows one attempt object instead of by-reference locals - #220
Merged
Conversation
Closes #199. The four flows each declared `$padId`, the file record and — in one case — `$path` as locals, and threaded them through three closures by reference: the action writes them, the rollback reads them, the log formatters read some of them. Twelve capture sites that had to stay in step, with nothing enforcing it. Two defects had already come out of that, both found in review rather than by a test: a flow that never recorded its file, and a flow that recorded one and then discovered it was somebody else's pad — correct only because a variable was un-set a few lines above the throw. `withCreateRollback()` now makes a `PadCreateAttempt` and hands it to every closure, so there is one owner of what has been created and nothing to keep in step. "This file is not mine after all" is `disownFile()` rather than an assignment whose position decides the answer. The template flow keeps its file-only rollback, and now says why in code: it records its pad to log it, because `materializeTemplateInto()` removes that pad itself before rethrowing. Behaviour is unchanged, and the 921 existing tests pass untouched — no test file is modified by this commit. The new ones cover the attempt itself, including the two defects above stated as properties of the object.
…ssed Splitting the docblock left two of them: PHP attaches only the nearest, so `@template T` / `@return T` stopped applying and the method returned `mixed`. Psalm then stopped checking each flow's result against its own declared return shape. Counter-checked in a throwaway worktree with `pad_id => false`: main reports 3 errors, this branch reported 0 before the fix and 2 after it. Create-by-parent recorded its path after `requireFileId()` and the ownership check, so a failure in either still logged an empty path. It is recorded as soon as the node exists.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoCentralize create-flow rollback state in PadCreateAttempt
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Recording the path first was the wrong half of the trade. `toUserAbsolutePath()` throws when the node cannot be mapped into the user's tree, and by then `newFile()` has already made the file — so the rollback was handed a null claim and left the empty .pad behind. The id is what the rollback needs to remove the file at all, so the claim comes first; the path follows and still reaches the log lines whenever it resolves. When it does not, the log loses a path and the file is still cleaned up, which is the right way round. Counter-checked in a throwaway worktree: with the previous order the new test fails with a null claim.
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.
Closes #199.
The four create flows each declared
$padId, the created-file record and — in one case —$pathas locals, then threaded them through three closures by reference: the action writes them, the rollback reads them, the log formatters read some of them. Twelve capture sites that had to stay in step, with nothing enforcing it.Two defects had already come out of that, both found in review rather than by a test: a flow that never recorded its file, so a failure after the file existed reached the rollback with nothing to clean up; and a flow that recorded one and then discovered it was somebody else's pad — correct only because a variable was un-set a few lines above the throw.
withCreateRollback()now makes aPadCreateAttemptand hands it to every closure. There is one owner of what has been created, and nothing to keep in step. "This file is not mine after all" isdisownFile()rather than an assignment whose position decides the answer.Two details worth naming:
requireFileId()and the ownership check, so a failure in either logged an empty path. It is recorded as soon as the node exists.materializeTemplateInto()removes that pad itself before rethrowing. Different resource ownership, not drift.Behaviour is unchanged, and that is checkable
The 921 existing tests pass, and no existing test file is modified by this PR — the only test file it touches is the new one for
PadCreateAttempt. For a pure state refactor that property is the strongest evidence available, so it was worth keeping intact.The new tests state the two defects above as properties of the object rather than as orderings inside a closure.
What this is not
This unifies the state, not yet the flow. Production code grows by about 80 lines net: the four flows remain, their variables now live in the attempt. It is groundwork for a shared create core rather than the core itself.
What remains, in the order I would do it:
create()andcreateInParent()to one path. After the target is chosen they do the same thing. The public methods stay as thin entry points; folder resolution andparent_folder_idare their differences. This one changes flow, so it wants its own review.Templates and external pads should not be pressed into the same path: templates clean up their own Etherpad resources, and external pads must never be deleted. Shared file handling yes; differing resource ownership stays visible.
Verification
925 PHP tests / 3078 assertions, 261 JS tests, Psalm clean, Playwright 35 passed / 1 skipped.
One regression was introduced and fixed during review: splitting the docblock left two of them, and PHP attaches only the nearest — so
@template T/@return Tstopped applying and each flow's result was no longer checked against its declared return shape. Counter-checked in a throwaway worktree by returningpad_id => false:mainreports 3 errors, this branch reported 0 before the fix and 2 after it.