fix(sessions): confirm the rename dialog on Enter - #6011
Conversation
The Rename session modal has a single text field, but Enter did nothing: the only way to confirm was clicking the Rename button. Wire onPressEnter on the Input to the same submit path as onOk, closing the dialog first so the flow matches a button click. A blank name is ignored, exactly as onOk already did. Fixes Agenta-AI#5951
dosubot
Bot
added
the
size:M
|
@MFA-G is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
|
✅ Thanks @MFA-G! This PR now meets the contribution requirements and has been reopened. A maintainer will review it soon. |
|
|
github-actions
Bot
added
the
incomplete-pr
There was a problem hiding this comment.
Pull request overview
This PR fixes the session rename UX in the Agent Chat slice by making the “Rename session” confirm modal submit via the Enter key (in addition to the Rename button), and adds a focused DOM-level test to prevent regressions.
Changes:
- Refactors the rename confirm modal to share a single
submithandler between the OK button and keyboard submission. - Adds
onPressEnterto the modal input so Enter triggers rename. - Introduces a new Vitest test covering Enter-to-confirm and blank-name behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| web/oss/src/components/AgentChatSlice/hooks/useSessionActions.tsx | Adds an Enter key handler to the rename modal input and refactors the submission logic into a shared closure. |
| web/oss/src/components/AgentChatSlice/hooks/useSessionActions.test.tsx | Adds DOM-level tests validating the Enter key contract for the rename modal. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| onPressEnter={() => { | ||
| if (!next.trim()) return | ||
| dialog.current?.destroy() | ||
| void submit() | ||
| }} | ||
| /> | ||
| ), | ||
| okText: "Rename", | ||
| onOk: async () => { | ||
| const title = next.trim() | ||
| if (!title) return | ||
| if (isCached(target) && target.appId) { | ||
| await store.set(renameSessionAtomFamily(target.appId), { | ||
| id: target.sessionId, | ||
| title, | ||
| }) | ||
| } else { | ||
| const ok = await setSessionHeader({ | ||
| sessionId: target.sessionId, | ||
| projectId, | ||
| name: title, | ||
| }) | ||
| if (!ok) { | ||
| message.error("Couldn't rename this session") | ||
| return | ||
| } | ||
| } | ||
| revalidate() | ||
| }, | ||
| onOk: submit, |
| const host = document.createElement("div") | ||
| document.body.appendChild(host) | ||
| await act(async () => { | ||
| createRoot(host).render( | ||
| <App> | ||
| <Probe /> | ||
| </App>, | ||
| ) | ||
| }) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe session rename dialog now supports Enter-key confirmation. Shared submission logic validates names, manages loading and modal state, preserves failed edits, and updates the session. Tests cover successful, blank, pending, failed, and button-state behavior. ChangesSession rename behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The rename dialog now confirms a trimmed nonblank name when Enter is pressed, while blank names keep the dialog open. The change is mergeable with owner awareness that the repository-required frontend lint-fix command should still be run before merge. Sequence Diagram(s)sequenceDiagram
participant User
participant RenameDialog
participant useSessionActions
participant RenameAction
User->>RenameDialog: Press Enter or click Rename
RenameDialog->>useSessionActions: Submit edited name
useSessionActions->>RenameAction: Rename with trimmed name
RenameAction-->>useSessionActions: Resolve or reject
useSessionActions->>RenameDialog: Close on success or preserve edits on failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
github-actions
Bot
removed
the
incomplete-pr
|
@MFA-G thank you for the PR, please sign the CLA for us to be able to merge it. Thanks! Also don't forget to address the comments |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Approved conditioned by the comment above
Address review: the Enter path guarded a blank name but onOk did not, so the two confirmation paths could drift. The Rename button is now disabled while the field is blank, which is the same condition Enter checks, and the test unmounts its React root in afterEach like ProjectWatch.test.tsx.
|
Thanks @mmabrouk and @copilot — both review comments addressed in 1e60d09. 1. Blank-name guard could drift between Enter and the Rename button. The Enter handler checked const isBlank = () => !next.trim()
...
onChange={(event) => {
next = event.target.value
dialog.current?.update({okButtonProps: {disabled: isBlank()}})
}}
onPressEnter={() => {
if (isBlank()) return
...
}}
okButtonProps: {disabled: isBlank()},That also makes the constraint visible to the user instead of a silent no-op on click. 2. Leaked React root in the test. Also added a third test asserting the button's disabled state tracks the field (enabled → blank disables → typing re-enables), so the two paths can't drift again.
Re: the CLA — I'll get that signed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/oss/src/components/AgentChatSlice/hooks/useSessionActions.tsx (1)
105-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the new implementation comments.
web/oss/src/components/AgentChatSlice/hooks/useSessionActions.tsx#L105-L109: Remove the explanation or reduce it to one short line. The shared predicate makes the intent clear.web/oss/src/components/AgentChatSlice/hooks/useSessionActions.test.tsx#L38-L44: Reduce the mock-path explanation to one short line.As per coding guidelines: “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: cc7baf54-8227-4049-b9d2-09da172d187d
📒 Files selected for processing (2)
web/oss/src/components/AgentChatSlice/hooks/useSessionActions.test.tsxweb/oss/src/components/AgentChatSlice/hooks/useSessionActions.tsx
The blank-name cases deliberately leave the confirm dialog open, and root.unmount() does not close it: modal.confirm instances live in antd's global destroy registry, not the React root. Call Modal.destroyAll() before unmounting so no dialog leaks into the next test.
|
Thanks @coderabbitai — good catch, fixed in 4e43bed. You're right that Suite still green: |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
web/oss/src/components/AgentChatSlice/hooks/useSessionActions.test.tsx:99
- The test dispatches a
KeyboardEventusing the deprecatedkeyCodefield and anas nevercast to satisfy typing. This is brittle and unnecessary here;key/codeare sufficient for antd’s Enter handling and keep the test type-safe.
await act(async () => {
input.dispatchEvent(
new KeyboardEvent("keydown", {key: "Enter", keyCode: 13, bubbles: true} as never),
)
})
web/oss/src/components/AgentChatSlice/hooks/useSessionActions.tsx:130
onPressEnterdestroys the confirm immediately and then runssubmit(). This bypassesModal.confirm’s normalonOkpromise lifecycle (e.g., keeping the modal open + showing the OK button loading state while the async rename runs), so Enter won’t behave the same as clicking Rename.
onPressEnter={() => {
if (isBlank()) return
dialog.current?.destroy()
void submit()
}}
Enter destroyed the dialog before submitting, so it skipped the loading state and error handling that clicking Rename gets from onOk. Both paths now share one confirm() that keeps the dialog open while the rename is in flight.
|
Thanks @Copilot — both suppressed comments were fair, fixed in e872786. 1. const confirm = async () => {
if (isBlank()) return
dialog.current?.update({okButtonProps: {loading: true}, cancelButtonProps: {disabled: true}})
try {
await submit()
dialog.current?.destroy()
} catch {
dialog.current?.update({
okButtonProps: {loading: false, disabled: isBlank()},
cancelButtonProps: {disabled: false},
})
}
}Enter is now literally 2. Suite green: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
web/oss/src/components/AgentChatSlice/hooks/useSessionActions.tsx:132
onPressEntercallsconfirm()directly, butconfirm()has no in-flight guard. Users can hit Enter multiple times while the request is pending (the input stays enabled), which can issue duplicate rename mutations and potentially race the dialog lifecycle. Add a localconfirmingflag (or disable the input) so the Enter path is idempotent while a rename is in flight.
const confirm = async () => {
if (isBlank()) return
dialog.current?.update({
okButtonProps: {loading: true},
cancelButtonProps: {disabled: true},
The OK button's loading state blocks a second click while a rename is in flight, but the input stays enabled, so repeated Enter presses could each call submit() and issue duplicate rename mutations.
|
Thanks @copilot — valid, fixed in 0378eaf.
let pending = false
const confirm = async () => {
if (pending || isBlank()) return
pending = true
...
} finally {
pending = false
}
}The On a regression test for this: I tried to add one and could not make it prove anything, so I left it out rather than ship a test that passes either way. The suite drives the dialog by dispatching native ( The existing 4 rename tests still pass ( |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
web/oss/src/components/AgentChatSlice/hooks/useSessionActions.tsx:152
- The
onChangehandler updatesokButtonPropswithout preserving theloadingflag set by the Enter path (confirm()), so typing while a rename is pending will clear the loading state and can re-enable the OK button even though the request is still in-flight.
onChange={(event) => {
next = event.target.value
dialog.current?.update({okButtonProps: {disabled: isBlank()}})
web/oss/src/components/AgentChatSlice/hooks/useSessionActions.test.tsx:98
pressEnter()dispatches the key event insideact(), but it doesn't wait for the async rename started byonPressEnter={() => void confirm()}to settle. Assertions immediately afterawait pressEnter(...)can race with the modal updates / destroy, making the tests flaky.
const pressEnter = async (input: HTMLInputElement) => {
await act(async () => {
input.dispatchEvent(new KeyboardEvent("keydown", {key: "Enter", bubbles: true}))
})
}
| const submit = async () => { | ||
| const title = next.trim() | ||
| if (!title) return | ||
| if (isCached(target) && target.appId) { | ||
| await store.set(renameSessionAtomFamily(target.appId), { | ||
| id: target.sessionId, | ||
| title, | ||
| }) | ||
| } else { | ||
| const ok = await setSessionHeader({ | ||
| sessionId: target.sessionId, | ||
| projectId, | ||
| name: title, | ||
| }) | ||
| if (!ok) { | ||
| message.error("Couldn't rename this session") | ||
| return | ||
| } | ||
| } | ||
| revalidate() | ||
| } |
There was a problem hiding this comment.
Good catch — fixed in f7aec58.
You were right on both counts, and the second one was the worse bug: onOk: submit meant the button path had the same problem, not just Enter. A failed rename closed the dialog and threw the typed name away behind an error toast.
submit() now rejects instead of returning on failure. While wiring that up, returning the rejection from onOk turned out to be the wrong lever: antd's ActionButton re-raises a rejected onOk as an unhandled rejection (_util/ActionButton.js, the Promise.reject(e) branch), which vitest flags as an unhandled error even though the dialog behaves correctly. So the OK button now confirms through the same confirm() helper as Enter, via okButtonProps.onClick — antd spreads okButtonProps over its own handler, so this replaces it. One lifecycle, one in-flight guard, one failure path for both.
Two tests were added, both of which fail on the previous commit:
✓ confirms on the Rename button with the edited name
× keeps the dialog open when the rename fails, confirmed with Enter
× keeps the dialog open when the rename fails, confirmed with the Rename button
and pass now:
Test Files 1 passed (1)
Tests 7 passed (7)
eslint clean on both files; tsgo --noEmit reports nothing for them.
submit() returned normally when setSessionHeader() reported failure, so both confirmation paths treated a failed rename as a success: the button's onOk closed the dialog, and Enter's catch block never ran, discarding the typed name behind an error toast. submit() now rejects on failure. The OK button confirms through the same confirm() helper as Enter rather than through onOk, because onOk's only way to hold the dialog open is a rejected promise, which antd re-raises as an unhandled rejection.
dosubot
Bot
added
size:L
Fixes #5951.
Summary
What changed: the "Rename session" dialog now confirms on Enter.
Why: it is a modal with a single text field, so Enter is the obvious way to confirm it — but nothing happened. The dialog stayed open and the name was unchanged, and the only way out was reaching for the mouse and clicking Rename.
Root cause: the
<Input>insidemodal.confirminuseSessionActions.renamehad noonPressEnter, and antd'sModal.confirmdoes not submit on Enter by itself — there is no<form>wrapping the field, and the OK button is not the focused element.The fix (
web/oss/src/components/AgentChatSlice/hooks/useSessionActions.tsx):submitclosure, andonOknow is that closure — so the Enter path and the button path cannot drift.onPressEnteron theInputdestroys the dialog and runssubmit(), which is exactly what a button click does (antd closes the modal itself ononOk).onOkalready did.No change to the cached-vs-remote branching, the error toast, or the revalidation.
Testing
Verified locally
Mounted the real hook in a browser against a stubbed
setSessionHeaderand drove it with a genuine Chrome key press (not a synthetic event), on the branch and onmain:mainUntitled session(unchanged)Pricing agent QAPATCH /sessions/session-1 → name: "Pricing agent QA"Also ran:
Added or updated tests
web/oss/src/components/AgentChatSlice/hooks/useSessionActions.test.tsx— two cases:setSessionHeaderwith the trimmed name and closes the dialog.Rendered with
react-dom/clientrather than a testing library (the repo has no@testing-library/react, per the note inApprovedContentManifest.test.tsx);modal.confirmrenders intodocument.body, so the assertions read the real DOM either way.Verified the test actually catches the regression: reverting only the hook change makes the first case fail, and it passes with the fix in place.
QA follow-up
Worth a click-through of the rename entry point on both surfaces that use this hook — the sessions list and the playground's session bar — to confirm Enter and the Rename button behave identically, including on a session that is in the local tab cache (the atom path, which the harness above did not exercise; the unit test and the recording both take the remote path). Escape still cancels.
Demo
Same flow, same key press,
mainvs. this branch.Before (
main) — Enter does nothing, the dialog stays open:After (this branch) — Enter confirms, the dialog closes and the rename is sent:
Both clips render the real
useSessionActionshook; onlysetSessionHeaderis stubbed, and it prints the request it would send.Checklist