-
Notifications
You must be signed in to change notification settings - Fork 892
improvement: additional print configuration, fix race conditions in export to pdf #8038
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this 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
AsyncCaptureTrackerto prevent race conditions when taking screenshots of multiple cells concurrently - Removes the
snappyparameter from screenshot functions, simplifying the API and always using optimized style properties - Migrates from custom
no-printCSS class to Tailwind'sprint:hiddenutility across all UI components - Removes reference counting mechanism for
body.printingclass 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", |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
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.
| "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", |
|
|
||
| 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"; |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
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.
|
|
||
| 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"; |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
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.
|
|
||
| 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"> |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
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.
| <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"> |
|
|
||
| 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"> |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
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.
| <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"> |
There was a problem hiding this comment.
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
| if ( | ||
| classes.contains("mpl-toolbar") || | ||
| classes.contains("print:hidden") |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
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).
| <Button | ||
| size="xs" | ||
| data-testid="select-all-button" | ||
| data-testid="select-all-button print:hidden" |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
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".
| <Button | ||
| size="xs" | ||
| data-testid="clear-selection-button" | ||
| data-testid="clear-selection-button print:hidden" |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
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".
e9ea1e2 to
d300643
Compare
manzt
left a comment
There was a problem hiding this 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"> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "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"; |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
frontend/src/core/export/hooks.ts
Outdated
|
|
||
| // Await in-flight captures started by concurrent callers | ||
| for (const { cellId, promise } of inFlightWaiters) { | ||
| const result = await promise; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.19.7-dev36 |
no-printforprint:hidden(since this will also hide into when printing using Cmd+P)snappyand always passincludeStyleProperties(otherwise we will get inconsistenties).printingon the body when just snapshotting the cell output