fix(cli): reset slash-command conflict dedupe when conflicts reappear#27860
fix(cli): reset slash-command conflict dedupe when conflicts reappear#27860aniruddhaadak80 wants to merge 1 commit into
Conversation
|
📊 PR Size: size/S
|
Summary of ChangesHello, 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 addresses a bug in the slash-command conflict notification system where resolved conflicts remained in the 'notified' state indefinitely. By modifying the conflict handler to synchronize the tracking set with the current active conflicts, the system now correctly allows for re-notification if a conflict is resolved and later recurs, improving the reliability of user feedback. 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 the 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 counterproductive. 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. Footnotes
|
![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
This pull request updates the SlashCommandConflictHandler to re-notify users of a slash command conflict if it disappears and then reappears, and adds a corresponding unit test. A review comment identifies a potential duplication bug during the debounce window when multiple conflict payloads are received in rapid succession, which could lead to duplicate entries in pendingConflicts. A code suggestion is provided to filter out conflicts already present in pendingConflicts before pushing them.
| const currentKeys = new Set<string>(); | ||
| const newConflicts: SlashCommandConflict[] = []; | ||
|
|
||
| for (const c of payload.conflicts) { | ||
| // Use a unique key to prevent duplicate notifications for the same conflict | ||
| const sourceId = | ||
| c.loserExtensionName || c.loserMcpServerName || c.loserKind; | ||
| const key = `${c.name}:${sourceId}:${c.renamedTo}`; | ||
| if (this.notifiedConflicts.has(key)) { | ||
| return false; | ||
| currentKeys.add(key); | ||
|
|
||
| if (!this.notifiedConflicts.has(key)) { | ||
| newConflicts.push(c); | ||
| } | ||
| this.notifiedConflicts.add(key); | ||
| return true; | ||
| }); | ||
| } | ||
|
|
||
| this.notifiedConflicts = currentKeys; | ||
|
|
||
| if (newConflicts.length > 0) { | ||
| this.pendingConflicts.push(...newConflicts); |
There was a problem hiding this comment.
There is a potential duplication bug during the debounce window when multiple conflict payloads are received in rapid succession (e.g., during startup or incremental loading of extensions/MCP servers).
Scenario:
- Payload 1:
[conflictA]is received.this.notifiedConflictsbecomes{'conflictA'}andconflictAis added tothis.pendingConflicts. - Payload 2 (within 500ms):
[]or[conflictB]is received.this.notifiedConflictsis overwritten andconflictAis removed from it, butconflictAremains inthis.pendingConflictsawaiting flush. - Payload 3 (within 500ms):
[conflictA]is received again. SinceconflictAis not inthis.notifiedConflicts, it is treated as a new conflict and added tothis.pendingConflictsagain, resulting in duplicate entries (e.g.,[conflictA, conflictA]).
When flush() runs, this causes duplicate notifications or incorrect grouping (e.g., showing a single conflict as a grouped list of duplicates).
Solution:
Extract the key generation logic to a local helper and filter out conflicts that are already present in this.pendingConflicts before pushing them.
const getConflictKey = (c: SlashCommandConflict) => {
const sourceId = c.loserExtensionName || c.loserMcpServerName || c.loserKind;
return c.name + ":" + sourceId + ":" + c.renamedTo;
};
const currentKeys = new Set<string>();
const newConflicts: SlashCommandConflict[] = [];
for (const c of payload.conflicts) {
const key = getConflictKey(c);
currentKeys.add(key);
if (!this.notifiedConflicts.has(key)) {
newConflicts.push(c);
}
}
this.notifiedConflicts = currentKeys;
if (newConflicts.length > 0) {
const pendingKeys = new Set(this.pendingConflicts.map(getConflictKey));
const uniqueNewConflicts = newConflicts.filter(c => !pendingKeys.has(getConflictKey(c)));
this.pendingConflicts.push(...uniqueNewConflicts);
}
gemini-cli
Bot
added
priority/p2
1 participant
Fixes #24333
Description
This PR fixes the slash-command conflict notifier deduplication bug where a conflict that has been resolved (and disappears) is not re-notified if it subsequently reappears.
Changes
otifiedConflicts\ set based on the active conflicts in the current payload. This ensures that when a conflict is resolved and no longer active, its key is removed from
otifiedConflicts, allowing it to be re-notified if it reappears in a later reload.