feat(angular): add CopilotActivity for standalone activity rendering - #6033
rainerhahnekamp merged 3 commits into
Conversation
Rendering activity messages outside of CopilotChatMessageView (custom chat shells, dashboards, headless setups) currently requires instantiating the whole message view or copying its private resolution logic. Tool calls already have a public component for this (RenderToolCalls), activities do not. This adds CopilotActivity (<copilot-activity [message] [agentId]>), a standalone host for a single activity message. CopilotChatMessageView now delegates to it, so there is one implementation for the activity role. No behavior change. The renderer resolution moves into an internal pickActivityRenderer that is not exported. Co-Authored-By: Claude Fable 5 <[email protected]>
ff503eb to
bda5c41
Compare
a1d079e to
bda5c41
Compare
rainerhahnekamp
left a comment
There was a problem hiding this comment.
Serwas @manfredsteyer, I did the review fully manually, so there are a few. comments I've left. we'll use the information for the review to come up with specific review skills in the future.
/cc @wolfmanfx
|
|
||
| const parseResult = renderer.content.safeParse(message.content); | ||
| if (parseResult.success === false) { | ||
| console.warn( |
There was a problem hiding this comment.
I don't know. In Angular we usually log in ngDevMode. I'll leave that to your decision @manfredsteyer
There was a problem hiding this comment.
If it's fine for you, I would stick with it because ngDevMode is currently not used in the code base and because we already have other places where a warning is print to the console.
| * Kept internal for now (not part of the public API); it can be exposed later | ||
| * without a breaking change if there is demand for a headless resolver. | ||
| */ | ||
| export function pickActivityRenderer( |
There was a problem hiding this comment.
that could actually be a private function of CopilotKitActivity.
There was a problem hiding this comment.
This was indeed the case before my changes. The reason for this change is mainly symmetry with the counter part pickToolCallHandler. Do you want it to be changed back to CopilotKitActivity anyway?
| * without a breaking change if there is demand for a headless resolver. | ||
| */ | ||
| export function pickActivityRenderer( | ||
| options: PickActivityRendererOptions, |
There was a problem hiding this comment.
i am not a big fan of having an explicit type for three paramters. it would be more readable if that function just has three parameters.
There was a problem hiding this comment.
Also this was done for symmetry with pickToolCallHandler. Do you want it to be still changed back?
| `, | ||
| }) | ||
| export class CopilotActivity { | ||
| readonly #copilotKit = inject(CopilotKit); |
There was a problem hiding this comment.
i think with the upcoming private in Angular 22,1, we should not use # anymore. Just private. angular/angular#70188
There was a problem hiding this comment.
| imports: [NgComponentOutlet], | ||
| changeDetection: ChangeDetectionStrategy.Eager, | ||
| template: ` | ||
| @let render = resolveRender(message()); |
There was a problem hiding this comment.
I am not really happy with calling a function here. Why not use a computed?
|
|
||
| @Component({ | ||
| selector: "secondary-activity-renderer", | ||
| changeDetection: ChangeDetectionStrategy.Eager, |
|
|
||
| @Component({ | ||
| selector: "wildcard-activity-renderer", | ||
| changeDetection: ChangeDetectionStrategy.Eager, |
| expect(rendered?.getAttribute("data-content")).toBe( | ||
| JSON.stringify({ operations: [] }), | ||
| ); | ||
| expect(getAgent).toHaveBeenCalledWith("demo-button"); |
There was a problem hiding this comment.
is that necessary? i think the other assertions must failed if the agent wasn't called.
| ...overrides, | ||
| }); | ||
|
|
||
| describe("CopilotActivity", () => { |
There was a problem hiding this comment.
Test which could be added
- once the Component is using a computed, we could also add a test where an agentid switches.
- multiple messages and how the rendering changes.
| ...overrides, | ||
| }); | ||
|
|
||
| describe("pickActivityRenderer", () => { |
There was a problem hiding this comment.
i'd say pickRenderer is an implementation detail of CopilotActivity and should therefore be tested there. In fact, we could do specific picking tests in a nested describe.
…opilotActivity.mdx Co-authored-by: Rainer Hahnekamp <[email protected]>
|
Thanks @rainerhahnekamp for your review. Please see my answers above. |
|
Serwas @manfredsteyer, yeah, so I fully understand that you were following the existing coding standards, but here's the thing: We don't fully see the existing codebase as "Angularized," and we won't have the resources to do a one-time "Angularization" project. Instead, we want to do this incrementally, and we have to start somewhere. So here's what we can do. We can merge your PR as-is. Its public API is perfeclty fine nothing will change there. The new component will be available with the next Angular release. @wolfmanfx or me would then apply the mentioned comments in a separate PR. In the meantime, we two could then continue with #6075 |
|
@rainerhahnekamp Sounds great. And I’ll keep these comments regarding modern Angular (OnPush, ...) in mind for future PRs. |
|
Yeah, we should probably also collect all the reviews and make a code guidelines skill out of them. Then the models do it automatically. |
|
Danke Manfred! |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds standalone ChangesAngular activity rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I thank you, @rainerhahnekamp |
There is currently no dedicated way to render a single activity message in headless mode. Building it yourself means duplicating the private resolution logic of
CopilotChatMessageView.Tool calls already have a public component for this (
RenderToolCalls), activities do not. This is an asymmetry in the Angular API and a gap compared to React'suseRenderActivityMessage().This PR adds
CopilotActivity(<copilot-activity [message] [agentId]>), a standalone host for a single activity message.CopilotChatMessageViewnow delegates to it, so there is one implementation for the activity role. No behavior change. The renderer resolution moves into an internalpickActivityRendererthat is not exported.Summary by CodeRabbit
New Features
CopilotActivityAngular component for rendering individual activity messages.messageand optionalagentIdinputs for custom activity surfaces.Improvements
CopilotChatMessageViewto useCopilotActivityfor activity rendering.Documentation