Reviewed and merged GitHub PR #863

Summary

Closed, Completed

Understand
Reviewed PR description, comments, and commits.
Understand
Understand
Analyzed linked issues and comments.
Understand
Understand
Read relevant source files for context.
Understand
Edit
Added changelog entries.
Edit
Verify
Ran checks on the changes.
Verify

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/863

For each PR URL, do the following in order:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

continue

Now let me read the relevant source files to validate the changes against the current codebase:

Readsession-selector.ts

Readsession-manager.ts

ReadCHANGELOG.md

Now let me check the interactive-mode.ts and session-picker.ts files to understand the full context:

Readinteractive-mode.ts100 lines

Readsession-picker.ts

Let me read more of the interactive-mode.ts to see the resume command handling:

Readinteractive-mode.ts70 lines

Readinteractive-mode.ts80 lines

Now I have a full picture. Let me check if there are any issues mentioned in the PR:

This is a PR, not an issue. Let me verify the tests run correctly and check for any related issues that might be linked:

The test files don't exist on main branch yet (they're part of the PR). Let me verify the diff more thoroughly to identify any issues:

Now let me analyze the changes in more detail. Let me look at the hint line changes:

I see a discrepancy! Looking at the PR diff, the shortcut for "sort" is being changed from ctrl+r to ctrl+n, and ctrl+r is being repurposed for "rename". But the diff shows:

But the current main branch still has:

Let me also check if there's a keybinding conflict in the SessionList:

So in the current main branch:

ctrl+r is used for "sort"
There's no ctrl+n binding

In the PR diff:

ctrl+n will be used for "sort"
ctrl+r will be used for "rename"

This is a keybinding change that may impact users who are used to ctrl+r for sorting. Let me also check the README doc:

Looking at line 555-556, the README documents Ctrl+P and Ctrl+D but not Ctrl+R for sort. The PR adds documentation for Ctrl+R rename at that location.

Now let me check if there are any docs that need updating:

Readsession.md

The session.md mentions the /resume delete feature but not the sort toggle. The README at line 555-556 documents Ctrl+P and Ctrl+D. The PR adds Ctrl+R for rename.

Now let me provide my review:

PR: https://github.com/badlogic/pi-mono/pull/863

Changelog:

Missing. A changelog entry is required before merge. The entry should go in packages/coding-agent/CHANGELOG.md under ## [Unreleased] → ### Added for the rename feature, and could include a note about the "modified timestamp" fix as well.

Good:

Well-structured feature: Allows renaming sessions directly from /resume picker without opening them first, which is a UX improvement
Proper separation of concerns: rename mode uses full-page replacement instead of overlay, consistent with existing patterns
Smart "modified timestamp" change: Using last message timestamp instead of file mtime prevents metadata-only changes (like renaming) from reordering the recent list
Refresh logic: After rename/delete, the session list properly refreshes to show updated names immediately
Test coverage: Two new test files covering rename hint behavior, rename flow, and modified timestamp derivation
Correct keybinding conflict resolution: Ctrl+R was previously used for "sort", now moved to Ctrl+N to free Ctrl+R for "rename"
Proper focus handling: The focus is now set to the SessionSelectorComponent itself (not just getSessionList()) to allow rename input handling

Bad:

Missing changelog entry: PR needs an entry before merge
Missing pkg:coding-agent label: Should be added to the PR
Keybinding change not documented for sort: The README documents Ctrl+R for rename but doesn't mention that "sort" moved from Ctrl+R to Ctrl+N. Users familiar with Ctrl+R for sorting will be surprised. The hint line in the UI is updated, but changelog should note this keybinding change.
session.md doc not updated: The packages/coding-agent/docs/session.md mentions /resume delete via Ctrl+D but could mention rename via Ctrl+R

Ugly:

None identified. The code is clean and follows existing patterns.

Questions or Assumptions:

Is the Ctrl+N keybinding intentional for sort? Ctrl+N is often associated with "new" in other applications. Was an alternative considered (e.g., Ctrl+S for sort)?
The showRenameHint option defaults to this.canRename when not explicitly provided in options. In the --resume CLI picker (session-picker.ts), it's explicitly set to false. This is correct behavior, but worth confirming the intent is that rename is only available in interactive /resume, not CLI --resume.

Change summary:

Session rename from /resume picker: Press Ctrl+R to enter rename mode, type new name, press Enter to save or Esc/Ctrl+C to cancel
Modified timestamp fix: Session "modified" time now uses last user/assistant message timestamp (with fallbacks to header timestamp and file mtime), so renaming doesn't reorder the list
Keybinding change: Sort toggle moved from Ctrl+R to Ctrl+N to free up Ctrl+R for rename
Refresh after mutations: Session list refreshes after rename/delete operations
Refactored loadScope() helper: Consolidated loading logic reduces code duplication

Tests:

session-info-modified-timestamp.test.ts: Tests that SessionInfo.modified uses last message timestamp instead of file mtime
session-selector-rename.test.ts: Tests rename hint visibility (shown in /resume, hidden in --resume), and rename flow (Ctrl+R → UI → typing → Enter)

merge via gh cli, pull from origin, change to CTRL+S for sort if its not already used

Let me check if Ctrl+S is already used in the session selector:

Ctrl+S is not used in the session selector. Let me merge the PR, pull, and make the change:

Now let me read the current session-selector.ts and update ctrl+n to ctrl+s for sort:

Editedsession-selector.ts

Now let me add the changelog entry:

ReadCHANGELOG.md80 lines

EditedCHANGELOG.md

Now let me run the check:

Now let me commit and push:

Done. I merged PR #863 via squash merge, pulled the changes, and updated the sort keybinding from Ctrl+N to Ctrl+S. Also added changelog entries:

Added: Session renaming in /resume picker via Ctrl+R
Changed: Sort toggle moved from Ctrl+R to Ctrl+S
Fixed: Session "modified" time now uses last message timestamp instead of file mtime