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()
}}
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
|
Hi @MFA-G, thanks for the thorough work here — the bug is real, your root-cause write-up is spot on, and the tests are great. 🙌 My main ask is to simplify. The issue is just "Enter should confirm the rename," but this rebuilds the whole dialog submit flow by hand: a Could you trim it to the minimal fix — keep antd's built-in |
Confirming through antd's own OK button keeps submit, the loading state and close-on-success on the existing onOk path instead of reimplementing them for the keyboard.
dosubot
Bot
added
size:S
|
Thanks @ashrafchowdury — you're right, and it's trimmed in aff223d. I'd been chasing "Enter must behave exactly like the button" by rebuilding the button's behaviour, when the simpler answer is to just press the button. const okClass = "rename-session-ok"
...
onPressEnter={(event) =>
event.currentTarget
.closest('[role="dialog"]')
?.querySelector<HTMLButtonElement>(`.${okClass}`)
?.click()
}
...
okButtonProps: {className: okClass},16 added lines, no antd internals touched — the loading state, close-on-success, and the double-submit guard all come from antd's own One note on why it's a marker class rather than a ref: Tests are down from 6 to 4 — the two that only existed to pin the hand-rolled state machine (keep-open-on-error, blank-name disabled button) are dropped. What's left covers the actual contract: Enter submits the edited name, Enter ignores a blank name, Enter goes through
|
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
thanks for the pr. i found one thing that needs a change before qa.
| const input = renameInput() | ||
| await type(input!, " ") | ||
| await pressEnter(input!) | ||
|
|
There was a problem hiding this comment.
This only proves that a blank Enter does not send a rename request. Enter now clicks the OK button, and onOk is async, so the blank return resolves and Ant Design closes the modal. That breaks the stated contract that a blank name leaves the dialog open. Please assert renameInput() is still present here and make the blank OK path keep the modal open.
Disable the Rename button while the field is blank, so both Enter (which clicks it) and the button itself are no-ops and leave the dialog up.
|
Thanks @mmabrouk — you are right, fixed in 409f1be. Enter clicks the Rename button, and Rather than special-casing the blank return, the button that performs the rename is now disabled while the name is blank. Enter clicks that button, so both paths become no-ops from the same condition and the dialog stays open: const okButtonProps = () => ({className: okClass, disabled: !next.trim()})
...
onChange={(event) => {
next = event.target.value
dialog.update({okButtonProps: okButtonProps()})
}}Tests now assert the contract you asked for, on both paths:
One test-harness fix was needed for the recovery assertion:
|
Main moved the session verbs into @agenta/sessions-ui, so the Enter fix moves with them and the app hook goes back to main's thin adapter. Two things the rename would have silently lost in the new renderer: - The confirm is now a Radix AlertDialog (`role="alertdialog"`), so the `[role="dialog"]` lookup that reaches the OK button found nothing. - `Input` from @agenta/ui/ui is a native control and does not take antd's `onPressEnter`, so the handler was dropped on the floor. It is `onKeyDown` now. The suite moves to the package with it, retargeted at the marker class and `data-loading` instead of antd's button classes, and dismisses leftover confirms in teardown (the store is module-global and there is no `Modal.destroyAll` any more). Needs the JSX transform the other .tsx suites use.
2a939bc
into
Agenta-AI:release/v0.114.1
|
Thank you for the PR @MFA-G @all-contributors please add @MFA-G for code |
|
I've put up a pull request to add @MFA-G! 🎉 |
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