refactor: streamline team data service and enhance change review logic
- Removed unnecessary member metadata retrieval in TeamDataService to simplify task handling and prevent self-notifications for lead-owned tasks. - Updated lead owner validation to ensure consistent case handling. - Introduced a new reference in ChangeReviewDialog to manage in-flight file operations, preventing duplicate actions during file rejection. - Enhanced changeReviewSlice to return null for invalid states, improving error handling and state management. Made-with: Cursor
This commit is contained in:
parent
dae8c50e4c
commit
2a187894a5
3 changed files with 30 additions and 47 deletions
|
|
@ -914,11 +914,10 @@ export class TeamDataService {
|
||||||
const comment = await this.taskWriter.addComment(teamName, taskId, text);
|
const comment = await this.taskWriter.addComment(teamName, taskId, text);
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const [tasks, toolPath, config, metaMembers] = await Promise.all([
|
const [tasks, toolPath, config] = await Promise.all([
|
||||||
this.taskReader.getTasks(teamName),
|
this.taskReader.getTasks(teamName),
|
||||||
this.toolsInstaller.ensureInstalled(),
|
this.toolsInstaller.ensureInstalled(),
|
||||||
this.configReader.getConfig(teamName).catch(() => null),
|
this.configReader.getConfig(teamName).catch(() => null),
|
||||||
this.membersMetaStore.getMembers(teamName).catch(() => []),
|
|
||||||
]);
|
]);
|
||||||
const task = tasks.find((t) => t.id === taskId);
|
const task = tasks.find((t) => t.id === taskId);
|
||||||
const leadName = this.resolveLeadNameFromConfig(config);
|
const leadName = this.resolveLeadNameFromConfig(config);
|
||||||
|
|
@ -930,12 +929,9 @@ export class TeamDataService {
|
||||||
}
|
}
|
||||||
|
|
||||||
if (task?.owner) {
|
if (task?.owner) {
|
||||||
// Solo team UX: if the user comments on a lead-owned task, don't echo the
|
// UX: don't echo a user comment as an inbox notification "from the lead" when the
|
||||||
// comment back as an inbox notification from the lead. The comment is already visible.
|
// task is already owned by the lead. This creates confusing self-notifications.
|
||||||
if (
|
if (this.isLeadOwner(task.owner, leadName)) {
|
||||||
this.isSoloTeamFromMembers(config, metaMembers, leadName) &&
|
|
||||||
this.isLeadOwner(task.owner, leadName)
|
|
||||||
) {
|
|
||||||
return comment;
|
return comment;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -980,30 +976,9 @@ export class TeamDataService {
|
||||||
}
|
}
|
||||||
|
|
||||||
private isLeadOwner(owner: string, leadName: string): boolean {
|
private isLeadOwner(owner: string, leadName: string): boolean {
|
||||||
const normalized = owner.trim();
|
const normalized = owner.trim().toLowerCase();
|
||||||
if (!normalized) return false;
|
if (!normalized) return false;
|
||||||
return normalized === leadName || normalized === 'team-lead';
|
return normalized === leadName.trim().toLowerCase() || normalized === 'team-lead';
|
||||||
}
|
|
||||||
|
|
||||||
private isSoloTeamFromMembers(
|
|
||||||
config: TeamConfig | null,
|
|
||||||
metaMembers: TeamMember[],
|
|
||||||
leadName: string
|
|
||||||
): boolean {
|
|
||||||
const configMembers = config?.members ?? [];
|
|
||||||
const combined = [...configMembers, ...(metaMembers ?? [])];
|
|
||||||
|
|
||||||
const activeNonLead = combined.filter((m) => {
|
|
||||||
const name = m.name?.trim();
|
|
||||||
if (!name) return false;
|
|
||||||
if (m.removedAt) return false;
|
|
||||||
if (m.agentType === 'team-lead') return false;
|
|
||||||
if (name === 'team-lead') return false;
|
|
||||||
if (name === leadName) return false;
|
|
||||||
return true;
|
|
||||||
});
|
|
||||||
|
|
||||||
return activeNonLead.length === 0;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
async sendDirectToLead(
|
async sendDirectToLead(
|
||||||
|
|
|
||||||
|
|
@ -115,6 +115,7 @@ export const ChangeReviewDialog = ({
|
||||||
// Track recent per-hunk actions so Ctrl/Cmd+Z can clear persisted decisions (reopen-safe)
|
// Track recent per-hunk actions so Ctrl/Cmd+Z can clear persisted decisions (reopen-safe)
|
||||||
const lastHunkActionAtRef = useRef<Record<string, number>>({});
|
const lastHunkActionAtRef = useRef<Record<string, number>>({});
|
||||||
const hunkDecisionUndoStackRef = useRef<Record<string, number[]>>({});
|
const hunkDecisionUndoStackRef = useRef<Record<string, number[]>>({});
|
||||||
|
const newFileApplyInFlightRef = useRef(new Set<string>());
|
||||||
|
|
||||||
// Proxy ref for useDiffNavigation (points to active file's editor)
|
// Proxy ref for useDiffNavigation (points to active file's editor)
|
||||||
const activeEditorViewRef = useRef<EditorView | null>(null);
|
const activeEditorViewRef = useRef<EditorView | null>(null);
|
||||||
|
|
@ -230,21 +231,28 @@ export const ChangeReviewDialog = ({
|
||||||
|
|
||||||
const handleRejectNewFile = useCallback(
|
const handleRejectNewFile = useCallback(
|
||||||
async (filePath: string) => {
|
async (filePath: string) => {
|
||||||
// Mark rejected in store + update CM view immediately for feedback
|
if (newFileApplyInFlightRef.current.has(filePath)) return;
|
||||||
rejectAllFile(filePath);
|
newFileApplyInFlightRef.current.add(filePath);
|
||||||
const view = editorViewMapRef.current.get(filePath);
|
try {
|
||||||
if (view) {
|
// Mark rejected in store + update CM view immediately for feedback
|
||||||
requestAnimationFrame(() => rejectAllChunks(view));
|
rejectAllFile(filePath);
|
||||||
}
|
const view = editorViewMapRef.current.get(filePath);
|
||||||
|
if (view) {
|
||||||
|
requestAnimationFrame(() => rejectAllChunks(view));
|
||||||
|
}
|
||||||
|
|
||||||
// Always apply immediately: rejecting a NEW file means deleting it from disk.
|
// Always apply immediately: rejecting a NEW file means deleting it from disk.
|
||||||
const isNew = activeChangeSet?.files.find((f) => f.filePath === filePath)?.isNewFile ?? false;
|
const isNew =
|
||||||
if (!isNew) return;
|
activeChangeSet?.files.find((f) => f.filePath === filePath)?.isNewFile ?? false;
|
||||||
|
if (!isNew) return;
|
||||||
|
|
||||||
const result = await applySingleFileDecision(teamName, filePath, taskId, memberName);
|
const result = await applySingleFileDecision(teamName, filePath, taskId, memberName);
|
||||||
const hasErrorForFile = !!result?.errors.some((e) => e.filePath === filePath);
|
const hasErrorForFile = !!result?.errors.some((e) => e.filePath === filePath);
|
||||||
if (result && !hasErrorForFile) {
|
if (result && !hasErrorForFile) {
|
||||||
removeReviewFile(filePath);
|
removeReviewFile(filePath);
|
||||||
|
}
|
||||||
|
} finally {
|
||||||
|
newFileApplyInFlightRef.current.delete(filePath);
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
[
|
[
|
||||||
|
|
|
||||||
|
|
@ -727,10 +727,10 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
|
||||||
memberName?: string
|
memberName?: string
|
||||||
) => {
|
) => {
|
||||||
const { hunkDecisions, fileDecisions, fileChunkCounts, activeChangeSet, fileContents } = get();
|
const { hunkDecisions, fileDecisions, fileChunkCounts, activeChangeSet, fileContents } = get();
|
||||||
if (!activeChangeSet) return;
|
if (!activeChangeSet) return null;
|
||||||
|
|
||||||
const file = activeChangeSet.files.find((f) => f.filePath === filePath);
|
const file = activeChangeSet.files.find((f) => f.filePath === filePath);
|
||||||
if (!file) return;
|
if (!file) return null;
|
||||||
|
|
||||||
const fileDecision = fileDecisions[filePath] ?? 'pending';
|
const fileDecision = fileDecisions[filePath] ?? 'pending';
|
||||||
const hunkDecs: Record<number, HunkDecision> = {};
|
const hunkDecs: Record<number, HunkDecision> = {};
|
||||||
|
|
@ -743,7 +743,7 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
|
||||||
|
|
||||||
const hasRejected =
|
const hasRejected =
|
||||||
fileDecision === 'rejected' || Object.values(hunkDecs).some((d) => d === 'rejected');
|
fileDecision === 'rejected' || Object.values(hunkDecs).some((d) => d === 'rejected');
|
||||||
if (!hasRejected) return;
|
if (!hasRejected) return null;
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const content = fileContents[filePath];
|
const content = fileContents[filePath];
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue