Skip to content

Fix: Reuse untitled session when new story is created#164

Merged
realproject7 merged 3 commits intomainfrom
task/162-fix-untitled-session-reuse
Apr 24, 2026
Merged

Fix: Reuse untitled session when new story is created#164
realproject7 merged 3 commits intomainfrom
task/162-fix-untitled-session-reuse

Conversation

@realproject7
Copy link
Copy Markdown
Owner

Summary

  • Adds POST /api/terminal/rename backend endpoint that moves a PTY session entry to a new key without killing the process
  • Frontend polling now calls rename when a new story folder is detected, instead of letting a new session auto-create
  • Preserves Claude conversation context — no orphaned untitled sessions

Fixes #162

Test plan

  • Click "+ New Story", brainstorm with Claude until it creates a story folder
  • Verify the Untitled tab renames to the story name (no new tab spawned)
  • Verify Claude conversation context is preserved (same session, same PID)
  • Verify no orphaned Untitled sessions remain
  • Verify closing and reopening the story tab still resumes correctly

🤖 Generated with Claude Code

Add POST /api/terminal/rename endpoint that moves a PTY session
entry to a new key without killing the process. Frontend polling
now calls rename instead of letting a new session auto-create,
preserving the Claude conversation context.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES

Summary

The PR is close, but the rename flow still has two blocking session-lifecycle bugs. One leaves renamed PTY entries stuck in the active-session map after the process exits, and the other still allows a second session to spawn if the async rename does not complete before selection changes.

Findings

  • [high] Renaming the ptySessions map key does not update the exit handler's lookup key, so a renamed session is no longer cleaned up when the PTY exits.
    • File: app/routes/terminal.ts:203
    • Suggestion: store the current story/session key on the session object or otherwise make the term.onExit cleanup follow the renamed key before moving the map entry.
  • [high] The new-story polling path fires renameRef.current(oldName, name) without awaiting success, then immediately removes the untitled entry and selects the new story.
    • File: app/web/components/StoriesPage.tsx:90
    • Suggestion: await the rename result and only switch selection / consume the untitled session after the rename succeeds; otherwise a slow or failed rename can still auto-spawn a second paper-chair session while the original untitled PTY keeps running.

Decision

Requesting changes because the current implementation can still orphan or duplicate sessions, which violates the issue's acceptance criteria around reusing the original Untitled PTY.

@realproject7
Copy link
Copy Markdown
Owner Author

@re2 Review — REQUEST CHANGES

Overall the approach is clean — rename-in-place is the right call for preserving the PTY process and Claude conversation context. One issue needs fixing:

🔴 Missing server-side guard for existing newName (terminal.ts)

The frontend checks sessions.has(newName) before calling rename, but the backend endpoint does not check ptySessions.has(newName). If two requests race or a client bypasses the check, the rename would silently overwrite an existing live session — orphaning its PTY process and WebSocket.

Fix: Add a guard after the session lookup:

if (ptySessions.has(newName)) return c.json({ error: "Target session already exists" }, 409);

Same applies to the session map — if sessionMap[newName] already exists, the old entry gets lost.

✅ Everything else looks good

  • safeName() validation on both inputs — correct
  • No-op on oldName === newName — good edge case handling
  • Scrollback migration with cleanup of old key — solid
  • disconnected Set update in React state — correct
  • useCallback deps and ref cleanup in effect — clean
  • Patch version bump — appropriate

One fix, then APPROVE.

…session

- onExit handler now finds session by term reference instead of
  captured storyName, so cleanup works after rename
- StoriesPage awaits rename result and only consumes the untitled
  session on success, preventing orphaned PTYs on failure

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES

Summary

The original rename-lifecycle bugs are fixed on the current head, but one backend blocker remains in the rename endpoint.

Findings

  • [high] POST /api/terminal/rename still does not reject newName when that target session key already exists in ptySessions, so a duplicate/racy rename can overwrite a live session entry and orphan its PTY.
    • File: app/routes/terminal.ts:206
    • Suggestion: add an explicit guard before moving the map entry, e.g. return 409 when ptySessions.has(newName).

Decision

Keeping this in request-changes state until the rename endpoint protects against collisions. The earlier cleanup/await fixes look correct.

Reject rename when target session already exists in ptySessions
to prevent overwriting a live session and orphaning its PTY.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

Verdict: COMMENT

Summary

The current head addresses the remaining rename-collision blocker. I found no further code issues in the rename/session-reuse flow.

Findings

  • No remaining code blockers on the current diff.

Decision

Holding final approval until the current lint-and-typecheck run is green.

Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

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

Verdict: APPROVE

Summary

The rename/session-reuse implementation now satisfies issue #162. The current head fixes the session cleanup, async rename sequencing, and rename-collision handling.

Findings

  • No remaining blocking issues.

Decision

Approving because the implementation meets the acceptance criteria and lint-and-typecheck passed on the current head.

@realproject7
Copy link
Copy Markdown
Owner Author

@re2 Re-review — APPROVE ✅

All issues addressed across the three commits:

  1. 409 collision guard (my original request) — ptySessions.has(newName) check now in place, returns 409. Good.
  2. onExit handler fix (@re1's catch) — now finds the session by term reference instead of the captured closure name. Correct — rename no longer breaks cleanup.
  3. Await rename + boolean returnStoriesPage now awaits the rename and only consumes the untitled session on success. Failed renames leave state intact. Solid error handling.

CI (lint-and-typecheck) is passing. No remaining blockers from my side.

APPROVE

@realproject7 realproject7 merged commit 44b6ff5 into main Apr 24, 2026
1 check passed
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.

[Bug] New story creates separate terminal session — should reuse the Untitled session

2 participants