From 6c9a8b200ffb36c67242a048fbacfd0ca65e15dd Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Sat, 14 Feb 2026 15:22:32 +0100 Subject: [PATCH] Fix follow-up review findings: data loss bug, dialog nesting, and quality improvements - Preserve manual competitors in analyze(enabled=False) path (HIGH data loss bug) - Fix incomplete rollback by restoring full previous competitorAnalysis state - Reset showAddDialog state when ExistingCompetitorAnalysisDialog reopens - Add encoding option to writeFileWithRetry for competitor analysis save - Replace X icon with AlertCircle in error alert for clarity - Add type="button" to all raw button elements in dialog - Add warning logs to silent exception handlers in competitor_analyzer.py - Forward onCompetitorAdded callback through ExistingCompetitorAnalysisDialog Co-Authored-By: Claude Opus 4.6 --- .../runners/roadmap/competitor_analyzer.py | 13 ++++++++++--- .../src/main/ipc-handlers/roadmap-handlers.ts | 3 ++- .../renderer/components/AddCompetitorDialog.tsx | 9 ++++++--- .../ExistingCompetitorAnalysisDialog.tsx | 16 +++++++++++++++- 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/apps/backend/runners/roadmap/competitor_analyzer.py b/apps/backend/runners/roadmap/competitor_analyzer.py index c6260ed9..d4fdbe12 100644 --- a/apps/backend/runners/roadmap/competitor_analyzer.py +++ b/apps/backend/runners/roadmap/competitor_analyzer.py @@ -42,7 +42,10 @@ class CompetitorAnalyzer: """ if not enabled: print_status("Competitor analysis not enabled, skipping", "info") + manual_competitors = self._get_manual_competitors() self._create_disabled_analysis_file() + if manual_competitors: + self._merge_manual_competitors(manual_competitors) return RoadmapPhaseResult( "competitor_analysis", True, [str(self.analysis_file)], [], 0 ) @@ -133,7 +136,8 @@ class CompetitorAnalyzer: for c in data.get("competitors", []) if isinstance(c, dict) and c.get("source") == "manual" ] - except (json.JSONDecodeError, OSError): + except (json.JSONDecodeError, OSError) as e: + print_status(f"Warning: could not read manual competitors: {e}", "warning") return [] def _merge_manual_competitors(self, manual_competitors: list[dict]) -> None: @@ -195,8 +199,11 @@ Output your findings to competitor_analysis.json. "competitor_analysis", True, [str(self.analysis_file)], [], 0 ) - except json.JSONDecodeError: - pass + except json.JSONDecodeError as e: + print_status( + f"Warning: competitor analysis file is not valid JSON: {e}", + "warning", + ) return None diff --git a/apps/frontend/src/main/ipc-handlers/roadmap-handlers.ts b/apps/frontend/src/main/ipc-handlers/roadmap-handlers.ts index 09eb8260..b8b02785 100644 --- a/apps/frontend/src/main/ipc-handlers/roadmap-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/roadmap-handlers.ts @@ -877,7 +877,8 @@ ${(feature.acceptance_criteria || []).map((c: string) => `- [ ] ${c}`).join("\n" await writeFileWithRetry( competitorAnalysisPath, - JSON.stringify(serialized, null, 2) + JSON.stringify(serialized, null, 2), + { encoding: 'utf-8' } ); }); diff --git a/apps/frontend/src/renderer/components/AddCompetitorDialog.tsx b/apps/frontend/src/renderer/components/AddCompetitorDialog.tsx index 8f0b5ba2..da309245 100644 --- a/apps/frontend/src/renderer/components/AddCompetitorDialog.tsx +++ b/apps/frontend/src/renderer/components/AddCompetitorDialog.tsx @@ -21,7 +21,7 @@ */ import { useState, useEffect } from 'react'; import { useTranslation } from 'react-i18next'; -import { Loader2, X } from 'lucide-react'; +import { Loader2, AlertCircle } from 'lucide-react'; import { Dialog, DialogContent, @@ -142,6 +142,9 @@ export function AddCompetitorDialog({ setError(null); try { + // Capture pre-add state for complete rollback + const previousAnalysis = useRoadmapStore.getState().competitorAnalysis; + // Add competitor to store const newCompetitorId = addCompetitor({ name: name.trim(), @@ -156,7 +159,7 @@ export function AddCompetitorDialog({ const result = await window.electronAPI.saveCompetitorAnalysis(projectId, competitorAnalysis); if (!result.success) { // Rollback store state since save failed - useRoadmapStore.getState().removeCompetitor(newCompetitorId); + useRoadmapStore.getState().setCompetitorAnalysis(previousAnalysis); throw new Error(result.error || t('addCompetitor.failedToAdd')); } } @@ -262,7 +265,7 @@ export function AddCompetitorDialog({ {/* Error */} {error && (
- + {error}
)} diff --git a/apps/frontend/src/renderer/components/ExistingCompetitorAnalysisDialog.tsx b/apps/frontend/src/renderer/components/ExistingCompetitorAnalysisDialog.tsx index 0a62dfa4..1dde532a 100644 --- a/apps/frontend/src/renderer/components/ExistingCompetitorAnalysisDialog.tsx +++ b/apps/frontend/src/renderer/components/ExistingCompetitorAnalysisDialog.tsx @@ -1,4 +1,4 @@ -import { useState } from 'react'; +import { useState, useEffect } from 'react'; import { useTranslation } from 'react-i18next'; import { Globe, RefreshCw, TrendingUp, CheckCircle, UserPlus } from 'lucide-react'; import { @@ -18,6 +18,7 @@ interface ExistingCompetitorAnalysisDialogProps { onUseExisting: () => void; onRunNew: () => void; onSkip: () => void; + onCompetitorAdded?: (competitorId: string) => void; analysisDate?: Date; projectId: string; } @@ -28,12 +29,20 @@ export function ExistingCompetitorAnalysisDialog({ onUseExisting, onRunNew, onSkip, + onCompetitorAdded, analysisDate, projectId, }: ExistingCompetitorAnalysisDialogProps) { const { t, i18n } = useTranslation(['dialogs']); const [showAddDialog, setShowAddDialog] = useState(false); + // Reset child dialog state when this dialog reopens + useEffect(() => { + if (open) { + setShowAddDialog(false); + } + }, [open]); + const handleUseExisting = () => { onUseExisting(); onOpenChange(false); @@ -75,6 +84,7 @@ export function ExistingCompetitorAnalysisDialog({
{/* Option 1: Use existing (recommended) */}