Make proposed AI edits UI consistent with editor styling - #10595
Open
akshayka wants to merge 2 commits into
Open
Make proposed AI edits UI consistent with editor styling#10595akshayka wants to merge 2 commits into
akshayka wants to merge 2 commits into
Conversation
AI-generated notebook cells should look like part of the editor and make the available actions clear. Replace the distracting proposal glows with the existing cell styling, keep the review controls inside each cell, and use explicit labels for proposed deletions. Do not offer to run a cell after the user accepts its deletion.
AI-generated cells and inline refactoring need the same review language and button treatment. Keep added-cell status concise, use explicit keep and revert actions for changes, and make normal edits, runs, and deletes clear the AI review state so the proposal UI does not persist after the user takes ownership of a cell.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
akshayka
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the frontend “staged AI cell” review experience so AI-proposed edits visually match standard editor cells, and so proposal review actions/labels better reflect “keep vs revert” semantics across add/change/delete proposals (including the inline refactor banner flow).
Changes:
- Replaces AI proposal “glow/opacity” styling with standard cell styling plus border-color cues, and introduces a consistent AI cell footer style.
- Updates per-cell and global proposal review labels/actions (e.g., “Keep change” / “Revert change”, “Keep all” / “Discard all”), and adjusts run-all behavior to skip deletion proposals.
- Clears staged/proposal state in more user-ownership flows (running, editing, deleting cells) so review markers don’t linger.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/css/app/Cell.css | Switches AI proposal visuals to border-based styling and adds .mo-ai-cell-footer styling. |
| frontend/src/components/editor/notebook-cell.tsx | Moves staged AI footer rendering into the main cell container so it participates in cell layout/styling. |
| frontend/src/components/editor/chrome/wrapper/pending-ai-cells.tsx | Updates global tray labels and prevents running staged deletion proposals. |
| frontend/src/components/editor/cell/useRunCells.ts | Clears staged AI state on run to reflect user ownership after execution. |
| frontend/src/components/editor/cell/useDeleteCell.tsx | Clears staged AI state when deleting one or many cells. |
| frontend/src/components/editor/cell/StagedAICell.tsx | Revises staged AI footer messaging and conditionally shows review actions based on proposal type. |
| frontend/src/components/editor/cell/code/cell-editor.tsx | Clears staged AI state on direct user edits in CodeMirror. |
| frontend/src/components/editor/ai/completion-handlers.tsx | Adds label customization support to completion action buttons. |
| frontend/src/components/editor/ai/ai-completion-editor.tsx | Aligns inline refactor banner styling/copy with the new “keep vs revert” terminology. |
Suppressed comments (1)
frontend/src/components/editor/cell/useDeleteCell.tsx:77
- Same as the single-cell delete: this
get+setpattern can drop concurrent staged AI updates. Use a functionalstore.setupdater to remove all deleted cellIds from the lateststagedAICellsAtomvalue atomically.
const stagedAICells = store.get(stagedAICellsAtom);
if (stagedAICells.size > 0) {
const nextStagedAICells = new Map(stagedAICells);
for (const cellId of cellIds) {
nextStagedAICells.delete(cellId);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+53
to
+57
| }> = ({ | ||
| isLoading, | ||
| onAccept, | ||
| onDecline, | ||
| runCell, |
Comment on lines
+27
to
+32
| const stagedAICells = store.get(stagedAICellsAtom); | ||
| if (stagedAICells.has(cellId)) { | ||
| const nextStagedAICells = new Map(stagedAICells); | ||
| nextStagedAICells.delete(cellId); | ||
| store.set(stagedAICellsAtom, nextStagedAICells); | ||
| } |
|
|
||
| <p className="transition-opacity duration-200 text-muted-foreground"> | ||
| {isLoading ? "Generating fix..." : "Showing fix"} | ||
| {isLoading ? "Generating change..." : "AI changed this cell"} |
Comment on lines
+74
to
+82
| <CompletionActionsCellFooter | ||
| isLoading={false} | ||
| onAccept={() => handleCompletion("accept")} | ||
| onDecline={() => handleCompletion("reject")} | ||
| size="xs" | ||
| runCell={isDeletion ? undefined : runCell} | ||
| acceptLabel={isDeletion ? "Delete cell" : "Keep change"} | ||
| declineLabel={isDeletion ? "Keep cell" : "Revert change"} | ||
| /> |
Contributor
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/editor/ai/completion-handlers.tsx">
<violation number="1" location="frontend/src/components/editor/ai/completion-handlers.tsx:109">
P3: When a custom label is supplied ("Keep change", "Keep all"), the run-cell play button's tooltip still hardcodes "Accept and run cell"/"Accept and run all cells". Derive the tooltip text from the label (or a related custom tooltip prop) so it matches the surfaced button label.</violation>
</file>
<file name="frontend/src/components/editor/ai/ai-completion-editor.tsx">
<violation number="1" location="frontend/src/components/editor/ai/ai-completion-editor.tsx:444">
P3: The visible text now says "Generating change..."/"AI changed this cell", but the Loading/CircleCheck icon `aria-label`s right above still say "Generating fix" and "Fix generated". Since this PR is about replacing the "fix" wording with "change", update both aria-labels to match so screen readers stay consistent.</violation>
</file>
<file name="frontend/src/components/editor/cell/useDeleteCell.tsx">
<violation number="1" location="frontend/src/components/editor/cell/useDeleteCell.tsx:31">
P2: The staged AI state is removed optimistically before `sendDeleteCell` completes. If the delete fails (the `.catch(() => undoDeleteCell())` path) or the user clicks the Undo button, the cell is restored via `undoDeleteCell()` but its entry in `stagedAICellsAtom` is already gone, so the restored cell no longer shows the "AI-added cell" label or appears in the proposal tray. Clear the staged entry only after the delete succeeds, or restore it alongside `undoDeleteCell()`.</violation>
</file>
<file name="frontend/src/components/editor/cell/useRunCells.ts">
<violation number="1" location="frontend/src/components/editor/cell/useRunCells.ts:73">
P2: Proposal state is cleared before the run is dispatched, and since sendRun's errors are caught inside runCells and swallowed, a failed run still discards the proposal. For an update_cell proposal this loses the staged previousCode and the user's ability to revert the failed change. Consider clearing the staged cell only on successful execution (or passing execution success back from runCells) so a failed run retains the review state.</violation>
</file>
<file name="frontend/src/components/editor/cell/StagedAICell.tsx">
<violation number="1" location="frontend/src/components/editor/cell/StagedAICell.tsx:80">
P3: When `isDeletion` is true, the destructive `Delete cell` action uses the green accept styling while the non-destructive `Keep cell` action uses the red reject styling. Give deletion actions explicit destructive and non-destructive variants so their colors match their consequences.</violation>
</file>
Architecture diagram
sequenceDiagram
participant UI as Notebook UI
participant Cell as Cell Component
participant CellEditor as Cell Editor (CodeMirror)
participant CellFooter as StagedAICellFooter
participant ActionBar as CompletionActions (Accept/Reject)
participant Store as Jotai Store (stagedAICellsAtom)
participant DeleteHook as useDeleteCell
participant RunHook as useRunCells
participant AIBanner as Inline AI Banner
participant CSS as CSS Styles
Note over UI,CSS: AI Proposal Cell Styling Flow
UI->>CSS: Apply mo-ai-generated-cell class
CSS->>UI: CHANGED: No glow/opacity, blue border instead
CSS->>UI: Apply mo-ai-deleted-cell class
CSS->>UI: CHANGED: No opacity/glow, red border + strikethrough
Note over CellFooter,ActionBar: Cell Footer (new layout)
Cell->>CellFooter: Render for staged AI cell
CellFooter->>CellFooter: Determine type (add/change/delete)
alt isAddition
CellFooter->>CellFooter: Show "AI-added cell" label, no action buttons
else isChange or isDeletion
CellFooter->>CellFooter: Show "AI changed this cell" or "AI suggests deleting this cell"
CellFooter->>ActionBar: Pass acceptLabel/declineLabel
alt isDeletion
ActionBar->>ActionBar: Labels: "Delete cell" / "Keep cell"
else isChange
ActionBar->>ActionBar: Labels: "Keep change" / "Revert change"
end
end
Note over CellEditor,Store: Editing clears proposal state
CellEditor->>CellEditor: Detect user input/delete/undo/redo event
CellEditor->>Store: CHANGED: Remove cellId from stagedAICellsAtom
Store-->>CellEditor: Updated state
Note over DeleteHook,Store: Deleting a proposed cell clears state
DeleteHook->>Store: Check if cellId is in stagedAICells
Store-->>DeleteHook: Cell found
DeleteHook->>Store: CHANGED: Remove cellId from stagedAICellsAtom
DeleteHook->>Store: CHANGED: For multi-delete, remove all cellIds
Note over RunHook,Store: Running a proposed cell clears state
RunHook->>Store: CHANGED: Remove each cellId from stagedAICellsAtom
RunHook->>RunHook: Run cells logic continues
Note over RunHook,PendingAICells: Global proposal tray
PendingAICells->>ActionBar: Pass "Keep all" / "Discard all" labels
PendingAICells->>RunHook: Run all cells (exclude deletion proposals)
RunHook->>Store: CHANGED: Remove user-accepted cells from staged state
Note over ActionBar,Store: Inline AI refactoring
ActionBar->>AIBanner: CHANGED: Border style + updated labels ("Keep change"/"Revert change")
AIBanner->>AIBanner: Updated loading text ("Generating change...")
AIBanner->>ActionBar: Accept/reject completion
Note over Store,CellFooter: State-driven re-render
Store-->>CellFooter: stagedAICellsAtom update
CellFooter->>CellFooter: Re-render based on current state
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (stagedAICells.has(cellId)) { | ||
| const nextStagedAICells = new Map(stagedAICells); | ||
| nextStagedAICells.delete(cellId); | ||
| store.set(stagedAICellsAtom, nextStagedAICells); |
Contributor
There was a problem hiding this comment.
P2: The staged AI state is removed optimistically before sendDeleteCell completes. If the delete fails (the .catch(() => undoDeleteCell()) path) or the user clicks the Undo button, the cell is restored via undoDeleteCell() but its entry in stagedAICellsAtom is already gone, so the restored cell no longer shows the "AI-added cell" label or appears in the proposal tray. Clear the staged entry only after the delete succeeds, or restore it alongside undoDeleteCell().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/editor/cell/useDeleteCell.tsx, line 31:
<comment>The staged AI state is removed optimistically before `sendDeleteCell` completes. If the delete fails (the `.catch(() => undoDeleteCell())` path) or the user clicks the Undo button, the cell is restored via `undoDeleteCell()` but its entry in `stagedAICellsAtom` is already gone, so the restored cell no longer shows the "AI-added cell" label or appears in the proposal tray. Clear the staged entry only after the delete succeeds, or restore it alongside `undoDeleteCell()`.</comment>
<file context>
@@ -23,6 +24,12 @@ export function useDeleteCellCallback() {
+ if (stagedAICells.has(cellId)) {
+ const nextStagedAICells = new Map(stagedAICells);
+ nextStagedAICells.delete(cellId);
+ store.set(stagedAICellsAtom, nextStagedAICells);
+ }
const notebook = store.get(notebookAtom);
</file context>
| // Running a proposed cell means the user has reviewed and kept the | ||
| // current code, including when the proposal was a deletion. | ||
| for (const cellId of cellIds) { | ||
| removeStagedCell(cellId); |
Contributor
There was a problem hiding this comment.
P2: Proposal state is cleared before the run is dispatched, and since sendRun's errors are caught inside runCells and swallowed, a failed run still discards the proposal. For an update_cell proposal this loses the staged previousCode and the user's ability to revert the failed change. Consider clearing the staged cell only on successful execution (or passing execution success back from runCells) so a failed run retains the review state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/editor/cell/useRunCells.ts, line 73:
<comment>Proposal state is cleared before the run is dispatched, and since sendRun's errors are caught inside runCells and swallowed, a failed run still discards the proposal. For an update_cell proposal this loses the staged previousCode and the user's ability to revert the failed change. Consider clearing the staged cell only on successful execution (or passing execution success back from runCells) so a failed run retains the review state.</comment>
<file context>
@@ -65,6 +67,12 @@ export function useRunCells() {
+ // Running a proposed cell means the user has reviewed and kept the
+ // current code, including when the proposal was a deletion.
+ for (const cellId of cellIds) {
+ removeStagedCell(cellId);
+ }
+
</file context>
| }; | ||
|
|
||
| const text = multipleCompletions ? "Accept all" : "Accept"; | ||
| const text = label ?? (multipleCompletions ? "Accept all" : "Accept"); |
Contributor
There was a problem hiding this comment.
P3: When a custom label is supplied ("Keep change", "Keep all"), the run-cell play button's tooltip still hardcodes "Accept and run cell"/"Accept and run all cells". Derive the tooltip text from the label (or a related custom tooltip prop) so it matches the surfaced button label.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/editor/ai/completion-handlers.tsx, line 109:
<comment>When a custom label is supplied ("Keep change", "Keep all"), the run-cell play button's tooltip still hardcodes "Accept and run cell"/"Accept and run all cells". Derive the tooltip text from the label (or a related custom tooltip prop) so it matches the surfaced button label.</comment>
<file context>
@@ -90,7 +106,7 @@ export const AcceptCompletionButton: React.FC<{
};
- const text = multipleCompletions ? "Accept all" : "Accept";
+ const text = label ?? (multipleCompletions ? "Accept all" : "Accept");
const baseClasses = `h-6 text-(--grass-11) bg-(--grass-3)/60
</file context>
|
|
||
| <p className="transition-opacity duration-200 text-muted-foreground"> | ||
| {isLoading ? "Generating fix..." : "Showing fix"} | ||
| {isLoading ? "Generating change..." : "AI changed this cell"} |
Contributor
There was a problem hiding this comment.
P3: The visible text now says "Generating change..."/"AI changed this cell", but the Loading/CircleCheck icon aria-labels right above still say "Generating fix" and "Fix generated". Since this PR is about replacing the "fix" wording with "change", update both aria-labels to match so screen readers stay consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/editor/ai/ai-completion-editor.tsx, line 444:
<comment>The visible text now says "Generating change..."/"AI changed this cell", but the Loading/CircleCheck icon `aria-label`s right above still say "Generating fix" and "Fix generated". Since this PR is about replacing the "fix" wording with "change", update both aria-labels to match so screen readers stay consistent.</comment>
<file context>
@@ -445,7 +441,7 @@ const CompletionBanner: React.FC<CompletionBannerProps> = ({
<p className="transition-opacity duration-200 text-muted-foreground">
- {isLoading ? "Generating fix..." : "Showing fix"}
+ {isLoading ? "Generating change..." : "AI changed this cell"}
</p>
</div>
</file context>
| onDecline={() => handleCompletion("reject")} | ||
| size="xs" | ||
| runCell={isDeletion ? undefined : runCell} | ||
| acceptLabel={isDeletion ? "Delete cell" : "Keep change"} |
Contributor
There was a problem hiding this comment.
P3: When isDeletion is true, the destructive Delete cell action uses the green accept styling while the non-destructive Keep cell action uses the red reject styling. Give deletion actions explicit destructive and non-destructive variants so their colors match their consequences.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/editor/cell/StagedAICell.tsx, line 80:
<comment>When `isDeletion` is true, the destructive `Delete cell` action uses the green accept styling while the non-destructive `Keep cell` action uses the red reject styling. Give deletion actions explicit destructive and non-destructive variants so their colors match their consequences.</comment>
<file context>
@@ -47,21 +48,40 @@ export const StagedAICellFooter: React.FC<{ cellId: CellId }> = ({
+ onDecline={() => handleCompletion("reject")}
+ size="xs"
+ runCell={isDeletion ? undefined : runCell}
+ acceptLabel={isDeletion ? "Delete cell" : "Keep change"}
+ declineLabel={isDeletion ? "Keep cell" : "Revert change"}
+ />
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request makes AI-generated notebook cells look consistent with the rest of the editor and clarifies how users review proposed changes.
It also updates the inline AI refactoring flow that opens with
Cmd+Shift+Eon macOS or the equivalent shortcut on other platforms.What changed
AI-added cellwithout adding separate Accept and Reject buttons to every new cell.Keep changeandRevert changeactions.Delete cellandKeep cellactions.Keep allandDiscard alllabels.Keep changeandRevert changelabels.Why
AI proposals are already inserted into the notebook document before the user reviews them. The review state only indicates that the proposal still needs attention. The updated UI uses the existing cell styling and reserves explicit review actions for changes that need a keep or revert decision.
Running or editing a proposed cell indicates that the user has reviewed the current code. Clearing the proposal state in those cases prevents the review marker from remaining after the user takes ownership of the cell.
With changes (this PR)
Without changes (before this PR)