feat: add metrics infrustructure to ui #2698
Conversation
sareyu
requested review from
artemmufazalov,
astandrik,
Raubzeug and
adameat
as code owners
|
bugbot run |
There was a problem hiding this comment.
Pull Request Overview
This PR adds metrics infrastructure to the UI by implementing a wrapper for Yandex.Metrica counters. The implementation provides a consistent interface for tracking analytics events while gracefully handling cases where Metrica is not available.
Key changes:
- Creates a wrapper utility for accessing Yandex.Metrica instances with fallback handling
- Extends the UIFactory interface to include metrics configuration
- Implements a fake counter class for development/testing environments
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/utils/yaMetrica.ts | Implements the core metrics wrapper with Counter interface, FakeMetrica fallback class, and getMetrica function |
| src/uiFactory/types.ts | Adds yaMetricaMap property to UIFactory interface for metrics configuration |
src/utils/yaMetrica.ts
Outdated
|
|
||
| private warnOnce() { | ||
| if (!this.warnShown) { | ||
| console.warn(`YaMetrica counter "${this.name}" is not defined\n`); |
There was a problem hiding this comment.
The term 'YaMetrica' should be 'YandexMetrica' or 'Yandex.Metrica' for consistency with the official product name.
| console.warn(`YaMetrica counter "${this.name}" is not defined\n`); | |
| console.warn(`Yandex.Metrica counter "${this.name}" is not defined\n`); |
Copilot uses AI. Check for mistakes.
src/utils/yaMetrica.ts
Outdated
|
|
||
| export interface Counter { | ||
| hit: (...args: unknown[]) => void; | ||
| params: (...args: unknown[]) => void; | ||
| userParams: (...args: unknown[]) => void; |
There was a problem hiding this comment.
The Counter interface and its methods lack documentation. Consider adding JSDoc comments to explain the purpose of each method and their parameters.
| export interface Counter { | |
| hit: (...args: unknown[]) => void; | |
| params: (...args: unknown[]) => void; | |
| userParams: (...args: unknown[]) => void; | |
| /** | |
| * Represents a Yandex.Metrica counter interface for tracking analytics events. | |
| */ | |
| export interface Counter { | |
| /** | |
| * Sends a page hit event to the counter. | |
| * @param args - Arguments for the hit event (varies by implementation). | |
| */ | |
| hit: (...args: unknown[]) => void; | |
| /** | |
| * Sets custom parameters for the counter. | |
| * @param args - Arguments for setting parameters (varies by implementation). | |
| */ | |
| params: (...args: unknown[]) => void; | |
| /** | |
| * Sets user-specific parameters for the counter. | |
| * @param args - Arguments for setting user parameters (varies by implementation). | |
| */ | |
| userParams: (...args: unknown[]) => void; | |
| /** | |
| * Sends a goal achievement event to the counter. | |
| * @param args - Arguments for the goal event (varies by implementation). | |
| */ |
Copilot uses AI. Check for mistakes.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The getMetrica function lacks documentation. Consider adding JSDoc to explain its purpose, parameters, and return value, especially the fallback behavior to FakeMetrica.
| /** | |
| * Retrieves a YaMetrica counter instance by name. | |
| * | |
| * @param {string} name - The name of the YaMetrica counter to retrieve. | |
| * @returns {Counter} The YaMetrica counter instance if found, otherwise a FakeMetrica instance. | |
| * If the counter is not defined, FakeMetrica will log a warning once when its methods are called. | |
| */ |
Copilot uses AI. Check for mistakes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const yaMetricaId = yaMetricaMap?.[name]; | ||
| const metricaInstance = yaMetricaId | ||
| ? (window[`yaCounter${yaMetricaId}`] as Counter) | ||
| : undefined; |
There was a problem hiding this comment.
Bug: Metrica ID Check Fails on Zero
The getMetrica function's truthiness check on yaMetricaId (e.g., yaMetricaId ? ...) incorrectly treats 0 as falsy. Since 0 is a valid Yandex Metrica counter ID, this causes the function to return a FakeMetrica instance instead of the real window['yaCounter0']. The condition should use yaMetricaId !== undefined or yaMetricaId != null to correctly identify valid IDs.
|
/claude_review |
|
Claude finished @astandrik's task —— View job Code Review: Metrics Infrastructure ImplementationReview Tasks
SummaryThe implementation adds a clean wrapper for Yandex.Metrica integration with good fallback handling. However, there are several issues that need to be addressed to meet YDB Embedded UI standards. Critical Issues
// Current (buggy)
const metricaInstance = yaMetricaId
? (window[`yaCounter${yaMetricaId}`] as Counter)
: undefined;
// Should be
const metricaInstance = yaMetricaId !== undefined
? (window[`yaCounter${yaMetricaId}`] as Counter)
: undefined;
Code Quality Issues
Positive Aspects✅ Good patterns observed:
Recommendations
Example Fix for Critical Bugexport function getMetrica(name: string): Counter {
const yaMetricaId = yaMetricaMap?.[name];
const metricaInstance = yaMetricaId !== undefined
? (window[`yaCounter${yaMetricaId}`] as Counter)
: undefined;
return metricaInstance ?? new FakeMetrica(name);
}The implementation is well-structured but needs these fixes to meet the project's standards and avoid the counter ID 0 bug. |
Added wrapper for yaMetrica instance. Suppose yametrica will be initiated before app starts (it's part of core's renderLayout2 method) so there's no need for event queueing.
Call:
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: ✅
Current: 85.33 MB | Main: 85.33 MB
Diff: 0.00 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information