Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/close-pr-banner-toggle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'elden-ring-github': minor
---

Add a PR close banner option with settings toggle, popup control, and reusable banner rendering helper.
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ A Chrome extension that displays epic Elden Ring-themed banners when you create,
- πŸ†• **PR Creation Banner** - Celebrate new pull request creation with a dedicated banner
- βœ… **PR Approval Banner** - Epic celebration when you approve pull requests
- πŸŽ‰ **PR Merge Banner** - Shows an epic "MERGE ACCOMPLISHED" banner when PRs are merged
- ☠️ **PR Close Banner** - Dramatic "You Died" moment whenever a pull request is closed
- πŸ”Š **Sound Effects** - Plays the iconic Elden Ring achievement sound
- βš™οΈ **Independent Controls** - Separate settings to enable/disable creation, approval, and merge banners

Expand Down Expand Up @@ -106,7 +107,8 @@ src/
β”œβ”€β”€ lost-grace-discovered.mp3 # Lost Grace discovery sound
β”œβ”€β”€ pull-request-created.png # PR creation banner
β”œβ”€β”€ pull-request-merged.png # PR merge banner
β”œβ”€β”€ approve-pull-request.webp # PR approval banner
β”œβ”€β”€ approve-pull-request.png # PR approval banner
β”œβ”€β”€ close-pull-request.png # PR close banner
└── icon*.png # Extension icons

dist/ # Built extension (Chrome loads this)
Expand Down
Binary file added src/assets/close-pull-request.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
68 changes: 68 additions & 0 deletions src/content/banner.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { renderBanner, type BannerType } from './banner';

describe('renderBanner', () => {
const soundUrl = 'chrome-extension://mock/sound.mp3';

beforeEach(() => {
document.body.innerHTML = '';
vi.useFakeTimers();

const chromeGlobal = globalThis as unknown as {
chrome?: { runtime?: { getURL?: (path: string) => string } };
};
chromeGlobal.chrome = chromeGlobal.chrome || {};
chromeGlobal.chrome.runtime = chromeGlobal.chrome.runtime || {};
chromeGlobal.chrome.runtime.getURL = vi.fn((path: string) => `chrome-extension://mock/${path}`);

global.Audio = vi.fn().mockImplementation(() => {
return {
play: vi.fn().mockResolvedValue(undefined),
volume: 0,
} as unknown as HTMLAudioElement;
});
});

afterEach(() => {
vi.useRealTimers();
});

it('should render banners for each type', () => {
const types: BannerType[] = ['merged', 'created', 'approved', 'closed'];

types.forEach((type) => {
const onHide = vi.fn();
renderBanner({
type,
soundUrl,
soundEnabled: true,
onHide,
});

const banner = document.getElementById('elden-ring-banner');
expect(banner).toBeTruthy();
expect(banner?.innerHTML).toContain('.png');
const chromeRuntime = (globalThis as any).chrome.runtime;
expect(chromeRuntime.getURL).toHaveBeenCalled();

vi.runAllTimers();
expect(onHide).toHaveBeenCalled();

document.body.innerHTML = '';
});
});

it('should skip audio when sound is disabled', () => {
const onHide = vi.fn();
renderBanner({
type: 'merged',
soundUrl,
soundEnabled: false,
onHide,
});

expect(global.Audio).not.toHaveBeenCalled();
vi.runAllTimers();
expect(onHide).toHaveBeenCalled();
});
});
70 changes: 70 additions & 0 deletions src/content/banner.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
export type BannerType = 'merged' | 'created' | 'approved' | 'closed';

interface RenderBannerOptions {
type: BannerType;
soundUrl: string;
soundEnabled: boolean;
onHide: () => void;
}

const bannerAssetMap: Record<BannerType, { image: string; alt: string }> = {
merged: {
image: 'pull-request-merged.png',
alt: 'Pull Request Merged',
},
created: {
image: 'pull-request-created.png',
alt: 'Pull Request Created',
},
approved: {
image: 'approve-pull-request.png',
alt: 'Pull Request Approved',
},
closed: {
image: 'close-pull-request.png',
alt: 'Pull Request Closed',
},
};

/**
* Renders the Elden Ring banner with correct imagery and optional audio with safe cleanup.
*/
export const renderBanner = ({
type,
soundUrl,
soundEnabled,
onHide,
}: RenderBannerOptions): boolean => {
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}">`;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

document.body.appendChild(banner);

if (soundEnabled) {
const audio = new Audio(soundUrl);
audio.volume = 1.0;
audio.play().catch((err) => console.log('Sound playback failed:', err));
}

setTimeout(() => banner.classList.add('show'), 50);
setTimeout(() => {
banner.classList.remove('show');
setTimeout(() => {
if (banner.parentNode) {
banner.remove();
}
onHide();
}, 500);
}, 3000);

return true;
} catch (error) {
console.log('Image banner failed, using text banner:', error);
onHide();
return false;
}
Comment on lines 67 to 72
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

};
33 changes: 33 additions & 0 deletions src/content/closeWatcher.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { waitForCloseComplete } from './closeWatcher';

describe('waitForCloseComplete', () => {
beforeEach(() => {
document.body.innerHTML = '';
});

it('should trigger callback immediately if closed state already exists', () => {
document.body.innerHTML = `
<span class="State State--closed">Closed</span>
`;

const onClose = vi.fn();
waitForCloseComplete(onClose);

expect(onClose).toHaveBeenCalledTimes(1);
});

it('should trigger callback when closed state is added later', async () => {
const onClose = vi.fn();
waitForCloseComplete(onClose);

const closedElement = document.createElement('span');
closedElement.className = 'State State--closed';
closedElement.textContent = 'Closed';
document.body.appendChild(closedElement);

await new Promise((resolve) => setTimeout(resolve, 150));

expect(onClose).toHaveBeenCalledTimes(1);
});
});
72 changes: 72 additions & 0 deletions src/content/closeWatcher.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
let closeHandled = false;

const isClosedState = (element: Element | null): boolean => {
if (!element) return false;
if (!element.matches('.State.State--closed')) {
return false;
}
const text = element.textContent?.toLowerCase().trim() || '';
return text.includes('closed');
};

const handleClose = (observer: MutationObserver, onClose: () => void): void => {
if (closeHandled) return;
closeHandled = true;
console.log('☠️ Pull request closed!');
onClose();
observer.disconnect();
};

const checkExistingClosedState = (observer: MutationObserver, onClose: () => void): boolean => {
const closedElement = document.querySelector('.State.State--closed');
if (isClosedState(closedElement)) {
handleClose(observer, onClose);
return true;
}
return false;
};

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);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


setTimeout(() => {
observer.disconnect();
console.log('⏰ Close detection timeout');
}, timeoutMs);
};
Comment on lines 40 to 95
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

49 changes: 45 additions & 4 deletions src/content/content.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ describe('EldenRingMerger', () => {
});

it('should handle different banner types', () => {
const types = ['merged', 'created', 'approved'] as const;
const types = ['merged', 'created', 'approved', 'closed'] as const;

types.forEach((type) => {
let imageName: string;
Expand All @@ -121,8 +121,11 @@ describe('EldenRingMerger', () => {
imageName = 'pull-request-created.png';
altText = 'Pull Request Created';
} else if (type === 'approved') {
imageName = 'approve-pull-request.webp';
imageName = 'approve-pull-request.png';
altText = 'Pull Request Approved';
} else if (type === 'closed') {
imageName = 'close-pull-request.png';
altText = 'Pull Request Closed';
} else {
imageName = 'pull-request-merged.png';
altText = 'Pull Request Merged';
Expand All @@ -135,8 +138,11 @@ describe('EldenRingMerger', () => {
expect(imageName).toBe('pull-request-created.png');
expect(altText).toBe('Pull Request Created');
} else if (type === 'approved') {
expect(imageName).toBe('approve-pull-request.webp');
expect(imageName).toBe('approve-pull-request.png');
expect(altText).toBe('Pull Request Approved');
} else if (type === 'closed') {
expect(imageName).toBe('close-pull-request.png');
expect(altText).toBe('Pull Request Closed');
} else {
expect(imageName).toBe('pull-request-merged.png');
expect(altText).toBe('Pull Request Merged');
Expand Down Expand Up @@ -225,12 +231,43 @@ describe('EldenRingMerger', () => {
expect(mergedElement?.textContent).toBe('Merged');
});

it('should detect close pull request buttons by text', () => {
document.body.innerHTML = `
<div id="partial-new-comment-form-actions">
<button>Close pull request</button>
<button>Something else</button>
</div>
`;

const container = document.querySelector('#partial-new-comment-form-actions');
const closeButtons = Array.from(container?.querySelectorAll('button') || []).filter((button) =>
button.textContent?.toLowerCase().includes('close pull request'),
);

expect(closeButtons.length).toBe(1);
expect(closeButtons[0]?.textContent?.trim()).toBe('Close pull request');
});

it('should detect closed state element with closed class', () => {
document.body.innerHTML = `
<span reviewable_state="ready" title="Status: Closed" data-view-component="true" class="State State--closed">
<svg></svg>
Closed
</span>
`;

const closedElement = document.querySelector('.State.State--closed');
expect(closedElement).toBeTruthy();
expect(closedElement?.textContent?.toLowerCase()).toContain('closed');
});

it('should handle storage changes', () => {
const mockCallback = vi.fn();
const changes = {
soundEnabled: { newValue: false },
showOnPRMerged: { newValue: false },
showOnPRCreate: { newValue: true },
showOnPRClose: { newValue: false },
};

// Simulate storage change handling
Expand All @@ -243,11 +280,15 @@ describe('EldenRingMerger', () => {
if (changes.showOnPRCreate) {
mockCallback('showOnPRCreate', changes.showOnPRCreate.newValue);
}
if (changes.showOnPRClose) {
mockCallback('showOnPRClose', changes.showOnPRClose.newValue);
}

expect(mockCallback).toHaveBeenCalledTimes(3);
expect(mockCallback).toHaveBeenCalledTimes(4);
expect(mockCallback).toHaveBeenCalledWith('soundEnabled', false);
expect(mockCallback).toHaveBeenCalledWith('showOnPRMerged', false);
expect(mockCallback).toHaveBeenCalledWith('showOnPRCreate', true);
expect(mockCallback).toHaveBeenCalledWith('showOnPRClose', false);
});

it('should handle PR creation flag storage', () => {
Expand Down
Loading