Repository navigation
Let the embed-create page wait for its create, and tell the host when the outcome is not known - #300
Merged
Merged
Conversation
… the outcome is not known The create is a write, but it went with the default ten-second timeout: a slow create reported 'network' with "Request timed out." while the server went on and made the pad and the file, and a second try met the name it had taken. It now has no client-side time limit, like the embed page's initialise and recovery; a slow create is not a failed one. 'network' now means no answer from this app, which is the one case where the outcome is not known: fetch failed, also while the body streams in, or a proxy answered in its place, with its 502, 503 or 504 in status. A gateway's answer counted as 'server' before, which says nothing was created - true only of this app's own answers, since the server rolls a failed create back. After 'network' the page and the host's message say, in a translated sentence, that the pad may have been created anyway and to look in the folder before trying again. architecture.md says a host that stops waiting on its own must assume the same.
…server' can promise A 5xx counted as no answer from this app only as a 502, 503 or 504 without retryable. This app answers every error in JSON, so a 5xx whose body is not JSON came from somewhere else too: a proxy's 524 after a slow origin, or PHP dying midway, after the create may have written the file. It is unanswered now, in every client. A 4xx page still counts as an answer: a proxy refusing a body too large never reached the create. When the body breaks off after the status line, the status goes along on the error. The embed-create page takes a 4xx as refused even when its body broke off, and anything else without this app's answer as 'network', always with the sentence to look in the folder. 'server' no longer promises that nothing was created, since the rollback is best effort and a success the page cannot use counts there too. The create's own fallback sentence reaches the page, the template no longer carries a third copy of the sentence, and a controller test holds that both embed pages get theirs. isUnanswered() is the one check for the flag.
…ses under 'server' epnc:create-failed carried reason, status and message, but not the server's code or retryable. A locked folder answers 503 with retryable, and the host could not offer a second try, as the viewer and the embed page do since #299; nor tell pad_file_changed from a name taken, both 409. The payload carries both now, null and false without them. 'server' was described as this app refusing or failing the create, yet a proxy's or a firewall's 4xx never reached it. It now reads: refused with a 4xx, by this app or before it, so nothing was created; failed by this app with a 5xx of its own, rolled back as far as it can; or an answer the page cannot use.
"Pad creation failed." reaches the reader since the create passes it as its fallback, after a proxy's 413 page, say, or a 409 whose body broke off, but it was English. The page takes it translated from its template, as its other sentences.
The create's host contract stood in full in failCreate()'s docblock and in architecture.md, and the two had begun to drift. It is in architecture.md now, one point for each reason; the docblock names the four in a line each and points there. The rule for what counts as no answer stood four times. The server's facts are in api-reference.md, the clients' rule under "Errors of the API", and the helper's comment and its test point there. The create's catch reads as three cases - refused, no answer, this app's own 5xx - with the unanswered flag read once and without classifyHttpStatus(). Its tests are one table of what the host is told, one row for each kind of failure; which failures count as no answer is the helper's test's to hold. The stubbed answers come from tests/js/responses.js instead of six copies.
…helpers left The table of the create's failures lost the old conflict test's check that the page does not redirect after one. It holds it for every row now. Where the answer stubs were removed, double blank lines stayed behind in five test files.
|
ⓘ 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 QodoWait for embed pad creation and report unknown outcomes
AI Description
Diagram
High-Level Assessment
Files changed (37)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR |
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.
The embed-create page waits for its create, and tells the host when the outcome is not known. A follow-up to #299, which made the embed page's initialise and recovery wait.
What changes
The create is a write, but it went with
fetchJsonWithTimeout()'s default ten seconds. A slow create reportedepnc:create-failedwithreason: 'network'and "Request timed out." while the server went on and made the pad and the file. A second try then met the name the first one had taken (409), and the host never heard of the pad that did come about.No time limit. The create now waits, as the embed page's initialise and recovery do since #299. A slow create is not a failed one: the page and the host hear nothing until the server or a proxy answers.
'network'means the outcome is not known. The failure reasons stay the same four, but'network'now covers every case without an answer from this app:502,503or504withoutretryable, which this app never sends (Offer to try the open again wherever it may work later #299).What came goes along in
status. A gateway's or PHP's 5xx counted as'server'before.'server'covers three cases:413, say), so nothing was created;What the host and the page say. After
'network', whatever fetch rejected with, the page shows and the host'smessagecarries a translated sentence: "Nextcloud did not answer. The pad may have been created anyway; look in the folder before you try again." Before, it was the browser's English ("Failed to fetch") or "Request failed.". An answer without a sentence of its own now reads "Pad creation failed.", translated, which never reached the page before.The payload of
epnc:create-failedalso carries the server'scodeandretryable,nullandfalsewithout them, as the viewer and the embed page read them since #299. A host can offer a second try when the folder was locked (503withretryable), and can tellpad_file_changedfrom a name taken, both409.For every client.
fetchJsonWithTimeout()takes a 5xx whose body is not JSON for no answer from this app, since this app answers every error in JSON. The viewer and the embed page offer "Try again" after one when they open. When the body breaks off after the status line, the status goes along on the error.isUnanswered()is the one check for the flag.architecture.mdholds the host contract, one point for each reason, and adds that a host which stops waiting on its own must assume the pad may still be created. The rule for what counts as no answer is stated once there too ("Errors of the API"), andapi-reference.mdholds the server's facts it rests on. The page's docblock and the helper's comment point there instead of repeating them.Checked in the stack
NC 34.0.3, driven with Playwright. The create page was framed in a Nextcloud page of the same origin that recorded its messages:
epnc:create-succeededat 13 s, and the file was there.create-failed,reason: 'network',status: null, with the new sentence, on the page too.504gateway page or an HTML500as from a PHP fatal: the same withstatus: 504and500.413page:'server',413, "Pad creation failed.".503withretryable, as for a locked folder:'server',503,retryable: true, and the server's sentence.Bundles
Built with Node 24 in a copy of the tree with a real
node_modules, as for #299; building again gives the committedjs/. Thefetch-helpersandpad-open-flowchunks get new hashes, and the embed-create, embed and viewer bundles change.Tests
create-succeeded, with no failure before.'network': the network failing, the browser stopping the request, a502page, a success whose body broke off;'conflict': a name taken, a file that changed while its pad was set up (withpad_file_changed), a name taken whose body broke off;'server': a pad type switched off (with itscode), a proxy's413, this app failing with and without a sentence, a locked folder (withretryable).fetchJsonWithTimeout(): a500or524page is no answer; this app's JSON500and a413page are. The status goes along when the body breaks off or times out.isUnanswered()takes onlytrue.EmbedController: both embed pages get their sentence for no answer, and the create page its sentence for a failure without one.retryablefortrue, which behaves the same, since the helper sets the flag only totrue. Among the faults:'network';codeorretryablenot passed on to the host.Left as it is
create-pendingevent was considered and left out: the existing event carries more now, but no host has to handle a new one.'network'meets a409if the first create went through, and cannot tell that pad from another file of the same name. An idempotent create (a key the retry sends, or opening the existing file of that name) would settle it. That is a change to the create API, and it belongs with the common path for creating pads.embed_url, which only a server bug gives.Numbers
tests/js/responses.js. PHP tests unchanged at 1404, lint green.