Fix: Tab Rename Crash (Issue #1334) #1351
Conversation
Co-authored-by: Jesse Paterson <jesse.r.pat@gmail.com>
- Replace throw statements with safe return/logging in TabBar validation - Add comprehensive error handling and validation in editable-text component - Wrap spaceStore operations in try/catch to prevent React crashes - Remove AnimatePresence to fix unnecessary component re-renders - Add visual error feedback for invalid tab names - Optimize tab switching with early return for same tab
|
@Jhonattan2121 is attempting to deploy a commit to the Nounspace Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors error handling and async flows across EditableText, TabBar, and spaceStore: input validation guards prevent invalid edits, tab operations switch from throwing to logging and early returns with fallbacks, and store functions stop throwing on validation failures. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditableText
participant UpdateMethod
User->>EditableText: Enter edit, submit text
EditableText->>EditableText: Validate (/[^a-zA-Z0-9-_ ]/)
alt Invalid
EditableText-->>User: Revert text, console.warn, keep edit mode
else Valid
EditableText->>UpdateMethod: updateMethod(initialText, text)
UpdateMethod-->>EditableText: Success / Error
EditableText-->>User: Exit edit mode or show error
end
sequenceDiagram
participant User
participant TabBar
participant SpaceStore
User->>TabBar: Create / Rename / Delete tab
TabBar->>TabBar: Sanitize & validate name
alt Validation fails
TabBar-->>User: console.error / tooltip, use fallback or abort
else Valid
TabBar->>SpaceStore: update local tab state (order/name)
TabBar->>SpaceStore: commit asynchronously (setTimeout)
SpaceStore-->>TabBar: commit result (may fail silently)
TabBar-->>User: update UI (switch tab / remove tab)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical app crash issue when renaming tabs with invalid characters by replacing error-throwing mechanisms with graceful error handling and improved user feedback.
- Replaced
throw new Error()withconsole.error()and early returns to prevent React component crashes - Added comprehensive error handling with try/catch blocks around critical operations
- Implemented visual error feedback and improved validation in the editable text component
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| spaceStore.ts | Added try/catch blocks and replaced throws with safe error logging |
| TabBar.tsx | Removed AnimatePresence, added error handling in tab operations, and early returns for validation |
| editable-text.tsx | Complete rewrite with proper validation, visual error feedback, and useCallback optimization |
| return uniqueName || `Tab ${Date.now()}`; // Fallback to timestamp if unique name fails | ||
| } | ||
|
|
There was a problem hiding this comment.
Using Date.now() as a fallback tab name may create confusing user experience. Consider a more user-friendly fallback like 'Untitled Tab' or incrementing a counter.
| return uniqueName || `Tab ${Date.now()}`; // Fallback to timestamp if unique name fails | |
| } | |
| return uniqueName || generateUniqueUntitledTabName(); | |
| } | |
| function generateUniqueUntitledTabName(): string { | |
| let base = "Untitled Tab"; | |
| let name = base; | |
| let iter = 1; | |
| while (tabList.includes(name)) { | |
| name = `${base} - ${iter}`; | |
| iter += 1; | |
| } | |
| return name; | |
| } |
| switchTabTo(nextTab); | ||
|
|
||
| // Delete the tab | ||
| deleteTab(tabName); |
There was a problem hiding this comment.
The deleteTab function is called without await, but based on the original code it appears to be an async operation. This could lead to race conditions or incomplete deletion.
| deleteTab(tabName); | |
| await deleteTab(tabName); |
| } | ||
|
|
||
| // Simple rename without complex rollback logic | ||
| renameTab(tabName, uniqueName); |
There was a problem hiding this comment.
The renameTab function is called without await, but it's likely an async operation that should be awaited to ensure completion before proceeding.
| console.warn("Invalid characters in tab name"); | ||
| setHasError(true); | ||
| settext(initialText); // Reset to original text | ||
| setTimeout(() => setHasError(false), 3000); // Clear error after 3s |
There was a problem hiding this comment.
The magic number 3000 is used for error timeout. Consider defining this as a named constant like ERROR_DISPLAY_DURATION = 3000 to improve maintainability.
| setTimeout(() => setHasError(false), 3000); // Clear error after 3s | |
| setTimeout(() => setHasError(false), ERROR_DISPLAY_DURATION); // Clear error after 3s |
| console.error("Failed to update tab name:", error); | ||
| setHasError(true); | ||
| settext(initialText); // Reset to original text on error | ||
| setTimeout(() => setHasError(false), 3000); // Clear error after 3s |
There was a problem hiding this comment.
Duplicate magic number 3000 for error timeout. This should be extracted to a constant to maintain consistency.
| setTimeout(() => setHasError(false), 3000); // Clear error after 3s | |
| setTimeout(() => setHasError(false), ERROR_TIMEOUT_MS); // Clear error after 3s |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/common/data/stores/app/space/spaceStore.ts (1)
189-223: Replace DOM manipulation with React-based error handling.The current implementation directly manipulates the DOM, which bypasses React's rendering system and could cause memory leaks or conflicts. Consider using React's state management and portal system for error notifications.
This approach has several issues:
- Memory leaks if the component unmounts before timeout
- Multiple errors will overlap at the same position
- Bypasses React's reconciliation
Consider using a proper toast/notification library like
react-hot-toastor implementing a React-based notification system with portals.src/common/components/organisms/TabBar.tsx (1)
131-133: Consider potential race conditions with background commits.Using
setTimeoutfor background commits could cause race conditions if users perform rapid actions. The commit might also fail silently.Consider using a more robust queueing mechanism or at least tracking pending commits to prevent conflicts. You could also add error handling for the background commit:
// Commit in background setTimeout(() => { - commitTabOrder(); + try { + commitTabOrder(); + } catch (error) { + console.error("Failed to commit tab order:", error); + } }, 100);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/common/components/atoms/editable-text.tsx(1 hunks)src/common/components/organisms/TabBar.tsx(5 hunks)src/common/data/stores/app/space/spaceStore.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: willyogo
PR: Nounspace/nounspace.ts#1118
File: src/common/fidgets/FidgetWrapper.tsx:160-175
Timestamp: 2025-06-05T19:55:02.560Z
Learning: In the Nounspace.ts codebase, fidget edit mode icons in FidgetWrapper.tsx require an extremely high z-index (999999) to prevent being covered by the tab bar. Lower z-index values like 1050 break this fix, indicating other UI elements use very high z-index values.
📚 Learning: in the nounspace.ts codebase, fidget edit mode icons in fidgetwrapper.tsx require an extremely high ...
Learnt from: willyogo
PR: Nounspace/nounspace.ts#1118
File: src/common/fidgets/FidgetWrapper.tsx:160-175
Timestamp: 2025-06-05T19:55:02.560Z
Learning: In the Nounspace.ts codebase, fidget edit mode icons in FidgetWrapper.tsx require an extremely high z-index (999999) to prevent being covered by the tab bar. Lower z-index values like 1050 break this fix, indicating other UI elements use very high z-index values.
Applied to files:
src/common/components/organisms/TabBar.tsx
📚 Learning: in the nounspace.ts codebase, there is an internal phoneframe svg component at src/common/components...
Learnt from: j-paterson
PR: Nounspace/nounspace.ts#1279
File: src/app/(spaces)/Space.tsx:415-421
Timestamp: 2025-07-12T00:20:33.439Z
Learning: In the Nounspace.ts codebase, there is an internal PhoneFrame SVG component at src/common/components/atoms/PhoneFrame.tsx that should be used for mobile mockups instead of external image URLs. The component provides a scalable 344x744 phone frame with proper styling.
Applied to files:
src/common/components/organisms/TabBar.tsx
🔇 Additional comments (4)
src/common/data/stores/app/space/spaceStore.ts (1)
337-343: Merge behavior is intentional and documentedThe custom
mergeWithlogic inspaceStore.tsmirrors its usage in:
src/common/data/stores/app/homebase/homebaseStore.tssrc/common/data/stores/app/homebase/homebaseTabsStore.tsEach instance includes an inline comment explaining that nested objects are fully replaced. No code changes are required—this override is by design. If your configuration objects ever require true deep-merge behavior, consider introducing a specialized merge utility at that time.
src/common/components/organisms/TabBar.tsx (3)
76-81: Good error handling with timestamp fallback!The fallback to timestamp when unique name generation fails is a robust solution that prevents crashes.
178-181: Good null handling for unique name generation!Properly checking for null return from
generateUniqueTabNameprevents potential crashes.
228-231: Good optimization for current tab clicks!Preventing unnecessary operations when clicking the already-selected tab improves performance and prevents potential state inconsistencies.
| return event.key === "Enter" || event.key === "Escape"; | ||
| } | ||
|
|
||
| const EditableText = ({ initialText, updateMethod }) => { |
There was a problem hiding this comment.
Add TypeScript type definitions for component props.
The component accepts props without type definitions, which reduces type safety and could lead to runtime errors.
Add proper TypeScript types:
-const EditableText = ({ initialText, updateMethod }) => {
+interface EditableTextProps {
+ initialText: string;
+ updateMethod?: (oldText: string, newText: string) => Promise<void> | void;
+}
+
+const EditableText: React.FC<EditableTextProps> = ({ initialText, updateMethod }) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const EditableText = ({ initialText, updateMethod }) => { | |
| interface EditableTextProps { | |
| initialText: string; | |
| updateMethod?: (oldText: string, newText: string) => Promise<void> | void; | |
| } | |
| const EditableText: React.FC<EditableTextProps> = ({ initialText, updateMethod }) => { |
🤖 Prompt for AI Agents
In src/common/components/atoms/editable-text.tsx at line 7, the EditableText
component props lack TypeScript type definitions, reducing type safety. Define
an interface or type for the props specifying the types of initialText and
updateMethod, then annotate the component's props parameter with this type to
ensure proper type checking.
| try { | ||
| // Don't update if text hasn't changed | ||
| if (text === initialText) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Reset processing state when text hasn't changed.
When the text hasn't changed, the function returns early without resetting isProcessing, which will leave the component stuck in a processing state.
// Don't update if text hasn't changed
if (text === initialText) {
+ setIsProcessing(false);
return;
}🤖 Prompt for AI Agents
In src/common/components/atoms/editable-text.tsx around lines 26 to 30, the
function returns early when the text hasn't changed but does not reset the
isProcessing state, causing the component to remain stuck in processing. Modify
the code to reset isProcessing to false before returning early to ensure the
component exits the processing state properly.
| // Add basic validation | ||
| if (!text || text.trim().length === 0) { | ||
| console.warn("Tab name cannot be empty"); | ||
| settext(initialText); // Reset to original text | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set error state for empty tab name validation.
The empty string validation doesn't set the error state, unlike the character validation. This creates inconsistent user feedback.
// Add basic validation
if (!text || text.trim().length === 0) {
console.warn("Tab name cannot be empty");
+ setHasError(true);
settext(initialText); // Reset to original text
+ setTimeout(() => setHasError(false), 3000); // Clear error after 3s
+ setIsProcessing(false);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Add basic validation | |
| if (!text || text.trim().length === 0) { | |
| console.warn("Tab name cannot be empty"); | |
| settext(initialText); // Reset to original text | |
| return; | |
| } | |
| // Add basic validation | |
| if (!text || text.trim().length === 0) { | |
| console.warn("Tab name cannot be empty"); | |
| setHasError(true); | |
| settext(initialText); // Reset to original text | |
| setTimeout(() => setHasError(false), 3000); // Clear error after 3s | |
| setIsProcessing(false); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/common/components/atoms/editable-text.tsx around lines 32 to 37, the
validation for empty tab names only logs a warning and resets the text but does
not set the error state, causing inconsistent user feedback compared to
character validation. Update the code to set the error state appropriately when
the tab name is empty, ensuring consistent error handling and user feedback.
| const trimmedText = text.trim(); | ||
|
|
||
| const onEditEnd = () => { | ||
| // Validate characters | ||
| if (/[^a-zA-Z0-9-_ ]/.test(trimmedText)) { | ||
| console.warn("Invalid characters in tab name"); | ||
| setHasError(true); | ||
| settext(initialText); // Reset to original text | ||
| setTimeout(() => setHasError(false), 3000); // Clear error after 3s | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce maxLength validation in the validation logic.
The input has a maxLength of 22, but this isn't enforced in the validation logic. Users could bypass this through programmatic means.
const trimmedText = text.trim();
+
+ // Validate length
+ if (trimmedText.length > 22) {
+ console.warn("Tab name exceeds maximum length");
+ setHasError(true);
+ settext(initialText);
+ setTimeout(() => setHasError(false), 3000);
+ setIsProcessing(false);
+ return;
+ }
// Validate characters
if (/[^a-zA-Z0-9-_ ]/.test(trimmedText)) {Also applies to: 96-96
🤖 Prompt for AI Agents
In src/common/components/atoms/editable-text.tsx around lines 39 to 48 and line
96, the validation logic currently checks for invalid characters but does not
enforce the maxLength constraint of 22 characters. To fix this, add a condition
to check if trimmedText.length exceeds 22 and handle it similarly by setting an
error state, resetting the text to initialText, and clearing the error after 3
seconds. This ensures the maxLength is enforced even if users bypass the input
attribute programmatically.
| } catch (e) { | ||
| console.error("Failed to commit space tab to remote:", e); | ||
| // Don't throw the error to prevent app crashes | ||
| // The tab is still saved locally, so the user can continue working | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Notify users when remote save fails.
When the remote commit fails, the error is only logged to console. Users should be notified that their changes are saved locally but not synced to the server.
} catch (e) {
console.error("Failed to commit space tab to remote:", e);
+ // Notify user that changes are saved locally but not synced
+ showTooltipError(
+ "Sync Failed",
+ "Your changes are saved locally but couldn't sync to the server. They'll sync automatically later."
+ );
// Don't throw the error to prevent app crashes
// The tab is still saved locally, so the user can continue working
}🤖 Prompt for AI Agents
In src/common/data/stores/app/space/spaceStore.ts around lines 298 to 302, the
catch block only logs the error to the console when the remote commit fails.
Modify this block to also notify the user through the UI or a notification
system that their changes are saved locally but not synced to the server,
ensuring they are aware of the sync failure without throwing an error.
3 participants
Problem
App crashed when renaming tabs with invalid characters. Unhandled throws broke React components.
Changes Made
TabBar.tsx
throw new Error()withconsole.error()+returnAnimatePresencecausing unnecessary re-rendershandleTabClickfor same tabeditable-text.tsx
useCallbackfor optimizationspaceStore.ts
try/catchSummary by CodeRabbit
New Features
Bug Fixes
Refactor