Skip to content

address review comments from #8019#9549

Merged
Light2Dark merged 1 commit into
mainfrom
address-pr-8019-comments
May 14, 2026
Merged

address review comments from #8019#9549
Light2Dark merged 1 commit into
mainfrom
address-pr-8019-comments

Conversation

@Light2Dark
Copy link
Copy Markdown
Collaborator

@Light2Dark Light2Dark commented May 14, 2026

image

📝 Summary

Follow-up to #8019, which merged before a few unresolved review threads were addressed. This PR addresses the actionable items.

Closes #

📋 Pre-Review Checklist

  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it.
  • Video or media evidence is provided for any visual changes (optional).

✅ Merge Checklist

  • I have read the contributor guidelines.
  • Documentation has been updated where applicable, including docstrings for API changes.
  • Tests have been added for the changes made.

Made with Cursor

@vercel
Copy link
Copy Markdown

vercel Bot commented May 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview, Comment May 14, 2026 0:50am

Request Review

@Light2Dark Light2Dark added the bug Something isn't working label May 14, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 6 files

Architecture diagram
sequenceDiagram
    participant UI as Cell Component
    participant Clipboard as useCellClipboard()
    participant PendingCut as pending-cut-service
    participant NClipboard as navigator.clipboard
    
    Note over UI,NClipboard: Cell Copy/Cut Flow
    
    UI->>Clipboard: copyCells() or cutCells()
    
    alt copyCells
        Clipboard->>Clipboard: toPlainText(cells)
        Clipboard->>Clipboard: build ClipboardCellData
        Clipboard->>NClipboard: write([clipboardItem])
        NClipboard-->>Clipboard: success
        Clipboard->>PendingCut: clear()
    else cutCells
        Clipboard->>Clipboard: toPlainText(cells)
        Clipboard->>Clipboard: build ClipboardCellData
        Clipboard->>NClipboard: write([clipboardItem])
        NClipboard-->>Clipboard: success
        Clipboard->>PendingCut: markForCut({cellIds})
        PendingCut-->>Clipboard: updated state (cellIds set)
    end
    
    Note over UI,PendingCut: Clipboard writes include marimo MIME type + text/plain
    
    alt clipboard write fails
        Clipboard->>Clipboard: construct plainText string
        Clipboard->>NClipboard: copyToClipboard(plainText)
        NClipboard-->>Clipboard: fallback success/failure
        opt cut fallback
            Clipboard->>PendingCut: markForCut({cellIds}) (even on fallback)
        end
    end
    
    Note over UI,PendingCut: Pending cut state only tracks cellIds (no clipboardData)
    
    UI->>PendingCut: useIsPendingCut(cellId)
    PendingCut-->>UI: boolean (true if cellId in pending set)
    
    UI->>PendingCut: useHasPendingCut()
    PendingCut-->>UI: boolean (true if any pending cuts)
Loading

@Light2Dark Light2Dark added enhancement New feature or request and removed bug Something isn't working labels May 14, 2026
@Light2Dark Light2Dark requested a review from manzt May 14, 2026 01:25
@Light2Dark Light2Dark marked this pull request as ready for review May 14, 2026 01:25
Copilot AI review requested due to automatic review settings May 14, 2026 01:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Follow-up to #8019 addressing leftover review feedback: removes the unused clipboardData field from the pending-cut state, refactors the clipboard helpers in clipboard.ts, simplifies the pending-cut cell styling, and updates tests to use the cellId branded helper.

Changes:

  • Remove clipboardData from PendingCutState and adjust markForCut callers/tests accordingly.
  • Refactor clipboard.ts to introduce ClipboardCellInput, a toPlainText helper, and a single writeCellsToClipboard(cells) entry point.
  • Tone down the .pending-cut CSS (gray background + opacity, drop dashed amber border) and switch tests to the cellId(...) branded helper.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
frontend/src/css/app/Cell.css Replace amber/dashed cut styling with gray-2 background and 0.55 opacity on inner areas.
frontend/src/core/cells/pending-cut-service.ts Drop clipboardData from state and the markForCut action.
frontend/src/core/cells/tests/pending-cut-service.test.tsx Remove mockClipboardData; use cellId(...) helper for branded IDs.
frontend/src/core/cells/tests/cells.test.ts Replace "x" as CellId casts with cellId("x") helper calls.
frontend/src/components/editor/navigation/clipboard.ts Inline buildClipboardPayload into writeCellsToClipboard; add ClipboardCellInput and toPlainText.
frontend/src/components/editor/navigation/tests/clipboard.test.ts Drop clipboardData: null from the mocked usePendingCutState.

@Light2Dark Light2Dark merged commit 16df3e6 into main May 14, 2026
48 of 50 checks passed
@Light2Dark Light2Dark deleted the address-pr-8019-comments branch May 14, 2026 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants