feat: add pr close banner #9
Conversation
leochiu-a
left a comment
leochiu-a
left a comment
There was a problem hiding this comment.
Code Review Summary
Great work on adding the PR close banner feature! The implementation is solid overall, but I've identified several issues that should be addressed:
Key Issues:
- 🔴 Security: Potential XSS vulnerability in banner HTML rendering
- 🟡 Performance: MutationObserver memory leak in edge cases
- 🟡 Bug: Race condition in close detection timeout logic
- 🟢 Maintainability: Inconsistent error handling patterns
Please review the inline comments below for specific fixes.
src/content/banner.ts
Outdated
| try { | ||
| const banner = document.createElement('div'); | ||
| banner.id = 'elden-ring-banner'; | ||
|
|
||
| const asset = bannerAssetMap[type]; | ||
| const imgPath = chrome.runtime.getURL(`assets/${asset.image}`); | ||
| banner.innerHTML = `<img src="${imgPath}" alt="${asset.alt}">`; |
There was a problem hiding this comment.
Security Issue: Potential XSS Vulnerability
While chrome.runtime.getURL() is safe, using innerHTML to inject content could be risky if the asset mapping logic is ever modified. It's better to use DOM APIs for security best practices.
Current:
const banner = document.createElement('div');
banner.id = 'elden-ring-banner';
const asset = bannerAssetMap[type];
const imgPath = chrome.runtime.getURL(`assets/${asset.image}`);
banner.innerHTML = `<img src="${imgPath}" alt="${asset.alt}">`;
document.body.appendChild(banner);Fix:
const banner = document.createElement('div');
banner.id = 'elden-ring-banner';
const asset = bannerAssetMap[type];
const imgPath = chrome.runtime.getURL(`assets/${asset.image}`);
const img = document.createElement('img');
img.src = imgPath;
img.alt = asset.alt;
banner.appendChild(img);
document.body.appendChild(banner);This eliminates any potential XSS attack surface.
| export const waitForCloseComplete = (onClose: () => void, timeoutMs: number = 10000): void => { | ||
| closeHandled = false; | ||
|
|
||
| const observer = new MutationObserver((mutations) => { | ||
| if (closeHandled) return; | ||
|
|
||
| mutations.forEach((mutation) => { | ||
| mutation.addedNodes.forEach((node) => { | ||
| if (node.nodeType !== Node.ELEMENT_NODE || closeHandled) { | ||
| return; | ||
| } | ||
|
|
||
| const element = node as Element; | ||
| if (isClosedState(element)) { | ||
| handleClose(observer, onClose); | ||
| return; | ||
| } | ||
|
|
||
| const closedElement = element.querySelector('.State.State--closed'); | ||
| if (isClosedState(closedElement)) { | ||
| handleClose(observer, onClose); | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| observer.observe(document.body, { | ||
| childList: true, | ||
| subtree: true, | ||
| }); | ||
|
|
||
| if (checkExistingClosedState(observer, onClose)) { | ||
| return; | ||
| } | ||
|
|
||
| setTimeout(() => { | ||
| checkExistingClosedState(observer, onClose); | ||
| }, 100); | ||
|
|
||
| setTimeout(() => { | ||
| observer.disconnect(); | ||
| console.log('⏰ Close detection timeout'); | ||
| }, timeoutMs); | ||
| }; |
There was a problem hiding this comment.
Performance Issue: MutationObserver Memory Leak
If closeHandled is set to true before the timeout callbacks execute, the observer will be disconnected but the timeout callbacks at lines 64 and 68 will still fire and potentially create references that prevent garbage collection.
Current:
setTimeout(() => {
checkExistingClosedState(observer, onClose);
}, 100);
setTimeout(() => {
observer.disconnect();
console.log('⏰ Close detection timeout');
}, timeoutMs);Fix:
const checkTimeout = setTimeout(() => {
if (!closeHandled) {
checkExistingClosedState(observer, onClose);
}
}, 100);
const cleanupTimeout = setTimeout(() => {
if (!closeHandled) {
observer.disconnect();
console.log('⏰ Close detection timeout');
}
}, timeoutMs);
// Also clear these timeouts in handleClose:
const handleClose = (observer: MutationObserver, onClose: () => void): void => {
if (closeHandled) return;
closeHandled = true;
console.log('☠️ Pull request closed!');
clearTimeout(checkTimeout);
clearTimeout(cleanupTimeout);
onClose();
observer.disconnect();
};This ensures proper cleanup and prevents memory leaks.
src/content/closeWatcher.ts
Outdated
| setTimeout(() => { | ||
| checkExistingClosedState(observer, onClose); | ||
| }, 100); |
There was a problem hiding this comment.
Bug: Race Condition with Duplicate Checks
The checkExistingClosedState call at line 64 could trigger onClose() even if it was already called by the observer or the initial check. While closeHandled prevents the banner from showing twice, it's cleaner to avoid the race condition.
Current:
setTimeout(() => {
checkExistingClosedState(observer, onClose);
}, 100);Fix:
const recheckTimeout = setTimeout(() => {
if (!closeHandled) {
checkExistingClosedState(observer, onClose);
}
}, 100);Adds a guard to prevent unnecessary work when already handled.
|
|
||
| return true; | ||
| } catch (error) { | ||
| console.log('Image banner failed, using text banner:', error); | ||
| onHide(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Maintainability: Inconsistent Error Handling
The function catches all errors and logs them, but returns different boolean values. However, the error case calls onHide() immediately which could cause issues if the caller expects the banner lifecycle to complete normally.
Current:
} catch (error) {
console.log('Image banner failed, using text banner:', error);
onHide();
return false;
}Suggestion:
} catch (error) {
console.error('Banner rendering failed:', error);
// Don't call onHide() here - let the caller handle the failure
return false;
}Or alternatively, ensure the caller properly handles the false return value. The current pattern is confusing because onHide() is called both on success (after timeout) and on error (immediately).
… close watcher with timeout management
Summary
This PR adds a dramatic "You Died" banner feature that displays whenever a pull request is closed on GitHub, completing the full lifecycle celebration suite alongside merge, creation, and approval banners. When users close a PR, they'll see an Elden Ring-themed "You Died" screen with optional sound effects, providing a thematic and humorous touch to the PR workflow.
Key Changes
close-pull-request.pngasset and close detection mechanism that watches for PR close button clicks and state changesbanner.tsmodule supporting all banner types (merged, created, approved, closed)closeWatcher.tswith MutationObserver to reliably detect when PRs transition to closed statebanner.test.ts) and close detection (closeWatcher.test.ts) with 100% coverage of new functionalityType of Change
Test Plan
Manual Testing
Automated Testing
npm testto execute the full test suitebanner.test.ts: Banner rendering for all types (merged, created, approved, closed)closeWatcher.test.ts: Close state detection and MutationObserver behaviorBreaking Changes
None
Checklist