feat(FRO-8): implement smart price target alerts frontend components …#11
feat(FRO-8): implement smart price target alerts frontend components …#11Anjaldev-vk wants to merge 2 commits into
Conversation
…and real-time audio notifications
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/styles/components/price-alerts.css (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeyframe names violate the project's kebab-case lint rule.
Stylelint flags
fadeIn,scaleUp, andslideLeftas errors underkeyframes-name-pattern.🧹 Proposed fix
-@keyframes fadeIn { +@keyframes fade-in { from { opacity: 0; } to { opacity: 1; } } -@keyframes scaleUp { +@keyframes scale-up { from { transform: scale(0.95); opacity: 0; } to { transform: scale(1); opacity: 1; } } -@keyframes slideLeft { +@keyframes slide-left { from { transform: translateX(100%); } to { transform: translateX(0); } }Update the corresponding
animation:references at lines 14, 30, and 43 to match the renamed keyframes.Also applies to: 30-30, 43-43, 498-536
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/styles/components/price-alerts.css` at line 14, Rename the keyframes fadeIn, scaleUp, and slideLeft to kebab-case names, and update the corresponding animation references in the price-alert styles so each animation uses its renamed keyframe consistently.Source: Linters/SAST tools
src/pages/LiveMarketDashboard.tsx (1)
82-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
fetchAlertsdependency in the WebSocket effect.Static analysis flags the
useEffect(empty dep array at line 141) for usingfetchAlertswithout listing it as a dependency. Zustand action references are typically stable so this is low-risk in practice, but it's a one-line fix to satisfy the rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/LiveMarketDashboard.tsx` around lines 82 - 141, Update the WebSocket useEffect dependency array to include fetchAlerts, while preserving the existing connection and cleanup behavior. Use the fetchAlerts symbol already referenced in the ws.onmessage handler.Source: Linters/SAST tools
src/components/PriceAlertModal.tsx (1)
62-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModal has no Escape-key close or focus trap.
Only the backdrop click and close button dismiss the modal; keyboard-only users can't close it with Escape, and focus isn't moved into/trapped within the dialog when it opens.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PriceAlertModal.tsx` around lines 62 - 79, Update the PriceAlertModal component to close when Escape is pressed and to move focus into the modal when it opens. Add a focus trap so Tab and Shift+Tab remain within the dialog’s interactive elements, while preserving backdrop and close-button dismissal behavior; use the existing modal content and close button symbols as the focus-management targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ActiveAlertsDrawer.tsx`:
- Around line 119-125: Update ActiveAlertsDrawer’s delete and dismiss button
handlers to catch rejected promises from the store’s removeAlert action,
preventing unhandled rejections and surfacing the failure. Destructure the store
error state and render it in the drawer body using the modal’s existing
error-banner pattern.
- Around line 68-76: Update the Create New Price Alarm button handler in
ActiveAlertsDrawer to close the drawer by setting isDrawerOpen to false before
or when invoking openModal(). Preserve the existing modal-opening behavior and
ensure only the modal overlay remains open after the click.
In `@src/components/PriceAlertModal.tsx`:
- Around line 48-52: Remove the unused err binding from the catch clause in the
addAlert flow of PriceAlertModal, leaving the existing store-managed error
handling and try/catch behavior unchanged.
In `@src/components/TradingViewChart.tsx`:
- Around line 92-95: Move the usePriceAlertStore hook call before the symbol
guard in TradingViewChart so it executes on every render. Keep the existing
early return for a missing symbol after all hooks have been invoked.
In `@src/pages/LiveMarketDashboard.tsx`:
- Around line 25-42: The playAlertChime function currently creates a new
AudioContext for every alert and never closes it. Add a lazily initialized
shared AudioContext for reuse across calls, resume it when its state is
suspended, and update playAlertChime to use that instance while preserving the
existing oscillator and error handling behavior.
- Around line 487-490: Remove the local PriceAlertModal and ActiveAlertsDrawer
mounts from LiveMarketDashboard, leaving their global mounts in App.tsx as the
sole instances. Do not alter the modal or drawer components or their shared
state behavior.
In `@src/stores/priceAlertStore.ts`:
- Around line 63-70: Update the fetchAlerts flow and the alert-mutating
operations around create/delete to track a monotonically increasing request
revision or token. Invalidate the active fetch revision whenever those
operations mutate alerts, and apply fetched data or loading/error updates only
when the response belongs to the current revision, preventing stale successes or
failures from overwriting newer state.
---
Nitpick comments:
In `@src/components/PriceAlertModal.tsx`:
- Around line 62-79: Update the PriceAlertModal component to close when Escape
is pressed and to move focus into the modal when it opens. Add a focus trap so
Tab and Shift+Tab remain within the dialog’s interactive elements, while
preserving backdrop and close-button dismissal behavior; use the existing modal
content and close button symbols as the focus-management targets.
In `@src/pages/LiveMarketDashboard.tsx`:
- Around line 82-141: Update the WebSocket useEffect dependency array to include
fetchAlerts, while preserving the existing connection and cleanup behavior. Use
the fetchAlerts symbol already referenced in the ws.onmessage handler.
In `@src/styles/components/price-alerts.css`:
- Line 14: Rename the keyframes fadeIn, scaleUp, and slideLeft to kebab-case
names, and update the corresponding animation references in the price-alert
styles so each animation uses its renamed keyframe consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e1312eda-a7b7-4a44-ad0f-2650653aca6f
📒 Files selected for processing (10)
src/App.tsxsrc/components/ActiveAlertsDrawer.tsxsrc/components/PortfolioView.tsxsrc/components/PriceAlertModal.tsxsrc/components/TradingViewChart.tsxsrc/features/dashboard/DashboardPage.tsxsrc/pages/LiveMarketDashboard.tsxsrc/services/priceAlert.service.tssrc/stores/priceAlertStore.tssrc/styles/components/price-alerts.css
| <button | ||
| className="vercel-btn-create-alert" | ||
| onClick={() => { | ||
| openModal(); | ||
| }} | ||
| > | ||
| <Plus size={14} strokeWidth={3} /> | ||
| <span>Create New Price Alarm</span> | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Opening the create-alert modal doesn't close the drawer.
openModal() here doesn't set isDrawerOpen to false, and the store's openModal action doesn't touch it either. Both the drawer and modal backdrops (position: fixed; inset: 0;, same z-index: 1100) can be open simultaneously, causing overlapping full-screen overlays.
🐛 Proposed fix
<button
className="vercel-btn-create-alert"
onClick={() => {
+ setDrawerOpen(false);
openModal();
}}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| className="vercel-btn-create-alert" | |
| onClick={() => { | |
| openModal(); | |
| }} | |
| > | |
| <Plus size={14} strokeWidth={3} /> | |
| <span>Create New Price Alarm</span> | |
| </button> | |
| <button | |
| className="vercel-btn-create-alert" | |
| onClick={() => { | |
| setDrawerOpen(false); | |
| openModal(); | |
| }} | |
| > | |
| <Plus size={14} strokeWidth={3} /> | |
| <span>Create New Price Alarm</span> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ActiveAlertsDrawer.tsx` around lines 68 - 76, Update the
Create New Price Alarm button handler in ActiveAlertsDrawer to close the drawer
by setting isDrawerOpen to false before or when invoking openModal(). Preserve
the existing modal-opening behavior and ensure only the modal overlay remains
open after the click.
| <button | ||
| className="vercel-alert-delete-btn" | ||
| onClick={() => removeAlert(alert.id)} | ||
| title="Delete Alert" | ||
| > | ||
| <Trash2 size={14} strokeWidth={2} /> | ||
| </button> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled rejection on delete/dismiss failure; no error surfaced.
removeAlert in the store rethrows the error after setting error in state, but neither call site here catches the promise, and this component never destructures/render's the store's error. A failed delete produces an unhandled promise rejection and silent failure from the user's perspective.
🐛 Proposed fix
- const {
- alerts,
- isDrawerOpen,
- setDrawerOpen,
- fetchAlerts,
- removeAlert,
- openModal,
- isLoading,
- } = usePriceAlertStore();
+ const {
+ alerts,
+ isDrawerOpen,
+ setDrawerOpen,
+ fetchAlerts,
+ removeAlert,
+ openModal,
+ isLoading,
+ error,
+ } = usePriceAlertStore(); <button
className="vercel-alert-delete-btn"
- onClick={() => removeAlert(alert.id)}
+ onClick={() => removeAlert(alert.id).catch(() => {})}
title="Delete Alert"(apply the same .catch to the triggered-list delete button, and render error in the drawer body similar to the modal's error banner.)
Also applies to: 152-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ActiveAlertsDrawer.tsx` around lines 119 - 125, Update
ActiveAlertsDrawer’s delete and dismiss button handlers to catch rejected
promises from the store’s removeAlert action, preventing unhandled rejections
and surfacing the failure. Destructure the store error state and render it in
the drawer body using the modal’s existing error-banner pattern.
| try { | ||
| await addAlert(cleanSymbol, priceNum, condition); | ||
| } catch (err: any) { | ||
| // Error handles in Zustand store state | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unused catch parameter.
Static analysis flags err as unused. Drop the binding since the store already handles the error state.
🧹 Proposed fix
try {
await addAlert(cleanSymbol, priceNum, condition);
- } catch (err: any) {
+ } catch {
// Error handles in Zustand store state
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await addAlert(cleanSymbol, priceNum, condition); | |
| } catch (err: any) { | |
| // Error handles in Zustand store state | |
| } | |
| try { | |
| await addAlert(cleanSymbol, priceNum, condition); | |
| } catch { | |
| // Error handles in Zustand store state | |
| } |
🧰 Tools
🪛 GitHub Check: Type-check · Lint · Format · Test · Build
[warning] 50-50: eslint(no-unused-vars)
Catch parameter 'err' is caught but never used.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/PriceAlertModal.tsx` around lines 48 - 52, Remove the unused
err binding from the catch clause in the addAlert flow of PriceAlertModal,
leaving the existing store-managed error handling and try/catch behavior
unchanged.
Source: Linters/SAST tools
| const playAlertChime = () => { | ||
| try { | ||
| const audioCtx = new (window.AudioContext || (window as any).webkitAudioContext)(); | ||
| const osc = audioCtx.createOscillator(); | ||
| const gain = audioCtx.createGain(); | ||
| osc.type = 'sine'; | ||
| osc.frequency.setValueAtTime(880, audioCtx.currentTime); | ||
| osc.frequency.exponentialRampToValueAtTime(1760, audioCtx.currentTime + 0.3); | ||
| gain.gain.setValueAtTime(0.15, audioCtx.currentTime); | ||
| gain.gain.exponentialRampToValueAtTime(0.001, audioCtx.currentTime + 0.35); | ||
| osc.connect(gain); | ||
| gain.connect(audioCtx.destination); | ||
| osc.start(); | ||
| osc.stop(audioCtx.currentTime + 0.35); | ||
| } catch (e) { | ||
| console.error('Audio chime error:', e); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)LiveMarketDashboard\.tsx$|src/pages|package.json|tsconfig' || true
echo "== target outline =="
ast-grep outline src/pages/LiveMarketDashboard.tsx --view expanded || true
echo "== target relevant lines =="
cat -n src/pages/LiveMarketDashboard.tsx | sed -n '1,130p;475,500p'
echo "== usages of playAlertChime / useAudioContext/WebSocket handler =="
rg -n "playAlertChime|AudioContext|webkitAudioContext|onmessage|fetchAlerts|useCallback" src/pages/LiveMarketDashboard.tsx .Repository: TradeDisciplineAI/frontned
Length of output: 8228
🌐 Web query:
MDN AudioContext close releases system audio resources browser autoplay policy AudioContext created not user gesture suspended resume
💡 Result:
The AudioContext.close method is designed to release system audio resources, such as threads, processes, and audio streams, that are used by the context [1][2]. By doing so, it stops the processing of audio data and suspends the progression of audio time [1]. Importantly, calling close does not automatically release all JavaScript objects (nodes, buffers, etc.) created within that context; these remain in memory until their references are dropped and they are garbage collected [1][3]. Once an AudioContext is closed, it cannot be resumed [4][2]. Regarding browser autoplay policies, an AudioContext created without a user gesture is typically initialized in a suspended state [5][6]. To begin audio playback, the context must be resumed [5]. This can be achieved by calling the resume method or by triggering a start call on an attached source node, usually within a handler for a user-initiated event (such as a click or keypress) [5][7]. While browser implementations may automatically suspend contexts due to autoplay restrictions, developers should use the AudioContext.state property to monitor whether the context is running or suspended and utilize the statechange event to respond to these changes asynchronously [5]. If a developer intentionally calls suspend, it is generally expected that the system will honor that state until an explicit resume is called by the application, though interaction between automatic and user-invoked suspension remains a complex area of browser-specific implementation [6].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/close
- 2: Section 2.1.7 unclear about resuming a closed context WebAudio/web-audio-api#1100
- 3: https://googlechrome.github.io/samples/webaudio-audiocontext-close/
- 4: https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/resume
- 5: https://developer.chrome.com/blog/autoplay
- 6: Clarify interaction between user-invoked suspend and autoplay policy WebAudio/web-audio-api#1802
- 7: https://developer.mozilla.org/en-US/docs/Web/Media/Guides/Autoplay
Avoid creating a new closed-less AudioContext for every alert chime.
Each call creates an AudioContext that is never closed, and the chime can trigger repeatedly through the WebSocket message handler. Reuse a single lazy AudioContext (resume it when state === 'suspended') instead of constructing a fresh one per alert.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/LiveMarketDashboard.tsx` around lines 25 - 42, The playAlertChime
function currently creates a new AudioContext for every alert and never closes
it. Add a lazily initialized shared AudioContext for reuse across calls, resume
it when its state is suspended, and update playAlertChime to use that instance
while preserving the existing oscillator and error handling behavior.
|
|
||
| {/* Price Alert Modal & Active Alerts Drawer */} | ||
| <PriceAlertModal /> | ||
| <ActiveAlertsDrawer /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Duplicate PriceAlertModal/ActiveAlertsDrawer mount — conflicts with the global mount in App.tsx.
App.tsx already mounts <PriceAlertModal /> and <ActiveAlertsDrawer /> globally (outside <Routes>), and this component is rendered at ROUTES.EXPLORE. Rendering both here again means two live instances of each component exist simultaneously on this route: two modal/drawer backdrops sharing the same store state, and — since each ActiveAlertsDrawer has its own useEffect(() => { if (isDrawerOpen) fetchAlerts(); }, [isDrawerOpen]) — opening the drawer fires fetchAlerts() twice.
🐛 Proposed fix
- {/* Price Alert Modal & Active Alerts Drawer */}
- <PriceAlertModal />
- <ActiveAlertsDrawer />
</div>
);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {/* Price Alert Modal & Active Alerts Drawer */} | |
| <PriceAlertModal /> | |
| <ActiveAlertsDrawer /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/LiveMarketDashboard.tsx` around lines 487 - 490, Remove the local
PriceAlertModal and ActiveAlertsDrawer mounts from LiveMarketDashboard, leaving
their global mounts in App.tsx as the sole instances. Do not alter the modal or
drawer components or their shared state behavior.
| fetchAlerts: async () => { | ||
| set({ isLoading: true, error: null }); | ||
| try { | ||
| const data = await priceAlertService.getUserAlerts(); | ||
| set({ alerts: data.items, isLoading: false }); | ||
| } catch (err: any) { | ||
| const msg = err.response?.data?.detail || 'Failed to fetch price alerts.'; | ||
| set({ error: msg, isLoading: false }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent stale requests from reverting alert state.
A GET started at Line 66 can resolve after a later fetch, create, or delete. Line 67 then replaces newer local state—hiding a new/triggered alert or resurrecting a deleted one—and the stale failure path can also overwrite current loading/error state. Track a list request revision/token, ignore obsolete responses, and invalidate pending fetches when Lines 82-86 or Lines 101-103 mutate alerts.
Also applies to: 82-86, 98-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/priceAlertStore.ts` around lines 63 - 70, Update the fetchAlerts
flow and the alert-mutating operations around create/delete to track a
monotonically increasing request revision or token. Invalidate the active fetch
revision whenever those operations mutate alerts, and apply fetched data or
loading/error updates only when the response belongs to the current revision,
preventing stale successes or failures from overwriting newer state.
1 participant
…and real-time audio notifications
Summary by CodeRabbit
New Features
Style