fix: bot offline

[mayuran-deriv]

No description provided.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

[github-actions]

Claude finished @mayuran-deriv's task in 2m 52s —— View job

---

Code Review Update ✅

---

Summary

This PR has been updated since the last review and the critical notification timing issue has been fixed! The notification now displays only after all validation checks pass, right before the bot starts running.

Three commits in this PR:

1. 5f6d468 - Initial bot offline improvements (notification, routing, messaging)
2. a3cab02 - Capital letter fix
3. 4ce6008 - Fixed notification timing (addressed critical review feedback)

---

What Changed Since Last Review

✅ FIXED: Notification timing issue (previously critical)

• The keep_screen_alive notification moved from line 174 (before validation) to line 231 (after validation, before bot starts)
• Now users only see the notification when the bot actually starts running
• Great response to feedback!

---

Current State Analysis

🟡 Medium Priority Issues

1. URL parsing on every render - performance concern (src/components/bot-stopped.tsx:24-26)

const urlParams = new URLSearchParams(window.location.search);
const account_param = urlParams.get('account');
const is_virtual = client?.is_virtual || account_param === 'demo' || false;

Issue: Component is wrapped in observer(), so it re-renders when MobX observables change. URL parsing happens on every render even though window.location.search rarely changes.

Impact: Minor performance overhead from repeated URL parsing.

Recommendation: Memoize URL parsing to avoid unnecessary work:

const is_virtual = useMemo(() => {
    const urlParams = new URLSearchParams(window.location.search);
    const account_param = urlParams.get('account');
    return client?.is_virtual || account_param === 'demo';
}, [client?.is_virtual]); // Only re-parse when client.is_virtual changes

Note: window.location.search changes don't trigger React re-renders, so we rely on client?.is_virtual as the dependency. If account switching updates the URL without updating client.is_virtual, this could be an issue.

Fix this →

---

2. Missing fallback for real accounts without currency (src/components/bot-stopped.tsx:55-58)

} else if (currency) {
    // For real accounts, set the account parameter to the currency
    url.searchParams.set('account', currency);
}

Issue: If currency is falsy (null, undefined, empty string) for a real account, the account parameter won't be set.

Impact: Real accounts without currency might not route correctly to reports page.

Recommendation: Add defensive handling:

} else if (currency) {
    url.searchParams.set('account', currency);
} else {
    // Real account should have currency - log warning for debugging
    console.warn('[BotStopped] Real account missing currency, reports may not load correctly');
}

Fix this →

---

🟢 Low Priority / Code Quality

3. Redundant boolean coercion (src/components/bot-stopped.tsx:26)

const is_virtual = client?.is_virtual || account_param === 'demo' || false;

Issue: The || false is unnecessary since the expression already evaluates to boolean.

Fix:

const is_virtual = client?.is_virtual || account_param === 'demo';

Fix this →

---

4. Unnecessary curly braces for static string (src/components/bot-stopped.tsx:43)

className={'dc-dialog bot-stopped-dialog'}

Fix:

className="dc-dialog bot-stopped-dialog"

Fix this →

---

🔒 Security Review

✅ No security issues detected

• Currency and account parameters properly encoded via URL API
• No direct DOM manipulation or XSS vulnerabilities
• All user-facing content uses Localize components (i18n safe)
• Navigation using window.location.href with properly constructed URL objects
• No open redirect vulnerabilities (standalone_routes is internal constant)

---

🧪 Test Coverage

⚠️ No test files found for:

• src/components/bot-stopped.tsx
• src/stores/run-panel-store.ts
• src/components/bot-notification/bot-notification-utils.ts

Recommended test scenarios:

1. BotStopped Component:

   describe('BotStopped', () => {
     it('shows internet disconnection message when offline and bot running', () => {});
     it('shows connection interrupted message for internal issues', () => {});
     it('constructs demo account URL with account=demo', () => {});
     it('constructs real account URL with account=currency', () => {});
     it('handles real account without currency gracefully', () => {});
     it('reloads page on close button click', () => {});
     it('navigates to reports on View Report button', () => {});
   });

2. RunPanelStore:

   describe('RunPanelStore.onRunButtonClick', () => {
     it('shows notification only after validation passes', () => {});
     it('does not show notification when user not logged in', () => {});
     it('does not show notification when self-exclusion blocks bot', () => {});
     it('does not show notification when strategy validation fails', () => {});
   });

3. URL Construction Edge Cases:

   • Virtual account → ?account=demo
   • Real account with USD → ?account=USD
   • Real account without currency → no param or warning

---

📈 Performance Review

Good practices:

• ✅ Dialog component lazy loading (implicit via Dialog component)
• ✅ MobX observer() wrapper for reactive updates
• ✅ Minimal re-render triggers

Could improve:

• 🟡 URL parsing on every render (see Medium Priority Ako/ remove cert file in vite conf #1)
• Consider if useNavigatorOnline() hook is optimized (didn't review implementation)

---

✨ What's Good

1. Critical issue fixed! - Notification timing now correct (line 231 after validation)
2. Improved UX messaging - "Connection Interrupted" clearer than "Platform temporarily unavailable"
3. Account-aware routing - Correctly handles demo vs real accounts with currency parameter
4. Proper URL handling - Using URL API instead of string concatenation
5. Accessibility - Good keyboard support with onKeyDown handler
6. MobX patterns - Correctly using observer() for reactive updates
7. Code readability - Well-commented, clear variable names
8. Proactive notification - Good UX to remind users to keep screen active

---

📋 Final Recommendations

Before merging (Optional improvements):

1. 🟡 Consider memoizing URL parsing for performance
2. 🟡 Add fallback/logging for real accounts without currency
3. 🟢 Clean up minor code style issues (redundant || false, unnecessary braces)

Future improvements:

1. Add test coverage for bot-stopped component and notification timing
2. Consider integration tests for account switching flows
3. Document the account parameter routing behavior

---

🎯 Overall Assessment

Excellent work addressing the critical review feedback! The notification timing fix is exactly what was needed. The PR improves offline handling with:

• ✅ Better UX with proactive screen-alive notification (now correctly timed)
• ✅ Clearer messaging for connection issues
• ✅ Smart account-aware routing to reports

The remaining issues are minor (performance optimization, defensive code, style). The core functionality is solid and the critical bug has been fixed.

Status: ✅ Ready to merge (with optional improvements to consider)

The critical issue from the previous review has been resolved. Remaining suggestions are performance optimizations and code quality improvements that can be addressed in follow-up PRs if desired.

---

[github-actions]

Claude finished @mayuran-deriv's task in 2m 34s —— View job

---

Code Review Complete ✅

---

Summary

This PR improves the bot offline handling with three key changes:

1. New notification (src/components/bot-notification/bot-notification-utils.ts:49): Added "keep_screen_alive" notification
2. Enhanced bot stopped dialog (src/components/bot-stopped.tsx): Better messaging and account-aware routing to reports
3. Notification timing fix (src/stores/run-panel-store.ts:231): Shows notification after all validations pass

Overall, this is a solid improvement that enhances user experience during offline/connection issues. The code is well-structured and the previous critical issue (notification timing) has been addressed.

---

🔴 High Priority Issues

None identified - No blocking or critical issues found.

---

🟡 Medium Priority Issues

1. URL parsing on every render - Performance concern

Location: src/components/bot-stopped.tsx:24-26

const urlParams = new URLSearchParams(window.location.search);
const account_param = urlParams.get('account');
const is_virtual = client?.is_virtual || account_param === 'demo' || false;

Issue: The component is wrapped in observer(), so it re-renders whenever MobX observables change. URL parsing happens on every render even though window.location.search rarely changes.

Impact: Minor performance overhead from repeated URLSearchParams instantiation and parsing.

Recommendation: Use useMemo to memoize the URL parsing:

import { useMemo } from 'react';

const is_virtual = useMemo(() => {
    const urlParams = new URLSearchParams(window.location.search);
    const account_param = urlParams.get('account');
    return client?.is_virtual || account_param === 'demo';
}, [client?.is_virtual]);

Fix this →

---

2. Missing error handling for accounts without currency

Location: src/components/bot-stopped.tsx:55-58

} else if (currency) {
    // For real accounts, set the account parameter to the currency
    url.searchParams.set('account', currency);
}

Issue: If getCurrency() returns an empty string (falsy) for a real account, the account parameter won't be set. Looking at the client store implementation (client-store.ts:270-273), getCurrency() returns '' when currency is not found, which is falsy.

Impact: Real accounts without currency will navigate to reports without an account parameter, potentially showing incorrect or no data.

Recommendation: Add defensive logging to track this edge case:

} else if (currency) {
    url.searchParams.set('account', currency);
} else {
    // Real account without currency - this shouldn't happen but log for debugging
    console.warn('[BotStopped] Real account missing currency, reports may not load correctly');
}

Fix this →

---

3. Potential stale closure with URL parameter check

Location: src/components/bot-stopped.tsx:24-26

Issue: The account parameter in the URL is read once when the component renders, but if the user switches accounts (which updates the URL), the component may not re-render because client?.is_virtual might not change if switching between two demo accounts or two real accounts.

Impact: The is_virtual check might use stale URL data if account switching doesn't trigger a re-render.

Context: Looking at the code pattern, it seems account switches likely cause full page reloads or proper re-renders, but this isn't guaranteed from this code alone.

Recommendation: Consider whether window.location.search changes should trigger re-computation. If account switches always cause component remount, this is fine. Otherwise, consider adding the URL search params to the useMemo dependency array (though this is tricky since window.location.search changes don't trigger React re-renders).

Alternative approach: Rely solely on client?.is_virtual without the URL fallback, or ensure the URL parameter is only a backup for initial load before client data is available.

---

🟢 Low Priority / Code Quality

4. Redundant boolean coercion

Location: src/components/bot-stopped.tsx:26

const is_virtual = client?.is_virtual || account_param === 'demo' || false;

Issue: The || false is unnecessary since the expression already evaluates to a boolean.

Fix:

const is_virtual = client?.is_virtual || account_param === 'demo';

Fix this →

---

5. Unnecessary curly braces for static string

Location: src/components/bot-stopped.tsx:43

className={'dc-dialog bot-stopped-dialog'}

Fix:

className="dc-dialog bot-stopped-dialog"

Fix this →

---

6. Redundant variable assignment

Location: src/components/bot-stopped.tsx:19-20

const getCurrency = client?.getCurrency;
const currency = getCurrency?.();

Issue: The intermediate getCurrency variable is unnecessary.

Better approach:

const currency = client?.getCurrency();

Fix this →

---

7. Inconsistent message capitalization

Location: src/components/bot-stopped.tsx:92

Observation: The new message "Your connection to the server was lost, all trades have been settled." uses lowercase "all" after a comma. While grammatically acceptable, consider:

<Localize i18n_default_text='Your connection to the server was lost. All trades have been settled. Check the Reports page for final results.' />

This makes each sentence independent and clearer. (This is subjective, so low priority.)

---

🔒 Security Review

✅ No security issues detected

• ✅ Currency and account parameters properly encoded via URL API (no XSS risk)
• ✅ No direct DOM manipulation
• ✅ All user-facing content uses Localize components (i18n safe)
• ✅ Navigation using window.location.href with properly constructed URL objects
• ✅ No open redirect vulnerabilities (standalone_routes.positions points to internal Deriv domain)
• ✅ Account data sourced from authenticated client store (MobX observable)
• ✅ No sensitive data exposed in URL parameters (currency and account type are non-sensitive)

---

🧪 Test Coverage

⚠️ No test files found for:

• src/components/bot-stopped.tsx
• src/stores/run-panel-store.ts
• src/components/bot-notification/bot-notification-utils.ts

Recommended test scenarios:

BotStopped Component Tests

describe('BotStopped', () => {
  it('shows offline message when internet disconnected and bot running', () => {});
  it('shows connection interrupted message for internal websocket issues', () => {});
  it('constructs demo account URL with account=demo parameter', () => {});
  it('constructs real account URL with account=currency parameter', () => {});
  it('handles real account without currency gracefully', () => {});
  it('reloads page on close button click', () => {});
  it('navigates to reports page on View Report button', () => {});
  it('does not show dialog when both online and websocket initialized', () => {});
  it('handles keyboard navigation (Enter key on close)', () => {});
});

RunPanelStore Tests

describe('RunPanelStore.onRunButtonClick', () => {
  it('shows keep_screen_alive notification only after validation passes', () => {});
  it('does not show notification when user not logged in', () => {});
  it('does not show notification when self-exclusion blocks bot', () => {});
  it('does not show notification when strategy validation fails', () => {});
  it('shows notification immediately before bot starts running', () => {});
});

URL Construction Edge Cases

• Virtual account → ?account=demo
• Real account with USD → ?account=USD
• Real account with empty currency → no param (should log warning)
• Account switch during dialog display

---

📈 Performance Review

✅ Good practices:

• Dialog component likely lazy-loaded (from shared_ui)
• MobX observer() wrapper for reactive updates
• Minimal re-render triggers (only observable changes)

🟡 Could improve:

• URL parsing happens on every render (see Medium Priority Ako/ remove cert file in vite conf #1)
• Consider if useNavigatorOnline() is optimized (didn't review implementation)
• getCurrency() reads from localStorage on every render (though this is cached by MobX if it's a computed value)

---

✨ What's Good About This PR

1. ✅ Critical issue fixed! - Notification timing moved to line 231 (after validation, before bot starts)
2. ✅ Improved UX messaging - "Connection Interrupted" is clearer than "Platform temporarily unavailable"
3. ✅ Account-aware routing - Smart handling of demo vs real accounts with proper URL parameters
4. ✅ Proper URL handling - Using URL API instead of string concatenation (prevents encoding issues)
5. ✅ Accessibility - Good keyboard support with onKeyDown handler for close button
6. ✅ MobX patterns - Correctly using observer() for reactive components
7. ✅ Code readability - Well-commented, clear variable names
8. ✅ Proactive UX - "keep screen alive" notification helps prevent accidental bot stops

---

📋 Recommendations Summary

Before merging (optional improvements):

1. 🟡 Medium Priority: Add useMemo for URL parsing to improve performance
2. 🟡 Medium Priority: Add defensive logging for missing currency on real accounts
3. 🟡 Medium Priority: Clarify the stale closure risk with URL parameter vs client state
4. 🟢 Low Priority: Clean up minor code style issues (redundant || false, unnecessary braces, intermediate variable)

Future improvements:

1. Add comprehensive test coverage for bot-stopped component
2. Add tests for notification timing in run-panel-store
3. Integration tests for account switching flows
4. Document the account parameter routing behavior in comments or docs

---

🎯 Overall Assessment

Status: ✅ Ready to merge with minor improvements recommended

This PR successfully addresses the bot offline handling with improved UX and correct notification timing. The critical notification timing issue from the previous review has been properly fixed. The remaining issues are minor optimization opportunities and code quality improvements.

What works well:

• Core functionality is solid and correct
• User experience improvements are meaningful
• Previous critical feedback addressed
• Already approved by nijil-deriv

What could be improved:

• Performance optimization (URL parsing)
• Defensive error handling (missing currency)
• Test coverage (add in follow-up PR)

The medium priority items are worth addressing but not blockers. The PR can merge as-is, with these improvements tackled in a follow-up if desired.

---

[github-actions]

Preview Link: https://mayuran-bot-offline.bot-65f.pages.dev

Name	Result
Build status	Completed ✅
Preview URL	Visit Preview
Action URL	Visit Action