Skip to content

Conversation

@arai-a
Copy link
Contributor

@arai-a arai-a commented Dec 23, 2025

Fixed #5731

This adds a button to the Marker Table settings bar, which shows a context menu that provides the following:

  • copy as plain text
  • copy as Markdown

(not sure if Markdown is actually necessary. If not, it can be simplified to just a single button that copies as plain text)

the plain text copies the table in the following style:

  Start  Duration                     Name  Details
 0.002s                   Process Priority  priority: BACKGROUND
 0.002s   unknown                    Awake
 0.002s  603.25us                 Runnable  TriggerPollJSSamplingOnMainThread - priority: Normal (4) task: 4d9c1b1537e9fcf80

the Markdown copies the table in the following style:

|   Start | Duration |                    Name | Details |
|--------:|---------:|------------------------:|---------|
|  0.002s |          |        Process Priority | priority: BACKGROUND |
|  0.002s |  unknown |                   Awake |  |
|  0.002s | 603.25us |                Runnable | TriggerPollJSSamplingOnMainThread - priority: Normal (4) task: 4d9c1b1537e9fcf80 |
copy-table

@arai-a arai-a requested a review from a team as a code owner December 23, 2025 12:34
@canova canova self-requested a review December 23, 2025 13:25
Copy link
Member

@canova canova left a comment

Choose a reason for hiding this comment

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

Nice, I like the idea of being able to copy the table!

One thing I noticed is that it is hanging for a long time if the table is huge and if there is no filtering applied to it. Normally, we virtualize this table, so it might look like we don't have a lot sometimes, but for large profile it might be too many marker lines.

Maybe we can have an upper limit like const MAX_COPY_ROWS = 10000, and show users a warning that the rest is ommitted. What do you think?;


lines.push([
data.start,
duration.replace(/μ/g, 'u'),
Copy link
Member

Choose a reason for hiding this comment

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

Why do we replace μ here? Are unicode chars a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because "μ" is wider than other ASCII characters (at least for me) and that breaks the alignment of the table

this._getMarkerLabel = getMarkerLabel;
}

copyTable = (format: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this function inside src/profile-logic/marker-data.ts. I think this is a function that's worth extracting outside of the react component. (not the copy() call, but the table building logic)

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 heavily tied to the MarkerTree class, which is local to MarkerTable component.
especially the marker handling, zeroAt, the column alignment etc are specific to the file and they depends on the memoization, and also the state map defined there.

So, the entire logic cannot be moved, and splitting a part of the logic will make it harder to follow.

@arai-a
Copy link
Contributor Author

arai-a commented Dec 25, 2025

I forgot to apply the upper limit.
I'll address that part shortly

@codecov
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

❌ Patch coverage is 94.25287% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.67%. Comparing base (1a808fe) to head (24dc2f2).
⚠️ Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
src/components/shared/MarkerSettings.tsx 91.17% 3 Missing ⚠️
src/components/marker-table/index.tsx 95.34% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5732      +/-   ##
==========================================
+ Coverage   85.64%   85.67%   +0.02%     
==========================================
  Files         313      314       +1     
  Lines       31013    31100      +87     
  Branches     8535     8469      -66     
==========================================
+ Hits        26562    26644      +82     
- Misses       4021     4026       +5     
  Partials      430      430              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@canova canova left a comment

Choose a reason for hiding this comment

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

Thanks, looks pretty good to me. I have some small nits, but otherwise we can land this after them.

(I was also going to suggest rebasing and fixing the prettier warnings with yarn prettier-fix but it looks like you already did it)

Copy link
Member

@canova canova left a comment

Choose a reason for hiding this comment

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

Thanks!

@canova canova merged commit 87cac64 into firefox-devtools:main Jan 8, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make the Marker Table copyable as text

3 participants