fix: default new cell type to markdown instead of code#78
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request changes the default cell type when adding a cell to an empty notebook from JavaScript/code to markdown, implementing Jupyter-style in-place markdown rendering. The change addresses issue #72 by making markdown the default cell type for new notebooks.
Changes:
- Added
addMarkupCellBefore(),appendMarkupCell(), andcreateMarkupCell()methods toNotebookData - Implemented
MarkdownCellcomponent with Jupyter-style in-place editing (double-click to edit, blur/Escape to render) - Changed empty-notebook "Add cell" button to create markdown cells instead of code cells
- Added
remark-gfmv4.0.1 for GitHub Flavored Markdown support
Reviewed changes
Copilot reviewed 110 out of 112 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
app/src/lib/notebookData.ts |
Added markup cell creation methods with debounced auto-save |
app/src/components/Actions/Actions.tsx |
Changed default cell type to markdown for empty notebooks |
app/src/components/Actions/MarkdownCell.tsx |
New Jupyter-style markdown renderer with in-place editing |
packages/react-components/src/components/Actions/MarkdownCell.tsx |
Duplicate markdown cell component (different API) |
packages/react-components/package.json |
Added remark-gfm dependency |
app/src/components/Actions/Editor.tsx |
Fixed Monaco width calculation to prevent expansion issues |
app/src/storage/storeResolver.ts |
Added store resolution logic for multiple storage backends |
| Various storage files | Added ContentsNotebookStore and FilesystemNotebookStore implementations |
| ESLint, Vite configs | Configuration updates and relaxed linting rules |
| Test fixtures | Added notebook fixtures for browser integration tests |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/src/storage/storeResolver.ts
Outdated
| export function resolveStore( | ||
| uri: string, | ||
| localStore: { save(...args: unknown[]): Promise<unknown>; load(...args: unknown[]): Promise<unknown>; getMetadata(...args: unknown[]): Promise<unknown> } | null, | ||
| fsStore: NotebookStore | null, | ||
| contentsStore: NotebookStore | null, | ||
| ): NotebookStore | null { | ||
| if (isContentsUri(uri) && contentsStore) { | ||
| return contentsStore; | ||
| } | ||
| if (isFsUri(uri) && fsStore) { | ||
| return fsStore; | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return localStore as unknown as NotebookStore | null; |
There was a problem hiding this comment.
The unsafe type cast as unknown as NotebookStore | null is documented but creates a type safety gap. The comment acknowledges that LocalNotebooks.save() returns Promise<void> while NotebookStore.save() returns Promise<ConflictResult>. This means any code expecting conflict detection from a resolved store could fail silently when the store is actually LocalNotebooks. Consider adding a runtime check or wrapper that returns { conflicted: false } for LocalNotebooks to maintain type safety.
| } | ||
| editorRef.current.layout?.({ width, height: desiredHeight }); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
The Editor component uses window.innerWidth as a fallback for width measurement, which was removed in the current change (line 82). However, line 84 introduces a new fallback using getLayoutInfo()?.width ?? 0. If both measuredWidth and fallbackWidth are 0, the function returns early without calling layout(). This is correct and prevents Monaco from expanding to window width, but verify that the resize observer effect (referenced in the comment) actually triggers when the container becomes visible, otherwise editors in hidden tabs might never get proper dimensions.
| useResizeObserver<HTMLDivElement>({ | |
| ref: containerRef, | |
| onResize: () => { | |
| adjustHeight(); | |
| }, | |
| }); |
| /** | ||
| * MarkdownCell - In-place markdown rendering component (Jupyter-style) | ||
| * | ||
| * This component implements Jupyter-style in-place markdown rendering: | ||
| * - When `rendered=true`: Shows rendered markdown HTML | ||
| * - When `rendered=false`: Shows the Monaco editor for editing | ||
| * - Double-click on rendered content switches to edit mode | ||
| * - Blur (clicking away) or pressing Escape switches back to render mode | ||
| * | ||
| * State Management: | ||
| * - `rendered` boolean controls which view is shown | ||
| * - The component swaps between Editor and rendered markdown in-place | ||
| * - No separate output cell is needed for markdown cells | ||
| * | ||
| * Security Note: | ||
| * - Raw HTML is disabled by default for security (XSS prevention) | ||
| * - Only trusted markdown features are rendered via remark-gfm | ||
| */ | ||
|
|
||
| import { | ||
| memo, | ||
| useCallback, | ||
| useEffect, | ||
| useMemo, | ||
| useState, | ||
| type FocusEvent, | ||
| type KeyboardEvent, | ||
| } from 'react' | ||
| import ReactMarkdown from 'react-markdown' | ||
| import type { Components } from 'react-markdown' | ||
| import remarkGfm from 'remark-gfm' | ||
|
|
||
| import { parser_pb } from '../../runme/client' | ||
| import Editor from './Editor' | ||
| import { fontSettings } from './CellConsole' | ||
|
|
||
| /** | ||
| * Custom markdown components for consistent styling. | ||
| * These match the styling used elsewhere in the app for markdown rendering. | ||
| */ | ||
| const markdownComponents: Components = { | ||
| h1: ({ children, ...props }) => ( | ||
| <h1 className="text-2xl font-bold mb-4 mt-6 text-gray-900" {...props}> | ||
| {children} | ||
| </h1> | ||
| ), | ||
| h2: ({ children, ...props }) => ( | ||
| <h2 className="text-xl font-bold mb-3 mt-5 text-gray-900" {...props}> | ||
| {children} | ||
| </h2> | ||
| ), | ||
| h3: ({ children, ...props }) => ( | ||
| <h3 className="text-lg font-semibold mb-2 mt-4 text-gray-900" {...props}> | ||
| {children} | ||
| </h3> | ||
| ), | ||
| p: ({ children, ...props }) => ( | ||
| <p className="mb-3 text-gray-800 leading-relaxed" {...props}> | ||
| {children} | ||
| </p> | ||
| ), | ||
| ul: ({ children, ...props }) => ( | ||
| <ul className="list-disc list-inside mb-3 ml-4 text-gray-800" {...props}> | ||
| {children} | ||
| </ul> | ||
| ), | ||
| ol: ({ children, ...props }) => ( | ||
| <ol className="list-decimal list-inside mb-3 ml-4 text-gray-800" {...props}> | ||
| {children} | ||
| </ol> | ||
| ), | ||
| li: ({ children, ...props }) => ( | ||
| <li className="mb-1" {...props}> | ||
| {children} | ||
| </li> | ||
| ), | ||
| code: ({ children, className, ...props }) => { | ||
| const isInline = !className | ||
| if (isInline) { | ||
| return ( | ||
| <code | ||
| className="bg-gray-100 px-1.5 py-0.5 rounded text-sm font-mono text-gray-800" | ||
| {...props} | ||
| > | ||
| {children} | ||
| </code> | ||
| ) | ||
| } | ||
| return ( | ||
| <code | ||
| className={`block bg-gray-100 p-3 rounded-md text-sm font-mono overflow-x-auto ${className}`} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </code> | ||
| ) | ||
| }, | ||
| pre: ({ children, ...props }) => ( | ||
| <pre className="bg-gray-100 p-3 rounded-md overflow-x-auto mb-3" {...props}> | ||
| {children} | ||
| </pre> | ||
| ), | ||
| blockquote: ({ children, ...props }) => ( | ||
| <blockquote | ||
| className="border-l-4 border-gray-300 pl-4 italic text-gray-700 my-3" | ||
| {...props} | ||
| > | ||
| {children} | ||
| </blockquote> | ||
| ), | ||
| a: ({ children, href, ...props }) => ( | ||
| <a | ||
| href={href} | ||
| className="text-blue-600 hover:underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| {...props} | ||
| > | ||
| {children} | ||
| </a> | ||
| ), | ||
| table: ({ children, ...props }) => ( | ||
| <div className="overflow-x-auto mb-3"> | ||
| <table className="min-w-full border border-gray-300" {...props}> | ||
| {children} | ||
| </table> | ||
| </div> | ||
| ), | ||
| th: ({ children, ...props }) => ( | ||
| <th | ||
| className="border border-gray-300 bg-gray-100 px-3 py-2 text-left font-semibold" | ||
| {...props} | ||
| > | ||
| {children} | ||
| </th> | ||
| ), | ||
| td: ({ children, ...props }) => ( | ||
| <td className="border border-gray-300 px-3 py-2" {...props}> | ||
| {children} | ||
| </td> | ||
| ), | ||
| hr: (props) => <hr className="my-4 border-gray-300" {...props} />, | ||
| img: ({ src, alt, ...props }) => ( | ||
| <img | ||
| src={src} | ||
| alt={alt || ''} | ||
| className="max-w-full h-auto rounded-md my-2" | ||
| {...props} | ||
| /> | ||
| ), | ||
| } | ||
|
|
||
| interface MarkdownCellProps { | ||
| /** The cell containing the markdown content */ | ||
| cell: parser_pb.Cell | ||
| /** Callback when cell content changes - receives the new cell with updated value */ | ||
| onCellChange?: (cell: parser_pb.Cell) => void | ||
| } | ||
|
|
||
| /** | ||
| * MarkdownCell renders markdown content in-place with edit/render mode toggle. | ||
| * | ||
| * Interaction patterns (Jupyter-style): | ||
| * - Double-click rendered content → enter edit mode | ||
| * - Click away (blur) from editor → render markdown (if not empty) | ||
| * - Press Escape while editing → render markdown (if not empty) | ||
| * - Empty cells start in edit mode and stay in edit mode | ||
| */ | ||
| const MarkdownCell = memo( | ||
| ({ cell, onCellChange }: MarkdownCellProps) => { | ||
| // `rendered` state controls whether we show rendered markdown or the editor | ||
| // Start in rendered mode unless the cell is empty (new cell) | ||
| const [rendered, setRendered] = useState(() => { | ||
| const value = cell?.value ?? '' | ||
| return value.trim().length > 0 | ||
| }) | ||
|
|
||
| // Get the current cell value | ||
| const value = cell?.value ?? '' | ||
|
|
||
| // Reset rendered state when cell identity changes (new cell loaded) | ||
| useEffect(() => { | ||
| setRendered((cell?.value ?? '').trim().length > 0) | ||
| }, [cell?.refId]) | ||
|
|
||
| // Enforce invariant: empty cells must be in edit mode. | ||
| // This handles external changes (undo/redo, sync) that clear the value. | ||
| useEffect(() => { | ||
| if (!value.trim() && rendered) { | ||
| setRendered(false) | ||
| } | ||
| }, [value, rendered]) | ||
|
|
||
| /** | ||
| * Handle switching to edit mode when user double-clicks rendered content. | ||
| * Also handles keyboard activation (Enter/Space) for accessibility. | ||
| */ | ||
| const handleDoubleClick = useCallback(() => { | ||
| setRendered(false) | ||
| }, []) | ||
|
|
||
| /** | ||
| * Handle keyboard activation on the rendered container for accessibility. | ||
| * Enter or Space activates edit mode (like a button). | ||
| * Only triggers when the container itself has focus, not nested elements | ||
| * (e.g., links inside the markdown). | ||
| */ | ||
| const handleRenderedKeyDown = useCallback( | ||
| (event: KeyboardEvent<HTMLDivElement>) => { | ||
| // Only handle if the container itself is focused, not nested elements | ||
| if (event.target !== event.currentTarget) { | ||
| return | ||
| } | ||
| if (event.key === 'Enter' || event.key === ' ') { | ||
| event.preventDefault() | ||
| setRendered(false) | ||
| } | ||
| }, | ||
| [] | ||
| ) | ||
|
|
||
| /** | ||
| * Handle blur event - switch back to rendered mode when clicking away. | ||
| * Uses relatedTarget to check if focus moved outside the editor container. | ||
| * Empty cells stay in edit mode. | ||
| */ | ||
| const handleBlur = useCallback( | ||
| (event: FocusEvent<HTMLDivElement>) => { | ||
| // If focus moved to an element still inside the editor container, don't render | ||
| if (event.currentTarget.contains(event.relatedTarget as Node | null)) { | ||
| return | ||
| } | ||
| // If content is empty, stay in edit mode | ||
| if (!value.trim()) { | ||
| return | ||
| } | ||
| setRendered(true) | ||
| }, | ||
| [value] | ||
| ) | ||
|
|
||
| /** | ||
| * Handle keyboard events on the editor container. | ||
| * Escape key switches back to rendered mode (if not empty). | ||
| */ | ||
| const handleEditorKeyDown = useCallback( | ||
| (event: KeyboardEvent<HTMLDivElement>) => { | ||
| if (event.key === 'Escape') { | ||
| // Only render if there's content; empty cells stay in edit mode | ||
| if (value.trim()) { | ||
| setRendered(true) | ||
| } | ||
| } | ||
| }, | ||
| [value] | ||
| ) | ||
|
|
||
| /** | ||
| * Handle content changes from the editor. | ||
| * Creates a new cell with updated value and notifies parent component. | ||
| */ | ||
| const handleEditorChange = useCallback( | ||
| (newValue: string) => { | ||
| if (!cell || !onCellChange) return | ||
| // Create a shallow copy with the new value | ||
| const updated = { ...cell, value: newValue } | ||
| onCellChange(updated as parser_pb.Cell) | ||
| }, | ||
| [cell, onCellChange] | ||
| ) | ||
|
|
||
| /** | ||
| * Handle "run" action for markdown cells - just renders the markdown. | ||
| * This matches Jupyter's behavior where Shift+Enter on a markdown cell | ||
| * renders it and moves to the next cell. | ||
| */ | ||
| const handleRun = useCallback(() => { | ||
| if (value.trim()) { | ||
| setRendered(true) | ||
| } | ||
| }, [value]) | ||
|
|
||
| // Memoize the rendered markdown to avoid unnecessary re-renders | ||
| const renderedMarkdown = useMemo(() => { | ||
| if (!value.trim()) { | ||
| return ( | ||
| <div className="text-gray-400 italic py-2"> | ||
| Double-click to edit markdown... | ||
| </div> | ||
| ) | ||
| } | ||
| return ( | ||
| <div className="prose prose-sm max-w-none"> | ||
| <ReactMarkdown | ||
| remarkPlugins={[remarkGfm]} | ||
| components={markdownComponents} | ||
| > | ||
| {value} | ||
| </ReactMarkdown> | ||
| </div> | ||
| ) | ||
| }, [value]) | ||
|
|
||
| if (!cell) { | ||
| return null | ||
| } | ||
|
|
||
| return ( | ||
| <div | ||
| id={`markdown-cell-${cell.refId}`} | ||
| className="relative w-full" | ||
| data-testid="markdown-cell" | ||
| data-rendered={rendered} | ||
| > | ||
| {rendered ? ( | ||
| // Rendered markdown view - double-click or keyboard to edit | ||
| <div | ||
| id={`markdown-rendered-${cell.refId}`} | ||
| className="cursor-text rounded-md border border-transparent hover:border-gray-200 hover:bg-gray-50/50 p-4 transition-colors" | ||
| onDoubleClick={handleDoubleClick} | ||
| onKeyDown={handleRenderedKeyDown} | ||
| tabIndex={0} | ||
| role="button" | ||
| aria-label="Double-click or press Enter to edit markdown" | ||
| data-testid="markdown-rendered" | ||
| > | ||
| {renderedMarkdown} | ||
| </div> | ||
| ) : ( | ||
| // Editor view - blur or Escape to render | ||
| <div | ||
| id={`markdown-editor-${cell.refId}`} | ||
| className="rounded-md border border-sky-200 overflow-hidden w-full min-w-0 max-w-full" | ||
| onBlur={handleBlur} | ||
| onKeyDown={handleEditorKeyDown} | ||
| data-testid="markdown-editor" | ||
| > | ||
| <Editor | ||
| id={`md-editor-${cell.refId}`} | ||
| value={value} | ||
| language="markdown" | ||
| fontSize={fontSettings.fontSize} | ||
| fontFamily={fontSettings.fontFamily} | ||
| onChange={handleEditorChange} | ||
| onEnter={handleRun} | ||
| /> | ||
| <div className="bg-gray-100 border-t border-gray-200 px-3 py-1 text-xs text-gray-500"> | ||
| Press <kbd className="px-1 py-0.5 bg-gray-200 rounded">Esc</kbd>{' '} | ||
| or click away to render | ||
| </div> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ) | ||
| }, | ||
| (prevProps, nextProps) => { | ||
| // Skip re-render only if both the cell identity and value haven't changed | ||
| return ( | ||
| prevProps.cell?.refId === nextProps.cell?.refId && | ||
| prevProps.cell?.value === nextProps.cell?.value | ||
| ) | ||
| } | ||
| ) | ||
|
|
||
| MarkdownCell.displayName = 'MarkdownCell' | ||
|
|
||
| export default MarkdownCell |
There was a problem hiding this comment.
This file appears to be a duplicate of app/src/components/Actions/MarkdownCell.tsx with nearly identical content. Having duplicate implementations of the same component in different packages creates maintenance burden and potential for divergence. Consider:
- If this component belongs in
react-componentspackage (publishable library), remove the duplicate fromapp/src/components/Actions/ - If it belongs in the app, remove this version and import it from the app
- The app version uses
useSyncExternalStorewithCellDatasubscription (lines 173-180 in app version), while this package version uses a simpleronCellChangecallback prop - these are different APIs that will confuse consumers
app/src/lib/notebookData.ts
Outdated
| loadNotebook( | ||
| notebook: parser_pb.Notebook, | ||
| { persist = true }: { persist?: boolean } = {}, | ||
| ): void { | ||
| this.notebook = clone(parser_pb.NotebookSchema, notebook); | ||
| this.rebuildIndex(); | ||
| this.validateCells(); | ||
| this.sequence = this.computeHighestSequence(); | ||
| this.loaded = true; | ||
| this.snapshotCache = this.buildSnapshot(); | ||
| this.emit(); | ||
| void this.persist(); | ||
| if (persist) { | ||
| this.schedulePersist(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The persist parameter defaults to true, which triggers auto-save on every load. However, when loading existing notebooks from disk (line 321 and 352 in NotebookContext.tsx), persist: false is passed to avoid redundant writes. The implementation looks correct, but the debounced schedulePersist() should prevent excessive disk I/O. Verify that the 750ms debounce delay doesn't cause user confusion if they make rapid edits and expect immediate saves.
| "react": "^19.0.0", | ||
| "react-dom": "^19.0.0", | ||
| "react-markdown": "^10.1.0", | ||
| "remark-gfm": "^4.0.0", |
There was a problem hiding this comment.
The code checks for remark-gfm version 4.0.1, but the knowledge cutoff is January 2025 (PR is from February 2026). While the version appears to exist based on the lock file, be aware that future reviews should verify compatibility if this component is published as a library and consumers may have different remark-gfm versions.
| hasError: boolean | ||
| } | ||
|
|
||
| export class ErrorBoundary extends Component< |
There was a problem hiding this comment.
hamelsmu
force-pushed
the
fix/issue-72-default-cell-markdown
branch
from
e083937 to
cf931f1
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1c1ce3643
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
app/src/storage/fs.ts
Outdated
| const oldName = segments[segments.length - 1]; | ||
| await dirHandle.removeEntry(oldName); |
There was a problem hiding this comment.
This rename path can delete a notebook when the requested name resolves to the existing filename: getFileHandle(safeName, { create: true }) returns the same file, then removeEntry(oldName) removes it anyway. In that same-name case (for example, confirming rename without edits, or typing the same name with/without .json), the operation fails after the delete and causes data loss. Add a no-op guard before file creation/removal when safeName matches the current basename.
Useful? React with 👍 / 👎.
app/src/storage/storeResolver.ts
Outdated
| return fsStore; | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return localStore as unknown as NotebookStore | null; |
There was a problem hiding this comment.
resolveStore currently falls back to localStore even for fs:// and contents:// URIs when their dedicated stores are unavailable. Callers in NotebookContext then invoke getMetadata/load on LocalNotebooks, which rejects non-local:// URIs, so early opens/restores for fs/contents docs fail instead of waiting for the proper store to initialize. For scheme-specific URIs, return null until the matching backend exists.
Useful? React with 👍 / 👎.
When creating a new notebook or adding a cell at the top, the default cell type was JavaScript/code. Changed to markdown since most notebooks start with documentation. - Add createMarkupCell(), addMarkupCellBefore(), appendMarkupCell() - Change top-of-notebook "Add cell" button to insert markup cells - Add 5 unit tests for the new markup cell methods - Add @vitest-environment node directive to fix jsdom ESM bug Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hamel Husain <hamel.husain@gmail.com>
1 participant
Summary
addMarkupCellBefore(),appendMarkupCell(), andcreateMarkupCell()methods toNotebookDataActions.tsxby removing the inline cell creationCloses #72
Test plan
npx vitest run— all tests pass🤖 Generated with Claude Code