mirror of
https://github.com/jlengrand/Maestro.git
synced 2026-03-10 08:31:19 +00:00
fix(lightbox): sync lightboxImages state when deleting staged images
When opening the lightbox from staged images, a snapshot of stagedImages was saved to lightboxImages. The LightboxModal used this snapshot for navigation. When deleting an image, only stagedImages was updated but lightboxImages remained stale, causing the deleted image to still appear. Fix: Update both stagedImages AND lightboxImages in handleDeleteLightboxImage. Added regression tests that simulate the parent state synchronization pattern and verify both states are updated on deletion.
This commit is contained in:
@@ -1124,4 +1124,135 @@ describe('LightboxModal', () => {
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
describe('integration: parent state synchronization', () => {
|
||||
/**
|
||||
* This test simulates the App.tsx pattern where lightboxImages is a separate
|
||||
* snapshot from stagedImages. The bug we're testing for: when deleting an image,
|
||||
* both stagedImages AND lightboxImages must be updated, otherwise the lightbox
|
||||
* continues showing the "deleted" image from the stale lightboxImages snapshot.
|
||||
*/
|
||||
it('parent must update lightboxImages when deleting (regression test)', () => {
|
||||
// Simulate the parent component pattern from App.tsx
|
||||
let lightboxImages = [...mockImages]; // Snapshot taken when lightbox opened
|
||||
let stagedImages = [...mockImages]; // Actual staged images state
|
||||
let currentImage = mockImages[2]; // Viewing last image
|
||||
|
||||
const onClose = vi.fn();
|
||||
const onNavigate = vi.fn((img: string) => {
|
||||
currentImage = img;
|
||||
});
|
||||
|
||||
// This simulates the FIXED handleDeleteLightboxImage from App.tsx
|
||||
// The bug was that it only updated stagedImages, not lightboxImages
|
||||
const onDelete = vi.fn((img: string) => {
|
||||
stagedImages = stagedImages.filter((i) => i !== img);
|
||||
// CRITICAL: Also update lightboxImages (this was the bug fix)
|
||||
lightboxImages = lightboxImages.filter((i) => i !== img);
|
||||
});
|
||||
|
||||
const { rerender } = renderWithLayerStack(
|
||||
<LightboxModal
|
||||
image={currentImage}
|
||||
stagedImages={lightboxImages} // Note: uses lightboxImages, not stagedImages
|
||||
onClose={onClose}
|
||||
onNavigate={onNavigate}
|
||||
onDelete={onDelete}
|
||||
/>
|
||||
);
|
||||
|
||||
// Verify initial state: viewing image 3 of 3
|
||||
expect(screen.getByText(/Image 3 of 3/)).toBeInTheDocument();
|
||||
|
||||
// Delete the image
|
||||
const deleteButton = screen.getByTitle('Delete image (Delete key)');
|
||||
fireEvent.click(deleteButton);
|
||||
const confirmButton = screen.getByRole('button', { name: /confirm/i });
|
||||
fireEvent.click(confirmButton);
|
||||
|
||||
// Verify onDelete was called with the correct image
|
||||
expect(onDelete).toHaveBeenCalledWith(mockImages[2]);
|
||||
|
||||
// Verify navigation happened (should go to previous image since we deleted last)
|
||||
expect(onNavigate).toHaveBeenCalledWith(mockImages[1]);
|
||||
|
||||
// CRITICAL: After deletion, lightboxImages should be updated too
|
||||
// This is what the bug fix ensures
|
||||
expect(lightboxImages).toHaveLength(2);
|
||||
expect(lightboxImages).not.toContain(mockImages[2]);
|
||||
|
||||
// Rerender with updated state (simulating React re-render)
|
||||
rerender(
|
||||
<LayerStackProvider>
|
||||
<LightboxModal
|
||||
image={currentImage}
|
||||
stagedImages={lightboxImages}
|
||||
onClose={onClose}
|
||||
onNavigate={onNavigate}
|
||||
onDelete={onDelete}
|
||||
/>
|
||||
</LayerStackProvider>
|
||||
);
|
||||
|
||||
// Now should show image 2 of 2 (not 2 of 3)
|
||||
expect(screen.getByText(/Image 2 of 2/)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('demonstrates the bug when lightboxImages is not updated (negative test)', () => {
|
||||
// This test demonstrates what happens when the bug exists:
|
||||
// Only stagedImages is updated, lightboxImages remains stale
|
||||
let lightboxImages = [...mockImages]; // Snapshot - NOT updated on delete (bug!)
|
||||
let stagedImages = [...mockImages];
|
||||
let currentImage = mockImages[2];
|
||||
|
||||
const onClose = vi.fn();
|
||||
const onNavigate = vi.fn((img: string) => {
|
||||
currentImage = img;
|
||||
});
|
||||
|
||||
// BUGGY version: only updates stagedImages
|
||||
const onDeleteBuggy = vi.fn((img: string) => {
|
||||
stagedImages = stagedImages.filter((i) => i !== img);
|
||||
// BUG: lightboxImages is NOT updated!
|
||||
});
|
||||
|
||||
const { rerender } = renderWithLayerStack(
|
||||
<LightboxModal
|
||||
image={currentImage}
|
||||
stagedImages={lightboxImages}
|
||||
onClose={onClose}
|
||||
onNavigate={onNavigate}
|
||||
onDelete={onDeleteBuggy}
|
||||
/>
|
||||
);
|
||||
|
||||
// Delete the image
|
||||
const deleteButton = screen.getByTitle('Delete image (Delete key)');
|
||||
fireEvent.click(deleteButton);
|
||||
const confirmButton = screen.getByRole('button', { name: /confirm/i });
|
||||
fireEvent.click(confirmButton);
|
||||
|
||||
expect(onNavigate).toHaveBeenCalledWith(mockImages[1]);
|
||||
|
||||
// BUG: lightboxImages still has 3 images because it wasn't updated
|
||||
expect(lightboxImages).toHaveLength(3); // This is the bug!
|
||||
|
||||
// Rerender with the buggy state
|
||||
rerender(
|
||||
<LayerStackProvider>
|
||||
<LightboxModal
|
||||
image={currentImage}
|
||||
stagedImages={lightboxImages} // Still has 3 images!
|
||||
onClose={onClose}
|
||||
onNavigate={onNavigate}
|
||||
onDelete={onDeleteBuggy}
|
||||
/>
|
||||
</LayerStackProvider>
|
||||
);
|
||||
|
||||
// BUG: Shows "Image 2 of 3" instead of "Image 2 of 2"
|
||||
// because lightboxImages wasn't updated
|
||||
expect(screen.getByText(/Image 2 of 3/)).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -550,6 +550,9 @@ function MaestroConsoleInner() {
|
||||
|
||||
// Rendering settings
|
||||
disableConfetti,
|
||||
|
||||
// Tab naming settings
|
||||
automaticTabNamingEnabled,
|
||||
} = settings;
|
||||
|
||||
// --- KEYBOARD SHORTCUT HELPERS ---
|
||||
@@ -2600,6 +2603,60 @@ function MaestroConsoleInner() {
|
||||
const actualSessionId = parsed.actualSessionId;
|
||||
const tabId = parsed.tabId ?? undefined;
|
||||
|
||||
// First, check if we need to trigger automatic tab naming BEFORE updating state
|
||||
// We need the current session state to determine this
|
||||
const currentSessions = sessionsRef.current;
|
||||
const currentSession = currentSessions.find((s) => s.id === actualSessionId);
|
||||
|
||||
// Capture info for tab naming (must be done before state update modifies awaitingSessionId)
|
||||
interface TabNamingInfo {
|
||||
tabId: string;
|
||||
userMessage: string;
|
||||
agentType: string;
|
||||
cwd: string;
|
||||
sessionSshRemoteConfig?: {
|
||||
enabled: boolean;
|
||||
remoteId: string | null;
|
||||
workingDirOverride?: string;
|
||||
};
|
||||
}
|
||||
let tabNamingInfo: TabNamingInfo | null = null;
|
||||
|
||||
if (currentSession) {
|
||||
// Find the target tab (same logic as in setSessions below)
|
||||
let targetTab = tabId
|
||||
? currentSession.aiTabs?.find((tab) => tab.id === tabId)
|
||||
: undefined;
|
||||
|
||||
if (!targetTab) {
|
||||
const awaitingTab = currentSession.aiTabs?.find(
|
||||
(tab) => tab.awaitingSessionId && !tab.agentSessionId
|
||||
);
|
||||
targetTab = awaitingTab || getActiveTab(currentSession);
|
||||
}
|
||||
|
||||
// Check if we should trigger automatic tab naming:
|
||||
// 1. Tab was awaiting session ID (this is a new session, not a resume)
|
||||
// 2. Tab doesn't already have a custom name
|
||||
// 3. Tab has at least one user message in logs
|
||||
if (targetTab?.awaitingSessionId && !targetTab.name) {
|
||||
const userMessages = targetTab.logs.filter((log) => log.source === 'user');
|
||||
if (userMessages.length > 0) {
|
||||
const firstUserMessage = userMessages[0];
|
||||
// Only use text messages for naming (skip image-only messages)
|
||||
if (firstUserMessage.text?.trim()) {
|
||||
tabNamingInfo = {
|
||||
tabId: targetTab.id,
|
||||
userMessage: firstUserMessage.text,
|
||||
agentType: currentSession.toolType,
|
||||
cwd: currentSession.cwd,
|
||||
sessionSshRemoteConfig: currentSession.sessionSshRemoteConfig,
|
||||
};
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Store Claude session ID in session state
|
||||
// Note: slash commands are now received via onSlashCommands from Claude Code's init message
|
||||
setSessions((prev) => {
|
||||
@@ -2663,6 +2720,61 @@ function MaestroConsoleInner() {
|
||||
return { ...s, aiTabs: updatedAiTabs, agentSessionId }; // Also keep session-level for backwards compatibility
|
||||
});
|
||||
});
|
||||
|
||||
// Trigger automatic tab naming if conditions are met
|
||||
// This runs after the state update so it happens in parallel with the main AI processing
|
||||
// Use ref to access the latest setting value (useEffect has [] deps so we'd get stale value otherwise)
|
||||
if (tabNamingInfo && automaticTabNamingEnabledRef.current) {
|
||||
console.log('[onSessionId] Triggering automatic tab naming', {
|
||||
tabId: tabNamingInfo.tabId,
|
||||
messageLength: tabNamingInfo.userMessage.length,
|
||||
agentType: tabNamingInfo.agentType,
|
||||
});
|
||||
|
||||
// Call the tab naming API (async, don't await)
|
||||
window.maestro.tabNaming
|
||||
.generateTabName({
|
||||
userMessage: tabNamingInfo.userMessage,
|
||||
agentType: tabNamingInfo.agentType,
|
||||
cwd: tabNamingInfo.cwd,
|
||||
sessionSshRemoteConfig: tabNamingInfo.sessionSshRemoteConfig,
|
||||
})
|
||||
.then((generatedName) => {
|
||||
if (!generatedName) {
|
||||
console.log('[onSessionId] Tab naming returned null (timeout or error)');
|
||||
return;
|
||||
}
|
||||
|
||||
console.log('[onSessionId] Tab naming generated:', generatedName);
|
||||
|
||||
// Update the tab name only if it's still null (user hasn't renamed it)
|
||||
setSessions((prev) =>
|
||||
prev.map((s) => {
|
||||
if (s.id !== actualSessionId) return s;
|
||||
|
||||
const tab = s.aiTabs.find((t) => t.id === tabNamingInfo!.tabId);
|
||||
// Only update if tab exists and name is still null (UUID display)
|
||||
if (!tab || tab.name !== null) {
|
||||
console.log('[onSessionId] Skipping tab name update (already named)', {
|
||||
tabExists: !!tab,
|
||||
currentName: tab?.name,
|
||||
});
|
||||
return s;
|
||||
}
|
||||
|
||||
return {
|
||||
...s,
|
||||
aiTabs: s.aiTabs.map((t) =>
|
||||
t.id === tabNamingInfo!.tabId ? { ...t, name: generatedName } : t
|
||||
),
|
||||
};
|
||||
})
|
||||
);
|
||||
})
|
||||
.catch((error) => {
|
||||
console.error('[onSessionId] Tab naming failed:', error);
|
||||
});
|
||||
}
|
||||
}
|
||||
);
|
||||
|
||||
@@ -3482,11 +3594,13 @@ function MaestroConsoleInner() {
|
||||
const customAICommandsRef = useRef(customAICommands);
|
||||
const speckitCommandsRef = useRef(speckitCommands);
|
||||
const openspecCommandsRef = useRef(openspecCommands);
|
||||
const automaticTabNamingEnabledRef = useRef(automaticTabNamingEnabled);
|
||||
addToastRef.current = addToast;
|
||||
updateGlobalStatsRef.current = updateGlobalStats;
|
||||
customAICommandsRef.current = customAICommands;
|
||||
speckitCommandsRef.current = speckitCommands;
|
||||
openspecCommandsRef.current = openspecCommands;
|
||||
automaticTabNamingEnabledRef.current = automaticTabNamingEnabled;
|
||||
|
||||
// Note: spawnBackgroundSynopsisRef and spawnAgentWithPromptRef are now provided by useAgentExecution hook
|
||||
// Note: addHistoryEntryRef is now provided by useAgentSessionManagement hook
|
||||
@@ -11248,8 +11362,11 @@ You are taking over this conversation. Based on the context above, provide a bri
|
||||
} else {
|
||||
setStagedImages((prev) => prev.filter((i) => i !== img));
|
||||
}
|
||||
// Also update lightboxImages so the lightbox navigation stays in sync
|
||||
// (lightboxImages is a snapshot taken when the lightbox opened, so it gets stale on deletion)
|
||||
setLightboxImages(lightboxImages.filter((i) => i !== img));
|
||||
},
|
||||
[setStagedImages]
|
||||
[setStagedImages, setLightboxImages, lightboxImages]
|
||||
);
|
||||
const handleCloseAutoRunSetup = useCallback(() => setAutoRunSetupModalOpen(false), []);
|
||||
const handleCloseBatchRunner = useCallback(() => setBatchRunnerModalOpen(false), []);
|
||||
|
||||
Reference in New Issue
Block a user