Skip to content

Commit 7177fb7

Browse files
alicodingclaude
andcommitted
fix: group-into-new-area anchors the new frame at its members, not the trigger click
The reported "jumps to the bottom of the board" bug was NOT the hypothesized nil-Position packer path -- select-then-group always persists a real Position. The actual bug: the new container's Position came from the triggering UI element's screen point (a member's right-click, or worst case the selection tray's own bottom-center Group button / bare-G, both anchored at the floating tray's rect) via screenToFlowPosition, never the grouped members' own rendered spot. useAtlasCreation's groupRequest effect now anchors the new container at computeEnclosedBoundingBoxOrigin's bounding-box top-left over the enclosed members' current freeMoves-aware boxes (AtlasBoard.tsx's topLevelBoxes/noteBoxes, hoisted above useAtlasCreation so they're available when the request is handled), falling back to the click point only if no member box resolves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FW5GkkAG8du7tNdYLk2zSd
1 parent b00bfe0 commit 7177fb7

5 files changed

Lines changed: 181 additions & 47 deletions

File tree

frontend/e2e/atlas-select-group.spec.ts

Lines changed: 71 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -258,17 +258,71 @@ test('atlas shift-click select: toggle membership, group via member right-click,
258258
await expect(page.getByTestId('atlas-creation-tray')).toBeVisible()
259259

260260
// Re-select, then bare G opens the SAME group popover a member
261-
// right-click's own menu item does -- closed here without
262-
// submitting so the flow below (member right-click -> Group) is
263-
// the one that actually creates the area.
261+
// right-click's own menu item does -- anchored at the selection
262+
// tray's own on-screen rect (bottom-center; the same anchor the
263+
// tray's Group button uses), nowhere near where the members
264+
// actually render. Completed here (not just opened) so the new
265+
// area's Position can be checked against this anchor specifically.
264266
await cardA.click({ modifiers: ['Shift'] })
265267
await cardB.click({ modifiers: ['Shift'] })
266268
await expect(selected).toHaveCount(2)
269+
const boxAPreBareG = await cardA.boundingBox()
270+
const boxBPreBareG = await cardB.boundingBox()
271+
if (!boxAPreBareG || !boxBPreBareG) throw new Error('missing bounding box before group')
272+
const preBareGTop = Math.min(boxAPreBareG.y, boxBPreBareG.y)
273+
const preBareGLeft = Math.min(boxAPreBareG.x, boxBPreBareG.x)
267274
await page.keyboard.press('g')
268275
await expect(popover).toBeVisible()
269276
await expect(page.getByTestId('atlas-placement-context')).toContainText('2 cards')
270-
await popover.getByTestId('atlas-placement-cancel').click()
277+
await selectKind(popover, ATLAS_KIND_TOPIC)
278+
await popover.getByTestId('atlas-placement-title').fill('ZzK2eBareGArea')
279+
await popover.getByTestId('atlas-placement-submit').click()
271280
await expect(popover).not.toBeVisible()
281+
const bareGArea = groupCard(page, 'ZzK2eBareGArea')
282+
await expect(bareGArea).toBeVisible()
283+
284+
// Regression: the new area's own Position previously came from the
285+
// triggering anchor point (the selection tray's own rect for bare
286+
// G / its Group button), not the grouped members' own rendered
287+
// spot -- the tray floats bottom-center, so this door landed the
288+
// new area far below the members regardless of where they visibly
289+
// were. The camera never moves for a group action, so screen-space
290+
// bounding boxes before and after are directly comparable.
291+
const bareGAreaBox = await bareGArea.boundingBox()
292+
if (!bareGAreaBox) throw new Error('missing bounding box for grouped area')
293+
const boardBoxAfterBareG = await board.boundingBox()
294+
if (!boardBoxAfterBareG) throw new Error('missing bounding box for board')
295+
expect(Math.abs(bareGAreaBox.y - preBareGTop)).toBeLessThan(250)
296+
expect(Math.abs(bareGAreaBox.x - preBareGLeft)).toBeLessThan(250)
297+
expect(bareGAreaBox.y).toBeLessThan(boardBoxAfterBareG.y + boardBoxAfterBareG.height - 250)
298+
299+
// Dissolve back to loose cards, deselect (group+dissolve preserves
300+
// the multi-selection), then re-select for the context-menu door
301+
// exercised next.
302+
await bareGArea.getByTestId('atlas-group-header').click({ button: 'right' })
303+
await expect(menu).toBeVisible()
304+
await menu.getByText('Dissolve area', { exact: true }).click()
305+
await expect(page.getByRole('button', { name: 'Dissolve' })).toBeVisible()
306+
await page.getByRole('button', { name: 'Dissolve' }).click()
307+
await expect(bareGArea).toHaveCount(0)
308+
// Low-right pane coords, matching this file's own later deselect --
309+
// absolute page coords near the left edge can land on a tray/menu
310+
// instead of React Flow's own pane handler.
311+
await page.locator('.react-flow__pane').click({ position: { x: 500, y: 450 } })
312+
await expect(selected).toHaveCount(0)
313+
await cardA.click({ modifiers: ['Shift'] })
314+
await cardB.click({ modifiers: ['Shift'] })
315+
await expect(selected).toHaveCount(2)
316+
317+
// Regression fixture: the new area's own Position must land at the
318+
// members' CURRENT rendered spot, not the triggering right-click
319+
// point -- captured before the group gesture so it survives the
320+
// popover interaction.
321+
const boxAPreGroup = await cardA.boundingBox()
322+
const boxBPreGroup = await cardB.boundingBox()
323+
if (!boxAPreGroup || !boxBPreGroup) throw new Error('missing bounding box before group')
324+
const preGroupTop = Math.min(boxAPreGroup.y, boxBPreGroup.y)
325+
const preGroupLeft = Math.min(boxAPreGroup.x, boxBPreGroup.x)
272326

273327
// Member right-click reaches the multi menu -> Group into new area
274328
// (same full-gesture retry as above: Primer's menu overlay animates
@@ -289,6 +343,19 @@ test('atlas shift-click select: toggle membership, group via member right-click,
289343
await expect(groupedArea).toBeVisible()
290344
await expect(groupedArea.getByTestId('atlas-group-header')).toContainText('2 cards')
291345

346+
// Regression: the new area's Position previously came from the
347+
// triggering right-click/button point, not the grouped members' own
348+
// spot -- landing far from where the members visibly were. The
349+
// camera never moves for a group action, so screen-space bounding
350+
// boxes before and after are directly comparable.
351+
const groupedAreaBox = await groupedArea.boundingBox()
352+
if (!groupedAreaBox) throw new Error('missing bounding box for grouped area')
353+
const boardBoxAfterGroup = await board.boundingBox()
354+
if (!boardBoxAfterGroup) throw new Error('missing bounding box for board')
355+
expect(Math.abs(groupedAreaBox.y - preGroupTop)).toBeLessThan(250)
356+
expect(Math.abs(groupedAreaBox.x - preGroupLeft)).toBeLessThan(250)
357+
expect(groupedAreaBox.y).toBeLessThan(boardBoxAfterGroup.y + boardBoxAfterGroup.height - 250)
358+
292359
// Dissolve back to loose cards, then Delete over a re-made
293360
// shift-click selection: the confirm names the count, and
294361
// confirming deletes both -- which is also this test's cleanup

frontend/src/atlas/AtlasBoard.tsx

Lines changed: 41 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,47 @@ function AtlasBoardInner({ cards, allCards, kinds, links, linkKinds, notes, pare
145145
const [hintedID, setHintedID] = useState<string | null>(null)
146146
const wrapperRef = useRef<HTMLDivElement>(null)
147147
const { fitBounds, fitView, getNodesBounds, screenToFlowPosition } = useReactFlow()
148-
const creation = useAtlasCreation({ parentID, allCards, notes, readOnly, screenToFlowPosition, placementRequest, promoteRequest, groupRequest })
148+
149+
// Free-mode overlap resolution (goal 0073, the growth class): a
150+
// frame's size is DERIVED from its children, so a clear layout can
151+
// start overlapping with nobody having moved a card. Resolved with
152+
// a deterministic minimal-displacement separation
153+
// (atlasOverlapResolution) and PERSISTED below, so the nudge is
154+
// stable across reloads. Leaf-on-leaf overlaps are hand placement
155+
// and never touched.
156+
// Auto-arrange rows wrap at the board's real width (a fixed cap
157+
// left a dead right-hand column); the pure-layout constant stays
158+
// the floor so a narrow pane still wraps.
159+
const [boardWidth, setBoardWidth] = useState(0)
160+
useEffect(() => {
161+
const el = wrapperRef.current
162+
if (!el) return
163+
const observer = new ResizeObserver((entries) => {
164+
const w = entries[0]?.contentRect.width
165+
if (w) setBoardWidth(w)
166+
})
167+
observer.observe(el)
168+
return () => observer.disconnect()
169+
}, [])
170+
171+
// The board's arteries, resolved once and shared: the edges memo
172+
// below renders them; Auto-arrange consumes them as the adjacency
173+
// that seats linked things beside each other.
174+
const arteries = useMemo(() => resolveBoardEdges(links, new Set(cards.map((c) => c.ID)), allCards), [links, cards, allCards])
175+
176+
const { freeMoves } = useAtlasArrange({ cards, allCards, arteries, boardWidth, arrangeRequest })
177+
178+
// Rendered flow-space boxes (atlasBoardBoxes.ts, split at the
179+
// 500-line seam) -- Free mode only. Computed BEFORE useAtlasCreation
180+
// (below) so select-then-group can anchor the new container at its
181+
// members' own current box, not the triggering click point.
182+
const topLevelBoxes: FrameBox[] = useMemo(
183+
() => (isFree ? computeTopLevelBoxes(cards, allCards, freeMoves) : []),
184+
[cards, allCards, freeMoves, isFree],
185+
)
186+
const noteBoxes = useMemo(() => (isFree ? computeNoteBoxes(notes) : []), [notes, isFree])
187+
188+
const creation = useAtlasCreation({ parentID, allCards, notes, readOnly, screenToFlowPosition, placementRequest, promoteRequest, groupRequest, cardBoxes: topLevelBoxes, noteBoxes })
149189
const selection = useAtlasSelection({ cards, notes, onMultiSelectContextMenu })
150190

151191
// Delete/Backspace over a live selection -> the shared confirm
@@ -217,43 +257,6 @@ function AtlasBoardInner({ cards, allCards, kinds, links, linkKinds, notes, pare
217257
if (focusRequest) setFlippedID(null)
218258
}, [focusRequest])
219259

220-
// Free-mode overlap resolution (goal 0073, the growth class): a
221-
// frame's size is DERIVED from its children, so a clear layout can
222-
// start overlapping with nobody having moved a card. Resolved with
223-
// a deterministic minimal-displacement separation
224-
// (atlasOverlapResolution) and PERSISTED below, so the nudge is
225-
// stable across reloads. Leaf-on-leaf overlaps are hand placement
226-
// and never touched.
227-
// Auto-arrange rows wrap at the board's real width (a fixed cap
228-
// left a dead right-hand column); the pure-layout constant stays
229-
// the floor so a narrow pane still wraps.
230-
const [boardWidth, setBoardWidth] = useState(0)
231-
useEffect(() => {
232-
const el = wrapperRef.current
233-
if (!el) return
234-
const observer = new ResizeObserver((entries) => {
235-
const w = entries[0]?.contentRect.width
236-
if (w) setBoardWidth(w)
237-
})
238-
observer.observe(el)
239-
return () => observer.disconnect()
240-
}, [])
241-
242-
// The board's arteries, resolved once and shared: the edges memo
243-
// below renders them; Auto-arrange consumes them as the adjacency
244-
// that seats linked things beside each other.
245-
const arteries = useMemo(() => resolveBoardEdges(links, new Set(cards.map((c) => c.ID)), allCards), [links, cards, allCards])
246-
247-
const { freeMoves } = useAtlasArrange({ cards, allCards, arteries, boardWidth, arrangeRequest })
248-
249-
// Rendered flow-space boxes (atlasBoardBoxes.ts, split at the
250-
// 500-line seam) -- Free mode only.
251-
const topLevelBoxes: FrameBox[] = useMemo(
252-
() => (isFree ? computeTopLevelBoxes(cards, allCards, freeMoves) : []),
253-
[cards, allCards, freeMoves, isFree],
254-
)
255-
const noteBoxes = useMemo(() => (isFree ? computeNoteBoxes(notes) : []), [notes, isFree])
256-
257260
const dragFiling = useAtlasDragFiling({ allCards, parentID, topLevelBoxes, wrapperRef })
258261

259262
// Slot-drag = instant typed link (goal 0081 A4): see useAtlasSlotDrag.ts.
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import { describe, expect, it } from 'vitest'
2+
import { computeEnclosedBoundingBoxOrigin } from './atlasBoardBoxes'
3+
4+
describe('computeEnclosedBoundingBoxOrigin', () => {
5+
// Regression: select-then-group anchored the new container at the
6+
// triggering click point (a member right-click, or the selection
7+
// tray's own bottom-center Group button) instead of where the
8+
// grouped members actually render -- the tray path always landed
9+
// the new area at the bottom of the board regardless of the
10+
// members' real position.
11+
it('returns the top-left of the union of the enclosed members boxes', () => {
12+
const cardBoxes = [
13+
{ id: 'a', x: 200, y: 400 },
14+
{ id: 'b', x: 350, y: 320 },
15+
{ id: 'unrelated', x: 0, y: 0 },
16+
]
17+
expect(computeEnclosedBoundingBoxOrigin(['a', 'b'], [], cardBoxes, [])).toEqual({ x: 200, y: 320 })
18+
})
19+
20+
it('considers notes alongside cards', () => {
21+
const cardBoxes = [{ id: 'a', x: 200, y: 400 }]
22+
const noteBoxes = [{ id: 'n1', x: 100, y: 500 }]
23+
expect(computeEnclosedBoundingBoxOrigin(['a'], ['n1'], cardBoxes, noteBoxes)).toEqual({ x: 100, y: 400 })
24+
})
25+
26+
it('returns null when none of the given ids resolve to a box', () => {
27+
expect(computeEnclosedBoundingBoxOrigin(['missing'], [], [{ id: 'a', x: 0, y: 0 }], [])).toBeNull()
28+
})
29+
})

frontend/src/atlas/atlasBoardBoxes.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,25 @@ export function computeTopLevelBoxes(
3232
export function computeNoteBoxes(notes: Note[]): { id: string; x: number; y: number; width: number; height: number }[] {
3333
return notes.map((n) => ({ id: n.ID, x: n.Position.X, y: n.Position.Y, width: STICKY_WIDTH, height: STICKY_HEIGHT }))
3434
}
35+
36+
// Select-then-group's own anchor (the multi-select context menu item
37+
// and the selection tray's Group button/bare-G): the new container's
38+
// Position is the enclosed members' own CURRENT rendered top-left, not
39+
// the triggering click point -- a click on a member (or the tray's
40+
// bottom-center button) is nowhere near the members themselves. Null
41+
// when none of the given ids resolve to a box (nothing to anchor to;
42+
// the caller falls back to the click-derived flow position).
43+
export function computeEnclosedBoundingBoxOrigin(
44+
cardIDs: string[],
45+
noteIDs: string[],
46+
cardBoxes: { id: string; x: number; y: number }[],
47+
noteBoxes: { id: string; x: number; y: number }[],
48+
): { x: number; y: number } | null {
49+
const ids = new Set([...cardIDs, ...noteIDs])
50+
const boxes = [...cardBoxes, ...noteBoxes].filter((b) => ids.has(b.id))
51+
if (boxes.length === 0) return null
52+
return {
53+
x: Math.min(...boxes.map((b) => b.x)),
54+
y: Math.min(...boxes.map((b) => b.y)),
55+
}
56+
}

frontend/src/atlas/useAtlasCreation.ts

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { useUISignalStore } from '../shared/uiSignalStore'
66
import { refreshAtlas } from './atlasStore'
77
import { titleFromNoteText } from './atlasCreateHelpers'
88
import { freeChildPosition } from './atlasContainmentPlacement'
9+
import { computeEnclosedBoundingBoxOrigin } from './atlasBoardBoxes'
910
import type { AtlasCreationTool } from './AtlasCreationTray'
1011

1112
export interface AtlasPlacementPopoverState {
@@ -70,7 +71,7 @@ export interface AtlasGroupRequest { cardIDs: string[]; noteIDs: string[]; pos:
7071
// parentID is the board's OWN current container (AtlasView's viewedID,
7172
// threaded down unchanged) -- the LOCKED design's "parent = where you
7273
// are" rule for every canvas-foremost creation door.
73-
export function useAtlasCreation({ parentID, allCards, notes, readOnly, screenToFlowPosition, placementRequest, promoteRequest, groupRequest }: {
74+
export function useAtlasCreation({ parentID, allCards, notes, readOnly, screenToFlowPosition, placementRequest, promoteRequest, groupRequest, cardBoxes, noteBoxes }: {
7475
parentID: string
7576
allCards: Card[]
7677
notes: Note[]
@@ -79,6 +80,12 @@ export function useAtlasCreation({ parentID, allCards, notes, readOnly, screenTo
7980
placementRequest?: AtlasPlacementRequest | null
8081
promoteRequest?: AtlasPromoteRequest | null
8182
groupRequest?: AtlasGroupRequest | null
83+
// Every top-level card/note's own rendered flow-space box (freeMoves-
84+
// aware -- AtlasBoard.tsx's topLevelBoxes/noteBoxes), used ONLY to
85+
// anchor select-then-group's new container at its members' own
86+
// current position instead of the triggering click point.
87+
cardBoxes?: { id: string; x: number; y: number }[]
88+
noteBoxes?: { id: string; x: number; y: number }[]
8289
}) {
8390
const [armedTool, setArmedTool] = useState<AtlasCreationTool | null>(null)
8491
const [popover, setPopover] = useState<AtlasPlacementPopoverState | null>(null)
@@ -326,14 +333,20 @@ export function useAtlasCreation({ parentID, allCards, notes, readOnly, screenTo
326333
// eslint-disable-next-line react-hooks/exhaustive-deps -- keyed on the request's own token
327334
}, [promoteRequest])
328335

329-
// Select-then-group (goal 0081 slice A2): reuses screenToFlowPosition
330-
// to place the new container at the right-click point, same as any
331-
// other canvas-foremost creation door.
336+
// Select-then-group (goal 0081 slice A2): the new container lands at
337+
// the grouped members' own current bounding-box top-left, not the
338+
// triggering click point -- a member right-click or the selection
339+
// tray's bottom-center Group button both click far from where the
340+
// members actually render (regression: the tray path always landed
341+
// the new area at the bottom of the board). Falls back to the click
342+
// point only when no member box resolves (shouldn't happen for a
343+
// real 2+ selection, but keeps this door from silently no-op'ing).
332344
const lastGroupToken = useRef(groupRequest?.token)
333345
useEffect(() => {
334346
if (!groupRequest || groupRequest.token === lastGroupToken.current) return
335347
lastGroupToken.current = groupRequest.token
336-
openAreaPopover(groupRequest.pos, screenToFlowPosition(groupRequest.pos), groupRequest.cardIDs, groupRequest.noteIDs)
348+
const anchor = computeEnclosedBoundingBoxOrigin(groupRequest.cardIDs, groupRequest.noteIDs, cardBoxes ?? [], noteBoxes ?? [])
349+
openAreaPopover(groupRequest.pos, anchor ?? screenToFlowPosition(groupRequest.pos), groupRequest.cardIDs, groupRequest.noteIDs)
337350
// eslint-disable-next-line react-hooks/exhaustive-deps -- keyed on the request's own token
338351
}, [groupRequest])
339352

0 commit comments

Comments
 (0)