fix: multi tool rendering - #1
Conversation
![managerbot-app[bot]](https://avatars.githubusercontent.com/in/853044?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed Changes
Managerbot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
Files not reviewed (0)
Comments suppressed due to low confidence (2)
-
lib/transcript_viewer/view.rb:222
- (are_you_sure) `tool_result_block` is now used as the variable name for each element in the content array without filtering, but it could be any block type. The original code used `.find` to locate the specific `tool_result` block. Consider renaming the variable and filtering: `data[:content].select { |block| block[:type] == "tool_result" }.each_with_index.map`. test/test_transcript_viewer.rb:118
- (test_completeness) The test asserts `html.scan(/tool-execution success/).length == 2`, which is a reasonable check, but also consider asserting that both "first result" and "second result" are associated with their respective tool call IDs (`tool-1` and `tool-2`) to ensure the mapping is correct, not just that two results are rendered.
| role: "toolResult", | ||
| toolCallId: tool_result_block[:tool_use_id], | ||
| toolName: tool_name_by_call_id[tool_result_block[:tool_use_id]], | ||
| content: normalize_tool_result_content(tool_result_block[:content]), |
There was a problem hiding this comment.
When iterating over data[:content] to map tool results, the code assumes every block in the content array is a tool_result block. If a user message contains mixed content (e.g., a text block alongside tool_result blocks), non-tool_result blocks will be incorrectly mapped as tool results (accessing block[:tool_use_id] on a text block will return nil). Consider filtering to only tool_result typed blocks before mapping.
| end | ||
|
|
||
| def tool_result_entry_id(event_id, index, result_count) | ||
| return event_id if result_count == 1 |
There was a problem hiding this comment.
The tool_result_entry_id method uses a colon-separated suffix format "#{event_id}:tool-result:#{index}". If event_id itself contains colons, this could cause ambiguity when parsing IDs later. Consider using a delimiter that is less likely to appear in event IDs, or document the ID format assumption.
| raw_entries = ancestor_entries + session_entries | ||
| tool_name_by_call_id = build_tool_call_name_map(raw_entries) | ||
| ancestor_count = ancestor_entries.map { |entry| map_entry(entry, tool_name_by_call_id) }.compact.length | ||
| ancestor_count = ancestor_entries.sum { |entry| map_entries(entry, tool_name_by_call_id).length } |
There was a problem hiding this comment.
ancestor_count is now computed by summing the lengths of mapped entry arrays. Ensure that map_entries is pure and side-effect free, since it is called twice for ancestor entries — once here and once during the flat_map on raw_entries. If map_entries has any side effects or is expensive, this double invocation could be problematic. Consider computing it from the already-mapped entries slice instead.
1 participant
No description provided.