Offer to try the open again wherever it may work later by Jaggob · Pull Request #299 · ether/etherpad-integration-for-nextcloud

Jaggob · 2026-09-26T08:10:38Z

The viewer and the embed page offer "Try again" wherever the same open may work later. Until now the embed page showed its errors without any action.

What changes

The viewer offered its second try only for a row that waits, by the code waiting_binding. The embed page offered nothing at all, not even for that. Since #298 the server says which answers the same request may get past later, with retryable: true:

a row still waiting (409);
a file locked for a moment (503);
this instance's Etherpad not reachable (503).

This change adds a fourth: a file's row that another request made first (BindingNotCreatedException, 400). Two initialisations at once leave the loser with it; its sentence already said to try again, and the next open finds the winner's pad. The create endpoints answer it with their own sentence and neither field, as before.

What the helper says. fetchJsonWithTimeout() carries retryable onto the error, as it carries code; only true counts. When nothing came back from this app, it marks the error unanswered:

its own timeout;
a failed network, also while the body streams in (that used to read as an empty body);
a 502, 503 or 504 without retryable. This app never answers 502 or 504, and every 503 of its own carries retryable, so without it a proxy or Nextcloud in maintenance answered in its place, with a page or with JSON of its own. The mapper tests hold that every 503 is retryable, and api-reference.md says so.

That is only a fact, not a verdict: whether another try is safe depends on the request. A caller's own abort stays unmarked.

Who decides. isRetryableOpenError() in pad-open-flow.js decides for both clients. It offers another try on:

the server's retryable;
pad_file_changed, which an open meets only while initialising the file, after the server undid its part;
any step of the open that got no answer, the initialise too.

The second try opens first, so it finds a pad the first try set up. One still being set up is safe to meet: the server compares the file before it writes, a file has one binding row, and a pad that lost either race is rolled back. Losing the race for the row is retryable itself (above), so that ends with a button too.

Both clients show "no answer" as their own translated sentence, "Nextcloud did not answer. Check your connection and try again.", rather than the browser's English ("Failed to fetch", "Load failed").

Writes wait. The embed page's initialise and recovery no longer time out after ten seconds, as the viewer's never did. Cut short, a write goes on with nobody left to read the outcome. A recovery that got no answer is not offered again, since a second one would meet the first: both clients open the file instead, which shows the pad if the recovery went through and the recovery card if not.

The embed page.

The error panel gets the same button as the viewer, styled like the recovery buttons and labelled with the translated "Try again" the page already had for content.
The button runs the whole open again, as the recovery does once it has made the row. Every open starts from the loading state, showLoading() at the start of run().
A success clears the panel, and a second failure offers one more try, not two.
An open that answers late cannot undo a newer one: run() has a generation guard, as the content view and the viewer do.

Focus and screen readers.

After a click whose button goes away or is disabled with the focus on it, the card's first action, or its message, takes the focus, in the viewer and on the embed page. That covers a second try, a recovery, a failed recovery, and a second try that ends on the recovery card, once its actions are drawn. Both clients use one handFocusTo() in src/lib/hand-focus.js.
On the first load nothing takes the focus.
The embed page does it without scrolling its host page.
The message of each error card has role="alert": the embed page's error panel, its recovery card, and the viewer's card. An error is read out when it appears, the first one too, without the buttons around it.

Internals. The embed page's buttons come from one buildButton() with one class constant, showError() takes a boolean, and the panels no longer hide each other by hand: every open starts from showLoading(). Both clients name their error message (messageOf()) and the missing-binding check (isMissingBindingError()) once.

The rule is stated once, under "Errors of the API" in architecture.md; api-reference.md refers to it. PHP changes: ApiErrorCode::retryable() takes the lost race for a row, the template's place for the button, the alert roles, the new sentence passed through EmbedController in de, es, fr and it, and the tests for the 503s.

Checked in the stack

NC 34.0.3, Etherpad 2, driven with Playwright against the stack, with screenshots.

With Etherpad disconnected, for the embed page of a protected pad:

It showed "Could not open pad / Etherpad cannot be reached right now. Try again later." with the button, and the focus stayed on the page.
A second try while still disconnected left one button, and it had the focus.
With Etherpad back, the button opened the pad in the frame, and the error panel was gone.

With the open request failed on the network, or answered by a 502 gateway page, a 503 HTML page as in maintenance, or a 502 with a gateway's own JSON message:

The page showed the new sentence with the button each time, and the focus stayed on the page.
With the request let through again, the button opened the pad.

For a copy of a .pad file, on its recovery card:

With the recovery request failed on the network, the page sent it once, opened the file again once, and showed the card again with the focus on its first action.
Let through, the recovery opened the pad.

In the Files viewer, with Etherpad disconnected:

The card showed the button, and on the first load the focus stayed outside it.
After a second try that failed, the focus was on the new button.
With Etherpad back, the button opened the pad.

Bundles

The bundles are built with Node 24 in a copy of the tree with a real node_modules, since a symlinked one writes its path into the source maps. The same environment builds main byte for byte like the committed js/, and building this branch again gives the committed js/.

What changes in them:

The chunks fetch-helpers and pad-open-flow get new hashes.
The embed and viewer bundles change.
The embed-create bundle changes only in the chunk name it imports.

Tests

fetch-helpers:
- retryable is carried only when the server says true.
- A timeout and a failed network are unanswered, not retryable, also while the body streams in.
- A body that is not JSON reads as an empty one.
- A 502, 503 or 504 without retryable is unanswered, as a page, as JSON of its own and as JSON with a message. This app's own 503 and a 500 page are not.
- A caller's abort is neither.
- The message for no answer is the caller's sentence.
Open flow: a table of what is worth another try, and an open, an initialise and the open after it that got no answer each leave one.
handFocusTo(): the first action, else the message with a tabindex, with preventScroll only when asked, and nothing without a target.
Viewer:
- A second try for Etherpad not reachable, a locked file, a waiting row, a lost race for the file's row, a timeout, a file that changed while it was set up, and an initialise that got no answer.
- None for a refusal, a missing binding or a plain server error.
- No answer is shown in the translated sentence, and the card's message is an alert.
- A recovery that got no answer opens the file again.
- The focus stays put on the first load. After a second try that fails it goes to the new button or the message, also after a failed open following a recovery, after a failed recovery, and to the recovery card only once its actions are drawn.
- The focus tests model Vue's order: the card is drawn on the next tick from what was set before the focus was asked for, and its recovery buttons stay disabled while a recovery runs. A focus asked for too early fails them.
Embed page:
- The button after a waiting row, after Etherpad not reachable, after a lost race for the file's row, after no answer (in the page's sentence), after a file that changed, and after an initialise that got no answer; the button opens the pad on the second try.
- None after a plain failure.
- The loading state, and nothing else, while a second try runs.
- Initialise and recovery wait a minute and still open the pad.
- A recovery that got no answer opens the file again; one that failed hands the focus back to the card.
- One button at a time over two failures.
- The focus stays put on the first load, and is handed on without scrolling after a second try, after a failed open following a recovery, and to the recovery card.
- A late answer does not undo a newer open, neither a failure after a success nor a success after a failure.
PHP: in both mappers, every answer with 503 carries retryable, and so does a lost race for a file's row; a create's own wording for it carries neither code nor flag.
Mutation check: each round of this branch against its own state, all caught; the last two had 20 and 7 faults, among them:
- the helper back on the message check, or taking this app's 503 or any status for no answer;
- the open flow not retrying an unanswered step;
- handFocusTo() without tabindex, scrolling regardless, or putting the message before the action;
- a recovery that got no answer offered again instead of reopening, in either client;
- a failed recovery keeping no focus, the viewer asking for the focus before the card is set, or before its button is enabled again;
- the embed page not showing its loading state, scrolling its host page, or showing the browser's English;
- the viewer's message not an alert;
- a lost race not retryable, or every binding error retryable;
- an open not starting from the loading state, or the guard missing after a success.

Left as it is

The embed-create page's POST still times out after ten seconds, though it writes. It reports the timeout to the host as network, and changing what a host is told belongs in its own change.
Writes have no client limit at all now, as in the viewer. A request that hangs past every server and proxy timeout leaves the page loading until it is reloaded, which is the honest state for a write whose outcome is not known.
A TypeError from fetch() counts as no answer. That also covers a URL or header value fetch() rejects, and a redirect to another origin, from a login proxy whose session ran out. Neither can be told from a failed network: the URLs come from the server's URL generator, the header is the server's token, and behind such a proxy the rest of Nextcloud fails the same way until a reload.
The error panel and the recovery card are the same widget by now: title, message, optional text, a row of buttons. One card with one showCard() would replace two template blocks, two sets of CSS and four show functions. That predates this change and belongs in one of its own.

Numbers

JS tests 269 → 333 (Node 24). PHP tests 1401 → 1404. PHP lint green.
Psalm green on OCP 31.0.9 and 34.0.4, baseline unchanged at 221.

The viewer offered "Try again" only for a row that waits, by its code, and the embed page offered nothing at all. Since #298 the server says which answers the same request may get past later - a row still waiting, a file locked for a moment, this instance's Etherpad not reachable - with retryable: true. fetchJsonWithTimeout() carries that flag onto the error as it carries the code, the viewer offers its second try on it, and the embed page's error panel gets the same button, labelled with the "Try again" it already had for content. The button runs the whole open again; a success clears the panel, and a second failure offers one more try, not two. Checked in the stack: with Etherpad disconnected the embed page shows "Etherpad cannot be reached right now. Try again later." and the button; with Etherpad back, the button opens the pad. The bundles are built with Node 24 from a tree whose build of main matches the committed js/ byte for byte.

… focus after one The client's own timeout and a failed network carried no retryable, so the two failures that are worth another try by nature were the ones without a button - a slow Etherpad ran the open into its ten seconds and left a dead end. fetchJsonWithTimeout() marks both retryable now; a caller's own abort stays unmarked. The embed page's button and the recovery's restart share restartOpen(), which hides every panel and shows the loading state before it runs the open again. After a second try that fails, the new button - or the message, when there is none - takes the focus the clicked button took with it, so a keyboard or screen reader keeps its place; not on the first load, where an embed that takes the focus scrolls its host page. architecture.md says the clients act on retryable.

The embed page's error panel starts from hideAllPanels() instead of hiding the loading state and the frame by hand, so the recovery card goes too. Its three kinds of button - the content's retry, the error panel's, the recovery card's - come from one buildButton(). The client comments no longer list which answers are retryable; they point to docs/api-reference.md, where the server's cases are. The rule for "Try again" is stated once, under "Errors of the API" in architecture.md, with the focus after a second try; the other places refer to it. Two pairs of tests that built the same scene are one each.

… answer fetchJsonWithTimeout() marked its own timeout and a failed network retryable for every caller, though only a caller knows whether its request writes. It now says only that nothing came back from this app, as unanswered: its timeout, a failed network, also while the body streams in, and a 502, 503 or 504 whose body is not this app's JSON, from a proxy or Nextcloud in maintenance. Those two answered with no button before. retryable is the server's word alone again. isRetryableOpenError() in pad-open-flow.js decides for both clients: the server's retryable, pad_file_changed, which an open meets only while initialising and after the server undid its part, and an open that got no answer. An initialise that got no answer gets no button, since its pad may be set up by now and another open would start a second one. The embed page's initialise and recovery no longer time out after ten seconds, as the viewer's never did. Both clients say "no answer" in a translated sentence of their own instead of the browser's English. The embed page hands the focus on after every click whose button went away, a recovery too, and to the recovery card when a second try ends there, without scrolling the host page. Its error panel is role="alert". An open that answers late cannot undo a newer one. The button classes are one constant, showError() takes a boolean, and the read-only view starts from hideAllPanels().

After a click on "Try again" or "Create new pad from this file" that ends in the error card again, the button the focus was on went away with it, and the focus fell to the page. The viewer now hands it to the card's first action, or to its message, on the next tick, as the embed page does; after a missing binding once the card's actions are drawn, and not on the first load. The card carries a ref for it. Checked in the stack with Etherpad disconnected: after a failed second try the focus is on the new button.

qodo-code-review · 2026-09-26T08:10:43Z

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

qodo-free-for-open-source-projects · 2026-09-26T08:11:45Z

PR Summary by Qodo

Offer retryable pad opens in viewer and embed

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Offer “Try again” for retryable and unanswered pad opens across both clients.
• Preserve safe write recovery, stale-response guards, focus, and screen-reader announcements.
• Mark binding races retryable and document and test the shared error contract.

Diagram

sequenceDiagram
    actor User
    participant Clients as Pad Clients
    participant Policy as Open Policy
    participant Fetch as Fetch Helper
    participant Gateway
    participant API as Nextcloud API
    participant Etherpad
    User->>Clients: Open pad
    Clients->>Policy: Run open
    Policy->>Fetch: Send request
    Fetch->>Gateway: HTTP request
    Gateway->>API: Forward request
    API->>Etherpad: Resolve pad
    Etherpad-->>API: Pad result
    alt Application response
        API-->>Fetch: Result or retryable error
    else Gateway failure
        Gateway-->>Fetch: Unanswered status
    end
    Fetch-->>Policy: Classified error
    Policy-->>Clients: Retry decision
    Clients-->>User: Alert and action
    opt User retries
        User->>Clients: Try again
        Clients->>Policy: Restart open
    end

High-Level Assessment

The shared, metadata-driven retry policy is the best approach. Centralizing error classification avoids drift between the viewer and embed page, while requiring a user-triggered retry avoids unsafe automatic repetition of writes whose outcomes are unknown. Reopening after an unanswered recovery is safer than issuing another recovery request.

Files changed (39) +1050 / -138

Enhancement (3) +60 / -3

embed.cssStyle retry actions on embed error cards +7/-1

Style retry actions on embed error cards
• Extends recovery action layout styles to the embed error panel and adds spacing when retry actions are present.
css/embed.css

EmbedController.phpProvide the localized unanswered message to embeds +1/-0

Provide the localized unanswered message to embeds
• Passes the translated no-answer sentence into the embed template data.
lib/Controller/EmbedController.php

fetch-helpers.jsClassify retryable and unanswered request failures +52/-2

Classify retryable and unanswered request failures

• Carries explicit retryability from API responses and marks timeouts, network failures, and non-application gateway responses as unanswered. Adds consistent localized message selection while preserving caller abort semantics.

src/lib/fetch-helpers.js

Bug fix (3) +166 / -92

ApiErrorCode.phpMark lost binding races as retryable +5/-2

Mark lost binding races as retryable
• Treats BindingNotCreatedException as retryable because a subsequent open can find the binding created by the winning request.
lib/Controller/ApiErrorCode.php

embed-main.jsAdd robust retries and recovery handling to embeds +112/-73

Add robust retries and recovery handling to embeds

• Adds whole-open retry actions, unanswered-request messaging, generation guards, and loading-state resets. Write operations no longer time out, and recovery failures safely reopen or restore focus without scrolling the host page.

src/embed-main.js

viewer-main.jsExpand viewer retries and accessible recovery focus +49/-17

Expand viewer retries and accessible recovery focus

• Uses the shared retry policy for server and network failures, localizes unanswered errors, and reopens after uncertain recovery outcomes. Error cards now announce messages and preserve keyboard focus after user actions.

src/viewer-main.js

Refactor (1) +25 / -0

pad-open-flow.jsCentralize open retryability predicates +25/-0

Centralize open retryability predicates
• Adds shared missing-binding and retryable-open checks covering server flags, changed files, and unanswered requests.
src/lib/pad-open-flow.js

Tests (9) +737 / -28

answers.jsCentralize API error fixtures for client tests +20/-0

Centralize API error fixtures for client tests
• Adds shared fixtures for retryable, missing-state, changed-file, race, and unanswered scenarios.
tests/js/answers.js

embed-main.test.jsCover embed retry, recovery, focus, and race behavior +283/-5

Cover embed retry, recovery, focus, and race behavior
• Tests retryable server and network failures, indefinite writes, recovery reopening, loading states, focus transfer, localized messages, and stale response guards.
tests/js/embed-main.test.js

api-client.test.jsAlign invalid JSON test with parser semantics +1/-1

Align invalid JSON test with parser semantics
• Uses a SyntaxError to model a response body that is not valid JSON.
tests/js/lib/api-client.test.js

fetch-helpers.test.jsCover request failure classification comprehensively +108/-5

Cover request failure classification comprehensively
• Tests retryable propagation, timeout and network handling, gateway responses, streamed-body failures, caller aborts, invalid JSON, and user-facing message selection.
tests/js/lib/fetch-helpers.test.js

hand-focus.test.jsTest shared focus transfer behavior +54/-0

Test shared focus transfer behavior
• Verifies action preference, message tabindex handling, optional scroll prevention, and no-op behavior without a target.
tests/js/lib/hand-focus.test.js

pad-open-flow.test.jsTest shared open retryability rules +45/-0

Test shared open retryability rules
• Covers missing-binding detection, retry decisions, and unanswered failures at each stage of frontmatter initialization.
tests/js/lib/pad-open-flow.test.js

viewer-main.test.jsCover viewer retries and accessible focus restoration +183/-16

Cover viewer retries and accessible focus restoration
• Tests all retryable open cases, localized unanswered errors, recovery reopening, alert semantics, and focus timing across retry and recovery outcomes.
tests/js/viewer-main.test.js

PadControllerErrorMapperTest.phpVerify signed-in retryable error contracts +23/-1

Verify signed-in retryable error contracts
• Tests retryability for lost binding races and guarantees every application-generated 503 carries the retryable flag while preserving create-specific responses.
tests/phpunit/unit/PadControllerErrorMapperTest.php

PublicViewerControllerErrorMapperTest.phpVerify public retryable error contracts +20/-0

Verify public retryable error contracts
• Tests retryable binding races and guarantees every public application-generated 503 is marked retryable.
tests/phpunit/unit/PublicViewerControllerErrorMapperTest.php

Documentation (2) +8 / -3

api-reference.mdDocument retryable API and client behavior +4/-2

Document retryable API and client behavior
• Documents retryable binding races, the application’s 503 contract, gateway failure interpretation, and retry behavior in both clients.
docs/api-reference.md

architecture.mdDefine the shared API error retry policy +4/-1

Define the shared API error retry policy

• Describes retryable opens, unanswered requests, safe recovery behavior, focus transfer, and alert semantics as one architectural rule.

docs/architecture.md

Other (21) +54 / -12

etherpad_nextcloud-embed-create-main.mjsPoint embed creation bundle at the rebuilt fetch chunk +1/-1

Point embed creation bundle at the rebuilt fetch chunk
• Updates the generated import to use the newly hashed fetch-helper bundle.
js/etherpad_nextcloud-embed-create-main.mjs

etherpad_nextcloud-embed-main.mjsRebuild embed bundle with retryable open handling +1/-1

Rebuild embed bundle with retryable open handling
• Contains the compiled embed-page retry, recovery, focus, localization, and stale-response behavior.
js/etherpad_nextcloud-embed-main.mjs

etherpad_nextcloud-embed-main.mjs.mapRefresh the embed bundle source map +1/-1

Refresh the embed bundle source map
• Regenerates source mappings for the updated embed client implementation.
js/etherpad_nextcloud-embed-main.mjs.map

etherpad_nextcloud-viewer-init.mjsRebuild viewer bundle with shared retry handling +1/-1

Rebuild viewer bundle with shared retry handling
• Contains the compiled viewer retry classification, localized errors, recovery reopening, and accessible focus behavior.
js/etherpad_nextcloud-viewer-init.mjs

etherpad_nextcloud-viewer-init.mjs.mapRefresh the viewer bundle source map +1/-1

Refresh the viewer bundle source map
• Regenerates viewer mappings to include the updated source and shared helpers.
js/etherpad_nextcloud-viewer-init.mjs.map

fetch-helpers-Dqr3YYFE.chunk.mjsAdd rebuilt fetch-helper chunk +2/-0

Add rebuilt fetch-helper chunk
• Adds the generated chunk carrying retryable and unanswered error metadata plus localized message selection.
js/fetch-helpers-Dqr3YYFE.chunk.mjs

fetch-helpers-Dqr3YYFE.chunk.mjs.licenseTrack the rebuilt fetch chunk license sidecar +0/-0

Track the rebuilt fetch chunk license sidecar
• Adds the generated license artifact associated with the new fetch-helper chunk.
js/fetch-helpers-Dqr3YYFE.chunk.mjs.license

fetch-helpers-Dqr3YYFE.chunk.mjs.mapAdd the rebuilt fetch-helper source map +1/-0

Add the rebuilt fetch-helper source map
• Adds source mappings for the new request error-classification implementation.
js/fetch-helpers-Dqr3YYFE.chunk.mjs.map

pad-open-flow-D6KDQoVq.chunk.mjsRebuild shared open-flow chunk +4/-4

Rebuild shared open-flow chunk
• Compiles the shared retry policy, missing-binding predicate, and focus helper into the runtime chunk.
js/pad-open-flow-D6KDQoVq.chunk.mjs

pad-open-flow-D6KDQoVq.chunk.mjs.licenseTrack the open-flow chunk license sidecar +0/-0

Track the open-flow chunk license sidecar
• Updates the generated license artifact associated with the rebuilt open-flow chunk.
js/pad-open-flow-D6KDQoVq.chunk.mjs.license

pad-open-flow-D6KDQoVq.chunk.mjs.mapAdd the rebuilt open-flow source map +1/-0

Add the rebuilt open-flow source map
• Adds source mappings for shared open-flow and focus-management changes.
js/pad-open-flow-D6KDQoVq.chunk.mjs.map

de.jsAdd German unanswered-request translation +1/-0

Add German unanswered-request translation
• Adds the German client-side translation for the new Nextcloud no-answer message.
l10n/de.js

de.jsonAdd German unanswered-request catalog entry +1/-0

Add German unanswered-request catalog entry
• Adds the German JSON translation for the new Nextcloud no-answer message.
l10n/de.json

es.jsAdd Spanish unanswered-request translation +1/-0

Add Spanish unanswered-request translation
• Adds the Spanish client-side translation for the new Nextcloud no-answer message.
l10n/es.js

es.jsonAdd Spanish unanswered-request catalog entry +1/-0

Add Spanish unanswered-request catalog entry
• Adds the Spanish JSON translation for the new Nextcloud no-answer message.
l10n/es.json

fr.jsAdd French unanswered-request translation +1/-0

Add French unanswered-request translation
• Adds the French client-side translation for the new Nextcloud no-answer message.
l10n/fr.js

fr.jsonAdd French unanswered-request catalog entry +1/-0

Add French unanswered-request catalog entry
• Adds the French JSON translation for the new Nextcloud no-answer message.
l10n/fr.json

it.jsAdd Italian unanswered-request translation +1/-0

Add Italian unanswered-request translation
• Adds the Italian client-side translation for the new Nextcloud no-answer message.
l10n/it.js

it.jsonAdd Italian unanswered-request catalog entry +1/-0

Add Italian unanswered-request catalog entry
• Adds the Italian JSON translation for the new Nextcloud no-answer message.
l10n/it.json

hand-focus.jsAdd shared error-card focus transfer +28/-0

Add shared error-card focus transfer

• Introduces a helper that focuses the first available action or makes the message programmatically focusable, with optional scroll prevention.

src/lib/hand-focus.js

embed.phpAdd accessible retry controls to the embed template +5/-3

Add accessible retry controls to the embed template
• Exposes the unanswered translation, adds an error-action container, and marks error and recovery messages as alerts.
templates/embed.php

qodo-free-for-open-source-projects · 2026-09-26T08:12:46Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~JSON gateway failures cannot be retried~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

fetchJsonWithTimeout() treats any syntactically valid JSON body as an answer from this app, and
only sets unanswered for gateway statuses when JSON parsing fails. A 502, 503, or 504 proxy or
maintenance response containing JSON without the application's error envelope therefore reaches both
open clients as non-retryable, although the documented rule says gateway responses not produced by
this app should offer another try.

Code

src/lib/fetch-helpers.js[R73-74]

+			if (!isJson && GATEWAY_STATUSES.includes(response.status)) {
+				error.unanswered = true

Evidence
The helper's isJson flag records only whether parsing succeeded, while the changed architecture
contract distinguishes this application's JSON from a gateway response. Application error responses
are consistently created from payloads containing message, so syntactically valid gateway JSON
without that envelope is distinguishable but currently missed.
src/lib/fetch-helpers.js[51-75]
docs/architecture.md[304-308]
lib/Controller/PadControllerErrorMapper.php[185-200]
lib/Controller/PublicViewerControllerErrorMapper.php[61-73]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`fetchJsonWithTimeout()` currently assumes every successfully parsed JSON body came from this application. JSON error responses generated by a proxy or maintenance layer therefore are not marked `unanswered`, so open clients omit the retry action.
## Fix Focus Areas
- src/lib/fetch-helpers.js[51-75]
- tests/js/lib/fetch-helpers.test.js[123-148]
## Recommended Fix
For 502, 503, and 504 responses, classify the response as answered only when the parsed body matches this application's error envelope, including a valid message string; otherwise set `error.unanswered = true`. Add coverage for a gateway returning valid JSON without the application error shape while preserving the existing application-generated 503 behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

A 502, 503 or 504 counted as no answer only when its body was not JSON, so a proxy or gateway that answers in JSON of its own left the open without "Try again". This app answers every error with a message, and every 503 of its own has one, so a gateway status without a string message is no answer from it now, JSON or not. The table test holds the answer's status too, since a TypeError from reading a null body would have passed for no answer by accident. (Qodo on #299)

…s back after a failed recovery A 502, 503 or 504 counted as this app's answer when its JSON carried a message, which is exactly the shape of the usual JSON gateways. This app never answers 502 or 504, and every 503 of its own carries retryable; without it, any of the three is a gateway answering in its place now. The mapper tests hold that every 503 is retryable. An initialise that got no answer gets "Try again" again: the second try opens first and finds the pad if the first one set it up, and one still running is safe to meet, since the server compares the file before it writes, a file has one binding row and the pad that lost a race is rolled back. A recovery that got no answer is not offered again but opens the file, which shows the pad if it went through and the card if not. After a recovery that failed, the focus goes back to the card, as after a second try. The focus rule is one handFocusTo() in src/lib for both clients. The alert role sits on the message of each error card, the recovery card's and the viewer's too, not on a panel with buttons in it. The embed page shows its loading state in one place, and both clients name their error message and the missing-binding check once. The client tests take the server's answers from one file, the viewer's focus tests model Vue's order of drawing and focusing, and a spy no longer outlives a failing test.

…ery open from the loading state Two initialisations at once - the first unanswered but still running, the second from "Try again" - leave the loser with BindingNotCreatedException. Its sentence says to try again, but it carried no retryable, so neither client offered the button, though the next open finds the winner's pad. ApiErrorCode::retryable() takes it now. The create endpoints answer it with their own sentence and no flag, as before, and a test holds that. The embed page's run() starts from showLoading(), so showIframe() and showPadContentView() no longer hide the other panels themselves, and restartOpen() is gone: the button and the recovery call run(true). A test holds the generation guard after a success too: an open that succeeds late does not undo a newer failure.

qodo-free-for-open-source-projects · 2026-09-26T09:29:55Z

Code review by qodo was updated up to the latest commit 791c337

Jaggob added 5 commits September 25, 2026 23:27

qodo-free-for-open-source-projects Bot reviewed Sep 26, 2026

View reviewed changes

Comment thread src/lib/fetch-helpers.js Outdated

Jaggob marked this pull request as draft September 26, 2026 08:45

Jaggob added 2 commits September 26, 2026 10:58

Jaggob marked this pull request as ready for review September 26, 2026 09:27

Jaggob merged commit b6e405e into main Sep 26, 2026
27 checks passed

Jaggob deleted the fix/retry-open branch September 26, 2026 09:35

Jaggob mentioned this pull request Sep 26, 2026

Let the embed-create page wait for its create, and tell the host when the outcome is not known #300

Merged

Navigation Menu

Offer to try the open again wherever it may work later - #299

Offer to try the open again wherever it may work later#299
Jaggob merged 8 commits into
mainfrom
fix/retry-open

Jaggob commented Sep 26, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Sep 26, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 26, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 26, 2026 •

edited

Loading

Uh oh!

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Jaggob commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes

Checked in the stack

Bundles

Tests

Left as it is

Numbers

Uh oh!

qodo-code-review Bot commented Sep 26, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Jaggob commented Sep 26, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Sep 26, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Sep 26, 2026 •

edited

Loading