Skip to content

Conversation

@mscolnick
Copy link
Contributor

@mscolnick mscolnick commented Jan 28, 2026

  • replace no-print for print:hidden (since this will also hide into when printing using Cmd+P)
  • fix race condition when capturing screenshots. there were a few here where: 1) old requests were not cancelled and could overwrite newer otuputs, 2) new request could get skipped if in the output did not change but already requested
  • remove snappy and always pass includeStyleProperties (otherwise we will get inconsistenties)
  • remove adding .printing on the body when just snapshotting the cell output
@vercel
Copy link

vercel bot commented Jan 28, 2026

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

Project Deployment Review Updated (UTC)
marimo-docs Ready Ready Preview, Comment Jan 28, 2026 9:27pm

Request Review

Copy link
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

This pull request improves print configuration and fixes race conditions in PDF export functionality by introducing a new AsyncCaptureTracker for managing concurrent screenshot captures and migrating from a custom no-print CSS class to Tailwind's print:hidden utility class.

Changes:

  • Introduces AsyncCaptureTracker to prevent race conditions when taking screenshots of multiple cells concurrently
  • Removes the snappy parameter from screenshot functions, simplifying the API and always using optimized style properties
  • Migrates from custom no-print CSS class to Tailwind's print:hidden utility across all UI components
  • Removes reference counting mechanism for body.printing class in favor of scoped cleanup functions

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
frontend/src/utils/async-capture-tracker.ts New utility class for tracking async capture operations with per-key abort signals and state management
frontend/src/utils/tests/async-capture-tracker.test.ts Comprehensive test suite for AsyncCaptureTracker covering all state transitions and edge cases
frontend/src/core/export/hooks.ts Integrates AsyncCaptureTracker to prevent duplicate/concurrent captures and handle in-flight waiters
frontend/src/core/export/tests/hooks.test.ts Updates tests to remove snappy parameter and adds retry test for failed captures
frontend/src/utils/html-to-image.ts Removes snappy parameter, always uses necessary style properties, updates filter to check for print:hidden class
frontend/src/utils/download.ts Simplifies screenshot preparation by removing ref counting, exports ADD_PRINTING_CLASS helper
frontend/src/utils/tests/download.test.tsx Removes tests for deprecated snappy parameter and ref counting logic
frontend/src/css/app/print.css Removes custom no-print and printing-output CSS rules
frontend/src/components/ui/dropdown-menu.tsx Adds print:hidden class to dropdown content
frontend/src/components/static-html/static-banner.tsx Replaces no-print with print:hidden
frontend/src/components/home/components.tsx Replaces no-print with print:hidden in tutorial dropdown
frontend/src/components/editor/renderers/vertical-layout/vertical-layout.tsx Uses ADD_PRINTING_CLASS for notebook screenshots, updates class names
frontend/src/components/editor/actions/useNotebookActions.tsx Uses ADD_PRINTING_CLASS and removes snappy parameter
frontend/src/components/editor/header/status.tsx Replaces no-print with print:hidden
frontend/src/components/editor/file-tree/file-explorer.tsx Replaces no-print with print:hidden
frontend/src/components/editor/controls/notebook-menu-dropdown.tsx Replaces no-print with print:hidden
frontend/src/components/editor/controls/Controls.tsx Replaces no-print with print:hidden (with duplicates)
frontend/src/components/editor/chrome/* Multiple files replacing no-print with print:hidden across chrome UI elements
frontend/src/components/debug/indicator.tsx Adds print:hidden to debug indicator
frontend/src/components/data-table/* Multiple files with class name updates and data-testid issues

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

data-testid="notebook-actions-dropdown"
className={cn(
"right-0 top-0 z-50 m-4 no-print flex gap-2 print:hidden",
"right-0 top-0 z-50 m-4 print:hidden flex gap-2 print:hidden",
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The class "print:hidden" is duplicated in this className string. It appears twice: once at the beginning and once at the end. Remove one occurrence to avoid redundancy.

Suggested change
"right-0 top-0 z-50 m-4 print:hidden flex gap-2 print:hidden",
"right-0 top-0 z-50 m-4 print:hidden flex gap-2",
Copilot uses AI. Check for mistakes.

const topRightControls =
"absolute top-3 right-5 m-0 flex items-center gap-2 min-h-[28px] no-print pointer-events-auto z-30 print:hidden";
"absolute top-3 right-5 m-0 flex items-center gap-2 min-h-[28px] print:hidden pointer-events-auto z-30 print:hidden";
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The class "print:hidden" is duplicated in this className string. It appears twice in the same line. Remove one occurrence to avoid redundancy.

Copilot uses AI. Check for mistakes.

const bottomRightControls =
"absolute bottom-5 right-5 flex flex-col gap-2 items-center no-print pointer-events-auto z-30 print:hidden";
"absolute bottom-5 right-5 flex flex-col gap-2 items-center print:hidden pointer-events-auto z-30 print:hidden";
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The class "print:hidden" is duplicated in this className string. It appears twice in the same line. Remove one occurrence to avoid redundancy.

Copilot uses AI. Check for mistakes.

return (
<div className="h-full pt-4 pb-1 px-1 flex flex-col items-start text-muted-foreground text-md select-none no-print text-sm z-50 dark:bg-background print:hidden hide-on-fullscreen">
<div className="h-full pt-4 pb-1 px-1 flex flex-col items-start text-muted-foreground text-md select-none print:hidden text-sm z-50 dark:bg-background print:hidden hide-on-fullscreen">
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The class "print:hidden" is duplicated in this className string. It appears twice in the same line. Remove one occurrence to avoid redundancy.

Suggested change
<div className="h-full pt-4 pb-1 px-1 flex flex-col items-start text-muted-foreground text-md select-none print:hidden text-sm z-50 dark:bg-background print:hidden hide-on-fullscreen">
<div className="h-full pt-4 pb-1 px-1 flex flex-col items-start text-muted-foreground text-md select-none text-sm z-50 dark:bg-background print:hidden hide-on-fullscreen">
Copilot uses AI. Check for mistakes.

return (
<footer className="h-10 py-1 gap-1 bg-background flex items-center text-muted-foreground text-md pl-2 pr-1 border-t border-border select-none no-print text-sm z-50 print:hidden hide-on-fullscreen overflow-x-auto overflow-y-hidden scrollbar-thin">
<footer className="h-10 py-1 gap-1 bg-background flex items-center text-muted-foreground text-md pl-2 pr-1 border-t border-border select-none print:hidden text-sm z-50 print:hidden hide-on-fullscreen overflow-x-auto overflow-y-hidden scrollbar-thin">
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The class "print:hidden" is duplicated in this className string. It appears twice in the same line. Remove one occurrence to avoid redundancy.

Suggested change
<footer className="h-10 py-1 gap-1 bg-background flex items-center text-muted-foreground text-md pl-2 pr-1 border-t border-border select-none print:hidden text-sm z-50 print:hidden hide-on-fullscreen overflow-x-auto overflow-y-hidden scrollbar-thin">
<footer className="h-10 py-1 gap-1 bg-background flex items-center text-muted-foreground text-md pl-2 pr-1 border-t border-border select-none print:hidden text-sm z-50 hide-on-fullscreen overflow-x-auto overflow-y-hidden scrollbar-thin">
Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, there is an extra print:hidden in here

Comment on lines +153 to +154
if (
classes.contains("mpl-toolbar") ||
classes.contains("print:hidden")
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The filter is checking for a class named "print:hidden", but this is a Tailwind CSS utility class that gets compiled to a different class name. The actual class name in the DOM will not be "print:hidden" but something like "print:hidden" or a transformed version. This filter will not work as intended. Consider using a different approach, such as filtering based on a custom class (e.g., "no-screenshot") or checking for the computed style property (e.g., checking if the element has display:none in print media).

Copilot uses AI. Check for mistakes.
<Button
size="xs"
data-testid="select-all-button"
data-testid="select-all-button print:hidden"
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The CSS class "print:hidden" should not be part of the data-testid attribute. This appears to be incorrectly placed - the class should be in the className attribute instead. The test identifier should remain as just "select-all-button".

Copilot uses AI. Check for mistakes.
<Button
size="xs"
data-testid="clear-selection-button"
data-testid="clear-selection-button print:hidden"
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The CSS class "print:hidden" should not be part of the data-testid attribute. This appears to be incorrectly placed - the class should be in the className attribute instead. The test identifier should remain as just "clear-selection-button".

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@manzt manzt left a comment

Choose a reason for hiding this comment

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

some minor nits, over all looks great


return (
<div className="fixed bottom-10 right-0 z-50 flex items-center justify-center bg-gray-800 py-[2px] px-1 font-mono text-[10px] text-white font-semibold">
<div className="fixed bottom-10 right-0 z-50 flex items-center justify-center bg-gray-800 py-[2px] px-1 font-mono text-[10px] text-white font-semibold print:hidden">
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm guessing no-print was missed here before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea and in many places


return (
<footer className="h-10 py-1 gap-1 bg-background flex items-center text-muted-foreground text-md pl-2 pr-1 border-t border-border select-none no-print text-sm z-50 print:hidden hide-on-fullscreen overflow-x-auto overflow-y-hidden scrollbar-thin">
<footer className="h-10 py-1 gap-1 bg-background flex items-center text-muted-foreground text-md pl-2 pr-1 border-t border-border select-none print:hidden text-sm z-50 print:hidden hide-on-fullscreen overflow-x-auto overflow-y-hidden scrollbar-thin">
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, there is an extra print:hidden in here


const topRightControls =
"absolute top-3 right-5 m-0 flex items-center gap-2 min-h-[28px] no-print pointer-events-auto z-30 print:hidden";
"absolute top-3 right-5 m-0 flex items-center gap-2 min-h-[28px] print:hidden pointer-events-auto z-30 print:hidden";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"absolute top-3 right-5 m-0 flex items-center gap-2 min-h-[28px] print:hidden pointer-events-auto z-30 print:hidden";
"absolute top-3 right-5 m-0 flex items-center gap-2 min-h-[28px] pointer-events-auto z-30 print:hidden";

const bottomRightControls =
"absolute bottom-5 right-5 flex flex-col gap-2 items-center no-print pointer-events-auto z-30 print:hidden";
"absolute bottom-5 right-5 flex flex-col gap-2 items-center print:hidden pointer-events-auto z-30 print:hidden";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"absolute bottom-5 right-5 flex flex-col gap-2 items-center print:hidden pointer-events-auto z-30 print:hidden";
"absolute bottom-5 right-5 flex flex-col gap-2 items-center pointer-events-auto z-30 print:hidden";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

React.HTMLAttributes<HTMLTableElement>
>(({ className, ...props }, ref) => (
<div className="w-full overflow-auto scrollbar-thin flex-1">
<div className="w-full overflow-auto scrollbar-thin flex-1 print:overflow-hidden">
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be print:hidden or is print:overflow-hidden correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is correct (i just want to hide scrollbars when printing)


// Await in-flight captures started by concurrent callers
for (const { cellId, promise } of inFlightWaiters) {
const result = await promise;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want an Promise.all or Promise.allSettled here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ill add it but we never reject these. purely just to wait

@mscolnick mscolnick merged commit d82b3f6 into main Jan 28, 2026
29 of 30 checks passed
@mscolnick mscolnick deleted the ms/race-condition-export branch January 28, 2026 21:32
@github-actions
Copy link

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.19.7-dev36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

3 participants