feat: add multi-value attribution cell with plural fallback for logs columns - #4865
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors log table attribution rendering to use an ChangesAttribution Cell Refactor
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
b885c80 to
2452cf2
Compare
2452cf2 to
995af61
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ui/app/workspace/logs/views/columns.tsx (2)
208-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse value as list key instead of array index.
visible.map((value, index) => <span key={index}>uses the array index as key.MAX_ATTRIBUTION_LINESis currently1so this is a no-op today, but as soon as the cap is raised the index-based key becomes fragile if the underlying array order changes (re-sorted, filtered, etc.). Prefer the value itself (orvalue-indexcomposite if duplicates are possible) as key.As per coding guidelines: "Always use stable, unique keys in lists; never use array index as key (unless unavoidable)."
♻️ Proposed fix
{visible.map((value, index) => ( - <span key={index} className="truncate"> + <span key={`${value}-${index}`} className="truncate"> {value} </span> ))}🤖 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 `@ui/app/workspace/logs/views/columns.tsx` around lines 208 - 247, The AttributionCell list rendering uses an unstable array index key in the visible.map loop. Update the key to a stable identifier based on the rendered value in AttributionCell, using the value itself or a value-index composite if duplicates are possible, so the span keys remain consistent if MAX_ATTRIBUTION_LINES changes or the values order is updated.Source: Coding guidelines
237-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNative
titletooltip is inconsistent with the existing Radix Tooltip convention used for the same "+N more" pattern elsewhere.
modelCatalogTable.tsximplements the identical multi-value truncation UX usingTooltip/TooltipTrigger/TooltipContent(see relevant snippet), giving keyboard/focus accessibility and richer content rendering. Here, the full list is instead exposed only via the nativetitleattribute on the wrappingdiv, which is not keyboard-accessible and has inconsistent styling/delay across browsers.As per path instructions: "reuse existing constants, page structure, shared components, and data-testid naming patterns before introducing one-off UI conventions."
🤖 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 `@ui/app/workspace/logs/views/columns.tsx` around lines 237 - 244, The multi-value column renderer in columns.tsx is using a native title attribute for the full list, which is inconsistent with the existing Radix Tooltip pattern used for the same “+N more” UX elsewhere. Update the component that renders the visible values and remaining count to wrap the content with Tooltip, TooltipTrigger, and TooltipContent like the approach in modelCatalogTable.tsx, so the full list is shown through the shared tooltip convention instead of the div title. Keep the existing truncation and “+{remaining} more” display, but move the full values into the tooltip content for consistent accessibility and styling.Source: Path instructions
🤖 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.
Nitpick comments:
In `@ui/app/workspace/logs/views/columns.tsx`:
- Around line 208-247: The AttributionCell list rendering uses an unstable array
index key in the visible.map loop. Update the key to a stable identifier based
on the rendered value in AttributionCell, using the value itself or a
value-index composite if duplicates are possible, so the span keys remain
consistent if MAX_ATTRIBUTION_LINES changes or the values order is updated.
- Around line 237-244: The multi-value column renderer in columns.tsx is using a
native title attribute for the full list, which is inconsistent with the
existing Radix Tooltip pattern used for the same “+N more” UX elsewhere. Update
the component that renders the visible values and remaining count to wrap the
content with Tooltip, TooltipTrigger, and TooltipContent like the approach in
modelCatalogTable.tsx, so the full list is shown through the shared tooltip
convention instead of the div title. Keep the existing truncation and
“+{remaining} more” display, but move the full values into the tooltip content
for consistent accessibility and styling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c97b78cd-beb4-422a-a0fa-5170bac1fd97
📒 Files selected for processing (2)
ui/app/workspace/logs/views/columns.tsxui/lib/types/logs.ts
995af61 to
b9f6c9d
Compare
Merge activity
|
* upstream/dev: feat: adds multiple teams / customers / bus to connectors (maximhq#4875) fix: small latency return fixes (maximhq#4876) Added missing OpenAI responses methods for lifecycle related tasks (maximhq#3125) feat: latency info on errors (maximhq#4867) feat: add `user_name`, `team_ids`, `team_names`, `customer_ids`, `customer_names`, `business_unit_ids`, `business_unit_names` to log list select columns (maximhq#4866) feat: add multi-value attribution cell with plural fallback for logs columns (maximhq#4865)

Summary
Attribution columns in the logs table previously only displayed a single name or ID per log entry. This PR introduces support for rendering multiple attribution values (teams, customers, business units) in a single cell, with a compact "+N more" overflow indicator when values exceed the display limit.
Changes
attributionCellhelper with a dedicatedAttributionCellcomponent that accepts both singular (name,id) and plural (names,ids) props, resolving values via a plural-first fallback: plural names → singular name → plural IDs → singular ID.team_names,team_ids,customer_names,customer_ids,business_unit_names,business_unit_ids) alongside their singular counterparts.virtual_key_nameandrouting_rule_namedirectly from the log entry rather than accessing nested objects.virtual_key_nameandrouting_rule_nameas top-level fields on theLogEntrytype.MAX_ATTRIBUTION_LINES(1) are shown with a+N moreindicator; hovering reveals all values via thetitleattribute.Type of change
Affected areas
How to test
+N moreindicator.Screenshots/Recordings
Add before/after screenshots of the attribution columns showing single vs. multi-value rendering.
Breaking changes
Related issues
Security considerations
None. No auth, secrets, or PII handling changes introduced.
Checklist
docs/contributing/README.mdand followed the guidelines