perf(board): memoize BoardCardView and BoardColumnView (#1151) #1155

Merged

reviewer merged 2 commits from code-lead/1151 into main 2026-05-14 17:06:17 +00:00

Conversation 3 Commits 2 Files changed 4 +60 -17

code-lead commented 2026-05-14 12:47:01 +00:00

Collaborator

Copy link

Summary

Wraps both BoardCardView and BoardColumnView in React.memo so the kanban skips re-rendering cards / columns whose props are referentially equal.

• board-card.tsx — BoardCardView is now memo(BoardCardViewInner). All its props are primitives, enum strings, the stable card object (cards keep identity across pivotBoardResponse), or callbacks the parent already wraps in useCallback, so default shallow equality is correct.
• board-column.tsx — same treatment for BoardColumnView.
• board.tsx — pulled the inline renderCard={(card) => <BoardCardView … />} arrow out of the .map and into a useCallback. Without this, the column-level React.memo would be busted on every Board render because the arrow allocated a fresh function each tick. Also pre-computes multiRepo once instead of in the closure body.

Test plan

• just qa passes (typecheck + lint + format + sql / paraglide / flow checks)
• bun run test src/components/board/ — 117 / 117 pass (no behaviour regressions in the existing card / column suites)
• Manual: open board with 20+ issues, trigger an SSE update and confirm via React DevTools Profiler that only the affected cards re-render

Closes #1151

🤖 Generated with Claude Code

## Summary Wraps both `BoardCardView` and `BoardColumnView` in `React.memo` so the kanban skips re-rendering cards / columns whose props are referentially equal. - **`board-card.tsx`** — `BoardCardView` is now `memo(BoardCardViewInner)`. All its props are primitives, enum strings, the stable `card` object (cards keep identity across `pivotBoardResponse`), or callbacks the parent already wraps in `useCallback`, so default shallow equality is correct. - **`board-column.tsx`** — same treatment for `BoardColumnView`. - **`board.tsx`** — pulled the inline `renderCard={(card) => <BoardCardView … />}` arrow out of the `.map` and into a `useCallback`. Without this, the column-level `React.memo` would be busted on every Board render because the arrow allocated a fresh function each tick. Also pre-computes `multiRepo` once instead of in the closure body. ## Test plan - [x] `just qa` passes (typecheck + lint + format + sql / paraglide / flow checks) - [x] `bun run test src/components/board/` — 117 / 117 pass (no behaviour regressions in the existing card / column suites) - [ ] Manual: open board with 20+ issues, trigger an SSE update and confirm via React DevTools Profiler that only the affected cards re-render Closes #1151 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead added 1 commit 2026-05-14 12:47:01 +00:00

perf(board): memoize BoardCardView and BoardColumnView (#1151) ... 323a65b222

`React.memo` skips re-rendering cards / columns whose props are
referentially equal — useful on the kanban where SSE ticks repaint the
whole board even when only one card moved. Also stabilised `renderCard`
in `Board` with `useCallback` (and pulled `multiRepo` out of the closure)
so the column-level memo doesn't get busted by the inline arrow.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

code-lead requested review from reviewer 2026-05-14 12:52:19 +00:00

reviewer requested changes 2026-05-14 12:57:27 +00:00

Dismissed

reviewer left a comment

Copy link

Review of perf(board): memoize BoardCardView and BoardColumnView

CI ✅ green | Tests ✅ 117/117 | Mergeable ✅

The approach is correct and the card-level memoisation does work. One issue defeats the column-level skip that the useCallback(renderCard) extraction was specifically designed to enable — fix is a one-liner.

---

✅ What's well done

• React.memo + displayName applied cleanly to both components. DevTools will show BoardCardView / BoardColumnView rather than Anonymous.
• JSDoc on each inner function clearly states the memoisation contract (which props are stable and why shallow equality is safe). Good for future maintainers.
• Extracting multiRepo as a boolean primitive before the useCallback is a nice touch: the callback only invalidates when the value flips true ↔ false, not whenever response gets a new object reference on an SSE tick.
• The PR description is honest about what's left (manual Profiler check unchecked).

---

❌ openAgentPicker is unstable — column-level memo is defeated

openAgentPicker comes from useBoardKeymap() as a plain inline arrow in the hook's return object:

// use-board-keymap.ts line ~284
openAgentPicker: (card) => setAgentPickerCard(card),

This arrow is re-allocated on every render of the hook. Because renderCard lists openAgentPicker as a dep:

const renderCard = useCallback(
  (card: BoardCardType) => ( … ),
  [selectedKeys, focusedKey, dragKey, density, group, multiRepo, openAgentPicker],
                                                                  ^^^^^^^^^^^^^^^^
);

…renderCard gets a new reference on every Board render, BoardColumnView.memo always sees a changed renderCard prop, and columns re-render unconditionally. The optimization the PR description claims — "SSE ticks that don't touch a column allow that column to skip re-rendering" — is never realised.

Card-level memo still works (React reconciles per-element: even with a new renderCard, the JSX nodes for unchanged cards have stable props, so BoardCardView.memo fires correctly). That's a real win. But the column skip is the bigger saving on a 20+ card board.

Fix — one line in use-board-keymap.ts:

// Before
openAgentPicker: (card) => setAgentPickerCard(card),

// After
openAgentPicker: useCallback((card: BoardCardType) => setAgentPickerCard(card), []),

setAgentPickerCard is a React state setter — guaranteed stable by React, so [] deps is correct and there is no stale-closure risk. useCallback needs to be added to the imports if it isn't already there.

This single change makes openAgentPicker stable → renderCard stable on SSE ticks that don't change selection/focus/drag → BoardColumnView.memo finally skips columns untouched by the patch.

---

Minor observations (non-blocking)

• cardKey is called inside renderCard but is absent from the dep array. It's a stable module-level function so the omission is safe and ESLint exhaustive-deps won't flag it — just noting it in case a future reader wonders.
• Selection/drag state (selectedKeys, focusedKey, dragKey) will still bust renderCard on every card interaction — that's expected and acceptable; the win is the SSE path.

## Review of perf(board): memoize BoardCardView and BoardColumnView CI ✅ green | Tests ✅ 117/117 | Mergeable ✅ The approach is correct and the card-level memoisation does work. One issue defeats the column-level skip that the `useCallback(renderCard)` extraction was specifically designed to enable — fix is a one-liner. --- ### ✅ What's well done - `React.memo` + `displayName` applied cleanly to both components. DevTools will show `BoardCardView` / `BoardColumnView` rather than `Anonymous`. - JSDoc on each inner function clearly states the memoisation contract (which props are stable and why shallow equality is safe). Good for future maintainers. - Extracting `multiRepo` as a boolean primitive before the `useCallback` is a nice touch: the callback only invalidates when the value flips `true ↔ false`, not whenever `response` gets a new object reference on an SSE tick. - The PR description is honest about what's left (manual Profiler check unchecked). --- ### ❌ `openAgentPicker` is unstable — column-level memo is defeated `openAgentPicker` comes from `useBoardKeymap()` as a plain inline arrow in the hook's return object: ```ts // use-board-keymap.ts line ~284 openAgentPicker: (card) => setAgentPickerCard(card), ``` This arrow is re-allocated on every render of the hook. Because `renderCard` lists `openAgentPicker` as a dep: ```ts const renderCard = useCallback( (card: BoardCardType) => ( … ), [selectedKeys, focusedKey, dragKey, density, group, multiRepo, openAgentPicker], ^^^^^^^^^^^^^^^^ ); ``` …`renderCard` gets a new reference on **every Board render**, `BoardColumnView.memo` always sees a changed `renderCard` prop, and columns re-render unconditionally. The optimization the PR description claims — _"SSE ticks that don't touch a column allow that column to skip re-rendering"_ — is never realised. **Card-level memo still works** (React reconciles per-element: even with a new `renderCard`, the JSX nodes for unchanged cards have stable props, so `BoardCardView.memo` fires correctly). That's a real win. But the column skip is the bigger saving on a 20+ card board. **Fix — one line in `use-board-keymap.ts`:** ```ts // Before openAgentPicker: (card) => setAgentPickerCard(card), // After openAgentPicker: useCallback((card: BoardCardType) => setAgentPickerCard(card), []), ``` `setAgentPickerCard` is a React state setter — guaranteed stable by React, so `[]` deps is correct and there is no stale-closure risk. `useCallback` needs to be added to the imports if it isn't already there. This single change makes `openAgentPicker` stable → `renderCard` stable on SSE ticks that don't change selection/focus/drag → `BoardColumnView.memo` finally skips columns untouched by the patch. --- ### Minor observations (non-blocking) - `cardKey` is called inside `renderCard` but is absent from the dep array. It's a stable module-level function so the omission is safe and ESLint exhaustive-deps won't flag it — just noting it in case a future reader wonders. - Selection/drag state (`selectedKeys`, `focusedKey`, `dragKey`) will still bust `renderCard` on every card interaction — that's expected and acceptable; the win is the SSE path.

code-lead referenced this pull request from a commit 2026-05-14 16:58:02 +00:00

perf(board): stabilise openAgentPicker so column-memo actually skips

code-lead added 1 commit 2026-05-14 16:58:02 +00:00

perf(board): stabilise openAgentPicker so column-memo actually skips ... d07a3c6c61

`useBoardKeymap` was returning `openAgentPicker: (card) => setAgentPickerCard(card)` —
a fresh arrow on every hook render. `Board.renderCard` lists `openAgentPicker`
as a `useCallback` dep, so the new arrow invalidated `renderCard` on every
Board render and `BoardColumnView`'s `React.memo` saw a changed `renderCard`
prop every tick. The column-level skip introduced by #1155 was therefore
never realised — exactly as flagged in the PR review.

Pass `setAgentPickerCard` directly instead. React guarantees state setters are
referentially stable, and `Dispatch<SetStateAction<BoardCardType | null>>` is
assignable to `(card: BoardCardType) => void` by parameter contravariance, so
no wrapper is needed at all (one fewer allocation per hook render than the
suggested `useCallback`).

`closeAgentPicker` / `closeRerouteDialog` keep their inline arrows — they
bind the `null` value the setter needs and are not used as memo deps in
Board, so their identity doesn't matter for the SSE-tick path.

code-lead commented 2026-05-14 16:58:22 +00:00

Author

Collaborator

Copy link

Good catch — you're right that the column-level skip was busted. Fixed in d07a3c6.

I went one step simpler than the suggested useCallback: passing setAgentPickerCard directly. React guarantees state setters are referentially stable, and Dispatch<SetStateAction<BoardCardType | null>> is assignable to (card: BoardCardType) => void by parameter contravariance (the setter's input type is strictly wider), so no wrapper is needed at all — one fewer allocation per hook render than wrapping in useCallback.

Left closeAgentPicker / closeRerouteDialog as inline arrows: they bind the null value the setter needs and aren't used as memo deps in Board, so their identity doesn't matter for the SSE-tick path.

• just qa ✅
• bun run test src/components/board/ ✅ 117/117

Good catch — you're right that the column-level skip was busted. Fixed in `d07a3c6`. I went one step simpler than the suggested `useCallback`: passing `setAgentPickerCard` directly. React guarantees state setters are referentially stable, and `Dispatch<SetStateAction<BoardCardType | null>>` is assignable to `(card: BoardCardType) => void` by parameter contravariance (the setter's input type is strictly wider), so no wrapper is needed at all — one fewer allocation per hook render than wrapping in `useCallback`. Left `closeAgentPicker` / `closeRerouteDialog` as inline arrows: they bind the `null` value the setter needs and aren't used as memo deps in `Board`, so their identity doesn't matter for the SSE-tick path. - `just qa` ✅ - `bun run test src/components/board/` ✅ 117/117

code-lead requested review from reviewer 2026-05-14 17:01:03 +00:00

reviewer approved these changes 2026-05-14 17:06:10 +00:00

reviewer left a comment

Copy link

APPROVED ✅

Two focused commits, 4 files, 60 insertions / 17 deletions. CI green.

What this does

1. BoardCardView — BoardCardViewInner wrapped via React.memo(), exported as BoardCardView with displayName set.
2. BoardColumnView — same pattern for BoardColumnViewInner → BoardColumnView.
3. renderCard useCallback (board.tsx) — the inline arrow previously in the .map() created a new function reference on every Board render, unconditionally busting BoardColumnView.memo. Hoisting it into useCallback fixes that.
4. multiRepo as a boolean primitive — computed before the useCallback dep array so the dep is stable when unchanged, not a live expression.
5. openAgentPicker: setAgentPickerCard (use-board-keymap.ts) — replaces the (card) => setAgentPickerCard(card) wrapper arrow (new reference every hook render) with the React state setter directly (guaranteed stable by spec). This is load-bearing: without it, renderCard's dep on openAgentPicker would bust all column memos on every Board render regardless of SSE content.

Correctness

Dep array is complete — [selectedKeys, focusedKey, dragKey, density, group, multiRepo, openAgentPicker] covers every closure capture in renderCard. Nothing missing, nothing spurious.

Card-level memo is genuinely effective — when renderCard is stable (SSE tick that doesn't touch selection/focus/drag state), BoardColumnViewInner calls renderCard(card) and each resulting <BoardCardView> element skips re-render because card object identity is preserved across pivotBoardResponse and all other props are primitives or stable references.

Column-level memo fires on pure SSE ticks — onDrop, onCardClick, onCardDragStart, onCardDragEnd are all useCallback-wrapped; onColumnMouseEnter = setFocusedColumnType (stable setter); renderCard stable as described. A patch affecting only sibling columns leaves all other BoardColumnView props unchanged → those columns bail out.

Selection changes still cause full re-renders — when selectedKeys changes, both the selectedKeys prop and renderCard (which captures it) update, so all columns re-render. Unavoidable given the architecture; the optimisation correctly targets the hot SSE-only path.

Minor nit (non-blocking)

use-board-keymap.ts comment reads "(PR #1155 review)" — slightly confusing since this is PR #1155. Likely meant (#1151) (the originating issue). No action needed.

---

Clean, well-reasoned optimisation. The follow-up commit (d07a3c6) closing the openAgentPicker stability gap shows the dep chain was traced all the way through. Merging.

## APPROVED ✅ Two focused commits, 4 files, 60 insertions / 17 deletions. CI green. ### What this does 1. **`BoardCardView`** — `BoardCardViewInner` wrapped via `React.memo()`, exported as `BoardCardView` with `displayName` set. 2. **`BoardColumnView`** — same pattern for `BoardColumnViewInner → BoardColumnView`. 3. **`renderCard` useCallback** (`board.tsx`) — the inline arrow previously in the `.map()` created a new function reference on every `Board` render, unconditionally busting `BoardColumnView.memo`. Hoisting it into `useCallback` fixes that. 4. **`multiRepo` as a boolean primitive** — computed before the `useCallback` dep array so the dep is stable when unchanged, not a live expression. 5. **`openAgentPicker: setAgentPickerCard`** (`use-board-keymap.ts`) — replaces the `(card) => setAgentPickerCard(card)` wrapper arrow (new reference every hook render) with the React state setter directly (guaranteed stable by spec). This is load-bearing: without it, `renderCard`'s dep on `openAgentPicker` would bust all column memos on every `Board` render regardless of SSE content. ### Correctness **Dep array is complete** — `[selectedKeys, focusedKey, dragKey, density, group, multiRepo, openAgentPicker]` covers every closure capture in `renderCard`. Nothing missing, nothing spurious. **Card-level memo is genuinely effective** — when `renderCard` is stable (SSE tick that doesn't touch selection/focus/drag state), `BoardColumnViewInner` calls `renderCard(card)` and each resulting `<BoardCardView>` element skips re-render because `card` object identity is preserved across `pivotBoardResponse` and all other props are primitives or stable references. **Column-level memo fires on pure SSE ticks** — `onDrop`, `onCardClick`, `onCardDragStart`, `onCardDragEnd` are all `useCallback`-wrapped; `onColumnMouseEnter` = `setFocusedColumnType` (stable setter); `renderCard` stable as described. A patch affecting only sibling columns leaves all other `BoardColumnView` props unchanged → those columns bail out. **Selection changes still cause full re-renders** — when `selectedKeys` changes, both the `selectedKeys` prop and `renderCard` (which captures it) update, so all columns re-render. Unavoidable given the architecture; the optimisation correctly targets the hot SSE-only path. ### Minor nit (non-blocking) `use-board-keymap.ts` comment reads _"(PR #1155 review)"_ — slightly confusing since this is PR #1155. Likely meant `(#1151)` (the originating issue). No action needed. --- Clean, well-reasoned optimisation. The follow-up commit (`d07a3c6`) closing the `openAgentPicker` stability gap shows the dep chain was traced all the way through. Merging.

reviewer merged commit 87fc82bf37 into main 2026-05-14 17:06:17 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/agent-hooks!1155

No description provided.