Integrate duplicate removal into organize operations#58
Conversation
10 tasks covering TDD implementation, UI removal, test updates.
Tests for: - Closing ungrouped duplicate of grouped tab - Closing multiple ungrouped tabs with same URL - Preserving cross-group duplicates
- Close ungrouped tabs that duplicate grouped tabs - Close duplicate ungrouped tabs (keep first) - Preserve cross-group duplicates (intentional) - Remove ungroupedDuplicates detection (now actually removes them) - Re-query tabs after removing duplicates before organizing
Removes the standalone 'Remove Duplicates' button from popup.html as duplicate removal is now integrated into the organize operations. Part of Task 3 from integrate-duplicate-removal implementation plan.
Removes: - dedupeBtn element reference (line 8) - dedupeBtn click event listener (lines 199-224) - ungroupedDuplicates warning from organize handlers Updates organize handlers to: - Show duplicatesClosed count when duplicates are removed - Remove warnings about ungrouped duplicates (now removed automatically) Part of Task 4 from integrate-duplicate-removal implementation plan.
Removes: - Import of removeDuplicateTabs module (line 5) - removeDuplicates message handler (lines 36-41) Part of Task 5 from integrate-duplicate-removal implementation plan.
Deletes: - src/background/removeDuplicateTabs.js - src/background/removeDuplicateTabs.test.js These files are no longer needed as duplicate removal is now integrated directly into the organizeTabs operation. Part of Task 6 from integrate-duplicate-removal implementation plan.
There was a problem hiding this comment.
Pull request overview
This PR integrates duplicate removal directly into the organize operations, eliminating the need for a separate "Remove Duplicates" button. The changes streamline the user experience by automatically closing duplicate tabs during organization.
Key changes:
- Integrated duplicate removal logic into
organizeTabs()function with automatic execution before organizing - Removed standalone "Remove Duplicates" button and all associated code/tests
- Updated status messages to report duplicates removed during organize operations
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
docs/plans/2025-12-08-integrate-duplicate-removal-impl.md |
Implementation plan document detailing the step-by-step integration approach |
chrome-extension/src/background/organizeTabs.js |
Core implementation - added duplicate removal logic for both single-window and all-windows modes |
chrome-extension/src/background/organizeTabs.test.js |
Updated tests to verify automatic duplicate removal behavior |
chrome-extension/src/popup/popup.html |
Removed "Remove Duplicates" button from UI |
chrome-extension/src/popup/popup.js |
Removed dedupeBtn handler and updated status messages to show duplicates removed |
chrome-extension/src/popup/popup.test.js |
Removed dedupeBtn tests and updated organize button tests |
chrome-extension/src/background/background.js |
Removed removeDuplicates message handler and import |
chrome-extension/src/background/background.test.js |
Removed tests for removeDuplicates action |
chrome-extension/src/background/removeDuplicateTabs.js |
Deleted - functionality now integrated into organizeTabs |
chrome-extension/src/background/removeDuplicateTabs.test.js |
Deleted - module no longer exists |
chrome-extension/e2e/ungrouped-duplicates.test.js |
Updated E2E tests to verify automatic duplicate removal during organize |
chrome-extension/package-lock.json |
Version bump to 1.5.0 |
CLAUDE.md |
Updated documentation to reflect removal of standalone duplicate removal feature |
Files not reviewed (1)
- chrome-extension/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
chrome-extension/src/background/organizeTabs.js:65
- In all-windows mode, the duplicate removal logic will incorrectly close grouped tabs that have the same URL but are in different groups (cross-group duplicates).
The issue is on line 39: when building groupedUrls, only the first grouped tab for each URL is stored. Later, when checking duplicates (line 54), any other grouped tab with the same URL will be marked for closing, even if it's in a different group.
This violates the stated rule: "Cross-group duplicates: Keep both (intentional use case)" from the PR description.
To fix this, the all-windows mode should skip grouped tabs in the duplicate detection loop, similar to the single-window mode (line 107). Add a check after line 51:
if (tab.groupId !== chrome.tabGroups.TAB_GROUP_ID_NONE) {
// Skip grouped tabs - only remove ungrouped duplicates
if (tab.windowId !== currentWindow.id) {
tabsToMove.push(tab);
}
continue;
} for (const tab of tabs) {
if (shouldSkipUrl(tab.url)) continue;
// Check if this is a duplicate of a grouped tab
if (groupedUrls.has(tab.url) && groupedUrls.get(tab.url).id !== tab.id) {
tabsToClose.push(tab.id);
} else if (seenUrls.has(tab.url)) {
// Duplicate of another seen tab
tabsToClose.push(tab.id);
} else {
seenUrls.set(tab.url, tab);
// If tab is in a different window, mark for moving
if (tab.windowId !== currentWindow.id) {
tabsToMove.push(tab);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // First pass: identify grouped tabs (prioritized) | ||
| const groupedUrls = new Map(); // url -> tab | ||
| for (const tab of tabs) { | ||
| if (shouldSkipUrl(tab.url)) continue; |
There was a problem hiding this comment.
Callee is not a function: it has type undefined.
| if (shouldSkipUrl(tab.url)) { | ||
| continue; | ||
| } | ||
| if (shouldSkipUrl(tab.url)) continue; |
There was a problem hiding this comment.
Callee is not a function: it has type undefined.
| // Step 1: Identify grouped tab URLs (these take priority) | ||
| const groupedUrls = new Map(); // url -> tab | ||
| for (const tab of tabs) { | ||
| if (shouldSkipUrl(tab.url)) continue; |
There was a problem hiding this comment.
Callee is not a function: it has type undefined.
| if (shouldSkipUrl(tab.url)) { | ||
| continue; | ||
| for (const tab of tabs) { | ||
| if (shouldSkipUrl(tab.url)) continue; |
There was a problem hiding this comment.
Callee is not a function: it has type undefined.
Code Review: PR #58 - Integrate Duplicate Removal into Organize OperationsSummaryThis PR successfully integrates duplicate tab removal directly into the organize operations, removing the standalone "Remove Duplicates" button. The implementation is well-designed with good test coverage and follows a sensible approach for handling different duplicate scenarios. Strengths1. Excellent Architecture and Design
2. Comprehensive Test Coverage
3. User Experience Improvements
4. Code Quality
Potential Issues and Suggestions1. Performance Consideration (Minor)Location: chrome-extension/src/background/organizeTabs.js:89-130 The duplicate removal logic iterates through tabs multiple times (first pass to identify grouped tabs, second pass to find duplicates). This is acceptable for typical tab counts - the current approach is O(n) which is efficient. Suggestion: Consider mentioning performance characteristics in comments if users might have 100+ tabs. 2. Edge Case: Tab Closure Timing (Minor)Location: chrome-extension/src/background/organizeTabs.js:121-129 Potential Issue: If chrome.tabs.remove() partially fails, the duplicatesClosed count might not reflect reality, and the re-query could include tabs we expected to be closed. Suggestion: Consider wrapping in try-catch for better error handling. 3. Code Duplication (Moderate)Location: chrome-extension/src/popup/popup.js:40-190 The organize button handlers have significant code duplication. Status message formatting logic is repeated 4 times. Suggestion: Extract a helper function for building status messages. 4. Missing Test Scenario (Minor)Location: chrome-extension/e2e/ungrouped-duplicates.test.js The E2E tests don't verify behavior when a user has a pinned duplicate tab. Suggestion: Add a test case for pinned tabs if they should be handled differently. 5. Inconsistent Messaging (Very Minor)Location: chrome-extension/src/popup/popup.js:61,97,134,174 Status messages use both "Removed X duplicate(s)" and "Removed X duplicates" Suggestion: Be consistent with singular/plural forms. Security ConsiderationsNo security concerns identified:
Test Coverage AssessmentExcellent coverage overall:
Missing scenarios:
Documentation QualityStrengths:
Minor suggestion: Consider adding a comment in organizeTabs.js explaining the prioritization logic for future maintainers Overall AssessmentRecommendation: APPROVE with minor suggestions This is a well-executed PR that:
The identified issues are minor and don't block merging. The code quality is high and the implementation is sound. Suggested Actions Before Merge:
Merge Decision:READY TO MERGE - The suggestions above are nice-to-haves, not blockers. Review performed by Claude Code |
Summary
This PR integrates duplicate removal directly into the "Organize by Domain" and "Organize by Category" operations, removing the need for a separate "Remove Duplicates" button.
Changes
Core Implementation
organizeTabs()functionremoveDuplicateTabsmodule and handlerUI Changes
Duplicate Removal Rules
Test Updates
Documentation
Test Plan
Implementation Details
See
docs/plans/2025-12-08-integrate-duplicate-removal-design.mdfor full design rationale and implementation approach.