Copilot commented on code in PR #8178:
URL: https://github.com/apache/texera/pull/8178#discussion_r3893941822
##########
frontend/src/app/workspace/component/codearea-custom-template/codearea-custom-template.component.spec.ts:
##########
@@ -147,6 +147,75 @@ describe("CodeareaCustomTemplateComponent", () => {
expect(remoteComponent.componentRef).toBeDefined();
});
+ it("tears this client's editor down when any co-editor closes one,
whichever operator it names", () => {
+ // The mirror image of the open case above, and the half that was
missing: a co-editor
+ // closing the dialog has to close it here too, or this client keeps
typing into an editor
+ // the other side has already dismissed.
+ const closed = new Subject<{ operatorId: string }>();
+ vi.spyOn(TestBed.inject(CoeditorPresenceService),
"getCoeditorClosedCodeEditorSubject").mockReturnValue(
+ closed.asObservable()
+ );
+
+ const remoteFixture =
TestBed.createComponent(CodeareaCustomTemplateComponent);
+ const remoteComponent = remoteFixture.componentInstance;
+ remoteComponent.field = { props: {}, formControl: new FormControl() } as
any;
+ remoteFixture.detectChanges();
+ remoteComponent.openEditor();
+ expect(remoteComponent.isEditorOpen).toBe(true);
+
+ // The subscriber binds the payload to `_` and discards it, so a
deliberately foreign operator
+ // id is the honest fixture: naming this panel's own operator would
advertise a targeting
+ // filter the component does not have, and would leave a later filter
free to be added without
+ // any test noticing.
+ const destroySpy = vi.spyOn(remoteComponent.componentRef!, "destroy");
+ closed.next({ operatorId: "a-completely-unrelated-operator" });
+
+ // The spy is what proves teardown. The shared flag below is reachable
*without* any teardown:
+ // a subscriber that merely published `false` for this operator would
satisfy it through
+ // ngOnInit's getEditorState subscription while the dialog stayed on
screen. vi.spyOn calls
+ // through, so the real destroy still runs and the flag assertions still
describe the result.
+ expect(destroySpy).toHaveBeenCalledTimes(1);
+ expect(remoteComponent.isEditorOpen).toBe(false);
+ let published: boolean | undefined;
+ codeEditorService.getEditorState(highlightedOperatorId()).subscribe(v =>
(published = v));
+ expect(published).toBe(false);
+ });
+
+ it("stays quiet when a co-editor closes an editor this panel never
opened", async () => {
+ // componentRef is still undefined here; the optional call is what keeps
a close broadcast
+ // from throwing in every panel that happens to be mounted but closed. A
close broadcast
+ // reaches EVERY mounted codearea, so most recipients are in exactly
this state.
+ const closed = new Subject<{ operatorId: string }>();
+ vi.spyOn(TestBed.inject(CoeditorPresenceService),
"getCoeditorClosedCodeEditorSubject").mockReturnValue(
+ closed.asObservable()
+ );
+
+ const remoteFixture =
TestBed.createComponent(CodeareaCustomTemplateComponent);
+ const remoteComponent = remoteFixture.componentInstance;
+ remoteComponent.field = { props: {}, formControl: new FormControl() } as
any;
+ remoteFixture.detectChanges();
+
+ // A throw inside a subscriber does NOT propagate out of Subject.next()
- RxJS swallows it and
+ // reports it on its unhandled-error channel one macrotask later. So
`expect(...).not.toThrow()`
+ // would pass even with the optional call removed; watch that channel
instead.
+ const unhandled = vi.fn();
+ const previousHandler = rxjsConfig.onUnhandledError;
+ rxjsConfig.onUnhandledError = unhandled;
+ try {
+ closed.next({ operatorId: "a-completely-unrelated-operator" });
+ await new Promise(resolve => setTimeout(resolve, 0));
+ } finally {
+ rxjsConfig.onUnhandledError = previousHandler;
+ }
Review Comment:
Mutating `rxjsConfig.onUnhandledError` changes a global RxJS setting. If the
test runner executes spec files in parallel, another test that triggers an RxJS
unhandled error during this window could be captured by `unhandled` and fail
this test (or vice versa). Prefer isolating this by running the
timer/error-flush deterministically under fake timers (minimizing the window),
and/or centralizing the handler swap in `beforeEach/afterEach` for the file so
it’s consistently restored even if the test aborts early.
##########
frontend/src/app/workspace/component/codearea-custom-template/codearea-custom-template.component.spec.ts:
##########
@@ -147,6 +147,75 @@ describe("CodeareaCustomTemplateComponent", () => {
expect(remoteComponent.componentRef).toBeDefined();
});
+ it("tears this client's editor down when any co-editor closes one,
whichever operator it names", () => {
+ // The mirror image of the open case above, and the half that was
missing: a co-editor
+ // closing the dialog has to close it here too, or this client keeps
typing into an editor
+ // the other side has already dismissed.
+ const closed = new Subject<{ operatorId: string }>();
+ vi.spyOn(TestBed.inject(CoeditorPresenceService),
"getCoeditorClosedCodeEditorSubject").mockReturnValue(
+ closed.asObservable()
+ );
+
+ const remoteFixture =
TestBed.createComponent(CodeareaCustomTemplateComponent);
+ const remoteComponent = remoteFixture.componentInstance;
+ remoteComponent.field = { props: {}, formControl: new FormControl() } as
any;
+ remoteFixture.detectChanges();
+ remoteComponent.openEditor();
+ expect(remoteComponent.isEditorOpen).toBe(true);
+
+ // The subscriber binds the payload to `_` and discards it, so a
deliberately foreign operator
+ // id is the honest fixture: naming this panel's own operator would
advertise a targeting
+ // filter the component does not have, and would leave a later filter
free to be added without
+ // any test noticing.
+ const destroySpy = vi.spyOn(remoteComponent.componentRef!, "destroy");
+ closed.next({ operatorId: "a-completely-unrelated-operator" });
+
+ // The spy is what proves teardown. The shared flag below is reachable
*without* any teardown:
+ // a subscriber that merely published `false` for this operator would
satisfy it through
+ // ngOnInit's getEditorState subscription while the dialog stayed on
screen. vi.spyOn calls
+ // through, so the real destroy still runs and the flag assertions still
describe the result.
+ expect(destroySpy).toHaveBeenCalledTimes(1);
+ expect(remoteComponent.isEditorOpen).toBe(false);
+ let published: boolean | undefined;
+ codeEditorService.getEditorState(highlightedOperatorId()).subscribe(v =>
(published = v));
+ expect(published).toBe(false);
Review Comment:
This subscription is never unsubscribed, which can leak across tests if the
underlying observable is long-lived (and can make later tests more
order-dependent). Prefer a one-shot read (e.g., `take(1)` or converting to a
promise via `firstValueFrom`) so the test asserts the current value without
leaving a live subscription behind.
##########
frontend/src/app/workspace/component/codearea-custom-template/codearea-custom-template.component.spec.ts:
##########
@@ -147,6 +147,75 @@ describe("CodeareaCustomTemplateComponent", () => {
expect(remoteComponent.componentRef).toBeDefined();
});
+ it("tears this client's editor down when any co-editor closes one,
whichever operator it names", () => {
+ // The mirror image of the open case above, and the half that was
missing: a co-editor
+ // closing the dialog has to close it here too, or this client keeps
typing into an editor
+ // the other side has already dismissed.
+ const closed = new Subject<{ operatorId: string }>();
+ vi.spyOn(TestBed.inject(CoeditorPresenceService),
"getCoeditorClosedCodeEditorSubject").mockReturnValue(
+ closed.asObservable()
+ );
+
+ const remoteFixture =
TestBed.createComponent(CodeareaCustomTemplateComponent);
+ const remoteComponent = remoteFixture.componentInstance;
+ remoteComponent.field = { props: {}, formControl: new FormControl() } as
any;
+ remoteFixture.detectChanges();
+ remoteComponent.openEditor();
+ expect(remoteComponent.isEditorOpen).toBe(true);
+
+ // The subscriber binds the payload to `_` and discards it, so a
deliberately foreign operator
+ // id is the honest fixture: naming this panel's own operator would
advertise a targeting
+ // filter the component does not have, and would leave a later filter
free to be added without
+ // any test noticing.
+ const destroySpy = vi.spyOn(remoteComponent.componentRef!, "destroy");
+ closed.next({ operatorId: "a-completely-unrelated-operator" });
+
+ // The spy is what proves teardown. The shared flag below is reachable
*without* any teardown:
+ // a subscriber that merely published `false` for this operator would
satisfy it through
+ // ngOnInit's getEditorState subscription while the dialog stayed on
screen. vi.spyOn calls
+ // through, so the real destroy still runs and the flag assertions still
describe the result.
+ expect(destroySpy).toHaveBeenCalledTimes(1);
+ expect(remoteComponent.isEditorOpen).toBe(false);
+ let published: boolean | undefined;
+ codeEditorService.getEditorState(highlightedOperatorId()).subscribe(v =>
(published = v));
+ expect(published).toBe(false);
+ });
+
+ it("stays quiet when a co-editor closes an editor this panel never
opened", async () => {
+ // componentRef is still undefined here; the optional call is what keeps
a close broadcast
+ // from throwing in every panel that happens to be mounted but closed. A
close broadcast
+ // reaches EVERY mounted codearea, so most recipients are in exactly
this state.
+ const closed = new Subject<{ operatorId: string }>();
+ vi.spyOn(TestBed.inject(CoeditorPresenceService),
"getCoeditorClosedCodeEditorSubject").mockReturnValue(
+ closed.asObservable()
+ );
+
+ const remoteFixture =
TestBed.createComponent(CodeareaCustomTemplateComponent);
+ const remoteComponent = remoteFixture.componentInstance;
+ remoteComponent.field = { props: {}, formControl: new FormControl() } as
any;
+ remoteFixture.detectChanges();
+
+ // A throw inside a subscriber does NOT propagate out of Subject.next()
- RxJS swallows it and
+ // reports it on its unhandled-error channel one macrotask later. So
`expect(...).not.toThrow()`
+ // would pass even with the optional call removed; watch that channel
instead.
+ const unhandled = vi.fn();
+ const previousHandler = rxjsConfig.onUnhandledError;
+ rxjsConfig.onUnhandledError = unhandled;
+ try {
+ closed.next({ operatorId: "a-completely-unrelated-operator" });
+ await new Promise(resolve => setTimeout(resolve, 0));
+ } finally {
+ rxjsConfig.onUnhandledError = previousHandler;
Review Comment:
The test relies on a real macrotask (`setTimeout(0)`) to flush RxJS’s
unhandled-error reporting, which can introduce avoidable nondeterminism and
slowdowns across the suite. Consider using Vitest fake timers for this test and
advancing timers explicitly so the behavior is deterministic and doesn’t wait
on real time.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
