Skip to content

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Jul 30, 2025

WHY are these changes introduced?

This PR introduces a component to collect runtime data about theme commands. The goal is to improve the quality of the signals we have around theme commands and help us better understand any friction users might be facing.

WHAT is this pull request doing?

This PR adds a new analytics module at themes/analytics.ts. This module is responsible for recording events in a small, bounded JavaScript object, which helps avoid performance issues from large objects and prevents sending oversized data to monorail.

The main features of this module include:

  • Core analytics functions for recording timings, errors, retries, and custom events
  • Bounded collections (BArray and BMap) to limit memory usage and prevent leaks
  • Error categorization to group errors into meaningful categories
  • A storage layer for managing analytics data during runtime
  • Integration with theme commands to track their execution and authentication steps
  • Comprehensive test coverage for all new components

This analytics module will help us identify performance bottlenecks, common errors, and usage patterns, giving us clearer insights to improve the theme development experience.

How to test your changes?

N/A — This component is not user-facing and does not require user-level testing (this will happen on following PRs using this component).

Post-release steps

N/A

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

@karreiro karreiro requested a review from EvilGenius13 July 30, 2025 10:38
Copy link
Contributor

github-actions bot commented Jul 30, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
78.51% (+0.1% 🔼)
13362/17020
🟡 Branches
72.43% (+0.04% 🔼)
6506/8983
🟡 Functions
78.72% (+0.14% 🔼)
3481/4422
🟡 Lines
78.88% (+0.11% 🔼)
12639/16024
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / analytics.ts
100% 100% 100% 100%
🟢
... / bounded-collections.ts
100% 87.5% 100% 100%
🟢
... / error-categorizer.ts
100% 100% 100% 100%
🟢
... / storage.ts
96.55% 87.5% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / deploy.ts
97.3% (-0.04% 🔻)
94.12% 100%
97.22% (-0.04% 🔻)
🟢
... / link.ts
96.51%
94.12% (-1.34% 🔻)
100% 96.34%
🟢
... / fs.ts
86.81% (-1.42% 🔻)
93.75% (-1.25% 🔻)
84.44% (-0.34% 🔻)
86.52% (-1.48% 🔻)

Test suite run success

3211 tests passing in 1348 suites.

Report generated by 🧪jest coverage report action from a744dda

Copy link
Contributor Author

karreiro commented Aug 13, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@karreiro karreiro force-pushed the theme-analytics branch 4 times, most recently from 543d986 to 6e1a174 Compare August 18, 2025 08:52
@karreiro karreiro marked this pull request as ready for review August 18, 2025 09:22
@karreiro karreiro requested review from a team as code owners August 18, 2025 09:22
Copy link
Contributor

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

Copy link
Contributor

@EvilGenius13 EvilGenius13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR and the solid tests to go with it @karreiro!

Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

packages/cli-kit/dist/public/node/themes/analytics.d.ts
/**
 * Records timing data for performance monitoring. Call twice with the same
 * event name to start and stop timing. First call starts the timer, second
 * call stops it and records the duration.
 *
 * @example
 * ```ts
 *   recordTiming('theme-upload') // Start timing
 *   // ... do work ...
 *   recordTiming('theme-upload') // Stop timing and record duration
 * ```
 *
 * @param eventName - Unique identifier for the timing event
 */
export declare function recordTiming(eventName: string): void;
/**
 * Records error information for debugging and monitoring. Use this to track
 * any exceptions or error conditions that occur during theme operations.
 * Errors are automatically categorized for easier analysis.
 *
 * @example
 * ```ts
 *   try {
 *     // ... risky operation ...
 *   } catch (error) {
 *     recordError(error)
 *   }
 * ```
 *
 * @param error - Error object or message to record
 */
export declare function recordError<T>(error: T): T;
/**
 * Records retry attempts for network operations. Use this to track when
 * operations are retried due to transient failures. Helps identify
 * problematic endpoints or operations that frequently fail.
 *
 * @example
 * ```ts
 *   recordRetry('https://api.shopify.com/themes', 'upload')
 * ```
 *
 * @param url - The URL or endpoint being retried
 * @param operation - Description of the operation being retried
 */
export declare function recordRetry(url: string, operation: string): void;
/**
 * Records custom events for tracking specific user actions or system events.
 * Use this for important milestones, user interactions, or significant
 * state changes in the application.
 *
 * @example
 * ```ts
 *   recordEvent('theme-dev-started')
 *   recordEvent('file-watcher-connected')
 * ```
 *
 * @param eventName - Descriptive name for the event
 */
export declare function recordEvent(eventName: string): void;
packages/cli-kit/dist/public/node/themes/analytics/bounded-collections.d.ts
/**
 * A bounded array that automatically maintains a maximum size by removing
 * the oldest entries when new items are added beyond the limit.
 *
 * Extends the native Array class to provide all standard array methods
 * while enforcing a fixed maximum size of MAX_ARRAY_SIZE (1000 entries).
 *
 * When the size limit is exceeded, the oldest entries (at the beginning
 * of the array) are automatically removed to make room for new ones.
 *
 * @example
 *   const commands = new BArray()
 *   commands.push(entry) // Automatically removes oldest if over 1000
 */
export declare class BArray<T> extends Array<T> {
    push(...items: T[]): number;
    clear(): void;
    toArray(): T[];
    private enforceLimit;
}
/**
 * A bounded map that automatically maintains a maximum number of keys by
 * removing the oldest entries when new keys are added beyond the limit.
 *
 * Extends the native Map class to provide all standard map methods while
 * enforcing a fixed maximum size of MAX_MAP_KEYS (1000 entries).
 *
 * Tracks insertion order to ensure the oldest keys are removed first when
 * the limit is exceeded. This provides LRU-like behavior based on insertion
 * time rather than access time.
 *
 * @example
 *   const events = new BMap()
 *   events.set('event', 1) // Automatically removes oldest if over 1000
 */
export declare class BMap<TKey, TValue> extends Map<TKey, TValue> {
    private insertionOrder;
    set(key: TKey, value: TValue): this;
    delete(key: TKey): boolean;
    clear(): void;
    toObject(): {
        [key: string]: TValue;
    };
    private enforceLimit;
}
packages/cli-kit/dist/public/node/themes/analytics/error-categorizer.d.ts
export declare enum ErrorCategory {
    ThemeCheck = "THEME_CHECK",
    Network = "NETWORK",
    FileSystem = "FILE_SYSTEM",
    Authentication = "AUTHENTICATION",
    Validation = "VALIDATION",
    Permission = "PERMISSION",
    RateLimit = "RATE_LIMIT",
    Parsing = "PARSING",
    Unknown = "UNKNOWN"
}
export declare function categorizeError(error: unknown): ErrorCategory;
packages/cli-kit/dist/public/node/themes/analytics/storage.d.ts
import { ErrorCategory } from './error-categorizer.js';
interface TimingEntry {
    event: string;
    duration: number;
}
interface ErrorEntry {
    category: ErrorCategory;
    message: string;
    timestamp: number;
}
interface RetryEntry {
    url: string;
    operation: string;
    attempts: number;
    timestamp: number;
}
interface EventEntry {
    name: string;
    timestamp: number;
}
export interface RuntimeData {
    timings: TimingEntry[];
    errors: ErrorEntry[];
    retries: RetryEntry[];
    events: EventEntry[];
}
export declare function recordTiming(eventName: string): void;
export declare function recordError(error: unknown): void;
export declare function recordRetry(url: string, operation: string): void;
export declare function recordEvent(eventName: string): void;
export declare function compileData(): RuntimeData;
export declare function reset(): void;
export {};

Existing type declarations

We found no diffs with existing type declarations

@karreiro karreiro enabled auto-merge August 21, 2025 08:57
@karreiro
Copy link
Contributor Author

👋 @Shopify/app-inner-loop could you please take a look at this one?

Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to block on this, but shouldn't you use the existing analytics component? If you need any additional feature, you could add it. But it feels weird to use a different system for themes, no?

@karreiro karreiro added this pull request to the merge queue Aug 21, 2025
Merged via the queue into main with commit c6dde65 Aug 21, 2025
47 of 52 checks passed
@karreiro karreiro deleted the theme-analytics branch August 21, 2025 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants