feat(map): merge same-groupId map operations onto one stack card (#1227)#1504
feat(map): merge same-groupId map operations onto one stack card (#1227)#1504isamu wants to merge 7 commits into
Conversation
…1227) Consumes @gui-chat-plugin/google-map 0.5.0's new `groupId` + View `results` replay prop. StackView now groups `mapControl` results by `groupId` (session-wide, positioned at the group's first occurrence) into ONE card and passes the ordered group to a single map View via `:results`, so markers / routes accumulate on one map and later same-groupId updates land on the same card instead of spawning a new one per call. - bump `@gui-chat-plugin/google-map` ^0.4.0 → ^0.5.0 - StackView: `displayItems` computed collapses same-groupId map results; group cards register their element under every member uuid so the scroll-spy / scroll-to-selection (keyed on individual uuids) keep working; `N×` badge shows the op count - non-grouped results and all other plugins render exactly as before (singletons, `:selected-result` only) Scope: stack layout only. The single-layout (`selectedResult`) path in App.vue still shows one result at a time — tracked as a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBumps google-map to v0.5.0, adds StackDisplayItem and helpers to group ordered mapControl results, integrates grouping into StackView (shared refs/head/members), and adds unit and E2E tests plus a Playwright pub/sub fixture for streaming behavior. ChangesMap Result Grouping and Plugin Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // scroll-spy and scroll-to-selection logic (which key on individual | ||
| // result uuids) resolve any member to this one card. | ||
| function setItemRefForMembers(members: ToolResultComplete[], element: HTMLElement | null): void { | ||
| for (const member of members) setItemRef(member.uuid, element); |
There was a problem hiding this comment.
Mapping every grouped member UUID to the same DOM element is necessary, but it breaks the assumption in computeActiveUuidFromScroll() that props.toolResults order matches DOM order one-to-one. With non-contiguous groups (A(map:g1), B(text), C(map:g1)), itemRefs.get(C.uuid) points to A's card, so the scroll-spy loop can overwrite the active item with C while the viewport is actually on B. That causes incorrect sidebar selection/jumps after grouping. Please update active-item computation to iterate rendered cards (displayItems) or otherwise dedupe by element before applying the top-edge scan.
|
CODEX VERDICT: CHANGES REQUESTED
|
|
CODEX VERDICT: CHANGES REQUESTED
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/StackView.vue`:
- Around line 231-249: The scroll logic is using props.toolResults order while
the rendered cards come from computed displayItems; update both
computeActiveUuidFromScroll() and the "scroll to latest" watcher to iterate over
displayItems and use a single canonical UUID per rendered card (e.g. use
item.head.uuid) instead of raw result.uuid, so scroll-spy and latest-result
scrolling follow the DOM order created by displayItems (ensure any map group
keys like `mapgroup:${groupId}` remain referenced only for identity, not for
scroll position).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2181f3d2-2c68-487e-b769-e8e092ad0857
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.jsonsrc/components/StackView.vue
…#1504) Codex CHANGES REQUESTED: with session-wide map grouping a group card shares one DOM element across multiple member uuids. The scroll-spy's `computeActiveUuidFromScroll` iterated the flat `toolResults`, so for non-contiguous reuse (`A(g1), B(text), C(g1)`) the later member `C` resolved back to the group's earlier element and could wrongly win the active slot while viewing `B` — corrupting sidebar selection. - Extract the grouping into a pure, testable helper `src/utils/canvas/stackGrouping.ts` (`buildStackDisplayItems`). - Scroll-spy now iterates `displayItems` (rendered card order) and emits each card's head uuid, so active-item tracking matches what's actually on screen. - Regression test `test/utils/canvas/test_stackGrouping.ts` covers the non-contiguous `A(g1), B, C(g1)` case + contiguous, distinct-group, ungrouped, and lone-group cases (5 tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CODEX VERDICT: CHANGES REQUESTED
|
…e (Codex #1504) Second scroll-order fix from the Codex review. `latestResultScrollKey` auto-scrolled the stack to the bottom on every new tool result. With session-wide map grouping a new `mapControl` result can merge into an EARLIER group card, so bottom-scrolling jumped away from where the update rendered. Now: bottom-scroll only when the newest result is in the last card; otherwise bring its (earlier) card into view via scrollIntoView({ block: "nearest" }). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed both scroll-order findings from the Codex / CodeRabbit review (same root cause: scroll logic assumed
|
|
CODEX VERDICT: CHANGES REQUESTED
|
…r level (Codex #1504) Codex's remaining ask: the scroll fixes were correct but only buildStackDisplayItems had a unit test — the DOM/scroll decisions that actually regressed (active-item selection, latest-result auto-scroll) were untested. Extract their pure cores out of StackView so they can be exercised without a browser: - pickActiveCardUuid — scroll-spy walk over rendered card order; returns the canonical (head) uuid of the last card at/above the padded line. - resolveLatestScrollTarget — bottom vs. earlier-card decision for a new result under session-wide grouping. StackView now delegates to both. Regression tests drive the exact non-contiguous sequence A(g1), B, C(g1) through buildStackDisplayItems into each helper: viewing B must select B (not member C), and a new C merging into the top group card must target that card, not the bottom. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| beginSuppressScrollSync(); | ||
| containerRef.value.scrollTop = containerRef.value.scrollHeight; | ||
| const newest = props.toolResults[props.toolResults.length - 1]; | ||
| const target = resolveLatestScrollTarget(displayItems.value, newest, (result) => result.uuid); |
There was a problem hiding this comment.
The scroll-order fixes look correct, but this path is still only regression-tested at helper level (stackGrouping.ts). We still do not have a StackView-level test that exercises the actual watcher/DOM wiring here (nextTick + scrollIntoView + beginSuppressScrollSync) for a non-contiguous grouped sequence like A(g1), B, C(g1). Please add one component test that proves (1) active selection resolves to B when viewport is on the B card, and (2) a latest C(g1) update targets the grouped card rather than unconditional bottom scroll.
|
CODEX VERDICT: CHANGES REQUESTED
|
|
Addressed the remaining Codex ask (regression test coverage for the scroll path itself, f60c59e). The earlier commits fixed the logic; Codex correctly noted only
|
#1504) Codex asked for a component-level regression covering the real Vue watcher / DOM wiring (nextTick + scrollIntoView + scroll-suppression), not just the pure helpers. The repo has no Vue component-unit harness — e2e/ is the sanctioned DOM surface — so add a Playwright spec: - preload a session whose transcript already has A(map,g1) + B(map,g2), then stream a live C(map,g1) AFTER the load settles (a startDelayMs avoids racing the transcript fetch, which otherwise overwrites live events); - assert C merges into the earlier g1 card (two map cards, never three) and that the latest-result watcher scrolls to that g1 card rather than slamming scrollTop to the bottom. A tall transcript turn between the two map cards separates them by more than a viewport so "scrolled to g1" and "bottom-pinned" are unambiguously distinct positions. Extract the pub/sub socket harness shared with streaming-autoscroll into e2e/fixtures/pubsub.ts (DRY) and add an optional startDelayMs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CODEX VERDICT: CHANGES REQUESTED
|
|
Added the component-level regression Codex asked for (real Vue watcher / DOM wiring, not just the pure helpers) — This repo has no Vue component-unit harness;
A tall transcript turn between the two map cards separates them by more than a viewport, so "scrolled to g1" (g1 in viewport, distance-from-bottom ≈ 1800px) and "bottom-pinned" are unambiguously distinct — the regression would surface g2 and push g1 off the top. The pub/sub socket harness shared with Validation: full e2e 438 passed, Coverage now spans all three layers: pure decision logic (unit), thin StackView delegation, and the end-to-end DOM/scroll wiring. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
e2e/tests/stack-map-grouping-scroll.spec.ts (2)
116-117: ⚡ Quick winPrefer
data-testidselectors over text/position selectors in E2E assertions.
getByText(...).first()/count checks are brittle to copy or localization changes. Please target stable functional test-ids instead.As per coding guidelines,
e2e/**/*.{ts,js}: "usedata-testidfor element selection named by function, not position".Also applies to: 123-123, 129-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/stack-map-grouping-scroll.spec.ts` around lines 116 - 117, Replace brittle text/position selectors using page.getByText(MAP_PLACEHOLDER).first() and page.getByText(MAP_PLACEHOLDER).toHaveCount(2) with stable data-testid selectors; change the assertions to use page.getByTestId('map-placeholder').first() and page.getByTestId('map-placeholder')/toHaveCount(2) (or the existing test id string used in the DOM) and update the test fixture/markup that renders MAP_PLACEHOLDER to include the corresponding data-testid attribute if it doesn't already exist so the selectors are stable across copy/localization changes.
57-59: ⚡ Quick winReplace event/tool string literals with shared constants.
Use the existing
as constevent/tool name objects for these transcript/live event payloads to keep tests aligned with app contracts.As per coding guidelines,
**/*.{ts,tsx,vue}: "Use existingas constobjects for scheduler types, event types, API routes, and tool names instead of string literals".Also applies to: 67-68, 70-70, 77-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/stack-map-grouping-scroll.spec.ts` around lines 57 - 59, Replace the string literals in the test payloads with the shared as-const constants: swap "tool_result" and "tool" for the project's event/type constants (e.g., EVENT_TYPES.TOOL_RESULT and EVENT_SOURCES.TOOL) and replace the inline "mapControl"/toolName and the `Map operation ${action}` message template with the shared tool name and message constants (e.g., TOOL_NAMES.MAP_CONTROL and the corresponding TOOL_MESSAGES.MAP_OPERATION template) used by the app; update all occurrences around the mapData(action, groupId) uses (and the other spots noted) so the test uses the same exported as‑const objects for event types, sources, tool names, and messages instead of string literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/fixtures/pubsub.ts`:
- Around line 60-84: The function mockAgentWithPubSub is doing two jobs
(WebSocket mock + HTTP /api/agent mock) and exceeds the max length; refactor by
extracting the WebSocket part into a helper (e.g., mockPubSubWebSocket(page:
Page, events: readonly unknown[], opts: StreamOptions)) that calls
page.routeWebSocket, sends the initial "0" frame, and wires handleSocketFrame,
and extract the HTTP route into a helper (e.g., mockAgentHttpRoute(page: Page))
that calls page.route for urlEndsWith("/api/agent") and fulfills the 202 json;
then replace the body of mockAgentWithPubSub with two short calls to those
helpers to keep mockAgentWithPubSub small and preserve existing behavior and
signatures.
- Around line 88-105: The waitForScrollHeightStable function can return silently
on timeout which hides failures; update waitForScrollHeightStable to throw a
descriptive error if the deadline is reached without stability, including
relevant debug details (e.g., last observed scrollHeight, last sample time,
testId and maxWaitMs) so callers fail fast and tests are diagnostic; keep the
existing loop and stability logic (references: function
waitForScrollHeightStable, variables last, stable, deadline) and add a final
throw after the while loop that includes those values.
In `@e2e/tests/stack-map-grouping-scroll.spec.ts`:
- Around line 111-136: The test body in the "a newly streamed same-groupId..."
callback is too long; split it into small helper functions and call them from
the test to reduce function size. Create helpers such as setupStackSession(page)
that wraps mockAgentWithPubSub and openSessionInStackMode,
assertPreloadedMapsVisible(page) that checks MAP_PLACEHOLDER count and
visibility, waitForStreamMerge(page) that waits STREAM_AFTER_LOAD_MS and calls
waitForScrollHeightStable("stack-scroll") and re-checks map count, and
assertScrollNotBottom(page) that uses scrollMetrics("stack-scroll") to assert
distanceFromBottomPx > BOTTOM_TOLERANCE_PX and that the first MAP_PLACEHOLDER is
in viewport; replace the inline blocks in the test with these helper calls so
the top-level test callback is under the function-size guideline.
---
Nitpick comments:
In `@e2e/tests/stack-map-grouping-scroll.spec.ts`:
- Around line 116-117: Replace brittle text/position selectors using
page.getByText(MAP_PLACEHOLDER).first() and
page.getByText(MAP_PLACEHOLDER).toHaveCount(2) with stable data-testid
selectors; change the assertions to use
page.getByTestId('map-placeholder').first() and
page.getByTestId('map-placeholder')/toHaveCount(2) (or the existing test id
string used in the DOM) and update the test fixture/markup that renders
MAP_PLACEHOLDER to include the corresponding data-testid attribute if it doesn't
already exist so the selectors are stable across copy/localization changes.
- Around line 57-59: Replace the string literals in the test payloads with the
shared as-const constants: swap "tool_result" and "tool" for the project's
event/type constants (e.g., EVENT_TYPES.TOOL_RESULT and EVENT_SOURCES.TOOL) and
replace the inline "mapControl"/toolName and the `Map operation ${action}`
message template with the shared tool name and message constants (e.g.,
TOOL_NAMES.MAP_CONTROL and the corresponding TOOL_MESSAGES.MAP_OPERATION
template) used by the app; update all occurrences around the mapData(action,
groupId) uses (and the other spots noted) so the test uses the same exported
as‑const objects for event types, sources, tool names, and messages instead of
string literals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87a89bb6-aaaf-4d13-81b4-eb6dfaa64f1b
📒 Files selected for processing (3)
e2e/fixtures/pubsub.tse2e/tests/stack-map-grouping-scroll.spec.tse2e/tests/streaming-autoscroll.spec.ts
…only at mount (Codex #1504) Writing the DOM-level scroll-spy regression Codex asked for surfaced a latent bug: the scroll container is in StackView's `v-else` branch (rendered only once `toolResults` is non-empty), but the scroll-spy listener was bound in `onMounted` against `containerRef`. Sessions mount empty — the transcript / stream populates them after mount — so the listener attached to a null ref and never fired; scroll-spy active-card selection silently did nothing for any session that started empty (i.e. essentially all of them). Pre-existing; not introduced by the grouping work, but it blocked the behaviour under test. Bind via `watch(containerRef)` instead so the listener (re)attaches whenever the container appears or is replaced. Also adds the scroll-spy regression test Codex requested and addresses CodeRabbit's review of the e2e additions: - new spec case: in the non-contiguous sequence A(g1), B, C(g1), scrolling to B selects B — not the merged group whose later member C sits above B in the flat list (the exact computeActiveUuidFromScroll regression); - split `mockAgentWithPubSub` into `mockPubSubSocket` + `mockAgentEndpoint`; - `waitForScrollHeightStable` now throws on timeout instead of returning silently; - extract shared spec helpers (serveTranscript / openStackSession / stackCard / expectNotBottomPinned) to keep each test body small. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CODEX VERDICT: LGTM Reviewed current head (
I did not find additional correctness/security/accessibility/i18n lockstep concerns beyond what the thread already covered. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
Added the scroll-spy DOM regression Codex asked for — and it surfaced a latent bug, which is exactly what that test was meant to catch ( Bug found & fixedStackView's scroll container is in the I confirmed this empirically: Codex's scroll-spy askNew spec case: in the non-contiguous sequence CodeRabbit's review of the e2e additions
Validation: full e2e 439 passed, Coverage now spans pure decision logic (unit) + both DOM/scroll paths (auto-scroll and scroll-spy) end-to-end. |
1 participant
Summary
When the agent makes several
mapControlcalls for one task (search places, drop markers, draw a route), they previously each rendered as a separate map card. This consumes@gui-chat-plugin/google-map0.5.0's newgroupIdto merge them: results sharing agroupIdcollapse into ONE stack card and accumulate on a single map.Depends on the just-published plugin release:
@gui-chat-plugin/google-map@0.5.0(PR receptron/GUIChatPluginGoogleMap#19).Items to Confirm / Review
groupIdupdate lands on the existing card (not a new one) — this matches the "更新まで考えると" requirement. The alternative (contiguous-only) was rejected because it wouldn't catch later updates.setItemRefForMembers), socomputeActiveUuidFromScrolland the scroll-to-selection watch (both keyed on individual uuids) resolve any member to the one card without changes to that machinery. Please sanity-check that interaction.selectedResult) path inApp.vuestill shows one result at a time. Tracked as a follow-up (see below).Changes
package.json:@gui-chat-plugin/google-map^0.4.0→^0.5.0(+yarn.lock).src/components/StackView.vue:displayItemscomputed — walkstoolResults, collapsesmapControlresults with the same non-emptygroupIdinto one item (members[], ordered;head= latest, drives the card header). Everything else stays a singleton.:results="item.members"(the View replays the group onto one map); all other cards keep:selected-resultonly.setItemRefForMembersregisters the group element under every member uuid; selection highlight fires if any member is selected;N×badge shows op count.Follow-up (separate issue)
Single-layout (
App.vue) map grouping + any UX polish on group card headers. Will file per the earlier recommendation.Test plan
yarn typecheck/yarn lint/yarn buildclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores