-
Notifications
You must be signed in to change notification settings - Fork 0
Added execution trace and a bunch of improvements to copilot #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
yusufaytas
commented
Jan 5, 2026
- Moved the alert external link into the header pill row and removed the older description button.
- Refactored Copilot rendering to use a new CopilotConclusion component and dropped the accordion-based details UI.
- Added CopilotConclusion to parse summaries vs bullet points and render expandable key points.
- Simplified copilot answer normalization to focus on conclusions, references, and execution traces.
- Updated ReferenceLinks to support expandable groups with total counts and consistent neutral styling.
- Added execution-trace UI and passed stored traces into response details.
- Expanded deployment reference URL logic and substantially reorganized/grew tests for edge cases.
Greptile Summary
Important Files Changed
Confidence score: 3/5
Sequence DiagramsequenceDiagram
participant User
participant CopilotPanel as "CopilotPanel"
participant CopilotAPI as "/api/copilot"
participant CopilotConclusion as "CopilotConclusion"
participant ResponseDetails as "ResponseDetailsContent"
User->>CopilotPanel: "Types question and clicks Ask Copilot"
CopilotPanel->>CopilotPanel: "Add user turn to conversation"
CopilotPanel->>CopilotAPI: "POST /api/copilot with message"
CopilotAPI-->>CopilotPanel: "Returns CopilotApiResponse with answer"
CopilotPanel->>CopilotPanel: "normalizeAnswer() - extract conclusion, references, executionTrace"
CopilotPanel->>CopilotPanel: "Add copilot turn to conversation"
CopilotPanel->>CopilotConclusion: "Render conclusion text with parseNarrative()"
CopilotConclusion-->>User: "Display summary and expandable key points"
CopilotPanel->>ResponseDetails: "Render references, missing data, execution trace"
ResponseDetails-->>User: "Display expandable tool executions and references"
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9 files reviewed, 6 comments
| <span className="text-xs text-slate-400"> | ||
| {totalReferences} total | ||
| </span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: duplicate display of total count - already shown in header on line 49
| <span className="text-xs text-slate-400"> | |
| {totalReferences} total | |
| </span> | |
| <svg | |
| className={`h-4 w-4 text-slate-400 transition-transform ${isExpanded ? "rotate-180" : ""}`} | |
| fill="none" | |
| viewBox="0 0 24 24" | |
| stroke="currentColor" | |
| > |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/(enterprise)/copilot/ReferenceLinks.tsx
Line: 52:54
Comment:
**style:** duplicate display of total count - already shown in header on line 49
```suggestion
<svg
className={`h-4 w-4 text-slate-400 transition-transform ${isExpanded ? "rotate-180" : ""}`}
fill="none"
viewBox="0 0 24 24"
stroke="currentColor"
>
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| // 'service' is not in AlertQuery, but 'scope' is. | ||
| // Wait, AlertQuery has query, statuses, severities, scope, limit. | ||
| // The previous test had 'service'. I should use 'query' or 'scope'. | ||
| query: 'error' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Comment about 'service' not being in AlertQuery should be removed or corrected - it's now using proper 'query' parameter
| // 'service' is not in AlertQuery, but 'scope' is. | |
| // Wait, AlertQuery has query, statuses, severities, scope, limit. | |
| // The previous test had 'service'. I should use 'query' or 'scope'. | |
| query: 'error' | |
| query: 'error' |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/referenceBuilder.test.ts
Line: 231:234
Comment:
**style:** Comment about 'service' not being in AlertQuery should be removed or corrected - it's now using proper 'query' parameter
```suggestion
query: 'error'
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| {bullets.map((bullet, idx) => ( | ||
| <li key={idx} className="flex gap-2 text-sm text-slate-800"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Using array index as key in React lists can cause rendering issues if the list order changes
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/(enterprise)/copilot/CopilotConclusion.tsx
Line: 54:55
Comment:
**style:** Using array index as key in React lists can cause rendering issues if the list order changes
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| return `/tickets?ticketId=${args.id}`; | ||
| } | ||
| if (args.ticketId) { | ||
| return `/tickets?ticketId=${args.ticketId}`; | ||
| } | ||
| return "/tickets"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: get-ticket uses query parameter while other 'get-*' tools use path parameters - this inconsistency might confuse users expecting similar URL patterns. Is this URL pattern difference intentional for tickets versus other resources?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/lib/referenceBuilder.ts
Line: 96:101
Comment:
**style:** get-ticket uses query parameter while other 'get-*' tools use path parameters - this inconsistency might confuse users expecting similar URL patterns. Is this URL pattern difference intentional for tickets versus other resources?
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| <div className="mt-1 flex flex-wrap gap-1.5"> | ||
| {iteration.plannedTools.map((tool, idx) => ( | ||
| <span | ||
| key={`${tool.name}-${idx}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: potential key collision if tool names are duplicated within same iteration. Should the key include iteration number to ensure uniqueness across all tools?
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/(enterprise)/copilot/ToolExecutionsView.tsx
Line: 216:216
Comment:
**logic:** potential key collision if tool names are duplicated within same iteration. Should the key include iteration number to ensure uniqueness across all tools?
How can I resolve this? If you propose a fix, please make it concise.| <div className="mt-3 space-y-2"> | ||
| {iteration.toolExecutions.map((exec, idx) => ( | ||
| <ToolExecutionRow | ||
| key={idx} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: using array index as key can cause React reconciliation issues if list order changes
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/(enterprise)/copilot/ToolExecutionsView.tsx
Line: 247:247
Comment:
**style:** using array index as key can cause React reconciliation issues if list order changes
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.