feat(cli): Store `metrics_enabled` key in profile config

Souvik9205 commented 1 week ago

User description

Description

index.types.d.ts: Added metrics_enabled boolean to the ProfileConfig.
create.profile.ts: Introduced -m, --enable-metrics for enabling/disabling metrics collection.
update.profile.ts: Added the flag to update the metrics collection status.
list.profile.ts: Displayed Metrics Enabled column in the profile table
base.command.ts: Added metricsEnabled to store and manage the metrics setting.

Fixes #528

Developer's checklist

[✅] My PR follows the style guidelines of this project
[❌] I have performed a self-check on my work

PR Type

Enhancement

Description

Added a metrics_enabled key to the ProfileConfig interface to support metrics collection.
Introduced --enable-metrics flag in create.profile.ts for enabling/disabling metrics during profile creation.
Updated update.profile.ts to allow modification of the metrics setting for existing profiles.
Enhanced list.profile.ts to display the metrics enabled status in the profile list.
Modified base.command.ts to store and manage the metrics setting within the command base class.

Changes walkthrough 📝

Relevant files

Enhancement

base.command.ts `Add metricsEnabled property to BaseCommand class` apps/cli/src/commands/base.command.ts Added `metricsEnabled` property to the `BaseCommand` class. Updated logic to initialize `metricsEnabled` from profile configuration.	+3/-0
create.profile.ts `Add enable-metrics flag to profile creation` apps/cli/src/commands/profile/create.profile.ts Introduced `--enable-metrics` flag for profile creation. Updated input parsing to include metrics option. Modified profile configuration to store metrics setting.	+30/-7
list.profile.ts `Display metrics enabled status in profile list` apps/cli/src/commands/profile/list.profile.ts - Added `Metrics Enabled` column to profile listing.	+3/-2
update.profile.ts `Add enable-metrics flag to profile update` apps/cli/src/commands/profile/update.profile.ts Added `--enable-metrics` flag for updating profiles. Updated profile data to handle metrics setting.	+14/-3
index.types.d.ts `Update ProfileConfig interface with metrics_enabled` apps/cli/src/types/index.types.d.ts - Added `metrics_enabled` boolean to `ProfileConfig` interface.	+1/-0

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

codiumai-pr-agent-free[bot] commented 1 week ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

**🎫 Ticket compliance analysis ✅** **[528](https://github.com/keyshade-xyz/keyshade/issues/528) - Fully compliant** Compliant requirements: - Added metrics_enabled to ProfileConfig interface - Added -m flag with prompt to create.profile.ts - Added metrics option to update.profile.ts - Added metrics column to list.profile.ts - Added metricsEnabled storage in base.command.ts

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Logic Error
The condition for checking enableMetrics is incorrect. '!enableMetrics === undefined' is a confusing double negative that may not work as intended. Type Mismatch
The setProfileConfigData function is called with setDefault parameter but the function signature was missing it in the old code. This could cause type errors. Incomplete Validation
The enableMetrics parameter is used without validation. Should validate boolean type before using.

codiumai-pr-agent-free[bot] commented 1 week ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Fix incorrect logical condition that prevents the metrics confirmation prompt from being shown ___ Fix the logical error in the metrics check condition. The current condition `!enableMetrics === undefined` is always false. It should be `enableMetrics ===` `undefined`. [apps/cli/src/commands/profile/create.profile.ts [98-102]](https://github.com/keyshade-xyz/keyshade/pull/536/files#diff-71f3f4ea9fcc92ffe9e871082369710fad3f7e2120141ffa595a07d0add4c329R98-R102) ```diff -if (!enableMetrics === undefined) { +if (enableMetrics === undefined) { enableMetrics = await confirm({ message: 'Should keyshade collect anonymous metrics for development?' }) } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 9 Why: The current condition `!enableMetrics === undefined` is a critical logical error that would prevent the metrics prompt from ever showing. This bug would effectively break the new metrics feature functionality.	9
Best practice	Make new configuration field optional to maintain backward compatibility ___ Make the metrics_enabled property optional in the ProfileConfig interface since it's a new field being added and may not exist in existing profiles. [apps/cli/src/types/index.types.d.ts [10-14]](https://github.com/keyshade-xyz/keyshade/pull/536/files#diff-e98c0a3c91ecf88e57bf648844ce8e40d14cfbc93779cdbc61d279c8f609b880R10-R14) ```diff [name: string]: { apiKey: string baseUrl: string - metrics_enabled: boolean + metrics_enabled?: boolean } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: Making the metrics_enabled field optional is crucial for backward compatibility with existing profiles that don't have this field, preventing potential runtime errors when reading older configurations.	8
Possible issue	Add input validation to ensure the metrics flag receives a valid boolean value ___ Add validation for the enableMetrics boolean input parameter to ensure it's a valid boolean value before updating the profile. [apps/cli/src/commands/profile/update.profile.ts [91-93]](https://github.com/keyshade-xyz/keyshade/pull/536/files#diff-1022d0c11dea57a922404e0431dd9410310900120338d22d8a4ebaffb0889f88R91-R93) ```diff if (enableMetrics !== undefined) { - this.profiles[profile].metrics_enabled = enableMetrics + const metricsValue = String(enableMetrics).toLowerCase(); + if (metricsValue !== 'true' && metricsValue !== 'false') { + throw new Error('Enable metrics must be a boolean value (true/false)'); + } + this.profiles[profile].metrics_enabled = metricsValue === 'true'; } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: Adding validation for the metrics flag input would prevent potential runtime errors and improve user experience by providing clear error messages for invalid inputs.	7

💡 Need additional feedback ? start a PR chat

keyshade-xyz / keyshade