feat: add master checkbox for bulk selection in manual cleanup#50
feat: add master checkbox for bulk selection in manual cleanup#50
Conversation
- Add tri-state master checkbox to DuplicateGroupsPreview header - Use useMemo for allSelected and isIndeterminate calculations - Use useRef + useEffect to sync indeterminate DOM property - Add accessibility support (aria-label, keyboard navigation) - Replace old text buttons with master checkbox UI - Match styling to existing individual checkboxes - Hide master checkbox when no groups present Resolves #49 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @rdfitted, I'm Gemini Code Assist1! 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 significantly improves the user experience for manual cleanup of duplicate groups by introducing a tri-state master checkbox. This new control allows users to efficiently select all, deselect all, or partially select groups, streamlining the bulk action process and replacing the less intuitive separate 'Select All' and 'Deselect All' buttons. The implementation focuses on robust state management, accessibility, and consistent UI design. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
WalkthroughAdds a tri-state master checkbox to DuplicateGroupsPreview for bulk selection in manual cleanup mode; introduces an Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CleanupExecutor
participant Detector
participant JobEmitter
Caller->>CleanupExecutor: execute(job, { approvedGroups? })
alt approvedGroups provided
CleanupExecutor->>CleanupExecutor: limitedGroups = approvedGroups
CleanupExecutor->>JobEmitter: emit detectComplete(limitedGroups)
CleanupExecutor->>JobEmitter: update job status -> in_progress
else no approvedGroups
CleanupExecutor->>JobEmitter: update job status -> detecting
CleanupExecutor->>Detector: detectDuplicateGroups(jobId, filters)
Detector-->>CleanupExecutor: detectedGroups
CleanupExecutor->>CleanupExecutor: apply maxGroups -> limitedGroups
CleanupExecutor->>JobEmitter: emit detectComplete(limitedGroups)
end
CleanupExecutor->>CleanupExecutor: proceed with manual/auto steps using limitedGroups
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a tri-state master checkbox for bulk selection in the DuplicateGroupsPreview component, which is a significant improvement for the manual cleanup user experience. The implementation correctly leverages React hooks such as useMemo, useRef, and useEffect for managing derived state and syncing DOM properties. Accessibility features like aria-label are appropriately added, and the refactoring to use the isManualInteractive variable consistently across the component enhances readability and maintainability. The handling of edge cases, such as hiding the master checkbox when no groups are present, is well-considered. Overall, the changes are well-implemented and align with best practices for React development.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/components/migration/DuplicateGroupsPreview.tsx`:
- Around line 132-141: The master checkbox doesn't expose the mixed state to
assistive tech; update the input rendered in DuplicateGroupsPreview to include
an aria-checked attribute driven by isIndeterminate: set aria-checked to "mixed"
when isIndeterminate is true, otherwise set it to the boolean value matching
allSelected (true/false); change only the input element that uses
masterCheckboxRef, checked={allSelected}, onChange={handleMasterToggle} and
aria-label={masterCheckboxAriaLabel} so screen readers receive the mixed state.
| {isManualInteractive && groups.length > 0 && ( | ||
| <label className="flex items-center gap-2 cursor-pointer"> | ||
| <input | ||
| ref={masterCheckboxRef} | ||
| type="checkbox" | ||
| checked={allSelected} | ||
| onChange={handleMasterToggle} | ||
| className="h-4 w-4 rounded border-gray-300 text-blue-600 focus:ring-blue-500 dark:border-gray-600 dark:bg-gray-700" | ||
| aria-label={masterCheckboxAriaLabel} | ||
| /> |
There was a problem hiding this comment.
Expose the mixed state via aria-checked.
The master checkbox uses indeterminate, but the mixed state isn’t announced to assistive tech. The PR objectives require aria-checked to be true / false / mixed. Please add aria-checked based on isIndeterminate.
✅ Suggested fix
<input
ref={masterCheckboxRef}
type="checkbox"
checked={allSelected}
onChange={handleMasterToggle}
className="h-4 w-4 rounded border-gray-300 text-blue-600 focus:ring-blue-500 dark:border-gray-600 dark:bg-gray-700"
aria-label={masterCheckboxAriaLabel}
+ aria-checked={isIndeterminate ? 'mixed' : allSelected}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {isManualInteractive && groups.length > 0 && ( | |
| <label className="flex items-center gap-2 cursor-pointer"> | |
| <input | |
| ref={masterCheckboxRef} | |
| type="checkbox" | |
| checked={allSelected} | |
| onChange={handleMasterToggle} | |
| className="h-4 w-4 rounded border-gray-300 text-blue-600 focus:ring-blue-500 dark:border-gray-600 dark:bg-gray-700" | |
| aria-label={masterCheckboxAriaLabel} | |
| /> | |
| {isManualInteractive && groups.length > 0 && ( | |
| <label className="flex items-center gap-2 cursor-pointer"> | |
| <input | |
| ref={masterCheckboxRef} | |
| type="checkbox" | |
| checked={allSelected} | |
| onChange={handleMasterToggle} | |
| className="h-4 w-4 rounded border-gray-300 text-blue-600 focus:ring-blue-500 dark:border-gray-600 dark:bg-gray-700" | |
| aria-label={masterCheckboxAriaLabel} | |
| aria-checked={isIndeterminate ? 'mixed' : allSelected} | |
| /> |
🤖 Prompt for AI Agents
In `@app/components/migration/DuplicateGroupsPreview.tsx` around lines 132 - 141,
The master checkbox doesn't expose the mixed state to assistive tech; update the
input rendered in DuplicateGroupsPreview to include an aria-checked attribute
driven by isIndeterminate: set aria-checked to "mixed" when isIndeterminate is
true, otherwise set it to the boolean value matching allSelected (true/false);
change only the input element that uses masterCheckboxRef,
checked={allSelected}, onChange={handleMasterToggle} and
aria-label={masterCheckboxAriaLabel} so screen readers receive the mixed state.
There was a problem hiding this comment.
✅ Verified by 4 OpenCode Agents
Consensus: MISTAKEN - Adding aria-checked to native HTML checkboxes is incorrect and can cause accessibility issues.
Multi-Agent Verification Results
Models Used:
- OpenCode BigPickle (Deep Analysis)
- OpenCode GLM 4.7 (Pattern Recognition)
- OpenCode Grok Code (Quick Search)
- OpenCode MiniMax M2.1 (Multi-Language)
Vote: 2-2 Tie → Claude Orchestrator Final Decision: MISTAKEN
Evidence
The current implementation uses a native HTML <input type="checkbox"> (line 136), not an ARIA role="checkbox".
According to W3C/MDN accessibility standards:
"You should NOT use aria-checked on native HTML checkboxes." The
indeterminateproperty on native checkboxes is automatically exposed to assistive technology without additional ARIA attributes. Addingaria-checkedcan create inconsistencies between the native checkbox state and the ARIA state, causing different behaviors across screen readers.
Current Implementation (Correct)
<input
ref={masterCheckboxRef}
type="checkbox"
checked={allSelected}
onChange={handleMasterToggle}
aria-label={masterCheckboxAriaLabel} // ✅ This is correct
/>The indeterminate property is set via ref + useEffect (lines 45-50), which is the proper pattern for native checkboxes.
When aria-checked IS Required
aria-checked is ONLY required when using role="checkbox" on non-native elements (e.g., <div role="checkbox">). For native <input type="checkbox"> elements, browsers handle accessibility automatically.
Sources
- ARIA: checkbox role - MDN
- ARIA: aria-checked attribute - MDN
- Accessibility Insights - ARIA Conditional Attributes
No changes needed - current implementation follows accessibility best practices.
There was a problem hiding this comment.
@rdfitted Thank you for the thorough verification and correction! You're absolutely right—my original suggestion was mistaken.
For native HTML <input type="checkbox"> elements, the indeterminate property (set via ref + useEffect as done in lines 45-50) is automatically exposed to assistive technology, and adding aria-checked would be incorrect and could introduce accessibility conflicts.
The aria-checked attribute is only needed when implementing custom checkboxes with role="checkbox" on non-native elements like <div>.
Your current implementation is correct and follows accessibility best practices. I appreciate the detailed explanation and sources—this ensures better guidance going forward!
✏️ Learnings added
Learnt from: rdfitted
Repo: rdfitted/podiomigrationassistant PR: 50
File: app/components/migration/DuplicateGroupsPreview.tsx:132-141
Timestamp: 2026-01-23T01:43:58.874Z
Learning: Do NOT add `aria-checked` to native HTML `<input type="checkbox">` elements. The `indeterminate` property on native checkboxes is automatically exposed to assistive technology. The `aria-checked` attribute should only be used with `role="checkbox"` on non-native elements (e.g., `<div role="checkbox">`). For native checkboxes, use the ref + useEffect pattern to set the indeterminate property.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- Added learning from resolveprcomments-50 session - Key insight: aria-checked should NOT be on native checkboxes - Multi-agent verification (4 OpenCode models) with tie-breaker works - Regenerated project-dna.md with 6 session learnings - Added accessibility standards patterns to DNA - Updated hot spots and keyword clusters Co-Authored-By: Codex GPT-5.2 <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.ai-docs/project-dna.md:
- Line 128: The guideline line "Don't add ARIA to native HTML elements" is too
broad; update it to clarify that ARIA should not duplicate or conflict with
built-in semantics but that certain ARIA attributes (e.g., aria-label,
aria-describedby, aria-expanded, aria-live) are valid on native elements;
specifically call out the checkbox indeterminate case (avoid using aria-checked
on a native <input type="checkbox"> with indeterminate state, prefer using the
native indeterminate property and clear guidance instead). Replace the single
sentence with a short rule like: "Avoid adding ARIA that duplicates or conflicts
with native semantics — use native controls where possible; use ARIA attributes
(aria-label, aria-describedby, aria-expanded, aria-live) when they add necessary
semantics; do not use aria-checked on native checkbox inputs with indeterminate
state (use the input's indeterminate property and accompanying accessible
instructions)."
Update overly broad ARIA guideline in project-dna.md from "Don't add ARIA to native HTML elements" to more specific guidance: "Don't override native semantic state with ARIA". The refined guideline clarifies: - Valid ARIA usage: aria-label, aria-describedby for supplementary info - Invalid ARIA usage: aria-checked, aria-selected, aria-pressed on native elements - Rationale: Native checkboxes/radios/buttons expose state automatically Verified by 4 OpenCode agents (BigPickle, GLM 4.7, Grok Code, MiniMax M2.1) confirming the codebase uses valid ARIA attributes on native elements. Simplified by Codex GPT-5.2 for improved clarity. Curated 7 learnings and updated project-dna.md with new patterns. Resolves CodeRabbit PR comment on #50 Co-Authored-By: Codex GPT-5.2 <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.ai-docs/project-dna.md:
- Around line 10-15: The markdown uses emphasis (e.g. "**Multi-Agent
Workflows**") as pseudo-headings which triggers MD036; replace those emphasized
labels with proper heading syntax (e.g. "## Multi-Agent Workflows") or convert
them to plain paragraph text where a heading is not intended; update all similar
occurrences referenced (lines covering the blocks like the list items and the
other flagged ranges 43-46, 47-49, 55-57, 59-61, 62-65, 153) by changing
instances of "**...**" or "*...*" used as headings into real headings (#, ##,
etc.) or plain text so markdownlint MD036 is satisfied.
Convert bold/italic emphasis used as headings to proper markdown heading syntax (####). Changes: - Convert **Section Name** to #### Section Name for all subsection headers - Update "What Works" section headers (11 fixes) - Update "What Doesn't Work" section headers (3 fixes) - Maintains proper heading hierarchy under ### parent sections - Aligns with project markdown standards (README.md, bug-patterns.md) Code simplification (Codex GPT-5.2): - Add spacing around arrows (→) for readability - Wrap technical terms in backticks for consistency - Improve grammatical accuracy Learning captured: - Multi-agent verification (4 OpenCode models) confirmed MD036 violations - Proper heading syntax provides semantic structure and consistency - Updated project-dna.md with new learning and curated insights Co-Authored-By: Codex GPT-5.2 <noreply@openai.com>
When clicking "Proceed" after a dry run, the executor was unnecessarily re-scanning the entire app even though approved groups were already provided. This caused: - Status to regress to "detecting" - Risk of getting stuck if scan was interrupted - Wasted time re-scanning large datasets Now when approvedGroups are provided, detection is skipped entirely and execution proceeds directly with the pre-approved groups. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
Resolves #49
Adds a tri-state master checkbox to the DuplicateGroupsPreview component header for improved bulk selection UX in manual cleanup mode.
Changes
useMemofor derived state (allSelected,isIndeterminate)useRef+useEffectto sync indeterminate property (DOM-only, not React attribute)aria-label, keyboard navigation support (Tab, Space)Workers
Difficulties Encountered
Pre-existing lint warnings in unrelated files (useCleanup.ts, useFlowClone.ts) - not addressed as they don't affect this feature
Session
20260122-144008-resolve-issue-49
Generated by Hive multi-agent system
Summary by CodeRabbit
New Features
Accessibility
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.