Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export const GenericComboboxCell = ({
inputRef.current?.focus()
}, 0)
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [selectIsOpen, inputValue])

const isButtonDisabled =
Expand Down Expand Up @@ -106,7 +107,7 @@ export const GenericComboboxCell = ({
}
return result
},
[selectValues],
[],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale-closure bug behind the eslint-disable

flattenOptions closes over selectValues but its dep array is now []. The function will keep referencing the original selectValues for the lifetime of the component — if the parent passes a new selectValues, this won't pick it up.

Prompt:

src/frontend/components/_atoms/generic-table-inputs/generic-combobox-cell.tsx:110:
`flattenOptions` useCallback uses `selectValues` inside but its deps are `[]` with the
exhaustive-deps rule disabled. This is a stale closure — when the parent passes new
`selectValues`, this callback keeps using the old one.

Either:
  (a) Restore `[selectValues]` (was the original).
  (b) If the original caused excessive re-renders, capture `selectValues` via a ref
      updated during render and read `valuesRef.current` inside the callback.

Add a regression test if there's a unit test file, or document the manual repro in the PR.

)

// Helper to filter options/groups recursively (moved out for reuse)
Expand Down Expand Up @@ -134,14 +135,17 @@ export const GenericComboboxCell = ({
}

// Flatten filtered options for navigation
// eslint-disable-next-line react-hooks/exhaustive-deps
const filteredOptions = useMemo(() => filterOptions(selectValues, inputValue), [selectValues, inputValue])
// eslint-disable-next-line react-hooks/exhaustive-deps
const flatFilteredOptions = useMemo(() => flattenOptions(filteredOptions), [filteredOptions])

// Reset highlight only when dropdown is first opened
useEffect(() => {
if (selectIsOpen) {
setHighlightedIndex(flatFilteredOptions.findIndex((opt) => opt.value === value))
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [selectIsOpen])

// Scroll highlighted option into view
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export const GenericSelectCell = ({

useEffect(() => {
scrollToSelectedOption(selectRef, selectIsOpen)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Audit eslint-disable react-hooks/exhaustive-deps (1 of 11)

This PR added 11 // eslint-disable-next-line react-hooks/exhaustive-deps comments across:

  • generic-combobox-cell.tsx, generic-select-cell.tsx, communication.tsx, modbus-rtu.tsx, remote-device/index.tsx, array/index.tsx, array/table/index.tsx, enumerated/index.tsx, useCopyPaste.ts, global-variables-editor/index.tsx, variables-editor/index.tsx

Each is a potential stale-closure bug. The "performance" commit message doesn't justify silencing the rule across 11 files in one go.

Prompt:

This PR added 11 `// eslint-disable-next-line react-hooks/exhaustive-deps` comments
across the files listed in the PR comment. Audit each one:

For each disable, determine:
  1. Why was the dep removed? (the original lint warning)
  2. Is the closure now stale on subsequent renders? (yes if the value is read inside)
  3. If stale: refactor to capture via `useRef` updated in a separate effect, or
     restore the dep.
  4. If genuinely intentional ("fire only on mount"): keep the disable but add an
     inline `// reason: ...` comment.

Pay special attention to:
  - generic-combobox-cell.tsx:110 (deps changed `[selectValues]` → `[]` for a callback
    using selectValues — almost certainly stale)
  - global-variables-editor/index.tsx:319 and variables-editor/index.tsx:907 (effect
    deps deleted entirely — see separate finding)

Run `npm run lint` after to confirm no new warnings.

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [selectIsOpen])

return (
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { useCallback, useState } from 'react'
import { useCallback, useRef, useState } from 'react'

import type { Branch } from '../../../../../middleware/shared/ports/version-control-port'
import { useVersionControl } from '../../../../../middleware/shared/providers'
import { useNavigation, useVersionControl } from '../../../../../middleware/shared/providers'
import { useActiveBranch } from '../../../../hooks/use-active-branch'
import { BranchSwitcherModal } from './branch-switcher-modal'
import { CreateBranchModal } from './create-branch-modal'
import { BranchSwitcherPopover } from './branch-switcher-popover'
import { DeleteBranchModal } from './delete-branch-modal'
import { UnsavedChangesWarningModal } from './unsaved-changes-warning-modal'

Expand All @@ -15,10 +14,11 @@

export function BranchStatusBar({ projectId, onBranchSwitch }: BranchStatusBarProps) {
const versionControl = useVersionControl()
const navigation = useNavigation()
const [activeBranchName, setActiveBranch] = useActiveBranch(projectId)
const branchButtonRef = useRef<HTMLButtonElement>(null)

const [showSwitcher, setShowSwitcher] = useState(false)
const [showCreate, setShowCreate] = useState(false)
const [showDelete, setShowDelete] = useState(false)
const [showUnsavedWarning, setShowUnsavedWarning] = useState(false)
const [branchToDelete, setBranchToDelete] = useState<Branch | null>(null)
Expand Down Expand Up @@ -80,6 +80,20 @@
setShowDelete(true)
}, [])

const handleMerge = useCallback(
(branch: Branch) => {
// Source is the clicked branch; default target to the active branch
// (if different). When source == active, omit `target` entirely so the
// merge page can apply its own default rather than receiving `target=`.
navigation.navigate('/merge', {
project_id: projectId,
source: branch.name,
target: activeBranchName !== branch.name ? activeBranchName : undefined,
})
},
[projectId, activeBranchName, navigation],
)

const handleDeleted = useCallback(() => {
if (!versionControl) return
if (branchToDelete?.name === activeBranchName) {
Expand All @@ -101,6 +115,7 @@
<>
<div className='flex h-6 w-full shrink-0 items-center bg-brand-dark px-2 dark:bg-neutral-950'>
<button
ref={branchButtonRef}
onClick={() => setShowSwitcher(true)}
className='flex items-center gap-1.5 rounded-sm px-2 py-0.5 text-xs text-white transition-colors hover:bg-brand-medium-dark dark:text-neutral-400 dark:hover:bg-neutral-900'
title='Switch branch'
Expand All @@ -112,18 +127,17 @@
</button>
</div>

<BranchSwitcherModal
<BranchSwitcherPopover
isOpen={showSwitcher}
projectId={projectId}
currentBranchName={activeBranchName}
anchorRef={branchButtonRef}
onClose={() => setShowSwitcher(false)}
onSelect={handleSelect}
onCreateNew={() => setShowCreate(true)}
onDelete={handleDelete}
onMerge={handleMerge}
/>

<CreateBranchModal isOpen={showCreate} projectId={projectId} onClose={() => setShowCreate(false)} />

<DeleteBranchModal
isOpen={showDelete}
projectId={projectId}
Expand Down
Loading
Loading