Stat File Error Overlay and Clear Errors on Connection Change #1997
Conversation
This adds a missing error overlay integration in preview.tsx.
WalkthroughThe changes modify the error handling in the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/app/view/preview/preview.tsx (1)
414-435:⚠️ Potential issueReturn null when file read fails.
The error handling is good, but the function should return null when an error occurs to prevent undefined behavior in the consuming code.
Apply this diff to fix the error case:
const fullFileAtom = atom<Promise<FileData>>(async (get) => { const fileName = get(this.metaFilePath); const path = await this.formatRemoteUri(fileName, get); if (fileName == null) { return null; } let file: FileData; try { file = await RpcApi.FileReadCommand(TabRpcClient, { info: { path, }, }); + return file; } catch (e) { const errorStatus: ErrorMsg = { status: "File Read Failed", text: `${e}`, }; globalStore.set(this.errorMsgAtom, errorStatus); + return null; } - return file; });
🧹 Nitpick comments (1)
frontend/app/view/preview/preview.tsx (1)
1240-1240: Optimize UUID generation in render cycle.Using
crypto.randomUUID()directly in the render cycle can impact performance as it generates a new UUID on every render. Consider moving the UUID generation to the button definition or using a stable index.Apply this diff to use the array index as a stable key:
-key={crypto.randomUUID()} +key={`error-button-${index}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/preview/preview.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
frontend/app/view/preview/preview.tsx (2)
391-404: LGTM! Error handling is well implemented.The try-catch block properly captures file stat operation errors and updates the error state with a clear message structure.
1074-1078: LGTM! Connection state management is well implemented.The effect properly clears error messages when the connection changes, preventing stale error states from persisting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/app/view/preview/preview.tsx (2)
385-405: Standardize error handling across RPC calls.The error handling in
statFileandfullFilefunctions is duplicated. Consider extracting the common error handling logic into a reusable function to improve maintainability and ensure consistent error reporting.+const handleRpcError = (error: Error, errorMsgAtom: PrimitiveAtom<ErrorMsg>) => { + const errorStatus: ErrorMsg = { + status: "File Read Failed", + text: `${error}`, + }; + globalStore.set(errorMsgAtom, errorStatus); +}; async statFile() { try { const statFile = await RpcApi.FileInfoCommand(TabRpcClient, { info: { path }, }); return statFile; } catch (e) { - const errorStatus: ErrorMsg = { - status: "File Read Failed", - text: `${e}`, - }; - globalStore.set(this.errorMsgAtom, errorStatus); + handleRpcError(e, this.errorMsgAtom); } } async fullFile() { try { const file = await RpcApi.FileReadCommand(TabRpcClient, { info: { path }, }); return file; } catch (e) { - const errorStatus: ErrorMsg = { - status: "File Read Failed", - text: `${e}`, - }; - globalStore.set(this.errorMsgAtom, errorStatus); + handleRpcError(e, this.errorMsgAtom); } }Also applies to: 414-434
1073-1077: Use a more descriptive variable name for connection state.The variable name
connectioncould be more descriptive to indicate it represents the immediate connection state.-const connection = useAtomValue(model.connectionImmediate); +const currentConnectionState = useAtomValue(model.connectionImmediate); useEffect(() => { setErrorMsg(null); -}, [connection]); +}, [currentConnectionState]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/preview/preview.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
frontend/app/view/preview/preview.tsx (1)
1193-1263: Well-implemented error overlay component!The ErrorOverlay component is well-structured with:
- Proper error message display and styling
- Good UX features (copy to clipboard, dismiss button)
- Support for custom buttons and error levels
- Performance optimization using memo
This adds two small features: