feat(web): add configurable tool grouping mode - #1157
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- No issues found.
Summary
- Review mode: initial
- Full diff reviewed. No Blocker/Major/Minor/Nit findings above confidence threshold.
- Residual risk: local automation could not run because
bunis unavailable in this runner.
Testing
- Not run (automation):
bunnot found when attempting targeted web tests.
HAPI Bot
There was a problem hiding this comment.
Findings
- No issues found.
Questions
- None.
Summary
- Review mode: follow-up after new commits
- Full latest PR diff reviewed; previous HAPI Bot review at
99dab452had no findings. No Blocker/Major/Minor/Nit findings above confidence threshold. - Residual risk: GitHub checks were still pending at review time.
Testing
- Not run (automation). GitHub checks observed pending:
pr-review,test.
HAPI Bot
|
Hi @swear01 — I wanted to make you aware of this PR because it builds on the Codex exploration presentation introduced in #1139. Hapi historically groups tools across agent backends, while #1139 introduced a Codex-specific classified presentation. This PR preserves both behaviors: Grouped remains the default for consistency with the other agents, and Classified keeps the dedicated Exploring/Explored presentation from #1139 as an explicit user option. No functionality from #1139 is removed. Feedback is welcome, especially if there are Codex-specific cases that should remain classified regardless of the selected mode. |
There was a problem hiding this comment.
Findings
- No issues found.
Questions
- None.
Summary
- Review mode: follow-up after new commits
- Full latest PR diff reviewed; previous HAPI Bot review at
c1948757had no findings. No Blocker/Major/Minor/Nit findings above confidence threshold. - Residual risk: web behavior only; I did not execute PR code in this review run.
Testing
- Not run (automation).
HAPI Bot
There was a problem hiding this comment.
Findings
-
[Major] Reset previous-group identity when the grouping mode changes -
SessionChatrecomputes the blocks for the new mode while still passing groups produced by the old mode. A grouped Codex run such as exploration / general command / exploration is one group; switching to Classified splits it into two exploration groups. BecausecreateToolGroupIdmatches either edge of the old group, both new groups reuse the same ID, which also becomes a duplicate assistant-uitoolCallId. Evidence:web/src/components/SessionChat.tsx:1170; matching logic atweb/src/chat/toolGroups.ts:246.Suggested fix:
const previousToolGroupingModeRef = useRef(toolGroupingMode) // When building: previousGroups: previousToolGroupingModeRef.current === toolGroupingMode ? visibleGroupsRef.current : [], // When committing the new visible groups: useEffect(() => { previousToolGroupingModeRef.current = toolGroupingMode visibleGroupsRef.current = visibleBlocks.filter(isToolGroupBlock) }, [toolGroupingMode, visibleBlocks])
Questions
- None.
Summary
Review mode: follow-up after new commits
- Full latest PR diff reviewed. The new head merges
origin/main; the nine PR-specific files are unchanged from the prior bot-reviewed commit. - One Major mode-transition correctness issue found.
Testing
- Not run (automation).
- Add a regression covering Grouped -> Classified for exploration / general command / exploration, asserting all resulting group IDs and tool-call IDs are unique.
HAPI Bot
| hasMoreMessages: props.hasMoreMessages, | ||
| previousGroups: visibleGroupsRef.current | ||
| previousGroups: visibleGroupsRef.current, | ||
| groupingMode: toolGroupingMode |
There was a problem hiding this comment.
[MAJOR] Reset cached group IDs when the mode changes
visibleGroupsRef.current contains groups built under the previous mode. For a Codex sequence exploration / general command / exploration, Grouped creates one group; Classified splits it into two. The first split group matches the old group's firstToolId, and the second matches its lastToolId at toolGroups.ts:246, so both reuse the same group ID and emit duplicate assistant-ui toolCallId values.
Suggested fix:
const previousToolGroupingModeRef = useRef(toolGroupingMode)
previousGroups: previousToolGroupingModeRef.current === toolGroupingMode
? visibleGroupsRef.current
: [],
useEffect(() => {
previousToolGroupingModeRef.current = toolGroupingMode
visibleGroupsRef.current = visibleBlocks.filter(isToolGroupBlock)
}, [toolGroupingMode, visibleBlocks])Also add a Grouped -> Classified regression asserting the split groups have unique IDs.
There was a problem hiding this comment.
Fixed in a92cff49.
SessionChat now tracks the mode associated with its cached groups, and buildVisibleChatBlocks ignores those groups when the mode changes. Added a Grouped -> Classified regression for exploration / general command / exploration and verified that the two classified groups receive unique IDs.
Validated with bun typecheck, the targeted 25 tests, and bun run build:web.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
- Full latest PR diff reviewed. The prior duplicate group-ID finding is resolved by discarding cached groups across mode changes (
web/src/chat/toolGroups.ts:267,web/src/components/SessionChat.tsx:1170) with a regression atweb/src/chat/toolGroups.test.ts:262. - No Blocker/Major/Minor/Nit findings above the confidence threshold.
- Residual risk: GitHub checks were still pending at review time.
Testing
git diff --checkpassed.- Targeted web tests and web typecheck not run:
bunis unavailable in this runner. - GitHub checks observed pending:
pr-review,test.
HAPI Bot
a92cff4 to
b62788b
Compare
There was a problem hiding this comment.
Findings
-
[Minor] Keep same-window consumers synchronized when persistence fails — the custom change event is inside the localStorage try block, so a SecurityError or quota failure leaves the Settings hook on the selected mode while the already-mounted SessionChat hook remains on the old mode. Evidence: web/src/hooks/useToolGroupingMode.ts:61.
Suggested fix:
const onChange = (event: Event) => { const detail = (event as CustomEvent<ToolGroupingMode>).detail setToolGroupingModeState(parseToolGroupingMode(detail ?? null)) } try { // Persist mode. } catch { // Ignore persistence errors. } window.dispatchEvent(new CustomEvent<ToolGroupingMode>(CHANGE_EVENT, { detail: mode }))
Questions
- None.
Summary
Review mode: follow-up after new commits
- Full latest PR diff reviewed; prior HAPI Bot review and the a92cff4...b62788b compare were used only as follow-up context.
- One Minor finding: the immediate same-window update path depends on successful localStorage persistence.
- Residual risk: the GitHub test check was still in progress at review time.
Testing
- Not run (automation; PR code was not executed).
- git diff --check passed.
- Suggested regression: mock localStorage.setItem to throw and verify a second useToolGroupingMode consumer still receives the selected mode.
HAPI Bot
| } else { | ||
| localStorage.setItem(STORAGE_KEY, mode) | ||
| } | ||
| window.dispatchEvent(new Event(CHANGE_EVENT)) |
There was a problem hiding this comment.
[MINOR] Keep the custom notification independent of localStorage success
The event is dispatched inside the try block. If localStorage.setItem/removeItem throws (blocked storage or quota failure), this hook updates its own state at line 60, but the already-mounted SessionChat hook never receives the new mode. The Settings radio can therefore show Classified while the chat stays Grouped.
Suggested fix:
const onChange = (event: Event) => {
const detail = (event as CustomEvent<ToolGroupingMode>).detail
setToolGroupingModeState(parseToolGroupingMode(detail ?? null))
}
try {
if (mode === DEFAULT_TOOL_GROUPING_MODE) {
localStorage.removeItem(STORAGE_KEY)
} else {
localStorage.setItem(STORAGE_KEY, mode)
}
} catch {
// Ignore persistence errors.
}
window.dispatchEvent(new CustomEvent<ToolGroupingMode>(CHANGE_EVENT, { detail: mode }))Please add a two-consumer regression test with localStorage.setItem throwing.
1 participant
Summary
Behavior
Default rationale
Grouped remains the default because it matches Hapi's longstanding tool presentation across agent backends. The Codex-specific classified presentation introduced in #1139 is preserved as an optional mode rather than removed.
Testing
bun typecheckbun run test:web -- --maxWorkers=4bun run build:webgit diff --check