Skip to content

Commit 2af4b2d

Browse files
committed
Add visibility guards to keyboard handlers on both surfaces
When keep-alive hides a surface with visibility:hidden, its keyboard listeners on window/document stay active. Without guards, Mod+Enter on the visible code review would also fire the hidden plan review's submit handler. Both App.tsx files now check getComputedStyle(rootRef).visibility at the start of every keyboard handler. If hidden, return early.
1 parent fcfaac2 commit 2af4b2d

2 files changed

Lines changed: 21 additions & 2 deletions

File tree

packages/plannotator-code-review/App.tsx

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,11 @@ function getFileTabTitle(filePath: string): string {
135135
const ReviewApp: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }> = ({ __embedded, headerLeft }) => {
136136
const fetch = useSessionFetch();
137137
const { resolvedMode } = useTheme();
138+
const rootRef = useRef<HTMLDivElement>(null);
139+
const isVisible = useCallback(() => {
140+
if (!rootRef.current) return true;
141+
return getComputedStyle(rootRef.current).visibility !== 'hidden';
142+
}, []);
138143
const [diffData, setDiffData] = useState<DiffData | null>(null);
139144
const [files, setFiles] = useState<DiffFile[]>([]);
140145
const [activeFileIndex, setActiveFileIndex] = useState(0);
@@ -635,6 +640,7 @@ const ReviewApp: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }
635640
useEffect(() => {
636641
if (!import.meta.env.DEV) return;
637642
const handler = (e: KeyboardEvent) => {
643+
if (!isVisible()) return;
638644
if ((e.metaKey || e.ctrlKey) && e.shiftKey && (e.key === 'T' || e.key === 't')) {
639645
e.preventDefault();
640646
setTourDialogJobId(prev => (prev === DEMO_TOUR_ID ? null : DEMO_TOUR_ID));
@@ -701,6 +707,7 @@ const ReviewApp: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }
701707
// Global keyboard shortcuts
702708
useEffect(() => {
703709
const handleKeyDown = (e: KeyboardEvent) => {
710+
if (!isVisible()) return;
704711
// Cmd/Ctrl+F to focus file search when diff files are available.
705712
if ((e.metaKey || e.ctrlKey) && e.key.toLowerCase() === 'f' && !isTypingTarget(e.target)) {
706713
if (hasSearchableFiles) {
@@ -1074,6 +1081,7 @@ const ReviewApp: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }
10741081

10751082
useEffect(() => {
10761083
const handler = (e: KeyboardEvent) => {
1084+
if (!isVisible()) return;
10771085
if (e.metaKey || e.ctrlKey || e.shiftKey || isTypingTarget(e.target)) return;
10781086
if (!isDiffPanelActive) return;
10791087
const filePath = files[activeFileIndex]?.path;
@@ -1676,6 +1684,7 @@ const ReviewApp: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }
16761684
const DOUBLE_TAP_WINDOW = 300;
16771685

16781686
const handleKeyDown = (e: KeyboardEvent) => {
1687+
if (!isVisible()) return;
16791688
if (e.key !== 'Alt' || e.repeat) return;
16801689
const tag = (e.target as HTMLElement)?.tagName;
16811690
if (tag === 'INPUT' || tag === 'TEXTAREA') return;
@@ -1708,6 +1717,7 @@ const ReviewApp: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }
17081717
// Cmd/Ctrl+Enter keyboard shortcut to approve or send feedback
17091718
useEffect(() => {
17101719
const handleKeyDown = (e: KeyboardEvent) => {
1720+
if (!isVisible()) return;
17111721
if (e.key !== 'Enter' || !(e.metaKey || e.ctrlKey)) return;
17121722

17131723
// If the platform post dialog is open, Cmd+Enter submits it
@@ -1772,7 +1782,7 @@ const ReviewApp: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }
17721782
<ReviewStateProvider value={reviewStateValue}>
17731783
<JobLogsProvider value={jobLogsValue}>
17741784
{isSwitchingPRScope && <PRSwitchOverlay />}
1775-
<div className={`${__embedded ? 'h-full' : 'h-screen'} flex flex-col bg-background overflow-hidden`}>
1785+
<div ref={rootRef} className={`${__embedded ? 'h-full' : 'h-screen'} flex flex-col bg-background overflow-hidden`}>
17761786
{/* Header */}
17771787
<header className="py-1 flex items-center justify-between px-2 md:px-4 border-b border-border/50 bg-card/50 backdrop-blur-xl z-50">
17781788
<div className={`min-w-0 flex items-center gap-2 md:gap-3 ${headerLeft ? '' : '-ml-1.5 md:-ml-3'}`}>

packages/plannotator-plan-review/App.tsx

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,11 @@ type NoteAutoSaveResults = {
9999

100100
const App: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }> = ({ __embedded, headerLeft }) => {
101101
const fetch = useSessionFetch();
102+
const rootRef = useRef<HTMLDivElement>(null);
103+
const isVisible = useCallback(() => {
104+
if (!rootRef.current) return true;
105+
return getComputedStyle(rootRef.current).visibility !== 'hidden';
106+
}, []);
102107
const [markdown, setMarkdown] = useState(DEMO_PLAN_CONTENT);
103108
const [annotations, setAnnotations] = useState<Annotation[]>([]);
104109
const [codeAnnotations, setCodeAnnotations] = useState<CodeAnnotation[]>([]);
@@ -307,6 +312,7 @@ const App: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }> = ({
307312
useEffect(() => {
308313
if (!isPlanDiffActive) return;
309314
const handleKeyDown = (e: KeyboardEvent) => {
315+
if (!isVisible()) return;
310316
if (e.key === 'Escape') {
311317
setIsPlanDiffActive(false);
312318
}
@@ -1131,6 +1137,7 @@ const App: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }> = ({
11311137
// Global keyboard shortcuts (Cmd/Ctrl+Enter to submit)
11321138
useEffect(() => {
11331139
const handleKeyDown = (e: KeyboardEvent) => {
1140+
if (!isVisible()) return;
11341141
// Only handle Cmd/Ctrl+Enter
11351142
if (e.key !== 'Enter' || !(e.metaKey || e.ctrlKey)) return;
11361143

@@ -1509,6 +1516,7 @@ const App: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }> = ({
15091516
// Cmd/Ctrl+S keyboard shortcut — save to default notes app
15101517
useEffect(() => {
15111518
const handleSaveShortcut = (e: KeyboardEvent) => {
1519+
if (!isVisible()) return;
15121520
if (e.key !== 's' || !(e.metaKey || e.ctrlKey)) return;
15131521

15141522
const tag = (e.target as HTMLElement)?.tagName;
@@ -1551,6 +1559,7 @@ const App: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }> = ({
15511559
// Cmd/Ctrl+P keyboard shortcut — print plan
15521560
useEffect(() => {
15531561
const handlePrintShortcut = (e: KeyboardEvent) => {
1562+
if (!isVisible()) return;
15541563
if (e.key !== 'p' || !(e.metaKey || e.ctrlKey)) return;
15551564

15561565
const tag = (e.target as HTMLElement)?.tagName;
@@ -1682,7 +1691,7 @@ const App: React.FC<{ __embedded?: boolean; headerLeft?: React.ReactNode }> = ({
16821691
}
16831692

16841693
const innerContent = (
1685-
<div data-print-region="root" className={`${__embedded ? 'h-full' : 'h-screen'} flex flex-col bg-background overflow-hidden`}>
1694+
<div ref={rootRef} data-print-region="root" className={`${__embedded ? 'h-full' : 'h-screen'} flex flex-col bg-background overflow-hidden`}>
16861695
<AppHeader
16871696
headerLeft={headerLeft}
16881697
isApiMode={isApiMode}

0 commit comments

Comments
 (0)