fix: add comprehensive error handling to removeFromWindow DOM cleanup

[flychen50]

Fixes memory leak issues in DOM cleanup as described in issue #28.

Changes

• Add try-catch blocks around element removal operations
• Ensure _addedElementIDs array is always cleared to prevent memory leaks
• Add null checks and error handling for FTL element removal
• Improve logging for DOM cleanup failures
• Prevent crashes during plugin shutdown/window cleanup

Fixes #28

🤖 Generated with Claude Code

[gemini-code-assist]

Summary of Changes

Hello @flychen50, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses memory leak issues and enhances the stability of DOM cleanup operations within the removeFromWindow function. It introduces comprehensive error handling using try-catch blocks to ensure that element removal processes are robust and do not lead to application crashes. Furthermore, it guarantees that tracking arrays are properly cleared to prevent memory leaks, even if errors occur during the cleanup, and improves logging for better diagnostics.

Highlights

• Enhanced Error Handling: The removeFromWindow function now includes robust try-catch blocks to prevent crashes during DOM element removal, ensuring the application remains stable even if individual element removals fail.
• Memory Leak Prevention: The _addedElementIDs array, which tracks added DOM elements, is now guaranteed to be cleared regardless of whether element removal operations succeed or fail, effectively preventing memory leaks.
• FTL Element Robustness: Specific error handling has been added for the removal of the citation-counts.ftl element, making the cleanup process more resilient to issues with this particular component.
• Improved Logging: Logging has been improved to provide more detailed warnings and error messages when DOM cleanup operations encounter issues, aiding in debugging and monitoring.
• Crash Prevention: By catching and handling errors during DOM cleanup, the changes prevent potential crashes that could occur during plugin shutdown or window cleanup processes.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[claude]

Claude finished @flychen50's task —— View job

---

PR Review: DOM Cleanup Error Handling

Reviewing the changes to address memory leak issues in DOM cleanup (issue #28).

Todo List

• Read and analyze the modified code
• Evaluate error handling implementation
• Check memory leak prevention
• Assess code quality and best practices
• Provide comprehensive review feedback

Overall Assessment: ✅ APPROVED with Minor Suggestions

This PR successfully addresses the memory leak issues described in issue #28 with comprehensive error handling. The implementation is solid and follows good defensive programming practices.

Strengths

1. Robust Error Handling: The nested try-catch structure ensures that even if individual element removal fails, the cleanup process continues and the tracking array is always cleared.

2. Memory Leak Prevention: The critical fix of ensuring _addedElementIDs = [] is always executed (lines 307 and 320) effectively prevents the memory leak described in issue Fix memory leak in DOM cleanup #28.

3. Granular Error Logging: Individual element removal errors are logged with specific element IDs, making debugging much easier.

4. FTL Element Safety: Separate error handling for FTL element removal prevents failures in one part from affecting the other.

5. Fail-Safe Design: The outer try-catch ensures that even if the entire cleanup process encounters unexpected errors, the tracking array is still cleared.

Code Quality

• Error Handling Pattern: The nested try-catch structure is appropriate here - inner catches handle specific operation failures while the outer catch provides a safety net
• Logging Strategy: Good use of warning vs error log levels to distinguish between expected vs unexpected failures
• Code Style: Consistent with the existing codebase patterns
• Null Safety: Proper null checks before element removal

Performance Considerations

• Minimal Overhead: The try-catch blocks add negligible performance overhead
• No Resource Leaks: Proper cleanup ensures no DOM references are retained

Security Assessment

• No Security Issues: The changes don't introduce any security vulnerabilities
• Error Information: Error messages don't expose sensitive information

Minor Suggestions for Enhancement

1. Error Context Enhancement (src/zoterocitationcounts.js:304):

   this._log(`Warning: Failed to remove element with id '${id}': ${error.message}`);

   Consider adding the error type for better debugging:

   this._log(`Warning: Failed to remove element with id '${id}': ${error.name}: ${error.message}`);

2. Consistent Error Reporting (src/zoterocitationcounts.js:315):
   Similar enhancement for FTL element error logging.

Test Coverage Recommendations

The changes should be tested with:

• Normal plugin shutdown scenarios
• Forced window closure during DOM manipulation
• Missing/already removed DOM elements
• Malformed element IDs in the tracking array

Run the existing test suite to ensure no regressions:

npm run test

Technical Implementation Analysis

The fix correctly addresses the root cause by:

1. Isolating individual element removal failures
2. Ensuring the tracking array is always cleared regardless of cleanup success
3. Providing detailed error information for debugging
4. Maintaining plugin stability during shutdown

This implementation pattern should be considered for other similar cleanup operations in the codebase.

Verdict: Ready to merge ✅

The PR effectively solves the memory leak issue while improving overall robustness of the DOM cleanup process.

[gemini-code-assist]

Code Review

This pull request adds important error handling to the DOM cleanup logic, which is a great improvement for robustness and preventing memory leaks. The changes are well-implemented. I have one suggestion to refactor the code slightly using a finally block, which would make the code a bit more concise and idiomatic by removing a duplicated line of code.

[gemini-code-assist]

While the added error handling is great and correctly addresses the potential for memory leaks, the code can be simplified by using a finally block. This avoids duplicating the this._addedElementIDs = []; line and makes the intent clearer: the array should be cleared regardless of whether the removal operations succeed or fail.

    try {
      for (let id of this._addedElementIDs) {
        try {
          const element = document.getElementById(id);
          if (element) {
            element.remove();
          }
        } catch (error) {
          this._log(`Warning: Failed to remove element with id '${id}': ${error.message}`);
        }
      }

      try {
        const ftlElement = document.querySelector('[href="citation-counts.ftl"]');
        if (ftlElement) {
          ftlElement.remove();
        }
      } catch (error) {
        this._log(`Warning: Failed to remove FTL element: ${error.message}`);
      }
    } catch (error) {
      this._log(`Error during window cleanup: ${error.message}`);
    } finally {
      // Always clear the tracking array to prevent memory leaks.
      this._addedElementIDs = [];
    }