Skip to content

Conversation

@willyogo
Copy link
Member

@willyogo willyogo commented Oct 9, 2025

This fixes the issue in canary / blocking release that occurred when renaming tabs on homebases and public spaces.

One of the underlying causes was that tab renames were only successfully updating tabOrder, and not updating the name of the tab files in the space's tabs folder.

Summary

  • optimistically update local homebase tab state when renaming so routing and configs stay in sync
  • block duplicate tab names and roll back to the previous state if the rename request fails
  • Coerced the resolved network value to string | undefined before committing renamed public space tabs, avoiding null arguments in the Supabase commit helper

Testing

  • yarn test --run (fails: mini app discovery API tests require a local server and cannot reach localhost in CI)

https://chatgpt.com/codex/tasks/task_e_68e7e3a8003083259a853e174c78fd86

Summary by CodeRabbit

  • New Features

    • Centralized, asynchronous tab renaming with server-backed commits.
    • Optimistic UI updates during rename with automatic rollback on failure.
    • Trim/character validation and case-insensitive duplicate-name protection with user-facing tooltips.
  • Bug Fixes

    • Prevents empty or unchanged names and stops renames when no active space exists.
    • Ensures reliable rollback preserving tab order and current selection if server commit fails.
    • Tab rename flow now awaited to surface async errors cleanly.

@vercel
Copy link

vercel bot commented Oct 9, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
nounspace-ts Ready Ready Preview Comment Oct 10, 2025 5:16pm

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 9, 2025

Walkthrough

Implements an async tab-rename flow: TabBar now awaits rename handlers; PublicSpace is wired to a new store action renameSpaceTab which performs validation, optimistic updates, backend commit, and rollback; homebase and space stores add duplicate checks and error/tooltips; saveLocalSpaceTab signature adjusted.

Changes

Cohort / File(s) Summary
PublicSpace wiring
src/app/(spaces)/PublicSpace.tsx
Gates rename on currentSpaceId; calls renameSpaceTab(spaceId, oldName, newName, config?, network?) instead of saveLocalSpaceTab; early-exit when no current space.
TabBar async contract
src/common/components/organisms/TabBar.tsx
TabBarProps.renameTab now `Promise
Home page handler
src/app/home/[tabname]/page.tsx
Adjusts renameTab prop to return void/Promise (no longer returns { tabName }).
Homebase tabs store
src/common/data/stores/app/homebase/homebaseTabsStore.ts
renameTab trims/validates names, rejects empty/unchanged, prevents case-insensitive duplicates, performs optimistic update, commits to server, and rolls back state & ordering on failure with tooltip errors.
Space store API & logic
src/common/data/stores/app/space/spaceStore.ts
Removes optional newName from saveLocalSpaceTab; adds renameSpaceTab(spaceId, tabName, newName, config?, network?) implementing validation, duplicate checks, optimistic rename, backend commit, rollback, and initializes localSpaces metadata for contract-backed spaces.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User
  participant TB as TabBar (UI)
  participant PS as PublicSpace
  participant SS as spaceStore.renameSpaceTab
  participant BE as Backend

  U->>TB: Initiate rename (oldName → newName)
  TB->>PS: await renameTab(oldName, newName)
  alt currentSpaceId present
    PS->>SS: renameSpaceTab(spaceId, oldName, newName, config?, network?)
    SS->>SS: Validate, check duplicates, optimistic update
    SS->>BE: Commit rename
    alt Success
      BE-->>SS: OK
      SS-->>PS: resolve
      PS-->>TB: resolve
    else Failure
      BE-->>SS: Error
      SS->>SS: Roll back state and ordering
      SS-->>PS: reject
      PS-->>TB: reject
      TB-->>U: Surface error
    end
  else No current space
    PS-->>TB: return (no-op)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

bug

Suggested reviewers

  • j-paterson
  • Jhonattan2121

Poem

I hop between tabs with gentle delight,
I trim the name, then optimistically write.
If backend storms blow through the night,
I roll things back and set them right.
Carrots renamed — hop on! 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “Fix tab renaming and routing” accurately summarizes the primary objective of the changeset, which addresses broken tab renaming flows and ensures routing remains in sync when tabs are renamed across homebases and public spaces. It is concise, clear, and directly related to the main fixes implemented.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/investigate-and-fix-tab-behavior-regression

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@willyogo willyogo changed the title Fix optimistic homebase tab rename Fix tab renaming and routing Oct 10, 2025
@willyogo willyogo marked this pull request as ready for review October 10, 2025 14:10
@willyogo
Copy link
Member Author

@CodeRabbit full review

@willyogo willyogo requested a review from j-paterson October 10, 2025 14:18
@willyogo willyogo added LGFTP Looks Good From Testing Perspective and removed codex labels Oct 10, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 10, 2025

✅ Actions performed

Full review triggered.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/common/components/organisms/TabBar.tsx (2)

17-34: Prop types mismatch with async usage across call sites

updateTabOrder, deleteTab, commitTab, commitTabOrder are used/passed as async functions elsewhere (e.g., PublicSpace, Home), but the props are typed as returning void. This can break TS builds and makes awaiting unsafe.

Apply:

   updateTabOrder: (newOrder: string[]) => void;
-  commitTabOrder: () => void;
+  commitTabOrder: () => Promise<void> | void;
   switchTabTo: (tabName: string, shouldSave?: boolean) => void;
-  deleteTab: (tabName: string) => void;
+  deleteTab: (tabName: string) => Promise<void> | void;
   createTab: (tabName: string) => Promise<{ tabName: string } | undefined>;
-  renameTab: (tabName: string, newName: string) => Promise<void> | void;
-  commitTab: (tabName: string) => void;
+  renameTab: (tabName: string, newName: string) => Promise<void> | void;
+  commitTab: (tabName: string) => Promise<void> | void;
   getSpacePageUrl: (tabName: string) => string;

188-227: Rename flow can desync when validation fails; also double-commits/updates

If stores reject a rename (e.g., case-insensitive duplicate), they currently return without throwing. debouncedRenameTab still:

  • updates local order,
  • switches tab,
  • commits tab and order.

This causes UI/store divergence and redundant commits when rename succeeds (store already optimistically updated and committed).

Two complementary fixes (pick one, or both):

A) Prefer single source of truth: let stores handle order/commit, TabBar only navigates on success.

-        await renameTab(tabName, uniqueName);
-        const newOrder = tabList.map((name) => (name === tabName ? uniqueName : name));
-        updateTabOrder(newOrder);
-        switchTabTo(uniqueName);
-        commitTab(uniqueName);
-        commitTabOrder();
+        await renameTab(tabName, uniqueName);
+        // Store performs optimistic rename, order update and commit.
+        // Only navigate locally.
+        switchTabTo(uniqueName);

B) If you keep local updates here, require renameTab to signal failure by throwing so the catch prevents local changes. See store diffs in related comments.

Also applies to: 213-219

src/common/data/stores/app/space/spaceStore.ts (1)

247-276: Guard remoteSpaces init before writes in commitSpaceTabToDatabase

If remoteSpaces[spaceId] is undefined (e.g., first commit before any loads), mutating tabs throws. Initialize the object.

       try {
         await axiosBackend.post(
           `/api/space/registry/${spaceId}/tabs/${oldTabName}`,
           { ...file, network },
         );
 
         set((draft) => {
+          if (!draft.space.remoteSpaces[spaceId]) {
+            draft.space.remoteSpaces[spaceId] = {
+              id: spaceId,
+              updatedAt: moment().toISOString(),
+              tabs: {},
+              order: [],
+            };
+          }
           draft.space.remoteSpaces[spaceId].tabs[tabName] = localCopy;
           delete draft.space.remoteSpaces[spaceId].tabs[oldTabName];
           delete draft.space.localSpaces[spaceId].changedNames[tabName];
         }, "commitSpaceTabToDatabase");
🧹 Nitpick comments (3)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)

422-432: Optional: also update current tab name on successful optimistic rename

On success, if the renamed tab is current, updating currentSpace.currentTabName improves consistency (you already handle rollback). Not blocking.

 set((draft) => {
   const tabEntry = draft.homebase.tabs[tabName];
   if (!tabEntry) return;
   draft.homebase.tabs[sanitizedNewName] = tabEntry;
   delete draft.homebase.tabs[tabName];
   draft.homebase.tabOrdering.local = updatedOrder;
+  if (draft.currentSpace.currentTabName === tabName) {
+    draft.currentSpace.currentTabName = sanitizedNewName;
+  }
 }, "renameHomebaseTabOptimistic");

Also applies to: 471-474

src/app/(spaces)/PublicSpace.tsx (2)

823-839: renameTab wiring OK; avoid returning undefined silently on invalid state

This is fine, but combined with TabBar’s current flow, silent undefined returns on failure let UI proceed. Pair this with store throwing on failure (and TabBar catching) to prevent desync. See TabBar and store comments.


694-767: Callback returns a cleanup function (ignored)

switchTabTo returns a function as if it were an effect cleanup; callers ignore it. Drop the return to avoid confusion.

-    // Clear flag on unmount
-    return () => { isMounted = false; };
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10a8ef9 and 03154b5.

📒 Files selected for processing (5)
  • src/app/(spaces)/PublicSpace.tsx (3 hunks)
  • src/app/home/[tabname]/page.tsx (1 hunks)
  • src/common/components/organisms/TabBar.tsx (2 hunks)
  • src/common/data/stores/app/homebase/homebaseTabsStore.ts (1 hunks)
  • src/common/data/stores/app/space/spaceStore.ts (5 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/common/components/organisms/TabBar.tsx (1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)
  • renameTab (376-476)
src/common/data/stores/app/space/spaceStore.ts (1)
src/constants/etherscanChainIds.ts (1)
  • EtherScanChainName (1-1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (2)
src/pages/api/space/homebase/tabs/index.ts (2)
  • UnsignedManageHomebaseTabsRequest (24-29)
  • ManageHomebaseTabsResponse (21-22)
src/common/lib/signedFiles.ts (1)
  • signSignable (54-62)
src/app/(spaces)/PublicSpace.tsx (2)
src/common/types/spaceData.ts (1)
  • isTokenSpace (85-87)
src/constants/etherscanChainIds.ts (1)
  • EtherScanChainName (1-1)
🔇 Additional comments (2)
src/app/home/[tabname]/page.tsx (1)

88-88: renameTab stub is fine, but routing should encode tab names

Your renameTab no-op matches the updated async signature. However, router.push should encode names with spaces/special chars.

Apply:

-    router.push(`/home/${newTabName}`);
+    router.push(`/home/${encodeURIComponent(newTabName)}`);

Likely an incorrect or invalid review comment.

src/common/data/stores/app/space/spaceStore.ts (1)

247-276: Confirm backend rename contract for tab uploads
You post to /api/space/registry/{spaceId}/tabs/{oldTabName} with fileName set to the new tabName. Verify the endpoint treats this as a rename rather than creating a new tab and leaving the old one, or remote state can drift.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/common/data/stores/app/space/spaceStore.ts (2)

267-271: Prevent crash when remoteSpaces[spaceId] is uninitialized

Writing to draft.space.remoteSpaces[spaceId].tabs can throw if the space entry isn’t present. Initialize it before mutation.

Apply this diff:

-        set((draft) => {
-          draft.space.remoteSpaces[spaceId].tabs[tabName] = localCopy;
-          delete draft.space.remoteSpaces[spaceId].tabs[oldTabName];
-          delete draft.space.localSpaces[spaceId].changedNames[tabName];
-        }, "commitSpaceTabToDatabase");
+        set((draft) => {
+          if (!draft.space.remoteSpaces[spaceId]) {
+            draft.space.remoteSpaces[spaceId] = {
+              id: spaceId,
+              updatedAt: moment().toISOString(),
+              tabs: {},
+              order: [],
+            };
+          }
+          draft.space.remoteSpaces[spaceId].tabs[tabName] = localCopy;
+          delete draft.space.remoteSpaces[spaceId].tabs[oldTabName];
+          delete draft.space.localSpaces[spaceId].changedNames[tabName];
+        }, "commitSpaceTabToDatabase");

630-649: Upload missing tab content in retry path
Retry succeeds on 429 but only re-registers and commits order—don’t forget to upload the tab JSON (createSignedFile + axiosBackend.post(/api/space/registry/${spaceId}/tabs/${tabName})) as in the primary path.

src/app/(spaces)/PublicSpace.tsx (1)

393-411: Background preloading condition never true (logic order bug)

You set initialDataLoadRef.current = true and then check if (!initialDataLoadRef.current), so loadRemainingTabs never runs after the initial load.

Apply one of:

  • Call preloading before flipping the flag, or
  • Drop the negation to always preload once currentSpaceId is known.

Example fix:

-          setLoading(false);
-          isLoadingRef.current = false;
-          initialDataLoadRef.current = true;
+          setLoading(false);
+          isLoadingRef.current = false;
+          // Preload remaining tabs before marking initial load complete
+          if (currentSpaceId) {
+            void loadRemainingTabs(currentSpaceId);
+          }
+          initialDataLoadRef.current = true;
-
-          // Load remaining tabs in the background if necessary
-          if (currentSpaceId && !initialDataLoadRef.current) {
-            void loadRemainingTabs(currentSpaceId);
-          }
🧹 Nitpick comments (9)
src/common/data/stores/app/space/spaceStore.ts (3)

495-503: Use remote order to update remoteSpaces[spaceId].order

You’re deriving the remote order from the local order when deleting. Prefer filtering the existing remote order for clarity and to avoid accidental divergence.

-          draft.space.remoteSpaces[spaceId].order = filter(
-            draft.space.localSpaces[spaceId].order,
-            (x) => x !== tabName,
-          );
+          draft.space.remoteSpaces[spaceId].order = filter(
+            draft.space.remoteSpaces[spaceId].order,
+            (x) => x !== tabName,
+          );

234-241: Deduplicate tab-name validation

validateTabName logic is duplicated across TabBar, space store, and homebase store. Extract to a shared util to avoid drift.


118-125: Unify network param types across API

SpaceActions.commitSpaceTabToDatabase uses network?: string while other methods use EtherScanChainName. Align to EtherScanChainName for consistency and better type safety.

Also applies to: 247-247

src/app/(spaces)/PublicSpace.tsx (2)

824-839: Tiny cleanup: avoid unnecessary await and undefined return

config isn’t a Promise; no need to await. Returning undefined explicitly isn’t required.

-        const resolvedConfig = await config;
-        return renameSpaceTab(
+        const resolvedConfig = config;
+        await renameSpaceTab(
           currentSpaceId,
           oldName,
           newName,
           resolvedConfig,
           isTokenSpace(spacePageData)
             ? (spacePageData.tokenData?.network as EtherScanChainName)
             : undefined,
         );

751-752: Remove unused cleanup return from event handler

Returning a cleanup function from switchTabTo (an event handler) has no effect; React ignores it.

-    // Clear flag on unmount
-    return () => { isMounted = false; };
+    // No cleanup return in event handler
src/common/components/organisms/TabBar.tsx (2)

271-287: Ensure duplicate checks are case-insensitive (align with stores)

Stores reject duplicates case-insensitively; generateUniqueTabName uses tabList.includes (case-sensitive). Normalize for consistency to avoid false “available” names.

-      while (tabList.includes(uniqueName) && iter <= maxIterations) {
+      const exists = (name: string) =>
+        tabList.some(t => t.toLowerCase() === name.toLowerCase());
+      while (exists(uniqueName) && iter <= maxIterations) {
         uniqueName = `${tabName} - ${iter}`;
         // Validate each generated name
         const validationError = validateTabName(uniqueName);
         if (validationError) {
           console.error("Could not generate unique name:", uniqueName, validationError);
           return null;
         }
         iter += 1;
       }

39-45: Deduplicate tab-name validation across layers

Same regex exists here and in stores. Extract to a shared util to prevent drift.

src/common/data/stores/app/homebase/homebaseTabsStore.ts (2)

106-112: Centralize validation util

Same regex appears in multiple places. Extract to a shared helper to avoid inconsistency.


79-86: Nit: extreme z-index

zIndex 9999999999999999 is excessive and can interfere with other overlays. Consider a scoped, reasonable layer or a portal to a controlled overlay root.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10a8ef9 and 03154b5.

📒 Files selected for processing (5)
  • src/app/(spaces)/PublicSpace.tsx (3 hunks)
  • src/app/home/[tabname]/page.tsx (1 hunks)
  • src/common/components/organisms/TabBar.tsx (2 hunks)
  • src/common/data/stores/app/homebase/homebaseTabsStore.ts (1 hunks)
  • src/common/data/stores/app/space/spaceStore.ts (5 hunks)
🔇 Additional comments (4)
src/common/data/stores/app/space/spaceStore.ts (1)

316-467: Rename flow is solid; ensure order commit is invoked upstream

Optimistic local rename + changedNames mapping + rollback look good. Make sure callers also commit the updated tab order (commitSpaceOrderToDatabase) after a successful rename. TabBar currently does this; verify all paths do.

src/app/home/[tabname]/page.tsx (1)

88-89: Prop shape aligned with async rename

renameTab now returns Promise | void; this stub fits and won’t be invoked with inEditMode=false.

src/common/components/organisms/TabBar.tsx (1)

213-219: Double-commit after rename may be redundant

renameTab now performs server-side rename with content commit. Committing the tab again here might be unnecessary. If renameTab guarantees commit, consider skipping commitTab() to reduce traffic.

src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)

380-396: Rename flow looks good: sanitization, optimistic update, rollback

Validation, duplicate checks, optimistic state switch, and rollback on failure are sound.

Also applies to: 398-409, 418-432, 453-475

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/common/data/stores/app/space/spaceStore.ts (2)

235-240: Regex bug: '-' creates unintended range; invalid chars slip through

The hyphen inside the character class makes a range 9-_, allowing punctuation like :;<=>?@[\]^_. This weakens validation.

Apply this fix:

-const validateTabName = (tabName: string): string | null => {
-  if (/[^a-zA-Z0-9-_ ]/.test(tabName)) {
+const validateTabName = (tabName: string): string | null => {
+  // Allow letters, numbers, underscore, hyphen, and spaces
+  if (/[^a-zA-Z0-9 _-]/.test(tabName)) {
     return "The tab name contains invalid characters. Only letters, numbers, hyphens, underscores, and spaces are allowed.";
   }
   return null;
 };

267-272: Rename commit can delete the just-saved tab; also guard remoteSpaces init

If oldTabName === tabName (no rename), we set tabs[tabName] then immediately delete it. Also, this block assumes remoteSpaces[spaceId] exists.

Apply this fix:

-        set((draft) => {
-          draft.space.remoteSpaces[spaceId].tabs[tabName] = localCopy;
-          delete draft.space.remoteSpaces[spaceId].tabs[oldTabName];
-          delete draft.space.localSpaces[spaceId].changedNames[tabName];
-        }, "commitSpaceTabToDatabase");
+        set((draft) => {
+          // Ensure remote space structure exists
+          if (!draft.space.remoteSpaces[spaceId]) {
+            draft.space.remoteSpaces[spaceId] = {
+              id: spaceId,
+              updatedAt: moment().toISOString(),
+              tabs: {},
+              order: [],
+            };
+          }
+          const remote = draft.space.remoteSpaces[spaceId];
+          remote.tabs[tabName] = localCopy;
+          if (oldTabName !== tabName) {
+            delete remote.tabs[oldTabName];
+          }
+          delete draft.space.localSpaces[spaceId].changedNames[tabName];
+        }, "commitSpaceTabToDatabase");
♻️ Duplicate comments (1)
src/common/data/stores/app/space/spaceStore.ts (1)

357-365: Past concern resolved: duplicate-name now throws

Throwing after the tooltip prevents UI/store desync. Looks good.

🧹 Nitpick comments (2)
src/common/data/stores/app/space/spaceStore.ts (2)

428-437: Persist tab order after rename

Rename updates order optimistically but never commits the remote tabOrder file. Call the order commit after the tab commit.

       if (commitPromise) {
         await commitPromise;
       }
+      // Persist the new order remotely as well
+      get().space.commitSpaceOrderToDatabase(spaceId, resolvedNetwork as EtherScanChainName);

Optional: if you need synchronous persistence here, consider calling .flush() on the debounced function (TypeScript cast required).


1380-1394: Local metadata init: preserve existing timestamps (optional)

Reassigning updatedAt each time can mask recency checks. Consider preserving updatedAt if the entry exists.

-                draft.space.localSpaces[spaceInfo.spaceId] = {
-                  id: spaceInfo.spaceId,
-                  updatedAt: moment().toISOString(),
+                draft.space.localSpaces[spaceInfo.spaceId] = {
+                  id: spaceInfo.spaceId,
+                  updatedAt: draft.space.localSpaces[spaceInfo.spaceId]?.updatedAt ?? moment().toISOString(),
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 03154b5 and fac9fce.

📒 Files selected for processing (2)
  • src/common/data/stores/app/homebase/homebaseTabsStore.ts (1 hunks)
  • src/common/data/stores/app/space/spaceStore.ts (5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/common/data/stores/app/space/spaceStore.ts (1)
src/constants/etherscanChainIds.ts (1)
  • EtherScanChainName (1-1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (2)
src/pages/api/space/homebase/tabs/index.ts (1)
  • UnsignedManageHomebaseTabsRequest (24-29)
src/common/lib/signedFiles.ts (1)
  • signSignable (54-62)
🔇 Additional comments (5)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (4)

380-396: LGTM! Robust input sanitization and validation.

The sanitization (trim, empty/unchanged check) and character validation (with clear error feedback) correctly prevent invalid renames.


398-410: Duplicate check now throws—fixes the past review concern.

The case-insensitive duplicate check now throws a DuplicateTabName error instead of silently returning, preventing UI/store desync.

Based on past review comments.


412-433: LGTM! Clean optimistic update with captured rollback state.

Capturing previous state (tabs, local/remote ordering) before the optimistic update enables comprehensive rollback on failure.


456-477: LGTM! Comprehensive rollback on error.

The rollback correctly restores tabs map, local/remote ordering, and currentTabName routing state, ensuring consistency after failures.

src/common/data/stores/app/space/spaceStore.ts (1)

125-136: No callers use removed newName parameter All three call sites pass only (spaceId, tabName, config), matching the updated signature.

Comment on lines 451 to +452
if (data.result === "success") {
// console.log('Successfully renamed tab:', { from: tabName, to: newName });
const currentTabData = get().homebase.tabs[tabName];
set((draft) => {
delete draft.homebase.tabs[tabName];
draft.homebase.tabs[newName] = currentTabData;
}, "renameHomebaseTab");
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify: Is tab ordering persisted after successful rename?

On success, the function returns without calling commitTabOrderingToDatabase(). The optimistic update (line 432) modifies tabOrdering.local, but it's unclear if this change is persisted.

Compare with createTab (line 332) and deleteTab (line 369), which both call commitTabOrderingToDatabase() after success.

Run the following script to check if there's an alternate ordering-commit mechanism or if callers (e.g., TabBar) handle it:


🏁 Script executed:

#!/bin/bash
# Description: Search for tab ordering commits after renameTab calls or in TabBar

# Check if TabBar or other callers commit ordering after renameTab
rg -A 10 'renameTab.*await|\.renameTab\(' --type=ts --type=tsx -g '!*.test.*' -g '!*.spec.*'

# Check for any auto-commit or periodic commit logic
rg -B 3 -A 5 'commitTabOrderingToDatabase' --type=ts --type=tsx -g '!*.test.*' -g '!*.spec.*'

Length of output: 55


🏁 Script executed:

#!/bin/bash
# Find all renameTab invocations
rg -n 'renameTab\(' -g '*.ts' -g '*.tsx' -g '!*.test.*' -g '!*.spec.*'

# Find all commitTabOrderingToDatabase invocations
rg -n 'commitTabOrderingToDatabase' -g '*.ts' -g '*.tsx' -g '!*.test.*' -g '!*.spec.*'

# Show renameTab implementation for context
sed -n '300,500p' src/common/data/stores/app/homebase/homebaseTabsStore.ts

Length of output: 7535


Persist tab ordering after rename
In renameTab’s if (data.result === "success") block you return without calling commitTabOrderingToDatabase(), so the optimistic update isn’t saved. Add await get().homebase.commitTabOrderingToDatabase() before the return.

🤖 Prompt for AI Agents
In src/common/data/stores/app/homebase/homebaseTabsStore.ts around lines 451 to
452, the renameTab function returns when data.result === "success" without
persisting the optimistic tab order change; add an await
get().homebase.commitTabOrderingToDatabase() immediately before the return so
the updated ordering is saved to the database.

Comment on lines +338 to +343
const validationError = validateTabName(sanitizedNewName);
if (validationError) {
showTooltipError("Invalid Tab Name", validationError);
console.error("Invalid tab name characters:", sanitizedNewName);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Invalid-name path should throw to abort UI rename flow

Returning after showing a tooltip can be interpreted as success by callers. Throw to keep semantics consistent with duplicate-name handling and to ensure TabBar aborts local changes.

-    if (validationError) {
-      showTooltipError("Invalid Tab Name", validationError);
-      console.error("Invalid tab name characters:", sanitizedNewName);
-      return;
-    }
+    if (validationError) {
+      showTooltipError("Invalid Tab Name", validationError);
+      const error = new Error("InvalidTabName");
+      error.name = "InvalidTabName";
+      throw error;
+    }
📝 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.

Suggested change
const validationError = validateTabName(sanitizedNewName);
if (validationError) {
showTooltipError("Invalid Tab Name", validationError);
console.error("Invalid tab name characters:", sanitizedNewName);
return;
}
const validationError = validateTabName(sanitizedNewName);
if (validationError) {
showTooltipError("Invalid Tab Name", validationError);
const error = new Error("InvalidTabName");
error.name = "InvalidTabName";
throw error;
}
🤖 Prompt for AI Agents
In src/common/data/stores/app/space/spaceStore.ts around lines 338 to 343, the
current code shows a tooltip and returns on invalid tab name which can be
treated as success by callers; change this to throw an error after showing the
tooltip so callers abort the UI rename flow consistently with duplicate-name
handling. Keep the showTooltipError call, then throw a new Error containing the
validation message (or a clear descriptive message plus validationError) so
upstream code can catch and stop local changes.

@Jhonattan2121
Copy link
Collaborator

@willyogo I ran the tests, and the bug I mentioned and found isn’t happening anymore, but the double re-render is still occurring. I noticed that when the tab is named, it also triggers a double re-render.

@willyogo
Copy link
Member Author

@willyogo I ran the tests, and the bug I mentioned and found isn’t happening anymore, but the double re-render is still occurring. I noticed that when the tab is named, it also triggers a double re-render.

thx @Jhonattan2121 🙏 the double re-render is very ugly UX and needs to be fixed, but isn't a regression from prod and is out of scope for this PR

@willyogo willyogo closed this Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug in canary LGFTP Looks Good From Testing Perspective

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants