Add session timer functionality #2
Conversation
WalkthroughThe pull request introduces a new session timer functionality across multiple components and stores. A new Pinia store Changes
Sequence DiagramsequenceDiagram
participant User
participant VoiceStore
participant SessionTimerStore
participant Channel
User->>VoiceStore: Initiate Channel Connection
VoiceStore->>SessionTimerStore: stopTimer()
VoiceStore->>Channel: Connect
alt Connection Successful
VoiceStore->>SessionTimerStore: startTimer()
SessionTimerStore->>SessionTimerStore: Update Session Time
else Connection Failed
VoiceStore->>SessionTimerStore: stopTimer()
end
Poem
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
|
All contributors have signed the CLA ✍️ ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tailwind.config.js (1)
93-95: LGTM! Consider adding a comment for maintainability.The font size configuration is well-structured and follows Tailwind's conventions. The size (12px) and line height (15px) are appropriate for a timer display.
Consider adding a brief comment to document the purpose:
fontSize: { + // Custom size for session timer display in UserBar component 'timer': ['12px', '15px'], }src/store/sessionTimer.ts (1)
18-20: Consider adding days to the session time display.The
sessionTimecomputed property only shows hours:minutes:seconds. For long sessions, consider including days in the display format.const sessionTime = computed(() => - `${pad(hoursSession.value % 24)}:${pad(minutesSession.value % 60)}:${pad(secondsSession.value % 60)}` + daysSession.value > 0 + ? `${daysSession.value}d ${pad(hoursSession.value % 24)}:${pad(minutesSession.value % 60)}:${pad(secondsSession.value % 60)}` + : `${pad(hoursSession.value % 24)}:${pad(minutesSession.value % 60)}:${pad(secondsSession.value % 60)}` )src/components/UserBar.vue (1)
52-52: Enhance timer display accessibility and UX.The timer display could benefit from:
- ARIA label for screen readers
- Tooltip explaining the session duration
- <span class="text-timer text-[#a2a6a8]">{{ sessionTimerStore.sessionTime }}</span> + <TooltipProvider> + <Tooltip> + <TooltipTrigger asChild> + <span + class="text-timer text-[#a2a6a8]" + aria-label="Session duration" + >{{ sessionTimerStore.sessionTime }}</span> + </TooltipTrigger> + <TooltipContent> + <p>Current session duration</p> + </TooltipContent> + </Tooltip> + </TooltipProvider>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (5)
src/components/ChatList.vue(3 hunks)src/components/UserBar.vue(2 hunks)src/store/sessionTimer.ts(1 hunks)src/store/voiceStore.ts(4 hunks)tailwind.config.js(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/store/sessionTimer.ts (3)
10-10: Add type annotation for the interval ref.The
intervalref lacks type annotation which could lead to type safety issues.- const interval = ref() + const interval = ref<number>()
14-16: Add input validation for session date updates.The
updateSessionDatefunction should validate the input date to prevent invalid dates from being set.function updateSessionDate(date: Date): void { + if (!(date instanceof Date) || isNaN(date.getTime())) { + throw new Error('Invalid date provided to updateSessionDate') + } sessionStartDate.value = date }
58-63: Consider exporting updateSessionDate for testing purposes.The
updateSessionDatefunction might be useful for testing scenarios to set up specific test cases.return { updateSessionTimer, sessionTime, startTimer, - stopTimer + stopTimer, + updateSessionDate };src/store/voiceStore.ts (2)
137-141: Consider propagating timer start errors.While the error is logged, failing to start the session timer might indicate a more serious issue that should affect the connection state.
try { sessionTimerStore.startTimer() } catch (error) { logger.error('Failed to start session timer:', error) + currentState.value = "NONE" + activeChannel.value = null + pool.selectedChannel = null + return }
168-172: Ensure cleanup continues after timer stop failure.While the error is logged, the function should continue with the remaining cleanup tasks even if stopping the timer fails.
try { sessionTimerStore.stopTimer() } catch (error) { logger.error('Failed to stop session timer:', error) + // Continue with cleanup despite timer stop failure }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/ChatList.vue(3 hunks)src/components/UserBar.vue(2 hunks)src/store/sessionTimer.ts(1 hunks)src/store/voiceStore.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ChatList.vue
- src/components/UserBar.vue
🧰 Additional context used
🪛 Biome (1.9.4)
src/store/sessionTimer.ts
[error] 24-24: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (3)
src/store/sessionTimer.ts (2)
18-20: LGTM! Well-formatted time display.The computed property correctly formats the time components with proper bounds.
40-56: LGTM! Proper cleanup implemented.The timer control functions handle cleanup and state reset appropriately.
src/store/voiceStore.ts (1)
20-28: LGTM! Clean integration of session timer.The session timer store is properly imported and initialized.
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/store/sessionTimer.ts (1)
12-29: Consider performance optimizations for timer updates.While the logic is correct, consider these optimizations:
- Cache the time calculations to avoid recalculating values
- Use template literals only when the format changes
function updateSessionTimer(): void { const now = new Date(); if (!(sessionStartDate.value instanceof Date) || Number.isNaN(sessionStartDate.value.getTime())) { stopTimer(); return; } const milliseconds = now.getTime() - sessionStartDate.value.getTime(); const seconds = Math.floor(milliseconds / 1000); const minutes = Math.floor(seconds / 60); const hours = Math.floor(minutes / 60); const days = Math.floor(hours / 24); + const formattedHours = pad(hours % 24); + const formattedMinutes = pad(minutes % 60); + const formattedSeconds = pad(seconds % 60); if(days < 1) { - sessionTimer.value = `${pad(hours % 24)}:${pad(minutes % 60)}:${pad(seconds % 60)}`; + sessionTimer.value = `${formattedHours}:${formattedMinutes}:${formattedSeconds}`; } else { - sessionTimer.value = `${days}:${pad(hours % 24)}:${pad(minutes % 60)}:${pad(seconds % 60)}`; + sessionTimer.value = `${days}:${formattedHours}:${formattedMinutes}:${formattedSeconds}`; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/ChatList.vue(1 hunks)src/components/UserBar.vue(2 hunks)src/store/sessionTimer.ts(1 hunks)src/store/voiceStore.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ChatList.vue
🔇 Additional comments (7)
src/store/sessionTimer.ts (2)
1-9: LGTM! Good choice of technologies.The combination of Pinia for state management and RxJS for interval handling is a solid choice for implementing timer functionality.
31-45: LGTM! Clean implementation of timer controls.The timer control functions properly manage subscriptions and state cleanup.
src/components/UserBar.vue (2)
52-52: LGTM! Clean integration of the timer display.The timer is properly integrated with appropriate conditional rendering and styling.
69-69: LGTM! Clean store integration.The session timer store is properly imported and instantiated following Vue Composition API best practices.
Also applies to: 74-74
src/store/voiceStore.ts (3)
20-20: LGTM! Clean store integration.The session timer store is properly imported and instantiated following Pinia best practices.
Also applies to: 27-27
63-63: LGTM! Proper timer control during connection.The timer is correctly stopped before connection attempt and started after successful connection.
Also applies to: 137-137
164-164: LGTM! Proper timer cleanup during disconnection.The timer is correctly stopped during channel disconnection, with appropriate error protection.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates