chore: Adds deprecation warning and serves new chart data endpoint #36845
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 885041c The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36845 +/- ##
===========================================
+ Coverage 66.53% 66.54% +0.01%
===========================================
Files 3344 3345 +1
Lines 114629 114629
Branches 21095 21227 +132
===========================================
+ Hits 76270 76284 +14
+ Misses 35670 35659 -11
+ Partials 2689 2686 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
apps/meteor/client/views/omnichannel/analytics/InterchangeableChart.tsx
Outdated
Show resolved
Hide resolved
WalkthroughAdds a new REST endpoint for Livechat analytics charts data, switches the client chart to use it, logs deprecation for the legacy Meteor method, augments REST typings/validators, and adds end-to-end tests (including a duplicated test block). Includes a changeset bumping related packages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client as InterchangeableChart (UI)
participant API as REST /v1/livechat/analytics/dashboards/charts-data
participant Route as Server Route
participant OA as OmnichannelAnalytics
User->>Client: Open dashboard
Client->>API: GET charts-data {chartName, start, end, dept?}
API->>Route: Validate auth, permission, params
Route->>OA: OmnichannelAnalytics.getAnalyticsChartData(params)
OA-->>Route: Chart data | null
alt Data present
Route-->>API: 200 { success, chartLabel, dataLabels, dataPoints }
else No data
Route-->>API: 200 { success, chartLabel, dataLabels: [], dataPoints: [] }
end
API-->>Client: JSON response
Client-->>User: Render chart or empty state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
lucas-a-pelegrino
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/meteor/client/views/omnichannel/analytics/InterchangeableChart.tsx (1)
58-59: UI resilience re: empty datasets is covered.Server guarantees a non-null chartLabel and empty arrays on no data, addressing prior crash concerns.
Also applies to: 84-86
🧹 Nitpick comments (7)
.changeset/fresh-deers-march.md (1)
6-6: Polish the changeset wording.Small grammar tweak for clarity.
Apply:
-Adds deprecation warning to `livechat:getAnalyticsChartData`, as well as it adds a new endpoint to replace it; `livechat/analytics/dashboards/charts-data` +Add a deprecation warning to `livechat:getAnalyticsChartData` and introduce a replacement endpoint: `livechat/analytics/dashboards/charts-data`.apps/meteor/client/views/omnichannel/analytics/InterchangeableChart.tsx (4)
58-59: Use params passed to draw to build the request payload.Keeps the function pure and avoids stale-closure bugs when start/end/departmentId change.
-const loadData = useEndpoint('GET', '/v1/livechat/analytics/dashboards/charts-data'); +const loadData = useEndpoint('GET', '/v1/livechat/analytics/dashboards/charts-data'); @@ - const result = await loadData({ - chartName, - start, - end, - ...(departmentId && { departmentId }), - }); + const { + daterange: { from, to }, + chartOptions: { name }, + departmentId: depId, + } = params; + const result = await loadData({ + chartName: name, + start: from, + end: to, + ...(depId && { departmentId: depId }), + });Also applies to: 77-83
84-86: Update the error message to reference the new REST endpoint.The message still mentions the deprecated Meteor method.
- if (!result?.chartLabel || !result?.dataLabels || !result?.dataPoints) { - throw new Error('Error! fetching chart data. Details: livechat:getAnalyticsChartData => Missing Data'); - } + if (!result?.chartLabel || !result?.dataLabels || !result?.dataPoints) { + throw new Error('Error fetching chart data from /v1/livechat/analytics/dashboards/charts-data: missing fields'); + }
100-101: Surface a readable message to the toast.Passing the raw error object can render poorly.
- dispatchToastMessage({ type: 'error', message: error }); + dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) });
105-115: Trim unnecessary dependency to avoid extra renders.
tisn’t used in the effect; alsoloadDatais stable fromuseEndpoint.-}, [chartName, departmentId, draw, end, start, t, loadData]); +}, [chartName, departmentId, draw, end, start]);packages/rest-typings/src/v1/omnichannel.ts (2)
3400-3426: Consider validating date-like strings or mirror server-side date checks.Schema currently accepts any string; the server endpoint should reject invalid dates. Either:
- add a lightweight pattern for YYYY-MM-DD (what tests send), or
- keep schema as-is and enforce parse checks server-side (recommended for flexibility).
If you prefer a pattern:
start: { - type: 'string', + type: 'string', + pattern: '^\\d{4}-\\d{2}-\\d{2}.*$', }, end: { - type: 'string', + type: 'string', + pattern: '^\\d{4}-\\d{2}-\\d{2}.*$', },If you keep schema generic, ensure the server adds explicit Date.parse checks (see server comment).
3995-4543: Expose the new endpoint in OmnichannelEndpoints for first-class typing (optional).You’re augmenting Endpoints from the server file. To improve DX and avoid cross-package augmentation coupling, also declare it here alongside other analytics routes.
Add this route to OmnichannelEndpoints:
'/v1/livechat/analytics/dashboards/charts-data': { GET: (params: GETLivechatAnalyticsDashboardsChartDataParams) => { chartLabel: string; dataLabels: string[]; dataPoints: number[]; }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/fresh-deers-march.md(1 hunks)apps/meteor/app/livechat/imports/server/rest/dashboards.ts(2 hunks)apps/meteor/app/livechat/server/methods/getAnalyticsChartData.ts(2 hunks)apps/meteor/client/views/omnichannel/analytics/InterchangeableChart.tsx(3 hunks)apps/meteor/tests/end-to-end/api/livechat/04-dashboards.ts(1 hunks)packages/rest-typings/src/v1/omnichannel.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/meteor/tests/end-to-end/api/livechat/04-dashboards.ts (1)
apps/meteor/tests/data/api-data.ts (1)
credentials(39-42)
apps/meteor/client/views/omnichannel/analytics/InterchangeableChart.tsx (2)
packages/ui-contexts/src/index.ts (1)
useEndpoint(32-32)apps/meteor/server/models/raw/LivechatRooms.ts (2)
countAllOpenChatsByDepartmentBetweenDate(835-879)calculateDurationTimingsBetweenDates(1013-1055)
apps/meteor/app/livechat/imports/server/rest/dashboards.ts (5)
packages/rest-typings/src/v1/omnichannel.ts (2)
GETLivechatAnalyticsDashboardsChartDataSuccessSchema(3457-3461)isGETLivechatAnalyticsDashboardsChartDataParams(3427-3429)packages/rest-typings/src/v1/Ajv.ts (2)
validateUnauthorizedErrorResponse(69-69)validateForbiddenErrorResponse(92-92)packages/core-services/src/index.ts (1)
OmnichannelAnalytics(184-184)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(71-75)packages/rest-typings/src/index.ts (1)
Endpoints(52-100)
packages/rest-typings/src/v1/omnichannel.ts (2)
packages/rest-typings/src/v1/Ajv.ts (1)
ajv(23-23)apps/meteor/server/models/raw/LivechatRooms.ts (4)
getAnalyticsMetricsBetweenDateWithMessages(2103-2185)getAnalyticsMetricsBetweenDate(2082-2101)calculateResponseTimingsBetweenDates(927-968)countAllOpenChatsByDepartmentBetweenDate(835-879)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/app/livechat/server/methods/getAnalyticsChartData.ts (1)
8-9: Deprecation logger placement looks good; verify target version.Logging before auth is acceptable here; please confirm the deprecation target version '8.0.0' matches the team plan and any public docs.
Would you like me to search and cross-check our deprecation/versioning guideline to ensure '8.0.0' is correct?
Also applies to: 19-19
apps/meteor/app/livechat/imports/server/rest/dashboards.ts (1)
251-289: Return shape on “no data” is consistent—good.Empty arrays plus the original chartLabel keeps the UI simple and matches the success schema.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/client/views/omnichannel/analytics/InterchangeableChart.tsx (1)
87-99: Always draw when canvas is ready; add safe fallback for chart label.
The current boolean chain skips drawing ifcontext.currentisnull. Simplify the guard and provide a fallback label to avoid crashes if the API returns a falsychartLabel.- (context.current || typeof context.current === 'undefined') && - canvas.current && - (context.current = await drawLineChart( - canvas.current, - context.current, - [result.chartLabel], - result.dataLabels, - [result.dataPoints], - { - tooltipCallbacks, - }, - )); + if (canvas.current) { + context.current = await drawLineChart( + canvas.current, + context.current, + [result.chartLabel ?? chartName], + result.dataLabels, + [result.dataPoints], + { tooltipCallbacks }, + ); + }
🧹 Nitpick comments (6)
apps/meteor/client/views/omnichannel/analytics/InterchangeableChart.tsx (6)
60-70: Unify parameter sources and types for departmentId and date range.
drawacceptsdepartmentId?and adaterange, but inside you ignore both and use outerstart,end, anddepartmentId. Use the passed params (or drop them) to avoid stale-closure surprises; also make the top-leveldepartmentIdprop optional for consistency withdepartmentId?:here.Apply within this block and the load call below:
- async (params: { + async (params: { daterange: { from: string; to: string; }; chartOptions: { name: string; }; departmentId?: string; }) => {And update the props type (outside this hunk) to align:
// at Lines 42-49 (props type) departmentId?: string;
77-83: Use passed daterange, and guard against 'undefined' string departmentId.
Avoid capturingstart/endfrom outer scope; the backend also treats the literal "undefined" specially—mirror that on the client.- const result = await loadData({ - chartName, - start, - end, - ...(departmentId && { departmentId }), - }); + const { from, to } = params.daterange; + const result = await loadData({ + chartName, + start: from, + end: to, + ...(params.departmentId && params.departmentId !== 'undefined' && { departmentId: params.departmentId }), + });
84-86: Harden response validation and localize the error.
Check array types explicitly and use an i18n key for the toast.- if (!result?.dataLabels || !result?.dataPoints) { - throw new Error('Error! fetching chart data.'); - } + if (!Array.isArray(result?.dataLabels) || !Array.isArray(result?.dataPoints)) { + throw new Error(t('Error_fetching_chart_data')); + }
100-101: Toast the error message string, not the Error object.
Passing the raw Error can render as[object Object].- dispatchToastMessage({ type: 'error', message: error }); + dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) });
112-113: Mirror backend filter behavior for departmentId.
Avoid sending the literal'undefined'which the server treats specially.- ...(departmentId && { departmentId }), + ...(departmentId && departmentId !== 'undefined' && { departmentId }),
114-114: Trim effect deps to avoid unnecessary redraws.
tandloadDataaren’t referenced inside the effect body; removing them avoids redraws on language changes or hook identity churn.-}, [chartName, departmentId, draw, end, start, t, loadData]); +}, [chartName, departmentId, draw, end, start]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/views/omnichannel/analytics/InterchangeableChart.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/views/omnichannel/analytics/InterchangeableChart.tsx (2)
packages/ui-contexts/src/index.ts (1)
useEndpoint(32-32)apps/meteor/server/models/raw/LivechatRooms.ts (2)
countAllOpenChatsByDepartmentBetweenDate(835-879)getAnalyticsBetweenDate(2187-2263)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/client/views/omnichannel/analytics/InterchangeableChart.tsx (2)
2-2: Switch to REST hook looks good; proceed.
Replacing the Meteor method withuseEndpointis the right direction for the deprecation plan.
58-58: Endpoint contract matches rest-typings. Params (chartName, start, end, optional departmentId) and response keys (chartLabel, dataLabels, dataPoints) align with the schemas—no changes needed.
lucas-a-pelegrino
added
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
lucas-a-pelegrino
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
This PR adds a new deprecation warning for
livechat:getAnalyticsChartDataas well as adding a new endpoint to replace it.Issue(s)
CTZ-53
Steps to test or reproduce
Further comments
Tests will be improved in a follow-up task.
Summary by CodeRabbit
New Features
Deprecations
Refactor
Tests