Repository navigation
Add whiteboard and chalkboard workspace themes - #23
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds a browser-local light, dark, and system theme system. It refactors workspace controls into components, preserves flyout and canvas state, renders Mermaid styles in React Flow, improves responsive toolbar placement, and adds a comprehensive production Playwright validation gate. ChangesWorkspace UX and theme system
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ 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 |
donovan-yohan
marked this pull request as ready for review
August 8, 2026 09:34
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (8)
.github/workflows/ci.yml (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the frozen lockfile in the workspace job.
A lockfile is committed, and this workflow runs in CI, so
pnpm installshould enforce lockfile consistency.--frozen-lockfile=falsedisables that protection and can resolve a dependency graph different frompnpm-lock.yaml. Usepnpm install --frozen-lockfile.Proposed fix
- - run: pnpm install --frozen-lockfile=false + - run: pnpm install --frozen-lockfile🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 40, Update the workspace job’s pnpm install command to use --frozen-lockfile instead of --frozen-lockfile=false, ensuring CI enforces the committed pnpm-lock.yaml.e2e/support/owned-services.ts (1)
138-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreserve the original error as
cause.The catch block builds a new
Errorfrom the message text only. The original stack is lost, which makes failures insiderun()hard to diagnose. Attach the original error.♻️ Proposed change
- throw new Error(`${error instanceof Error ? error.message : String(error)}${diagnostics ? `\n${diagnostics}` : ''}`); + throw new Error(`${error instanceof Error ? error.message : String(error)}${diagnostics ? `\n${diagnostics}` : ''}`, { cause: error });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/support/owned-services.ts` around lines 138 - 144, Update the catch block in run() to attach the caught error as the cause when constructing the diagnostic Error, while preserving the existing combined message and process-output diagnostics.e2e/support/browser.ts (1)
42-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
tmpdir()instead of a hardcoded/tmppath.
e2e/support/owned-services.tsalready usestmpdir()fromnode:os. Use the same approach here so screenshot capture does not depend on a POSIX-only path.♻️ Proposed change
+import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + export async function saveScreenshot(page: Page, name: string): Promise<string> { - const path = `/tmp/arielcharts-${name}.png`; + const path = join(tmpdir(), `arielcharts-${name}.png`); await page.screenshot({ path, fullPage: true }); return path; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/support/browser.ts` around lines 42 - 46, Update saveScreenshot to import and use tmpdir() from node:os when constructing the screenshot path, replacing the hardcoded /tmp prefix while preserving the existing filename and screenshot behavior.e2e/support/workspace.ts (2)
66-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
canonicalSourceandwaitForSourceduplicate the line-cleaning logic.Both functions strip
.cm-ySelectionCaretand.cm-widgetBufferand join lines. The two copies can diverge, and thenwaitForSourcewaits for a string thatcanonicalSourcenever produces. Define the cleaning function once and inject it into both browser contexts.♻️ Proposed change
+const READ_CANONICAL_SOURCE = `(() => [...document.querySelectorAll('.cm-line')].map((line) => { + const copy = line.cloneNode(true); + copy.querySelectorAll('.cm-ySelectionCaret, .cm-widgetBuffer').forEach((node) => node.remove()); + return copy.textContent ?? ''; +}).join('\\n'))()`; + export async function canonicalSource(page: Page): Promise<string> { - return page.locator('.cm-line').evaluateAll((lines) => lines.map((line) => { - const copy = line.cloneNode(true) as HTMLElement; - copy.querySelectorAll('.cm-ySelectionCaret, .cm-widgetBuffer').forEach((node) => node.remove()); - return copy.textContent ?? ''; - }).join('\n')); + return page.evaluate<string>(READ_CANONICAL_SOURCE); } export async function waitForSource(page: Page, expected: string): Promise<void> { - await page.waitForFunction((source) => { - const lines = [...document.querySelectorAll('.cm-line')]; - return lines.map((line) => { - const copy = line.cloneNode(true) as HTMLElement; - copy.querySelectorAll('.cm-ySelectionCaret, .cm-widgetBuffer').forEach((node) => node.remove()); - return copy.textContent ?? ''; - }).join('\n') === source; - }, expected, { timeout: 15_000 }); + await page.waitForFunction( + ([source, read]) => new Function(`return ${read}`)() === source, + [expected, READ_CANONICAL_SOURCE] as const, + { timeout: 15_000 }, + ); }Alternatively, poll
canonicalSourcefrom Node instead of usingwaitForFunction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/support/workspace.ts` around lines 66 - 83, Extract the shared line-cleaning and joining logic from canonicalSource and waitForSource into one reusable function, then inject or reuse that function in both browser contexts. Ensure both functions remove .cm-ySelectionCaret and .cm-widgetBuffer before joining line text, so they always produce identical canonical source strings.
59-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
ControlOrMeta+Afor select-all.
Control+Aworks on Linux and Windows, but macOS maps Control-A to move the cursor to the line start. Use Playwright’sControlOrMetamodifier soreplaceSource()replaces the editor contents on Apple silicons too.♻️ Proposed change
await editor.click(); - await page.keyboard.press('Control+A'); + await page.keyboard.press('ControlOrMeta+A'); await page.keyboard.insertText(source);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/support/workspace.ts` around lines 59 - 64, Update replaceSource to use Playwright’s ControlOrMeta modifier for the select-all keyboard shortcut instead of Control, while preserving the existing editor interaction and text replacement flow.e2e/support/mcp.ts (1)
21-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the MCP fetch call.
fetchhas nosignal. If the server accepts the connection and never responds, the gate hangs until the CI job timeout. That hides the failure cause. UseAbortSignal.timeout.♻️ Proposed change
const response = await fetch(this.endpoint, { method: 'POST', + signal: AbortSignal.timeout(15_000), headers: {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/support/mcp.ts` around lines 21 - 49, Update the MCP request fetch in the tool-calling method to include an AbortSignal.timeout signal, using an appropriate finite timeout so unresponsive servers fail promptly while preserving the existing response and HTTP error handling.e2e-workspace-ux-validate.ts (2)
238-243: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed 250 ms wait with a condition wait.
page.waitForTimeout(250)makes the camera-fit assertion time dependent. If the static renderer fits later than 250 ms, the assertion at Line 243 fails intermittently in CI. Every other transform check in this file already useswaitForFunction.♻️ Proposed change
- await page.waitForTimeout(250); - const staticTransform = await layer.getAttribute('style'); - assert(staticTransform !== zoomed, `Editable-to-static renderer transition did not fit the camera: ${zoomed}`); + await page.waitForFunction((previous) => document.querySelector('.diagram-canvas-svg')?.parentElement?.getAttribute('style') !== previous, zoomed, { timeout: 15_000 }); + const staticTransform = await layer.getAttribute('style'); + assert(staticTransform !== zoomed, `Editable-to-static renderer transition did not fit the camera: ${zoomed}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-workspace-ux-validate.ts` around lines 238 - 243, Replace the fixed page.waitForTimeout(250) in the static renderer transition flow with page.waitForFunction, waiting until the layer style transform changes from zoomed before reading staticTransform and asserting the camera fit. Follow the existing waitForFunction pattern used by other transform checks in the test.
129-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
renameActiveDiagraminstead of inlining the rename flow.Lines 129-137 repeat the exact steps of
renameActiveDiagramine2e/support/workspace.ts(Lines 107-115). The only addition is the screenshot. Extend the helper with an optional hook, or call the helper and take the screenshot around it. The helper is already imported at Line 34.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-workspace-ux-validate.ts` around lines 129 - 137, The inline rename sequence should reuse the existing renameActiveDiagram helper instead of duplicating its steps. Replace the direct button/input/tab assertions in this test with the imported helper, preserving the issue-14-rename screenshot by taking it before or after the helper as appropriate; extend the helper only if needed to support the screenshot hook.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 32: Update the actions/checkout@v4 step to disable persisted credentials
by configuring persist-credentials as false, while leaving the rest of the
workflow unchanged.
In `@apps/web/src/app/globals.css`:
- Around line 56-57: Update the --font-sans and --font-mono declarations to use
the configured lowercase spellings for Nunito, BlinkMacSystemFont, Consolas, and
Menlo, while preserving the existing font-stack order and fallback values.
In `@apps/web/src/components/diagram-canvas.tsx`:
- Around line 2111-2130: The getFlowEdgePresentation function uses fixed marker
IDs for arrow_circle and arrow_cross, preventing presentation.stroke from
coloring their markers. Update these marker types to use presentation-aware
marker definitions or marker references carrying markerColor, while preserving
the existing edge styling and behavior; add coverage verifying authored colors
for both arrow_circle and arrow_cross.
- Around line 1768-1782: Update both bottom toolbar positioning blocks in
apps/web/src/components/diagram-canvas.tsx:1547-1567 and
apps/web/src/components/diagram-canvas.tsx:1768-1782 to derive the bottom offset
from canvasSize.height - (canvasViewport.y + canvasViewport.height), preserving
the existing inset behavior so both toolbars remain within the unobscured canvas
viewport.
In `@apps/web/src/components/theme-provider.tsx`:
- Around line 52-56: Update handleStorageChange to call syncTheme when the
storage event targets THEME_STORAGE_KEY or has a null key from
window.localStorage, thereby handling localStorage.clear(). Add test coverage
for the null-key clear event.
In `@apps/web/src/lib/mermaid-presentation.ts`:
- Around line 72-77: Update the declaration merging logic around
getMatchingRules and parseDeclarations so !important priorities are tracked and
preserved when stylesheet and inline declarations conflict. Ensure important
stylesheet values override normal inline values, while retaining existing
cascade behavior for equal priorities; update the priority-marker handling near
the relevant declaration-processing code and add a regression test covering
conflicting class and inline paint declarations.
In `@e2e-workspace-ux-validate.ts`:
- Around line 400-403: Update expectNoDevelopmentIndicator so page.evaluate
returns a boolean indicating whether the Next.js development-indicator selector
matches, then assert that boolean is false. Keep the existing selectors and
failure message unchanged.
In `@e2e/support/browser.ts`:
- Around line 35-38: Update the close method so context shutdown uses
Promise.allSettled and browser.close() runs in a finally block, ensuring the
browser is always closed even when one or more context.close() calls fail.
In `@e2e/support/interactions.ts`:
- Around line 67-80: Update parseRgb to account for the alpha component in rgba
values before contrastRatio uses the parsed color, including transparent alpha 0
and fractional alpha values; alternatively, explicitly reject non-opaque RGBA
tokens with a clear assertion. Preserve existing RGB and hex parsing behavior.
In `@e2e/support/mcp.ts`:
- Around line 66-77: Update writeLatest to handle the revision race by retrying
the readDiagram/writeDiagram sequence once when writeDiagram fails due to a
revision conflict, while preserving the existing detail and actor metadata;
return the successful Diagram result and propagate non-conflict errors or
failures after the retry.
In `@e2e/support/owned-services.ts`:
- Around line 110-113: Update the comment above baseUrl and mcpUrl to describe
the production-service reason for using localhost, referencing the
ALLOWED_ORIGINS configuration set earlier in the setup. Remove the outdated next
dev and development-origin guard explanation while leaving the URL assignments
unchanged.
- Around line 36-49: Update waitFor in e2e/support/owned-services.ts (lines
36-49) to pass AbortSignal.timeout(5_000) in its fetch options. Update the tool
request in e2e/support/mcp.ts (lines 21-49) to pass AbortSignal.timeout(15_000),
ensuring both bounded operations cannot hang indefinitely.
- Around line 133-135: Replace the npx-based pnpm invocations in the production
build, server start, and web start calls with the repository’s trusted pnpm
command, preserving each existing argument list and environment. Use the
packageManager/Corepack-resolved pnpm version rather than explicitly resolving
pnpm@10.15.0 through npx.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 40: Update the workspace job’s pnpm install command to use
--frozen-lockfile instead of --frozen-lockfile=false, ensuring CI enforces the
committed pnpm-lock.yaml.
In `@e2e-workspace-ux-validate.ts`:
- Around line 238-243: Replace the fixed page.waitForTimeout(250) in the static
renderer transition flow with page.waitForFunction, waiting until the layer
style transform changes from zoomed before reading staticTransform and asserting
the camera fit. Follow the existing waitForFunction pattern used by other
transform checks in the test.
- Around line 129-137: The inline rename sequence should reuse the existing
renameActiveDiagram helper instead of duplicating its steps. Replace the direct
button/input/tab assertions in this test with the imported helper, preserving
the issue-14-rename screenshot by taking it before or after the helper as
appropriate; extend the helper only if needed to support the screenshot hook.
In `@e2e/support/browser.ts`:
- Around line 42-46: Update saveScreenshot to import and use tmpdir() from
node:os when constructing the screenshot path, replacing the hardcoded /tmp
prefix while preserving the existing filename and screenshot behavior.
In `@e2e/support/mcp.ts`:
- Around line 21-49: Update the MCP request fetch in the tool-calling method to
include an AbortSignal.timeout signal, using an appropriate finite timeout so
unresponsive servers fail promptly while preserving the existing response and
HTTP error handling.
In `@e2e/support/owned-services.ts`:
- Around line 138-144: Update the catch block in run() to attach the caught
error as the cause when constructing the diagnostic Error, while preserving the
existing combined message and process-output diagnostics.
In `@e2e/support/workspace.ts`:
- Around line 66-83: Extract the shared line-cleaning and joining logic from
canonicalSource and waitForSource into one reusable function, then inject or
reuse that function in both browser contexts. Ensure both functions remove
.cm-ySelectionCaret and .cm-widgetBuffer before joining line text, so they
always produce identical canonical source strings.
- Around line 59-64: Update replaceSource to use Playwright’s ControlOrMeta
modifier for the select-all keyboard shortcut instead of Control, while
preserving the existing editor interaction and text replacement flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3242791a-8479-4f60-ae93-dfe941ff3f8b
⛔ Files ignored due to path filters (2)
docs/design/arielcharts-chalkboard-target.pngis excluded by!**/*.pngdocs/design/arielcharts-whiteboard-target.pngis excluded by!**/*.png
📒 Files selected for processing (36)
.github/workflows/ci.ymlapps/web/AGENTS.mdapps/web/next.config.tsapps/web/public/theme-init.jsapps/web/src/app/globals.cssapps/web/src/app/layout.tsxapps/web/src/components/diagram-canvas.tsxapps/web/src/components/session-workspace.tsxapps/web/src/components/theme-control.tsxapps/web/src/components/theme-provider.tsxapps/web/src/components/workspace-flyouts.tsxapps/web/src/components/workspace-footer.tsxapps/web/src/components/workspace-tab-strip.tsxapps/web/src/lib/canvas-keyboard-ownership.test.tsapps/web/src/lib/canvas-keyboard-ownership.tsapps/web/src/lib/canvas-viewport.test.tsapps/web/src/lib/canvas-viewport.tsapps/web/src/lib/mermaid-presentation.test.tsapps/web/src/lib/mermaid-presentation.tsapps/web/src/lib/renderer-camera-policy.test.tsapps/web/src/lib/renderer-camera-policy.tsapps/web/src/lib/theme.test.tsapps/web/src/lib/theme.tsapps/web/src/lib/toolbar-safe-area.test.tsapps/web/src/lib/toolbar-safe-area.tsapps/web/src/lib/workspace-flyout-state.test.tsapps/web/src/lib/workspace-flyout-state.tsdocs/context-map.mde2e-workspace-ux-validate.tse2e/support/assert.tse2e/support/browser.tse2e/support/interactions.tse2e/support/mcp.tse2e/support/owned-services.tse2e/support/workspace.tspackage.json
13 tasks done
This was referenced Aug 8, 2026
This branch was successfully deployed
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
1 participant
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.
Outcome
Turns the workspace into a visual-first whiteboard/chalkboard surface while preserving fully customizable Mermaid item colors.
What changed
style/classDefcolors—including custom circle/cross markers and!importantcascade—across themesArchitecture decision
Extend the existing Yjs/Mermaid/React Flow architecture. Theme, open-flyout state, selection, camera, and drafts remain browser-local; Mermaid source and source-owned item styles remain canonical/shared. One ThemeProvider resolves visual theme and feeds derived render inputs without creating another state service.
Verification
pnpm install --frozen-lockfilepnpm typecheckpnpm lintpnpm test— 24 files / 94 testspnpm buildpnpm test:e2e-workspace-ux— 14 pass / 0 skippnpm test:e2e-sequencepnpm test:e2e-collaborationFollow-up discovered
#22 tracks a pre-existing #21 React Flow controlled-node warning exposed by real drag gates. Its safe repair needs a dedicated drag/Yjs ownership slice; this PR does not alter that ownership model.
Closes #14