RED-33: Add error codes to places where we display error toasts/etc to the user

[dnlbui]

https://linear.app/shm/issue/RED-33/add-error-codes-to-places-where-we-display-error-toastsetc-to-the-user
Summary: Added error codes, context detail to error message, and retry logic to fetcher

[linear]

RED-33 Add error codes to places where we display error toasts/etc to the user.

[magonjr]

LGTM

[chrypnotoad]

@CodiumAI-Agent /review

[CodiumAI-Agent]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, because the PR involves significant changes to the fetcher function, including error handling, retry logic, and custom error class implementation. The added complexity and the critical nature of the fetcher function in application data flow necessitate a thorough review to ensure reliability and maintainability.
🧪 Relevant tests	No
⚡ Possible issues	Retry Logic Concern: The retry logic does not differentiate between types of errors. It retries for all exceptions, which might not be appropriate for certain types of errors like 403 unauthorized, where retrying would not change the outcome.
	Error Handling: The custom error class FetchError is used inconsistently. For instance, it throws a FetchError for a 403 status but a generic Error for other fetch failures. This inconsistency could lead to difficulties in error handling upstream.
🔒 Security concerns	No

Code feedback:

---

relevant file	hooks/fetcher.ts
suggestion	Consider adding specific error handling for network errors (like timeouts or DNS failures) to provide better user feedback and potentially retry these errors, as they are often transient. [important]
relevant line	if (isDev()) {

---

relevant file	hooks/fetcher.ts
suggestion	Refactor the retry logic to exclude certain error types from retries, such as HTTP 403 errors, which are unlikely to be resolved by retrying. This can prevent unnecessary requests and improve the user experience. [important]
relevant line	if (isDev()) {

---

relevant file	hooks/fetcher.ts
suggestion	To improve debugging, consider logging more context-specific information, such as the error status code and the request method, especially when retries are involved. [medium]
relevant line	`Error encountered for request:`, input, "with init:", init, "Error:", error

---

relevant file	hooks/fetcher.ts
suggestion	Implement a backoff strategy for retries to handle high load or service unavailability scenarios more gracefully. This could involve increasing the delay between retries exponentially. [medium]
relevant line	await delay(retryDelay)

---

[chrypnotoad]

@CodiumAI-Agent /improve

[CodiumAI-Agent]

PR Code Suggestions ✨

Category	Suggestion	Score
Maintainability	Consolidate error handling logic in the catch block Refactor the error handling in the catch block to consolidate the error logging and toast display logic, reducing redundancy and improving maintainability. hooks/fetcher.ts [80-91] -if (isDev()) { - console.error( - `Error encountered for request:`, input, "with init:", init, "Error:", error - ) -} -if ( - error instanceof FetchError && - error.status === 403 -) { +if (error instanceof FetchError) { + if (isDev()) { + console.error(`Error encountered for request:`, input, "with init:", init, "Error:", error) + } showErrorToast(showToast, error.status, input) throw error } Suggestion importance[1-10]: 8 Why: Consolidating the error handling logic in the catch block reduces redundancy and improves maintainability. This refactor makes the code cleaner and easier to manage, addressing a significant aspect of code quality.	8
Possible issue	Add logging for error details on specific HTTP status codes To enhance error handling, consider logging data.errorDetails not only when res.ok is false but also for specific status codes like 403 and 500 where additional context might be crucial for debugging. hooks/fetcher.ts [66-68] if (res.status === 403) { + console.log(data.errorDetails) await authService.logout(apiBase) throw new FetchError("Unauthorized", res.status) } Suggestion importance[1-10]: 7 Why: Adding logging for error details on specific HTTP status codes can provide valuable context for debugging. This suggestion enhances error handling and can help in identifying issues more effectively.	7
Best practice	Simplify the check for undefined or null status values Replace the manual check for status !== undefined && status !== null with a more concise and idiomatic check using status != null which covers both null and undefined. hooks/fetcher.ts [37-39] const baseMessage = `Error${ - status !== undefined && status !== null ? ` (${status})` : "" + status != null ? ` (${status})` : "" }: An error occurred while retrieving ${lastSegment}.` Suggestion importance[1-10]: 6 Why: The suggested change simplifies the check for undefined or null status values, making the code more concise and idiomatic. While this is a good practice, it is a minor improvement and does not address a major issue.	6
Enhancement	Improve URL parameter extraction using URLSearchParams Consider using URLSearchParams to extract query parameters safely and effectively instead of manually splitting the URL string. This approach avoids potential issues with URL formats and improves code readability. hooks/fetcher.ts [32-34] const url = new URL(urlString) -const lastSegment = - url.pathname.split("/").filter(Boolean).pop() || "resource" +const params = new URLSearchParams(url.search) +const lastSegment = params.get('lastSegment') || 'resource' Suggestion importance[1-10]: 3 Why: While using URLSearchParams can be a good practice for extracting query parameters, the current code is extracting the last segment of the URL path, not query parameters. The suggestion does not accurately address the existing functionality, making it less relevant.	3

[dnlbui]

commenting to get reviewed again by @magonjr