refactor: extract feature handlers

[leochiu-a]

Summary

This PR refactors the content script architecture by extracting feature-specific handlers into separate modules, improving code organization, maintainability, and testability. The monolithic EldenRingMerger class has been replaced with a feature-based architecture using dedicated handler modules and a centralized settings management system.

Key Changes

• Extracted feature handlers: Split PR merge, creation, approval, and close detection into separate handler modules (mergeHandler.ts, createHandler.ts, approveHandler.ts, closeHandler.ts)
• Introduced feature pattern: Created features.ts with a GitHubFeature interface and feature classes (MergeFeature, CreationFeature, ApprovalFeature, CloseFeature) for consistent lifecycle management
• Centralized settings management: Implemented SettingsStore for state management and ShowSettings for feature-specific settings queries
• Renamed main orchestrator: Changed EldenRingMerger to EldenRingOrchestrator to better reflect its coordinating role
• Improved type safety: Enhanced ShowSettings with proper TypeScript typing and removed implicit any types
• Added comprehensive tests: Created mergeHandler.test.ts with test coverage for merge detection logic

Type of Change

• 🐛 Bug fix (non-breaking change which fixes an issue)
• ✨ New feature (non-breaking change which adds functionality)
• 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
• 📚 Documentation update
• ♻️ Refactoring (no functional changes)
• 🎨 Style/formatting changes
• 🧪 Test improvements
• 🔧 Configuration changes

Test Plan

Manual Testing

• Verify PR merge detection still works by merging a test PR (banner should appear)
• Verify PR creation detection by creating a new PR from compare page
• Verify PR approval detection by approving a PR in the Files view
• Verify PR close detection by closing an open PR
• Confirm all banner types (merged, created, approved, closed) render correctly
• Test that settings toggles (showOnPRMerged, showOnPRCreate, etc.) still control feature behavior

Automated Testing

• Run npm test to verify mergeHandler.test.ts passes
• Verify test coverage for merge button detection and observer pattern

Breaking Changes

None

Checklist

• 📝 Code follows the style guidelines
• 👀 Self-review has been performed
• 🧪 Tests have been added/updated
• 📖 Documentation has been updated

[leochiu-a]

Overall, this is a well-structured refactoring that successfully extracts feature handlers into separate modules. The code is cleaner and more maintainable with proper separation of concerns.

Key improvements:

• Clear separation of concerns with individual handler modules
• Introduction of GitHubFeature interface provides good abstraction
• Settings management extracted into dedicated classes
• Better testability with modular design

Issues found:

• Critical: Potential memory leak from unobserved MutationObserver in mergeHandler (line 78)
• Medium: No error handling in showSettings callback (line 22)
• Minor: Hardcoded timeout values without constants (lines 19, 82, 87)
• Minor: Missing type exports in features.ts

[leochiu-a]

Critical: Potential Memory Leak - Missing Observer Disconnect

When settings are disabled, the code returns early without disconnecting the observer that was created. While the outer observer.disconnect() at line 80 executes in this case, the issue is in the callback at lines 54-58 where the observer should disconnect but might not in the disabled case.

Actually, reviewing more carefully: the code looks correct here as observer.disconnect() is always called at line 80 regardless of the settings check. However, there's still a logic issue: if the merged state is NOT present after 100ms, the observer keeps running until the 10-second timeout.

Consider adding a flag to prevent multiple triggers:

let hasTriggered = false;

const observer = new MutationObserver((mutations) => {
  if (hasTriggered) return;
  // ... rest of logic
  if (merged found) {
    hasTriggered = true;
    observer.disconnect();
  }
});

[leochiu-a]

Medium: Missing Error Handling

The stateProvider() callback is called without error handling. If it throws, the entire feature will break.

Suggested fix:

isEnabled(setting: ShowSettingKey): boolean {
  try {
    const key = STATE_KEY_MAP[setting];
    return this.stateProvider()[key];
  } catch (error) {
    console.error('Error getting settings state:', error);
    return true; // Default to enabled
  }
}

[leochiu-a]

Minor: Magic Number - Extract Timeout Constant

The 30000ms timeout is hardcoded and appears in multiple handlers. Extract to a shared constant:

// In constants.ts
export const PR_ACTION_TIMEOUT_MS = 30000;

// Then use:
if (timeDiff < PR_ACTION_TIMEOUT_MS && ...)

Same issue in:

• createHandler.ts:18
• approveHandler.ts:23

[leochiu-a]

Minor: Hardcoded Timeout Values

Extract magic numbers to named constants:

const MERGE_CHECK_DELAY_MS = 100;
const MERGE_DETECTION_TIMEOUT_MS = 10000;

[leochiu-a]

Minor: Consider Exporting Interface

For better reusability and type checking, consider exporting MergeFeatureOptions and other option interfaces:

export interface MergeFeatureOptions {
  showSettings: ShowSettings;
  onMerged: () => void;
}

Same for CreationFeatureOptions, ApprovalFeatureOptions, and CloseFeatureOptions.