improvement: additional print configuration, fix race conditions in export to pdf#8038
improvement: additional print configuration, fix race conditions in export to pdf#8038
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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.
ditto, there is an extra print:hidden in here
| if ( | ||
| classes.contains("mpl-toolbar") || | ||
| classes.contains("print:hidden") |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
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.
i'm guessing no-print was missed here before?
There was a problem hiding this comment.
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.
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.
| "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.
| "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"; |
| 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.
should this be print:hidden or is print:overflow-hidden correct?
There was a problem hiding this comment.
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.
do we want an Promise.all or Promise.allSettled here?
There was a problem hiding this comment.
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