Do not let a page that was navigated away from fail the one that replaced it - #602
Merged
Merged
Conversation
…aced it `testDOCXEditMode` and `testODTEditMode` have been failing an api level at a time all week, always with "webview did not answer in 10000ms document=no result" - and `no result` is the tell: `lastDocument` is only null once something called `unload`, so the document had not merely failed to become editable, it had been given up on. Logcat from a failing run says what did it. A page that never commits leaves a reload scheduled 2.5s out - the workaround for a webview reporting progress 100 over a blank page - and nothing cancelled it when the next page was asked for. The test before had deliberately loaded a page that 404s, so its retry fired inside the next test, went back to a server that had gone with the document that owned it, and got a 404. `onReceivedHttpError` reported that against whatever document was on screen by then, which was the docx being opened for edit mode. So `loadUrl` cancels a retry the page before it left waiting, the way `destroy` already does when the whole view is replaced. And `failPage` answers only for the page it was last asked to load: a request made for a document already closed can still be answered long after, and the document on screen is not the one that failed. Nothing is loosened. `LandingTests.aDocumentThatFailsToOpenComesBackToTheList` holds the case that matters - a document whose page really is a 404 - and the whole instrumented suite, 80 tests, passes locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML
![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: ec683acd64
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Cancelling on `loadUrl` clears the retries queued until then, and not the one queued after: page A can finish - and schedule its retry - once B has already been asked for. `wasCommitCalled` is about whichever page is being waited on, so if B has not committed within the 2.5s, A's retry reads B's flag, believes it is about itself, and loads A back over B. Which is the navigation this set out to stop, arrived at from the other side. So the retry asks whether the page it names is still the page wanted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML
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.
Stacked on #600.
testDOCXEditModeandtestODTEditModehave been failing an api level at a time all week — six reruns in one afternoon — always with:document=no resultis the tell.lastDocumentis only null once something calledunload, so the document had not merely failed to become editable: it had been given up on.What logcat from a failing run says
A page that never commits leaves a reload scheduled 2.5s out — the workaround for a webview that reports progress 100 over a blank page — and nothing cancelled it when the next page was asked for. The previous test had deliberately loaded a page that 404s, so its retry fired inside the next test, went back to a server that had gone with the document that owned it, and got a 404.
onReceivedHttpErrorreported that against whatever document was on screen by then.The 404 arrives 200ms after the docx starts loading, which is far too soon to be that document's own page.
The fix
loadUrlcancels a retry the page before it left waiting, the waydestroyalready does when the whole view is replaced. Same handler, the gap was a new url in the same view.failPageanswers only for the page it was last asked to load.Nothing is loosened:
LandingTests.aDocumentThatFailsToOpenComesBackToTheListholds the case that matters — a document whose page really is a 404 — and the whole instrumented suite, 80 tests, passes locally on API 29.A flake fix cannot be proven by one green run; what can be shown is the mechanism, and that is in the log above.
🤖 Generated with Claude Code