You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "yangzhang75 (via GitHub)" <gi...@apache.org> on 2026/09/18 22:22:54 UTC
[PR] fix(workflow): stop the workspace destroying itself on beforeunload [texera]
yangzhang75 opened a new pull request, #8600:
URL: https://github.com/apache/texera/pull/8600
### What changes were proposed in this PR?
Switching between the operator canvas and the Form View is a full-page navigation, and both pages ran their entire teardown from a `beforeunload` host binding: clear the graph, destroy the Yjs shared document, disconnect the computing unit, reset the execution state and the results.
A browser does not always discard a document it navigates away from. Chrome may keep it in the back/forward cache, and going back restores the JavaScript state exactly as it was left, re-running nothing. What came back was the page these methods had already gutted:
- an empty canvas that answered no clicks, because the graph had been cleared;
- a workflow id reset to `DEFAULT_WORKFLOW`'s, which the share dialog then asked the backend about;
- an autosave still subscribed to that reset metadata, which wrote the default out as a brand-new blank workflow, so the workflow list grew by one on every trip.
There was never anything to tear down there. A document that really is discarded takes its websockets and its graph with it, and a document that comes back needs them. So `beforeunload` now only saves, and the teardown stays in `ngOnDestroy`, which runs when the page is genuinely replaced. Both views change the same way.
Two more defects turned this into a silent failure, and both are fixed here:
- `WorkflowResource.getWorkflowType` read `workflowDao.fetchOneByWid(wid)` and dereferenced it, so an id matching no row answered 500 with a stack trace rather than 404.
- The share dialog's publish-state subscription had no error handler. A failed request left `isPublic` null, and the template hides the Private/Public choice on exactly that (`*ngIf="isPublic !== null"`), so the dialog looked complete while offering one control fewer and the only way to find out was the network tab.
**One behaviour changes deliberately.** The shared document is no longer destroyed on unload, so a co-editor is no longer told explicitly that you left; the room notices when the socket closes with the document. Destroying it on unload is what made a restored page unusable, and a restored page needs its room.
### Any related issues, documentation, discussions?
Closes #8599.
### How was this PR tested?
Reproduced first, on a local instance running plain `main` (`e7d1676e1`): open a saved workflow, switch to the Form View, press the browser's Back button, and the canvas comes back blank and unclickable, the share dialog has no Private/Public choice, and the workflow list has gained a blank workflow. With this branch deployed to the same instance, none of the three happens.
The endpoint was checked against that instance directly: `GET /api/workflow/type/0` and `/type/999999` answered 500 before and answer 404 after.
Unit tests:
- `workspace.component.spec` and `workflow-form.component.spec`: `beforeunload` saves and tears nothing down; the existing tests that `ngOnDestroy` still tears everything down are unchanged and still pass.
- `workflow-form.rendered.spec`: the existing test that dispatches a real `beforeunload` DOM event now asserts the new contract, so the host binding being removed or miswired is still caught.
- `share-access.component.spec`: a failed publish-state request reports itself instead of hiding the choice silently.
- `WorkflowResourceCoverSpec`: `getWorkflowType` reports the publish state, and throws `NotFoundException` for an id that matches no workflow.
Each new guard was deletion-checked: restoring the teardown on `beforeunload` in either view, or dropping the error handler, turns exactly one named test red.
Full frontend suite: 224 files, 6129 passed, 1 skipped (pre-existing), 0 failed. `WorkflowExecutionService/testOnly ... WorkflowResourceCoverSpec`: 16 passed. `ng build --configuration=production` (AOT), `eslint`, `prettier --check`, `scalafmtCheck` on main and test sources: all clean.
**Not covered:** the issue reports reaching this by switching with the in-page button rather than the Back button. That path is a normal navigation, which does not use the back/forward cache, and it does not reproduce for me on plain `main`. If it turns out to have a separate cause, the two fixes above mean it will now announce itself rather than silently dropping a control.
### Was this PR authored or co-authored using generative AI tooling?
Yes. Generated-by: Claude Code (Claude Opus 5, Anthropic). Co-authored with Claude; the author reviewed the change line by line and reproduced both the failure and the fix in a running instance before submission.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): stop the workspace destroying itself on beforeunload [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8600:
URL: https://github.com/apache/texera/pull/8600#discussion_r4051213142
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.ts:
##########
@@ -137,10 +137,19 @@ export class ShareAccessComponent implements OnInit, OnDestroy {
this.owner = name;
});
// Stays null for kinds that cannot be published, which is what hides the publish buttons.
+ // A failed request leaves it null too, and the buttons are equally gone, so say so: without
+ // this the dialog looked complete while quietly offering one control fewer, and the only way
+ // to find out was the network tab.
this.descriptor
Review Comment:
Several successful access updates call `ngOnInit()` again to refresh this dialog. If the publish-state request then fails, `next` is never called and `isPublic` retains its previous value, so the notification appears while stale Private/Public controls remain enabled. Reset `isPublic` before starting each refresh so the controls stay hidden until a current value is available.
##########
frontend/src/app/workspace/component/workspace.component.spec.ts:
##########
@@ -497,6 +497,35 @@ describe("WorkspaceComponent", () => {
expect(workflowResultService.clearResults).toHaveBeenCalled();
});
+ // A full-page navigation away fires beforeunload, and the browser may then keep this document
+ // in its back/forward cache instead of discarding it. Coming back restores the JavaScript
+ // state as it was left and re-runs nothing, so anything torn down here stays torn down: the
+ // graph came back empty, the workflow id came back as the default, and the still-subscribed
+ // autosave then wrote that default out as a new, blank workflow (issue #8599).
+ it("saves on beforeunload and tears nothing down, so a page restored from the cache still works", async () => {
+ await createFixture();
+ fixture.detectChanges();
+
+ component.onBeforeUnload();
Review Comment:
This calls `onBeforeUnload()` directly, so the test bypasses Angular's `@HostListener` and would still pass if the browser event binding were removed or miswired. Dispatch a real `beforeunload` event, as the Form View rendered test does, so this regression test covers the user-facing event path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): stop the workspace destroying itself on beforeunload [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #8600:
URL: https://github.com/apache/texera/pull/8600#discussion_r4051309574
##########
frontend/src/app/workspace/component/workspace.component.ts:
##########
@@ -173,13 +173,27 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy {
this.codeEditorService.vc = this.codeEditorViewRef;
}
+ /**
+ * The browser is leaving this document: save the workflow, and change nothing else.
+ *
+ * Tearing the session down here was the cause of a page that came back dead. A full-page
+ * navigation away (the Form View switch is one) fires this, and the browser may then keep the
+ * document in its back/forward cache rather than discarding it. Coming back restores the
+ * JavaScript state exactly as it was left, so whatever this method had already destroyed stayed
+ * destroyed: an empty graph on a canvas that answered no clicks, and a workflow id reset to the
+ * default, which the share dialog then asked the backend about and got an error for. Nothing
+ * re-runs on a restore, because the component was never re-created.
+ *
+ * There is nothing to tear down on the way out anyway. A document that is really discarded takes
+ * its websockets and its graph with it, and a document that comes back needs them.
+ */
@HostListener("window:beforeunload")
- ngOnDestroy() {
- if (this.userService.isLogin() && this.workflowPersistService.isWorkflowPersistEnabled()) {
- const workflow = this.workflowActionService.getWorkflow();
- this.workflowPersistService.persistWorkflow(workflow).pipe(untilDestroyed(this)).subscribe();
- }
+ onBeforeUnload(): void {
Review Comment:
There is a second way to the same symptom that this does not close, and it is the gesture #8599 reports (the in-page switch, not Back). `ngAfterViewInit` calls `resetAsNewWorkflow()`, which puts the metadata at `DEFAULT_WORKFLOW` (wid 0) on every canvas load, and the real wid only arrives with the workflow. The Share button carries no gate (`menu.component.html`, no `disabled`/`*ngIf`) and the loading spinner sits in its own container, so a dialog opened in that window asks `GET /workflow/type/0` and loses the Private/Public choice for good — now with a toast rather than silently, which is an improvement but not the same as having the buttons. That window is exactly where a user lands right after the full-page switch, which may also be why the in-page path did not reproduce: it needs the click to come before the workflow does. Worth either gating the dialog on a loaded workflow or leaving #8599 open for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): stop the workspace destroying itself on beforeunload [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 merged PR #8600:
URL: https://github.com/apache/texera/pull/8600
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): stop the workspace destroying itself on beforeunload [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #8600:
URL: https://github.com/apache/texera/pull/8600#discussion_r4051306793
##########
frontend/src/app/workspace/component/workspace.component.ts:
##########
@@ -173,13 +173,27 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy {
this.codeEditorService.vc = this.codeEditorViewRef;
}
+ /**
+ * The browser is leaving this document: save the workflow, and change nothing else.
+ *
+ * Tearing the session down here was the cause of a page that came back dead. A full-page
+ * navigation away (the Form View switch is one) fires this, and the browser may then keep the
+ * document in its back/forward cache rather than discarding it. Coming back restores the
+ * JavaScript state exactly as it was left, so whatever this method had already destroyed stayed
+ * destroyed: an empty graph on a canvas that answered no clicks, and a workflow id reset to the
+ * default, which the share dialog then asked the backend about and got an error for. Nothing
+ * re-runs on a restore, because the component was never re-created.
+ *
+ * There is nothing to tear down on the way out anyway. A document that is really discarded takes
+ * its websockets and its graph with it, and a document that comes back needs them.
+ */
@HostListener("window:beforeunload")
- ngOnDestroy() {
- if (this.userService.isLogin() && this.workflowPersistService.isWorkflowPersistEnabled()) {
- const workflow = this.workflowActionService.getWorkflow();
- this.workflowPersistService.persistWorkflow(workflow).pipe(untilDestroyed(this)).subscribe();
- }
+ onBeforeUnload(): void {
Review Comment:
There is a second way to the same symptom that this does not close, and it is the gesture #8599 reports (the in-page switch, not Back). `ngAfterViewInit` calls `resetAsNewWorkflow()`, which puts the metadata at `DEFAULT_WORKFLOW` (wid 0) on every canvas load, and the real wid only arrives with the workflow. The Share button carries no gate (`menu.component.html`, no `disabled`/`*ngIf`) and the loading spinner sits in its own container, so a dialog opened in that window asks `GET /workflow/type/0` and loses the Private/Public choice for good — now with a toast rather than silently, which is an improvement but not the same as having the buttons. That window is exactly where a user lands right after the full-page switch, which may also be why the in-page path did not reproduce: it needs the click to come before the workflow does. Worth either gating the dialog on a loaded workflow or leaving #8599 open for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): stop the workspace destroying itself on beforeunload [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8600:
URL: https://github.com/apache/texera/pull/8600#issuecomment-5736900574
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@mengw15`, `@tanishqgandhi1908`, `@Ma77Ball`
You can notify them by mentioning `@mengw15`, `@tanishqgandhi1908`, `@Ma77Ball` in a comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): stop the workspace destroying itself on beforeunload [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8600:
URL: https://github.com/apache/texera/pull/8600#issuecomment-5736899940
<!-- backport-auto-label-report -->
### Backport auto-label report
This `fix:` PR was checked against each actively-supported release branch. A `release/*` label nominates a backport target; the branch's release manager approving this PR is what sends the fix there. The required **Backport Approvals** check stays red until every label below is approved, so each manager either approves or removes their own label — which is why the labels left on a merged PR are exactly the branches it reached.
| Release branch | Analysis |
| --- | --- |
| ✅ `release/v1.3` | Change detected on this branch — label added; this fix is queued to backport here. @mengw15 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other. Review requested. |
| ✅ `release/v1.2` | Change detected on this branch — label added; this fix is queued to backport here. @xuang7 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other. Review requested. |
_[Auto-label run](https://github.com/apache/texera/actions/runs/35401286637)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): stop the workspace destroying itself on beforeunload [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #8600:
URL: https://github.com/apache/texera/pull/8600#issuecomment-5737065213
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/8600?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 92.78%. Comparing base ([`d118784`](https://app.codecov.io/gh/apache/texera/commit/d11878426b59967e5466ef6a927ef1a97d63b606?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b86bb8b`](https://app.codecov.io/gh/apache/texera/commit/b86bb8b8f268ee204241cc3f6b66769ae179f46d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #8600 +/- ##
=========================================
Coverage 92.78% 92.78%
Complexity 4899 4899
=========================================
Files 1236 1236
Lines 52120 52139 +19
Branches 6405 6408 +3
=========================================
+ Hits 48357 48377 +20
Misses 2186 2186
+ Partials 1577 1576 -1
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `71.78% <ø> (ø)` | | Carriedforward from [d118784](https://app.codecov.io/gh/apache/texera/commit/d11878426b59967e5466ef6a927ef1a97d63b606?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `99.32% <ø> (ø)` | | Carriedforward from [d118784](https://app.codecov.io/gh/apache/texera/commit/d11878426b59967e5466ef6a927ef1a97d63b606?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `88.61% <ø> (ø)` | | Carriedforward from [d118784](https://app.codecov.io/gh/apache/texera/commit/d11878426b59967e5466ef6a927ef1a97d63b606?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.20% <ø> (ø)` | | Carriedforward from [d118784](https://app.codecov.io/gh/apache/texera/commit/d11878426b59967e5466ef6a927ef1a97d63b606?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.37% <ø> (ø)` | | Carriedforward from [d118784](https://app.codecov.io/gh/apache/texera/commit/d11878426b59967e5466ef6a927ef1a97d63b606?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.53% <ø> (ø)` | | Carriedforward from [d118784](https://app.codecov.io/gh/apache/texera/commit/d11878426b59967e5466ef6a927ef1a97d63b606?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `96.58% <100.00%> (+<0.01%)` | :arrow_up: | |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.73% <ø> (ø)` | | Carriedforward from [d118784](https://app.codecov.io/gh/apache/texera/commit/d11878426b59967e5466ef6a927ef1a97d63b606?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `98.48% <ø> (ø)` | | Carriedforward from [d118784](https://app.codecov.io/gh/apache/texera/commit/d11878426b59967e5466ef6a927ef1a97d63b606?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/8600/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `74.09% <ø> (ø)` | | Carriedforward from [d118784](https://app.codecov.io/gh/apache/texera/commit/d11878426b59967e5466ef6a927ef1a97d63b606?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/8600?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): stop pages destroying themselves on beforeunload [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8600:
URL: https://github.com/apache/texera/pull/8600#issuecomment-5736989449
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 4 better · 🔴 3 worse · ⚪ 8 noise (<±5%) · 0 without baseline
> Compared against main `e7d1676` benchmarked on **this same runner**, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.
[Dashboard](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/35401589478)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| 🔴 | bs=10 sw=10 sl=64 | 392 | 0.239 | 24,952/33,121/33,121 us | 🔴 +19.5% / 🔴 +114.3% |
| 🟢 | bs=100 sw=10 sl=64 | 803 | 0.49 | 123,603/147,189/147,189 us | 🟢 -14.1% / 🔴 +39.0% |
| ⚪ | bs=1000 sw=10 sl=64 | 909 | 0.555 | 1,091,171/1,179,158/1,179,158 us | ⚪ within ±5% / 🔴 +15.7% |
<details><summary>Baseline details</summary>
Latest main `e7d1676` from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 392 tuples/sec | 419 tuples/sec | 796.29 tuples/sec | -6.4% | -50.8% |
| bs=10 sw=10 sl=64 | MB/s | 0.239 MB/s | 0.256 MB/s | 0.486 MB/s | -6.6% | -50.8% |
| bs=10 sw=10 sl=64 | p50 | 24,952 us | 20,883 us | 12,549 us | +19.5% | +98.8% |
| bs=10 sw=10 sl=64 | p95 | 33,121 us | 36,738 us | 15,458 us | -9.8% | +114.3% |
| bs=10 sw=10 sl=64 | p99 | 33,121 us | 36,738 us | 19,275 us | -9.8% | +71.8% |
| bs=100 sw=10 sl=64 | throughput | 803 tuples/sec | 812 tuples/sec | 1,027 tuples/sec | -1.1% | -21.8% |
| bs=100 sw=10 sl=64 | MB/s | 0.49 MB/s | 0.496 MB/s | 0.627 MB/s | -1.2% | -21.8% |
| bs=100 sw=10 sl=64 | p50 | 123,603 us | 120,603 us | 100,085 us | +2.5% | +23.5% |
| bs=100 sw=10 sl=64 | p95 | 147,189 us | 171,286 us | 105,895 us | -14.1% | +39.0% |
| bs=100 sw=10 sl=64 | p99 | 147,189 us | 171,286 us | 116,076 us | -14.1% | +26.8% |
| bs=1000 sw=10 sl=64 | throughput | 909 tuples/sec | 914 tuples/sec | 1,055 tuples/sec | -0.5% | -13.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.555 MB/s | 0.558 MB/s | 0.644 MB/s | -0.5% | -13.8% |
| bs=1000 sw=10 sl=64 | p50 | 1,091,171 us | 1,093,690 us | 979,292 us | -0.2% | +11.4% |
| bs=1000 sw=10 sl=64 | p95 | 1,179,158 us | 1,138,720 us | 1,018,835 us | +3.6% | +15.7% |
| bs=1000 sw=10 sl=64 | p99 | 1,179,158 us | 1,138,720 us | 1,039,692 us | +3.6% | +13.4% |
</details>
<details><summary>Raw CSV</summary>
```csv
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,510.06,200,128000,392,0.239,24952.06,33121.10,33121.10
1,100,10,64,20,2489.93,2000,1280000,803,0.490,123602.77,147188.65,147188.65
2,1000,10,64,20,22006.74,20000,12800000,909,0.555,1091170.60,1179158.41,1179158.41
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): stop the workspace destroying itself on beforeunload [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on code in PR #8600:
URL: https://github.com/apache/texera/pull/8600#discussion_r4051387412
##########
frontend/src/app/workspace/component/workspace.component.ts:
##########
@@ -173,13 +173,27 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy {
this.codeEditorService.vc = this.codeEditorViewRef;
}
+ /**
+ * The browser is leaving this document: save the workflow, and change nothing else.
+ *
+ * Tearing the session down here was the cause of a page that came back dead. A full-page
+ * navigation away (the Form View switch is one) fires this, and the browser may then keep the
+ * document in its back/forward cache rather than discarding it. Coming back restores the
+ * JavaScript state exactly as it was left, so whatever this method had already destroyed stayed
+ * destroyed: an empty graph on a canvas that answered no clicks, and a workflow id reset to the
+ * default, which the share dialog then asked the backend about and got an error for. Nothing
+ * re-runs on a restore, because the component was never re-created.
+ *
+ * There is nothing to tear down on the way out anyway. A document that is really discarded takes
+ * its websockets and its graph with it, and a document that comes back needs them.
+ */
@HostListener("window:beforeunload")
- ngOnDestroy() {
- if (this.userService.isLogin() && this.workflowPersistService.isWorkflowPersistEnabled()) {
- const workflow = this.workflowActionService.getWorkflow();
- this.workflowPersistService.persistWorkflow(workflow).pipe(untilDestroyed(this)).subscribe();
- }
+ onBeforeUnload(): void {
Review Comment:
Gated. The Share button is disabled until the workflow id arrives (`[disabled]="!workflowId"`) and `onClickOpenShareAccess` refuses the same window, so the dialog can no longer be opened on wid 0. Checked your reading of it first and it holds: `resetAsNewWorkflow()` at `workspace.component.ts:163`, no gate on the button, and `<texera-menu>` sits outside the spinner container (`workspace.component.html:31`) — which is also why the in-page path never reproduced for me. Tests: the dialog is not opened for wid 0 or for no id, and the button is disabled until the id arrives; both deletion-checked. 7aa3c77
##########
frontend/src/app/workspace/component/workspace.component.ts:
##########
@@ -173,13 +173,27 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy {
this.codeEditorService.vc = this.codeEditorViewRef;
}
+ /**
+ * The browser is leaving this document: save the workflow, and change nothing else.
+ *
+ * Tearing the session down here was the cause of a page that came back dead. A full-page
+ * navigation away (the Form View switch is one) fires this, and the browser may then keep the
+ * document in its back/forward cache rather than discarding it. Coming back restores the
+ * JavaScript state exactly as it was left, so whatever this method had already destroyed stayed
+ * destroyed: an empty graph on a canvas that answered no clicks, and a workflow id reset to the
+ * default, which the share dialog then asked the backend about and got an error for. Nothing
+ * re-runs on a restore, because the component was never re-created.
+ *
+ * There is nothing to tear down on the way out anyway. A document that is really discarded takes
+ * its websockets and its graph with it, and a document that comes back needs them.
+ */
@HostListener("window:beforeunload")
- ngOnDestroy() {
- if (this.userService.isLogin() && this.workflowPersistService.isWorkflowPersistEnabled()) {
- const workflow = this.workflowActionService.getWorkflow();
- this.workflowPersistService.persistWorkflow(workflow).pipe(untilDestroyed(this)).subscribe();
- }
+ onBeforeUnload(): void {
Review Comment:
Gated. The Share button is disabled until the workflow id arrives (`[disabled]="!workflowId"`) and `onClickOpenShareAccess` refuses the same window, so the dialog can no longer be opened on wid 0. Checked your reading of it first and it holds: `resetAsNewWorkflow()` at `workspace.component.ts:163`, no gate on the button, and `<texera-menu>` sits outside the spinner container (`workspace.component.html:31`) — which is also why the in-page path never reproduced for me. Tests: the dialog is not opened for wid 0 or for no id, and the button is disabled until the id arrives; both deletion-checked. 7aa3c77
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.ts:
##########
@@ -137,10 +137,19 @@ export class ShareAccessComponent implements OnInit, OnDestroy {
this.owner = name;
});
// Stays null for kinds that cannot be published, which is what hides the publish buttons.
+ // A failed request leaves it null too, and the buttons are equally gone, so say so: without
+ // this the dialog looked complete while quietly offering one control fewer, and the only way
+ // to find out was the network tab.
this.descriptor
Review Comment:
Fixed. `isPublic` is reset in the error handler, so a failed refresh drops the previous value instead of leaving the buttons on screen showing a state nothing has confirmed — which is what the toast already claimed. Test: a second `ngOnInit` whose read fails leaves `isPublic` null; deletion-checked. 7aa3c77
##########
frontend/src/app/workspace/component/workspace.component.spec.ts:
##########
@@ -497,6 +497,35 @@ describe("WorkspaceComponent", () => {
expect(workflowResultService.clearResults).toHaveBeenCalled();
});
+ // A full-page navigation away fires beforeunload, and the browser may then keep this document
+ // in its back/forward cache instead of discarding it. Coming back restores the JavaScript
+ // state as it was left and re-runs nothing, so anything torn down here stays torn down: the
+ // graph came back empty, the workflow id came back as the default, and the still-subscribed
+ // autosave then wrote that default out as a new, blank workflow (issue #8599).
+ it("saves on beforeunload and tears nothing down, so a page restored from the cache still works", async () => {
+ await createFixture();
+ fixture.detectChanges();
+
+ component.onBeforeUnload();
Review Comment:
Fixed. The test dispatches a real `beforeunload` event now, as the Form View rendered test does. Deletion-checked: removing the `@HostListener` turns it red, which it would not have before. 7aa3c77
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org