Uh oh!
There was an error while loading. Please reload this page.
PLUG-111- Added Analytics Id For Mermaid User - #49
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
Adds support in the SDK’s public types for exposing the Mermaid Chart per-user analytics identifier returned by /rest-api/users/me, enabling downstream consumers (e.g., MCP server) to reliably identify users for analytics.
Changes:
- Added
analyticsIDto the exportedMCUsertype. - Updated SDK tests to include
analyticsIDin the mockedgetUser()response.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/sdk/src/types.ts | Extends the MCUser type to include the analytics identifier. |
| packages/sdk/src/index.test.ts | Updates the mocked user object to match the expanded MCUser shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
packages/sdk/src/types.ts:26
MCUseris defined in multiple packages in this repo (e.g.packages/google/src/Common.ts:51andpackages/office/src/lib/mermaidChartApi.ts:33). AddinganalyticsIDonly to the SDK copy risks the types drifting further, and consumers of the other packages calling the same/rest-api/users/meendpoint won’t see the field. Consider consolidating to a sharedMCUserdefinition, or updating the otherMCUserinterfaces if they model the same API response.
export interface MCUser {
fullName: string;
emailAddress: string;
analyticsID?: string;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.changeset/nice-buttons-happen.md:5
- Typo in changeset text: "evetns" should be "events" (and the sentence can be cleaned up for clarity/capitalization).
Added the optional analyticsID for MCUser so we can track the evetns on mermaid mcp server
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.changeset/nice-buttons-happen.md:2
- The new
analyticsIDfield is required on an exported interface, which is a breaking TypeScript change for any consumers constructingMCUserobjects (their builds will start failing). A patch release is likely too low; either make the field optional or bump the changeset to a breaking release (major) to reflect the API contract change.
'@mermaidchart/sdk': patch
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/sdk/src/types.ts:28
- Adding new required properties to
MCUseris a TypeScript breaking change for any consumer code that constructs/serializesMCUservalues (even if runtime behavior is unchanged). If you want this to be non-breaking, make the new fields optional; otherwise the changeset should be aminorbump at least.
emailAddress: string;
analyticsID: string;
avatarUrl: string | null;
mermaidTheme?: string;
mermaidLook?: string;
packages/sdk/src/types.ts:28
- The PR description says this is adding only
analyticsID, but this change also addsavatarUrl,mermaidTheme, andmermaidLooktoMCUser. If those extra fields are intended, the PR description should be updated; if not, the type change should be scoped down to justanalyticsID.
analyticsID: string;
avatarUrl: string | null;
mermaidTheme?: string;
mermaidLook?: string;
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/sdk/src/types.ts:26
MCUseris a public exported interface, and adding required properties (analyticsID,avatarUrl) is a breaking TypeScript change for consumers that construct/mockMCUservalues (it makes previously-valid objects no longer assignable). If you want to keep this as a patch-level release, consider making the new fields optional while still documenting that the API will usually provide them.
analyticsID: string;
avatarUrl: string | null;
Uh oh!
There was an error while loading. Please reload this page.
Add the
analyticsIDfield to theMCUsertype so the SDK'sgetUser()exposes the Mermaid Chart analytics ID that the/rest-api/users/meendpoint already returns. This is a type-only change (no runtime behavior changes) that lets consumers like the MCP server identify a stable per-user ID for analytics instead of treating every event as a new user.