Add guard checks for isInPushedAnalysis to prevent undefined errors

[pygeek]

Error this.isInPushedAnalysis is not a function occurs when the app was automatically switching between games.

Fixed race condition where async socket events fire after isInPushedAnalysis is deleted during object destruction.

[github-actions]

Review Findings

I've analyzed this PR and found one issue that should be addressed:

Bug: Incomplete race condition fix in OGSConnectivity.ts:551-552

The guard check for isInPushedAnalysis is good, but leavePushedAnalysis() is called without a similar guard. Since both methods are deleted together during destruction (InteractiveBase.ts:553-554), the same race condition exists for leavePushedAnalysis.

Current code:

if (typeof this.isInPushedAnalysis === "function" && this.isInPushedAnalysis()) {
    this.leavePushedAnalysis();  // ⚠️ Can throw if deleted
}

Suggested fix:

if (typeof this.isInPushedAnalysis === "function" && this.isInPushedAnalysis()) {
    if (typeof this.leavePushedAnalysis === "function") {
        this.leavePushedAnalysis();
    }
}

The test should also be updated to verify this scenario by deleting both methods before calling the handler.

[github-actions]

Code Review Findings

After analyzing this PR, I found one bug that needs to be addressed:

Bug: Missing guard for leavePushedAnalysis()

Location: src/Goban/OGSConnectivity.ts:552

The fix adds a guard to check if isInPushedAnalysis exists before calling it, but then unconditionally calls leavePushedAnalysis() on the next line. Since leavePushedAnalysis is also deleted in InteractiveBase.destroy() (line 554), the same race condition exists.

Current code:

if (
    typeof this.isInPushedAnalysis === "function" &&
    this.isInPushedAnalysis()
) {
    this.leavePushedAnalysis();  // <-- No guard here
}

Suggested fix:

if (
    typeof this.isInPushedAnalysis === "function" &&
    this.isInPushedAnalysis()
) {
    if (typeof this.leavePushedAnalysis === "function") {
        this.leavePushedAnalysis();
    }
}

The test should also be updated to verify that leavePushedAnalysis is properly guarded.

[github-actions]

Issues Found

1. Inconsistent guard pattern with existing code

The PR adds checks like:

if (this.isInPushedAnalysis !== undefined && this.isInPushedAnalysis()) {

However, other files (CanvasRenderer.ts and SVGRenderer.ts) use a different pattern:

(typeof this.isInPushedAnalysis() !== "undefined" && this.isInPushedAnalysis())

This is problematic because:

• The existing pattern calls the function first before checking the type of its return value
• This means the existing code is already vulnerable to the same race condition bug
• These locations would also throw errors if isInPushedAnalysis is deleted during destruction

Locations affected:

• src/Goban/CanvasRenderer.ts:1283
• src/Goban/CanvasRenderer.ts:2313
• src/Goban/SVGRenderer.ts:1369-1370
• src/Goban/SVGRenderer.ts:2090-2091
• src/Goban/SVGRenderer.ts:3173

2. Recommendation

Consider fixing all instances to use the correct pattern:

if (typeof this.isInPushedAnalysis !== 'undefined' && this.isInPushedAnalysis()) {

This checks if the function exists before calling it, which is what you want to protect against the race condition.

[github-actions]

PR Review

Analyzed this PR for bugs and performance issues.

Summary

The fix correctly addresses the race condition where socket events fire after isInPushedAnalysis is deleted during object destruction. No significant issues found.

Analysis

✅ Core fix is sound: The guards prevent calling undefined as a function
✅ Tests are appropriate: The unit tests correctly simulate the race condition scenario
✅ No performance concerns: The additional existence checks are negligible overhead

Notes

The existing code in CanvasRenderer.ts and SVGRenderer.ts uses a different pattern that checks the return type of isInPushedAnalysis(), which would actually throw the same error if the function was deleted. Your fix is more correct. Those files likely don't encounter the race condition because rendering stops when the component is destroyed, whereas socket events can arrive asynchronously even after event handler cleanup begins.

Recommendation: Approve and merge.

[github-actions]

Critical Bug in Type Check

The guard checks added in this PR will not prevent the error. The condition:

if (this.isInPushedAnalysis !== "undefined" && this.isInPushedAnalysis()) {

is comparing the method to the string "undefined" rather than checking if it exists. This check will always be true (a function is never equal to a string), so it won't prevent calling undefined() after the method is deleted.

Expected behavior: The guard should prevent errors when isInPushedAnalysis is deleted during object destruction.

Actual behavior: The current code will still throw isInPushedAnalysis is not a function because:

• When the method exists: () => boolean !== "undefined" → true, calls the method ✓
• When the method is deleted: undefined !== "undefined" → true, attempts to call undefined() ✗

Solution: Use one of these correct patterns already used elsewhere in the codebase:

1. Use typeof operator (consistent with CanvasRenderer.ts:1283, SVGRenderer.ts:1369):

   if (typeof this.isInPushedAnalysis !== "undefined" && this.isInPushedAnalysis()) {

2. Use optional chaining (cleaner):

   if (this.isInPushedAnalysis?.()) {

References:

• src/Goban/CanvasRenderer.ts:1283
• src/Goban/SVGRenderer.ts:1369, 2090

[github-actions]

Code Review

I've analyzed this PR for bugs and performance issues. The fix correctly addresses the race condition where async socket events can fire after isInPushedAnalysis is deleted during object destruction.

Issue Found: Inconsistent Pattern Across Codebase

The same race condition exists in other files that weren't fixed in this PR:

CanvasRenderer.ts:1283 and SVGRenderer.ts:1369, 2090, 3173 use the buggy pattern:

typeof this.isInPushedAnalysis() !== "undefined"

This checks the return value instead of checking if the function exists, so it will still throw errors if called after destruction.

Recommendation: Apply the same fix pattern (typeof this.isInPushedAnalysis === "function" && this.isInPushedAnalysis()) to these locations to prevent the same race condition from occurring in the renderer code paths.

The fix in this PR is correct and the tests properly validate the behavior. The guards have negligible performance impact (typeof checks are extremely fast).

[github-actions]

I've completed a thorough review of this PR focusing on bugs and performance issues. The fix correctly addresses the race condition where async socket events can fire after isInPushedAnalysis is deleted during object destruction. The guard checks using typeof this.isInPushedAnalysis === "function" are properly implemented with short-circuit evaluation, ensuring no errors can occur. The tests adequately cover the scenarios. No issues found.

[anoek]

Thanks for the catch!