fix: retry fixing get_settings rate limit issue

[aum-deriv]

🔗 Linked Issue

❓ Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no API changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:

📝 Description

🧪 How Has This Been Tested?

📷 Screenshots (if appropriate)

[review-deriv]

AI Code Review

⚠️ IMPORTANT: This review is AI-generated and should be validated by human reviewers
🔴 BUGS & LOGICAL ISSUES:

1. fetchSettings() Short-Circuits When settings Is Non-Null
   • Description: The new fetchSettings function early-returns a resolved Promise if settings is already non-null. This prevents genuinely “fresh” settings from ever being fetched again after the first load or after an updateSettings call.
   • Potential Impacts:
   – Users may see stale settings if the backend returns updated data after the initial fetch.
   – The subsequent call to fetchSettings in updateSettings will never actually refetch from the server unless settings is first reset to null (e.g., on disconnect).
   • Reproduction Scenarios:
   – Once settings is set (non-null), attempt to call fetchSettings again manually or from updateSettings to see if new data is fetched; observe it doesn’t request fresh data.
   • Fix Implementation (Code Example):
   – Remove the short-circuit check so that a legitimate refetch will still contact the server, e.g.:

   const fetchSettings = useCallback(() => {
       if (!isConnected || !isAuthorized) {
           return Promise.reject(new Error('Not connected or authorized'));
       }
       setIsLoading(true);
       setError(null);
   
       return new Promise((resolve, reject) => {
           sendMessage({ get_settings: 1 }, (response) => {
               if (response.error) {
                   setError(response.error);
                   setIsLoading(false);
                   return reject(response.error);
               }
               setSettings(response.get_settings);
               setError(null);
               setIsLoading(false);
               resolve(response.get_settings);
           });
       });
   }, [isConnected, isAuthorized, sendMessage]);
   

2. updateSettings Doesn’t Directly Reflect New set_settings Response
   • Description: In updateSettings, once set_settings is successful, fresh settings are fetched by fetchSettings. However, if fetchSettings is short-circuited (because settings is non-null), new data from set_settings is never actually reflected in the state.
   • Potential Impacts:
   – The UI may never show the newly updated user settings if fetchSettings is skipped.
   • Reproduction Scenarios:
   – Call updateSettings with new data; verify that the UI does not update even though the API returned success.
   • Fix Implementation (Code Example):
   – Either remove the short-circuit in fetchSettings (as above), or mirror the set_settings response directly into state before or after fetching. For example:

   const updateSettings = useCallback(async (newSettings) => {
       if (!isConnected || !isAuthorized) {
           throw new Error('Not connected or authorized');
       }
   
       setIsLoading(true);
       setError(null);
   
       try {
           // 1) Update settings with server
           const updatedData = await new Promise((resolve, reject) => {
               sendMessage(
                   { set_settings: 1, ...newSettings },
                   (response) => {
                       if (response.error) return reject(response.error);
                       resolve(response.set_settings);
                   }
               );
           });
           // 2) Update local state with new server response
           setSettings(updatedData);
   
           // 3) Optionally fetch additional info if needed (now that short-circuit is removed)
           await fetchSettings();
       } catch (err) {
           setError(err);
           throw err;
       } finally {
           setIsLoading(false);
       }
   }, [isConnected, isAuthorized, sendMessage, fetchSettings]);
   

🟡 RELIABILITY CONCERNS:

1. Connection Loss During Requests
   • Edge Cases Identified: If the connection is lost during updateSettings or fetchSettings, the code sets isLoading to false in the “finally” block or via the “connection lost” effect. There is a brief window where the UI might incorrectly assume the settings are fully loaded or updated.
   • Potential Failure Scenarios:
   – The user sees partial or stale data because the request was never actually finished.
   • Mitigation Steps (Code Example):
   – Use a dedicated request state or “request in flight” boolean. If the connection is lost mid-flight, set error status unambiguously so the UI shows a “reconnect” message:

   useEffect(() => {
       if (!isConnected) {
           setSettings(null);
           setError(new Error('Connection lost; please reconnect.'));
           setIsLoading(false);
       }
   }, [isConnected]);
   

2. Missing Retry Logic or Timeouts
   • Potential Failure Scenarios: If the API call times out or fails intermittently, the user is left stuck without settings.
   • Mitigation Steps (High-Level):
   – Implement a retry strategy or exponential backoff if error is network-related.
   – Wrap sendMessage in a timed Promise to handle server timeouts gracefully.

💡 ROBUSTNESS IMPROVEMENTS:

1. Error Handling Enhancements
   • Suggestion: Provide more informative UI states for different error types (authorization vs. connection vs. generic API error).
   • Code Example:
   return (
   <>
   {error && {error.message ?? 'An unexpected error occurred!'}}
   …
   </>
   );

2. Input Validation and Data Checks
   • Suggestion: Validate newSettings to ensure it has valid fields before sending {set_settings: 1, ...newSettings}.
   • Code Example:
   const updateSettings = useCallback(async (newSettings) => {
   if (!isValidSettings(newSettings)) {
   throw new Error('Invalid fields in settings');
   }
   …
   }, []);

3. State Management Improvements
   • Suggestion: Keep an explicit “hasFetched” or “isFetching” flag to avoid confusion with “settings !== null” as the fetch gating condition—this clarifies whether data is present vs. calls are in progress.
   • Code Example:
   const [hasFetched, setHasFetched] = useState(false);
   …
   const fetchSettings = useCallback(() => {
   if (!hasFetched) {
   setIsLoading(true);
   setHasFetched(true);
   …
   }
   }, [hasFetched, …]);

By removing the short-circuit in fetchSettings, handling errors more granularly, and introducing a clear fetch state, the code becomes more reliable, resilient, and logically sound.