-
Notifications
You must be signed in to change notification settings - Fork 53
Site Creation: Fix renderer crash for long site names when creating new site #2336
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
base: trunk
Are you sure you want to change the base?
Conversation
- Add error handling in handleSiteNameChange to catch ENAMETOOLONG errors - Display user-friendly error message when site name is too long - Add tests to verify ENAMETOOLONG error handling Fixes STU-1182
📊 Performance Test ResultsComparing 34e6d43 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
nightnei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/hooks/use-add-site.ts
Outdated
| ) | ||
| ); | ||
| return; | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the exception can be thrown by generateProposedSitePath, it would be clearer to wrap only this part with try/catch.
However, in generateProposedSitePath, we already catch the underlying exception and set returned props accordingly, so we could consider moving exception handling there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, My first approach was to handle exceptions in generateProposedSitePath. However, the main issue was with the error message. If we handle the exception in IPC handlers, it prefixes the following error message.
Error invoking remote method 'generateProposedSitePath'
if we want to remove this prefix, I would need to add an additional parameter, such as isError, to indicate that there was a problem. However, I believe this is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we handled the exception there, returned isNameTooLong prop, and set the error in the renderer process? It would be consistent with current error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. To make it consistent we can do that. I will update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 34e6d43!
- Move ENAMETOOLONG error handling to generateProposedSitePath IPC handler - Simplify hook to display error message from IPC handler - Update tests to reflect the change Addresses feedback from code review
This reverts commit 6931b1f.

Related issues
Proposed Changes
handleSiteNameChangeto catch ENAMETOOLONG errors and display a user-friendly error message when site name is too longTesting Instructions
npm test -- src/hooks/tests/use-add-site.test.tsxPre-merge Checklist