Skip to content

Fix splitCssText again #1640

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.

Sign up for GitHub

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

Merged
merged 18 commits into from
Feb 6, 2025
Merged

Fix splitCssText again #1640

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
fb694b2
Fix up the 'should replace the existing DOM nodes on iframe navigatio…
eoghanmurray Jan 23, 2025
5962d03
Fix a case I previously forgot about, when there are multiple matches…
eoghanmurray Jan 29, 2025
08a80ab
Failing example extracted from large files identified by Paul D'Ambra…
eoghanmurray Jan 29, 2025
43df807
Fix that we weren't stopping when going from many matches to zero mat…
eoghanmurray Jan 29, 2025
d59c1ef
Avoid (probably) a happydom bug
eoghanmurray Jan 29, 2025
21beccb
Disabliing to prove failure
eoghanmurray Jan 29, 2025
e789129
Make the matches go from 'many' to 'none'
eoghanmurray Jan 29, 2025
a72c4e1
Pick the best when there are many splits by looking at previous chunk…
eoghanmurray Jan 29, 2025
b3a5a12
This seems more straightforward
eoghanmurray Jan 29, 2025
e02e608
Rename some variables for clarity
eoghanmurray Jan 29, 2025
0347a36
yarn format
eoghanmurray Jan 29, 2025
d0fb0c5
Add changeset
eoghanmurray Jan 29, 2025
e5a9550
Mention what triggers the bug
eoghanmurray Jan 29, 2025
01d96bb
Solve the same bug another way; by normalization
eoghanmurray Jan 29, 2025
86796ac
Apply formatting changes
eoghanmurray Jan 29, 2025
3a452e3
Don't go past 100 iterations trying to find a unique substring
eoghanmurray Jan 29, 2025
7a32e49
Add a tscomment to explain the `_testNoPxNorm` test-only parameter
eoghanmurray Feb 4, 2025
5743c7e
Merge remote-tracking branch 'upstream/master' into fix-splitCssText-…
eoghanmurray Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
yarn format
  • Loading branch information
@eoghanmurray
eoghanmurray committed Jan 29, 2025
commit 0347a36a0d49212038a895a1b8bbb731cc87c1b5
8 changes: 5 additions & 3 deletions packages/rrweb-snapshot/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,16 +504,18 @@ export function splitCssText(
splitNorm = cssTextNorm.indexOf(startSubstring, 1);
} else if (cssNormSplits.length === 1) {
// try to roll back to get multiple matches again
startSubstring = startSubstring.substring(0, startSubstring.length - 1)
startSubstring = startSubstring.substring(
0,
startSubstring.length - 1,
);
cssNormSplits = cssTextNorm.split(startSubstring);
if (cssNormSplits.length <= 1) {
// no split possible
splits.push(cssText);
return splits;
}
const prevTextContent = childNodes[i - 1].textContent;
if (prevTextContent &&
typeof prevTextContent === 'string') {
if (prevTextContent && typeof prevTextContent === 'string') {
// pick the first matching point which respects the previous chunk's approx size
const prevMinLength = normalizeCssString(prevTextContent).length;
Copy link
Contributor

@pauldambra pauldambra Jan 31, 2025

Choose a reason for hiding this comment

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

i guess it's somewhere here that means if you run a test twice (or some multiple times) then you don't get the same output

(at least in my experience of testing whether this was deterministic when trying to figure out what was happening)

Copy link
Contributor Author

@eoghanmurray eoghanmurray Feb 4, 2025

Choose a reason for hiding this comment

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

I don't quite understand this comment, but I would say the algorithm is indeed deterministic, but maybe you mean it will behave differently based on different sized inputs because of the jLimit bit?

Copy link
Contributor

@pauldambra pauldambra Feb 4, 2025

Choose a reason for hiding this comment

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

oh, i wrote a test that ran the split multiple times and compared the output and it didn't match
was generally whitespace ending up on different sides of a split

Copy link
Contributor Author

@eoghanmurray eoghanmurray Feb 4, 2025

Choose a reason for hiding this comment

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

Ah okay, yes this is possible ... basically it stops when the normalised versions match (although I still think it would be deterministic given the same input twice). I can't imagine there being a problem if one side has more whitespace than it should have.

splitNorm = cssTextNorm.indexOf(startSubstring, prevMinLength);
Expand Down
3 changes: 2 additions & 1 deletion packages/rrweb-snapshot/test/css.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,8 @@ describe('css splitter', () => {
it('finds css textElement splits correctly, even with repeated sections', () => {
const window = new Window({ url: 'https://localhost:8080' });
const document = window.document;
document.head.innerHTML = '<style>.a{background-color: black; } </style>';
document.head.innerHTML =
'<style>.a{background-color: black; } </style>';
const style = document.querySelector('style');
if (style) {
style.append('.x{background-color:red;}');
Expand Down