Repository navigation
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a bounded chat-rendering bug fix that namespaces timeline IDs and removes duplicate singleton status rows across web and mobile, with targeted regression tests. It changes no stored data, schema, deployment behavior, product defaults, or security-sensitive code. You can add or adjust custom eligibility rules. Learn more. |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between e39cb9dc6a5d1b2b18da609b39cc9f85be915c2a and 17909343b9514daaa091e5e6dbdb828b330f6590. 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughResolved user-input activities now render once as individual activity rows in mobile and web timelines. Activity-group and work-entry identifiers use distinct prefixes. Work-group summaries and icon fallback logic were updated. ChangesActivity row rendering
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The timeline rendering changes have no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
The browser check exposed an ID collision between async answer messages and their status activities. Fixed in e01b00997 by giving activity entries distinct timeline IDs before folding and rendering on web and mobile. The answer now stays visible with the turn collapsed or expanded, and the expanded timeline has one submitted-status row with no empty slot. Before/after images are in the description. All 290 focused tests and both client typechecks pass. The generic docstring-coverage suggestion is left unapplied. AGENTS.md asks for comments that explain non-obvious constraints, rather than descriptions of straightforward rendering code. The regression tests cover the behavior and the new ID collision case. |
|
@macroscope-app review |
|
Sorry, I'm unable to act on this request because you do not have permissions within this repository. |
|
Visual QA on Expanded single-status no longer duplicates: one Before (from the PR): Before: duplicate User input submitted heading and event row After — single resolved user-input, collapsed: After: single User input submitted status, collapsed After — single resolved user-input, expanded (still one label): After: single User input submitted status, expanded After — multi-event work group, collapsed: After: collapsed multi-event work group After — multi-event work group, expanded (heading + event rows kept): After: expanded multi-event work group After — active / in-progress task group (existing live behavior): Checks: |
1790934 to
5ddf121
Compare
5ddf121 to
a05a0bd
Compare
Dismissing prior approval to re-evaluate 39fae55
…t-status-row # Conflicts: # apps/web/src/components/chat/MessagesTimeline.tsx
|
@juliusmarminge conflicts fixed. Conflict fix by Opus 5.5 (Claude Code), on behalf of Dominic. |
|
Thanks for working on this. We merged the orchestrator V2 rewrite in #2829, and we are closing this PR as part of that transition. The patch conflicts with the rewrite in apps/mobile/src/lib/threadActivity.ts, apps/web/src/components/chat/MessagesTimeline.logic.ts, apps/web/src/components/chat/MessagesTimeline.tsx and 1 other files. Even where the conflict is small enough to rebase, we are asking for fresh PRs against the new base so we can review and verify the behavior in V2. Sorry for the extra work this creates. If the change is still needed on V2, please rebuild it on current main, verify it there, and open a new PR linking back here. We're closing the current implementation without assuming the underlying request is resolved. |
|
Checked against current main after #2829. Both issues are resolved in V2, so this doesn't need a follow-up PR:
Checked by Opus 5.5 (Claude Code), on behalf of Dominic. |
A single status could render twice, as a work-group heading and an event row. Activity IDs could also collide with message IDs, hiding an async answer or leaving an empty timeline slot.
Render single inactive statuses directly and give work/activity rows distinct timeline IDs on web, desktop, and mobile. Keep expanded subagent rows visible when their agents settle by comparing expansion state with the underlying work-entry ID.
This complements the Q&A presentation shipped in #11014, which superseded #10417. It preserves that presentation and covers standalone statuses and ID collisions, including older answer records that cannot be folded into a Q&A row. It does not change stored messages or provider payloads and does not claim to fix the question-attribution issue in #10786.
Validation at
f02caf35c:vp testwrapper fails before running tests.The images below show the original fix before the merge with current main. Browser verification was not repeated after that merge; mobile was not simulator-tested.
Model: GPT-6. Harness: Codex.
Note
Fix single status event rendering to avoid duplicate headings in chat timeline
appendToolGroupRowsin threadActivity.ts now emits a single inactive non-tool activity directly as one activity-group row and returns before the tool-group summary path.deriveMessagesTimelineRowsin MessagesTimeline.logic.ts now treats any single visible work entry (not just tool-like ones) as a plain work row. The unusedsummaryToolIconfield is removed fromWorkGroupToggleTimelineRowand its fallback lookup in MessagesTimeline.tsx is dropped.WorkGroupToggleTimelineRow.summaryToolIconis removed; any out-of-tree consumers of this type field will need to update. Grouped-toggle summaries now always callsummarizeToolGroupdirectly.Macroscope summarized 4d832a5.
Summary by CodeRabbit