Feat/new update UI #595
Feat/new update UI #595
Conversation
|
Warning Rate limit exceeded@olgakup has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces a comprehensive updates tracking system for the Enkrypt browser extension. The changes include new metrics tracking for update events, a state management system for tracking version information, and a new user interface for displaying software updates. The implementation spans multiple files across the extension's source code, adding new types, components, and utility functions to support a robust update notification and tracking mechanism. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
💼 Build Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
packages/extension/src/libs/updates-state/types.ts (1)
5-9: Consider using more specific types and modern TypeScript conventions.A few suggestions for improvement:
- Consider using a more specific type for
currentReleaseTimestampsuch asUnixTimestamptype alias- Modern TypeScript style guides recommend dropping the 'I' prefix for interfaces
-export interface IState { +export interface State { lastVersionViewed: string; currentRelease: string; - currentReleaseTimestamp: number; + currentReleaseTimestamp: UnixTimestamp; } + +type UnixTimestamp = number;packages/extension/src/ui/action/types/updates.ts (1)
3-10: Follow TypeScript naming conventions and consider type restrictions.
- Property names should use camelCase instead of snake_case to follow TypeScript conventions
- Consider using a more specific type for
release_dateif it follows a specific formatexport interface Version { version: string; description?: string; chains_added?: NetworkNames[]; swap_added?: NetworkNames[]; - release_date: string; - release_link: string; + releaseDate: ISODateString; + releaseLink: string; } + +type ISODateString = string; // e.g. "2024-01-15T00:00:00Z"packages/extension/src/ui/action/views/updates/components/updates-header.vue (1)
21-53: Consider CSS improvements for maintainability.
- Consider extracting magic numbers into CSS variables
- The class prefix
settingsmight be too generic, consider something more specific likeupdates+:root { + --updates-header-height: 68px; + --updates-header-padding: 24px 72px 12px 32px; + --updates-close-position: 8px; +} + -.settings { +.updates { &__header { width: 100%; - height: 68px; + height: var(--updates-header-height); background: @white; box-sizing: border-box; - padding: 24px 72px 12px 32px; + padding: var(--updates-header-padding); margin-bottom: 8px; // ... rest of the stylespackages/extension/src/libs/metrics/types.ts (1)
56-59: Fix inconsistent string literal casing.The string literal for
logouses double quotes whilesettingsuses single quotes. Also, consider using more specific values to avoid potential conflicts.export enum UpdatesOpenLocation { - settings = 'settings', - logo = "logo", + settings = 'updates_settings', + logo = 'updates_logo', }packages/extension/src/ui/action/views/updates/components/updates-network.vue (2)
1-14: LGTM! Well-structured template with proper accessibility.The template follows good practices with conditional rendering, proper error handling, and BEM naming convention.
Consider providing a default alt text when props.alt is undefined:
- :alt="props.alt" + :alt="props.alt || `${networkName} network icon`"
40-47: Consider preserving the error stateThe error handler sets
isLoadedto true even when the network fails to load, which might mislead users. Consider setting it to false to indicate the failure state..catch((error: unknown) => { console.error( `Failed to load image in news for: ${props.networkId}`, error, ); - isLoaded.value = true; + isLoaded.value = false; resolvedImg.value = ''; });packages/extension/src/libs/metrics/index.ts (1)
80-87: Maintain code style consistencyThe new tracking function should follow the same style as other tracking functions in the file.
const trackUpdatesEvents = ( event: UpdatesEventType, options: { network: NetworkNames; location?: UpdatesOpenLocation; duration?: number; - }) => { + } + ): void => { metrics.track('updatesClick', { event, ...options }); - };packages/extension/src/libs/updates-state/index.ts (2)
38-38: Add missing semicolons for consistencyMaintain consistent use of semicolons across the codebase.
- return '' + return ''; - return 0 + return 0;Also applies to: 52-52
20-26: Simplify state checks with optional chainingThe state checks can be simplified using optional chaining, as suggested by the static analysis.
async getLastVersionViewed(): Promise<IState['lastVersionViewed']> { const state: IState | undefined = await this.getState(); - if (state && state.lastVersionViewed) { - return state.lastVersionViewed; - } - return ''; + return state?.lastVersionViewed ?? ''; } async getCurrentRelease(): Promise<IState['currentRelease']> { const state: IState | undefined = await this.getState(); - if (state && state.currentRelease) { - return state.currentRelease; - } - return ''; + return state?.currentRelease ?? ''; } async getCurrentReleaseTimestamp(): Promise<IState['currentReleaseTimestamp']> { const state: IState | undefined = await this.getState(); - if (state && state.currentReleaseTimestamp) { - return state.currentReleaseTimestamp; - } - return 0; + return state?.currentReleaseTimestamp ?? 0; }Also applies to: 33-39, 47-53
🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/extension/src/ui/action/views/updates/index.vue (2)
56-58: Update Twitter reference for consistency.The link text uses "x.com" but the comment still refers to "Twitter". Consider updating the comment to use "X" consistently.
- Make sure you follow Enkrypt on Twitter (X....ugh) and let us know + Make sure you follow Enkrypt on X and let us know
166-180: Remove duplicate height property.The
heightproperty is defined twice in theupdates__wrapclass. Remove the duplicate property for better maintainability.&__wrap { background: @white; box-shadow: 0px 3px 6px rgba(0, 0, 0, 0.039), 0px 7px 24px rgba(0, 0, 0, 0.19); border-radius: 12px; box-sizing: border-box; width: 460px; - height: auto; z-index: 107; position: relative; height: 568px; overflow-x: hidden; padding-bottom: 16px; }packages/extension/src/ui/action/App.vue (2)
261-263: Extract magic number into a named constant.The magic number
12096e5(2 weeks in milliseconds) should be extracted into a named constant for better readability and maintainability.+const TWO_WEEKS_IN_MS = 12096e5; + const getShowUpdatesBtn = async () => { try { const lastVersionViewed = await updatesState.getLastVersionViewed(); if ( lastVersionViewed === '' || (currentVersion && semverGT(currentVersion, lastVersionViewed)) ) { - const expireTimestamp = stateCurrentReleaseTimestamp.value + 12096e5; //2 weeks; + const expireTimestamp = stateCurrentReleaseTimestamp.value + TWO_WEEKS_IN_MS; showUpdatesBtn.value = stateCurrentReleaseTimestamp.value < expireTimestamp;
267-269: Improve error handling in getShowUpdatesBtn.The error is only logged to console. Consider setting a default state for
showUpdatesBtnin case of error.} catch (error) { console.error('Failed to get show updates button:', error); + showUpdatesBtn.value = false; }packages/extension/src/ui/action/utils/browser.ts (1)
69-69: Remove debug console.log statement.Remove the console.log statement before production deployment.
- console.log(fetchUrl)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/extension/src/libs/metrics/index.ts(3 hunks)packages/extension/src/libs/metrics/types.ts(1 hunks)packages/extension/src/libs/updates-state/index.ts(1 hunks)packages/extension/src/libs/updates-state/types.ts(1 hunks)packages/extension/src/types/provider.ts(1 hunks)packages/extension/src/ui/action/App.vue(11 hunks)packages/extension/src/ui/action/components/accounts-header/components/header-accounts.vue(1 hunks)packages/extension/src/ui/action/icons/updates/heart.vue(1 hunks)packages/extension/src/ui/action/icons/updates/updated.vue(1 hunks)packages/extension/src/ui/action/types/updates.ts(1 hunks)packages/extension/src/ui/action/utils/browser.ts(2 hunks)packages/extension/src/ui/action/views/updates/components/updates-header.vue(1 hunks)packages/extension/src/ui/action/views/updates/components/updates-network.vue(1 hunks)packages/extension/src/ui/action/views/updates/index.vue(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/extension/src/ui/action/icons/updates/heart.vue
- packages/extension/src/ui/action/components/accounts-header/components/header-accounts.vue
- packages/extension/src/ui/action/icons/updates/updated.vue
🧰 Additional context used
🪛 Biome (1.9.4)
packages/extension/src/libs/updates-state/index.ts
[error] 22-22: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
packages/extension/src/ui/action/views/updates/components/updates-network.vue (1)
54-73: LGTM! Well-organized styles.The styles follow BEM naming convention and use flexbox appropriately.
packages/extension/src/types/provider.ts (1)
55-55: LGTM! Consistent enum extension.The new enum value follows the existing naming pattern and is appropriately placed.
packages/extension/src/ui/action/views/updates/components/updates-header.vue
Outdated
Show resolved
Hide resolved
packages/extension/src/ui/action/views/updates/components/updates-network.vue
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| async getLastVersionViewed(): Promise<IState['lastVersionViewed']> { | ||
| const state: IState | undefined = await this.getState(); |
There was a problem hiding this comment.
const state: IState | null = await this.getState();
| return ''; | ||
| } | ||
| async setLastVersionViewed(lastVersionViewed: string): Promise<void> { | ||
| const state: IState | undefined = await this.getState(); |
There was a problem hiding this comment.
const state: IState | null = await this.getState();
packages/extension/src/ui/action/views/updates/components/updates-network.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/extension/src/ui/action/views/updates/index.vue (1)
Line range hint
257-273: Add error recovery mechanism in getShowUpdatesBtn.The error handling could provide a fallback behavior.
const getShowUpdatesBtn = async () => { try { const lastVersionViewed = await updatesState.getLastVersionViewed(); if ( lastVersionViewed === '' || (currentVersion && semverGT(currentVersion, lastVersionViewed)) ) { const expireTimestamp = stateCurrentReleaseTimestamp.value + 12096e5; showUpdatesBtn.value = stateCurrentReleaseTimestamp.value < expireTimestamp; } else { showUpdatesBtn.value = false; } } catch (error) { console.error('Failed to get show updates button:', error); + // Fallback to showing the button to ensure users don't miss updates + showUpdatesBtn.value = true; } };
🧹 Nitpick comments (7)
packages/extension/src/ui/action/views/updates/components/updates-network.vue (2)
40-47: Enhance error handling and user feedback.The error handling could be improved to provide better feedback and recovery options.
Consider this enhancement:
.catch((error: unknown) => { + const errorMessage = error instanceof Error ? error.message : String(error); console.error( - `Failed to load image in news for: ${props.networkId}`, + `Failed to load network data for ${props.networkId}: ${errorMessage}`, error, ); isLoaded.value = false; resolvedImg.value = ''; + networkName.value = props.networkId; // Fallback to network ID });
54-73: Consider using LESS variables for consistent styling.The styles could be more maintainable by using variables for common values.
Consider these improvements:
+// Add to theme.less +@network-icon-size: 24px; +@network-icon-shadow: 0px 0px 1px rgba(0, 0, 0, 0.16); +@network-spacing: 8px; .news_network { &__icon { - width: 24px; - height: 24px; + width: @network-icon-size; + height: @network-icon-size; background-color: white; border-radius: 50%; - box-shadow: inset 0px 0px 1px rgba(0, 0, 0, 0.16); - margin-right: 8px; + box-shadow: inset @network-icon-shadow; + margin-right: @network-spacing; } }packages/extension/src/libs/updates-state/index.ts (2)
5-10: Add class documentation for better maintainability.Consider adding JSDoc documentation to describe the class purpose and storage usage.
+/** + * Manages the state of application updates using browser storage. + * Handles version tracking, release information, and timestamps. + */ class UpdatesState { private storage: BrowserStorage; constructor() { this.storage = new BrowserStorage(InternalStorageNamespace.updatesState); }
16-27: Improve type safety in state management.The state retrieval could be more type-safe.
- async getState(): Promise<IState> { + async getState(): Promise<IState> { - const state = this.storage.get(StorageKeys.updatesInfo); + const state = await this.storage.get<IState>(StorageKeys.updatesInfo); if (!state) { const newState: IState = { lastVersionViewed: '', currentRelease: '', currentReleaseTimestamp: 0, } + await this.setState(newState); return newState } return state; }packages/extension/src/libs/metrics/index.ts (1)
80-87: Add input validation for tracking parameters.Consider validating the duration parameter to ensure it's a positive number.
const trackUpdatesEvents = (event: UpdatesEventType, options: { network: NetworkNames; location?: UpdatesOpenLocation; duration?: number; }): void => { + if (options.duration !== undefined && options.duration < 0) { + console.warn('Invalid duration provided to trackUpdatesEvents'); + options.duration = 0; + } metrics.track('updatesClick', { event, ...options }); };packages/extension/src/ui/action/views/updates/index.vue (1)
7-61: Consider decomposing the template into smaller components.The template contains repeated patterns that could be extracted into reusable components.
Consider creating these components:
UpdateVersionHeaderfor the version title and timestampUpdateFeatureSectionfor the chains/swap sectionsUpdateFooterfor the common footer contentpackages/extension/src/ui/action/App.vue (1)
264-267: Extract magic numbers into named constants.The timestamp calculation uses a magic number that should be extracted into a named constant.
+const TWO_WEEKS_IN_MS = 12096e5; + const getShowUpdatesBtn = async () => { try { const lastVersionViewed = await updatesState.getLastVersionViewed(); if ( lastVersionViewed === '' || (currentVersion && semverGT(currentVersion, lastVersionViewed)) ) { - const expireTimestamp = stateCurrentReleaseTimestamp.value + 12096e5; + const expireTimestamp = stateCurrentReleaseTimestamp.value + TWO_WEEKS_IN_MS; showUpdatesBtn.value = stateCurrentReleaseTimestamp.value < expireTimestamp;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/extension/src/libs/metrics/index.ts(3 hunks)packages/extension/src/libs/updates-state/index.ts(1 hunks)packages/extension/src/ui/action/App.vue(11 hunks)packages/extension/src/ui/action/utils/browser.ts(2 hunks)packages/extension/src/ui/action/views/updates/components/updates-header.vue(1 hunks)packages/extension/src/ui/action/views/updates/components/updates-network.vue(1 hunks)packages/extension/src/ui/action/views/updates/index.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension/src/ui/action/views/updates/components/updates-header.vue
- packages/extension/src/ui/action/utils/browser.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
packages/extension/src/ui/action/views/updates/components/updates-network.vue (1)
1-14: Well-structured template with proper error handling and accessibility!The template follows Vue.js best practices with proper conditional rendering, error handling, and accessibility attributes.
packages/extension/src/libs/metrics/index.ts (1)
13-14: Clean type imports following existing patterns!The new type imports are well-organized and consistent with the codebase structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/extension/src/ui/action/App.vue (3)
11-15: Consider adding a tooltip to the updated icon.The updated icon's purpose might not be immediately clear to users. Adding a tooltip would improve user experience.
<updated-icon v-if="loadedUpdates && showUpdatesBtn" @click="openUpdatesDialog(UpdatesOpenLocation.logo)" class="app__menu-updated" + title="New updates available!" />
264-264: Extract magic number to a constant.The 2-week expiration period (12096e5) should be extracted to a named constant for better maintainability and readability.
+const UPDATE_NOTIFICATION_EXPIRY_DAYS = 14; +const UPDATE_NOTIFICATION_EXPIRY_MS = UPDATE_NOTIFICATION_EXPIRY_DAYS * 24 * 60 * 60 * 1000; - const expireTimestamp = stateCurrentReleaseTimestamp.value + 12096e5; //2 weeks; + const expireTimestamp = stateCurrentReleaseTimestamp.value + UPDATE_NOTIFICATION_EXPIRY_MS;
737-746: Convert magic numbers to CSS variables.The brightness values and dimensions should be converted to CSS variables for consistency and maintainability.
+ :root { + --updated-icon-height: 24px; + --updated-icon-width: 90px; + --hover-brightness: 0.9; + --default-brightness: 1; + } &-updated { - height: 24px; - width: 90px; + height: var(--updated-icon-height); + width: var(--updated-icon-width); cursor: pointer; transition: 0.3s; - filter: brightness(1); + filter: brightness(var(--default-brightness)); &:hover { - filter: brightness(0.9); + filter: brightness(var(--hover-brightness)); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension/package.json(1 hunks)packages/extension/src/ui/action/App.vue(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
packages/extension/src/ui/action/App.vue (1)
281-284: Verify metrics implementation.Let's verify that the metrics tracking is properly implemented for update events.
✅ Verification successful
Metrics implementation verified successfully
The updates tracking is properly implemented with consistent event tracking for both opening and closing updates. The implementation follows the correct pattern using the defined UpdatesEventType enum and trackUpdatesEvents function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check metrics tracking implementation for updates # Check for other update event types that should be tracked rg -A 5 "UpdatesEventType\." --type ts # Verify metrics handler implementation ast-grep --pattern 'trackUpdatesEvents($_)'Length of output: 86
Script:
#!/bin/bash # Search for UpdatesEventType in all file types rg "UpdatesEventType" -t vue -t ts -t js # Look for tracking-related functions and enums rg "track.*Events.*update" -i -t vue -t ts -t js # Find enum or const definition related to updates rg "enum.*Updates|const.*Updates" -t vue -t ts -t jsLength of output: 230
Script:
#!/bin/bash # Search for UpdatesEventType without file type restriction rg "UpdatesEventType" # Look for tracking-related functions and enums rg "trackUpdatesEvents" # Find any Updates-related enums or types rg "type.*Updates|enum.*Updates|const.*Updates"Length of output: 2362
Summary by CodeRabbit
New Features
New Components
Improvements