Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding economic calendar #856

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ashkan-deriv
Copy link

Changes:

  • ...

Type of change

  • [ x ] New feature

@ashkan-deriv ashkan-deriv requested a review from a team as a code owner November 19, 2024 11:35
@kaveh-deriv kaveh-deriv requested review from review-deriv and removed request for review-deriv November 19, 2024 12:11
Copy link

@review-deriv review-deriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review (AI review can be wrong. Do not use it as the only source of feedback)

🔴 Critical Issues

No critical security vulnerabilities or major logical flaws were found in the provided code snippet.


🟡 Potential Concerns

1. Edge Case in Date Formatting

  • Problem: The formatDate function does not handle invalid or unexpected date strings. If dateString is invalid, new Date(dateString) may result in an Invalid Date, and calling toLocaleDateString on it may not produce the desired output.

  • Potential Impact: This could lead to displaying incorrect date information or 'Invalid Date' in the UI, negatively affecting the user experience.

  • Suggestion: Validate the date object before formatting and handle invalid dates gracefully, perhaps by returning a placeholder like '-'.

  • Fix:

    const formatDate = (dateString) => {
        if (!dateString) return '-';
        const date = new Date(dateString);
        if (isNaN(date.getTime())) return '-'; // Handle invalid date
        return date.toLocaleDateString('en-GB', {
            day: '2-digit',
            month: '2-digit',
            year: 'numeric',
        });
    };

2. Handling of Empty or Unexpected Data

  • Problem: The getDisplayValue function assumes that if field is an object with a display_value, that is the correct value to display. There may be cases where field is an object but does not have display_value, or display_value is not a string.

  • Potential Impact: Could display 'undefined' or '[object Object]' in the UI if field is an object without a proper display_value. This affects the user experience by showing confusing or incorrect data.

  • Suggestion: Add additional checks to ensure that field.display_value exists and is a string before returning it.

  • Fix:

    const getDisplayValue = (field) => {
        if (!field) return '-';
        if (typeof field === 'object' && 'display_value' in field) {
            return field.display_value.toString();
        }
        return field.toString();
    };

3. Potential Unhandled Errors in Data Fetching

  • Problem: If the BinarySocket.send operation fails due to network issues or server errors, and the .catch block is executed, the user is not notified; they're left looking at

Copy link

Name Result
Build status Failed ❌
Action URL Visit Action

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants