Skip to content

Fix moved session project context - #88

Merged
touch2be merged 1 commit into
masterfrom
fix/context-menu-onclick
Jun 13, 2026
Merged

touch2be merged 1 commit into
masterfrom
fix/context-menu-onclick

Conversation

@touch2be

Copy link
Copy Markdown
Collaborator

Summary

  • preserve a stable native project directory for sessions moved between Chats and Projects
  • use assigned/native session metadata consistently for prompts, queues, refreshes, aborts, and sidebar footer state
  • add regression coverage for moving sessions A -> B, B -> A, and B -> Chats/native

Testing

  • vp test src/hooks/agent-session-utils.test.ts src/hooks/agent-directory-change-notice.test.ts src/hooks/agent-local-intent.test.ts src/hooks/agent-send.test.ts src/hooks/agent-queue-dispatch.test.ts src/hooks/agent-reducer.test.ts src/hooks/agent-session-lifecycle.test.ts src/components/sidebar/use-sidebar-model.test.ts
  • vp check (passes with existing pi-bridge await-thenable warnings)

@github-actions

Copy link
Copy Markdown

PR Review: Fix moved session project context

Confidence Score: 4/5

The PR is well-structured with thorough test coverage. The architecture cleanly separates the concept of a session's "native" (original) project directory from its "assigned" (moved-to) directory, with optimistic UI updates and a system-append notice mechanism for context changes.

Issues Detected

1. Race condition in optimistic meta cleanup (src/components/AppSidebar.tsx:142-161)

The cleanup effect compares committed sessionMeta against optimistic meta on every render, but the comparison uses originMode/assignedProjectDir/detachedFromProject while ignoring nativeProjectDir, pendingDirectoryChangeNotice, and hideSystemAppendBlocks. If the committed meta matches on those three fields but differs on others, the optimistic entry is removed prematurely, and the error handler in moveSessionToProjectOptimistic may also remove it on a different operation's failure:

// Only compares 3 fields - but meta has 11+ fields
if (
  committedMeta?.originMode === optimisticMeta.originMode &&
  normalizeProjectPath(committedMeta?.assignedProjectDir ?? "") ===
    normalizeProjectPath(optimisticMeta.assignedProjectDir ?? "") &&
  committedMeta?.detachedFromProject === optimisticMeta.detachedFromProject
) {
  delete next[sessionId];
}

2. UI flicker during rapid re-moves (src/components/AppSidebar.tsx:163-198)

flushSync applies the optimistic update synchronously, but if the user rapidly moves a session between projects, the error handler on an earlier in-flight operation clears all optimistic meta for that session ID, undoing the latest optimistic move:

// Error from stale in-flight op clears the latest optimistic meta
} catch (error) {
  setOptimisticSessionMeta((current) => {
    const { [sessionId]: _removed, ...rest } = current;
    return rest;
  });
  throw error;
}

3. Missing baseUrl in ProjectTarget from getSessionProjectTarget (src/hooks/agent-session-utils.ts:108-118)

The returned ProjectTarget explicitly omits baseUrl. Every caller must re-derive it from workspace config. Inconsistent augmentation patterns exist — some callers do { ...target, workspaceId, baseUrl } while others pass projectTarget ?? undefined directly. A new caller that forgets the augmentation will silently route to the wrong server:

// Returns only directory + workspaceId — no baseUrl
export function getSessionProjectTarget(
  session: Session | undefined | null,
  meta?: SessionMeta,
): ProjectTarget | null {
  const directory = getEffectiveSessionDirectory(session, meta);
  if (!directory) return null;
  return {
    directory,
    workspaceId: getSessionWorkspaceId(session) ?? undefined,
  };
}

4. getSessionLookup re-entrant state access pattern (src/hooks/agent-session-queue.ts:47-53)

lookup() calls getState() which returns a fresh snapshot, then immediately passes state.sessionMeta (from the snapshot) alongside a session looked up from the same snapshot. If getState is called again inside enqueuePrompt via getSessionLookup, the session and meta could be from different snapshots — the session might have been moved/deleted between calls:

const lookup = (sessionId: string) => {
  const state = getState();
  return getSessionLookup(
    state.sessions.find((session) => session.id === sessionId),
    state.sessionMeta,  // from same snapshot — OK for single call
  );
};

5. pendingDirectoryChangeNotice is never explicitly reset on error (src/hooks/agent-directory-change-notice.ts:38-45)

The planDirectoryChangePrompt sets pendingDirectoryChangeNotice: false only when it successfully returns a meta patch. If sendPromptToAgent or the enclosing flow throws before planDirectoryChangePrompt is called, the pendingDirectoryChangeNotice flag remains true and the system-append block will be prepended to the next successful prompt instead, potentially misleading the model:

// Only clears the flag when the notice is generated
metaPatch: {
  pendingDirectoryChangeNotice: false,
  hideSystemAppendBlocks: true,
}

Suggestions

  • Add a nativeProjectDir check to the optimistic meta cleanup comparison in AppSidebar.tsx to prevent premature cleanup.
  • Add a deduplication guard in moveSessionToProjectOptimistic (e.g., a generation counter per sessionId) to reject stale in-flight operations.
  • Consider including baseUrl in getSessionProjectTarget or adding a helper function resolveSessionTarget that augments with workspace config.
  • Add a safety net in planDirectoryChangePrompt to reset pendingDirectoryChangeNotice on error paths so the notice isn't delivered belatedly.

New%20session%20-%202026-06-13T15%3A47%3A19.385Z
opencode session  |  github run

@touch2be
touch2be merged commit 6fb981a into master Jun 13, 2026
1 check passed
@akemmanuel
akemmanuel deleted the fix/context-menu-onclick branch October 2, 2026 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant