Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 12 additions & 5 deletions src/getScrollBarSize.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ type ExtendCSSStyleDeclaration = CSSStyleDeclaration & {

let cached: ScrollBarSize;

function measureScrollbarSize(ele?: HTMLElement): ScrollBarSize {
function measureScrollbarSize(
ele?: HTMLElement,
nonce?: string,
): ScrollBarSize {
const randomId = `rc-scrollbar-measure-${Math.random()
.toString(36)
.substring(7)}`;
Expand Down Expand Up @@ -53,6 +56,7 @@ ${widthStyle}
${heightStyle}
}`,
randomId,
nonce ? { csp: { nonce } } : undefined,
);
} catch (e) {
// Can't wrap, just log error
Expand Down Expand Up @@ -86,18 +90,21 @@ ${heightStyle}
};
}

export default function getScrollBarSize(fresh?: boolean): number {
export default function getScrollBarSize(
fresh?: boolean,
nonce?: string,
): number {
if (typeof document === 'undefined') {
return 0;
}

if (fresh || cached === undefined) {
cached = measureScrollbarSize();
cached = measureScrollbarSize(undefined, nonce);
}
return cached.width;
Comment on lines +93 to 104

Choose a reason for hiding this comment

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

high

The nonce parameter was added to getScrollBarSize, but it's not actually used. The nonce is passed to measureScrollbarSize(undefined, nonce), but inside measureScrollbarSize, the nonce is only used within the if (ele) block to call updateCSS. Since getScrollBarSize calls it with ele as undefined, the updateCSS function is never called, and the nonce is never used. This makes the nonce parameter on getScrollBarSize misleading and ineffective.

To fix this, the nonce parameter should be removed from getScrollBarSize since it has no effect.

export default function getScrollBarSize(fresh?: boolean): number {
  if (typeof document === 'undefined') {
    return 0;
  }

  if (fresh || cached === undefined) {
    cached = measureScrollbarSize();
  }
  return cached.width;
}

}

export function getTargetScrollBarSize(target: HTMLElement) {
export function getTargetScrollBarSize(target: HTMLElement, nonce?: string) {
if (
typeof document === 'undefined' ||
!target ||
Expand All @@ -106,5 +113,5 @@ export function getTargetScrollBarSize(target: HTMLElement) {
return { width: 0, height: 0 };
}

return measureScrollbarSize(target);
return measureScrollbarSize(target, nonce);
}
133 changes: 133 additions & 0 deletions tests/getScrollBarSize.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import getScrollBarSize, {
getTargetScrollBarSize,
} from '../src/getScrollBarSize';
import { spyElementPrototypes } from '../src/test/domHook';
import * as dynamicCSS from '../src/Dom/dynamicCSS';

const DEFAULT_SIZE = 16;

Expand Down Expand Up @@ -48,4 +49,136 @@ describe('getScrollBarSize', () => {
});
});
});

describe('nonce support', () => {
let updateCSSSpy: jest.SpyInstance;

beforeEach(() => {
updateCSSSpy = jest.spyOn(dynamicCSS, 'updateCSS');
});

afterEach(() => {
updateCSSSpy.mockRestore();
});

it('should pass nonce to updateCSS when provided in getScrollBarSize', () => {
const testNonce = 'test-nonce-123';

// We need to test with getTargetScrollBarSize since getScrollBarSize
// without an element doesn't trigger updateCSS
const mockElement = document.createElement('div');
document.body.appendChild(mockElement);

// Mock getComputedStyle to return webkit scrollbar dimensions
const originalGetComputedStyle = window.getComputedStyle;
window.getComputedStyle = jest
.fn()
.mockImplementation((element, pseudoElement) => {
if (pseudoElement === '::-webkit-scrollbar') {
return {
width: '10px',
height: '10px',
};
}
return {
scrollbarColor: '',
scrollbarWidth: '',
overflow: 'scroll',
};
}) as any;

// This will trigger updateCSS with webkit styles
getTargetScrollBarSize(mockElement, testNonce);

// Restore original
window.getComputedStyle = originalGetComputedStyle;
document.body.removeChild(mockElement);

// Check if updateCSS was called with nonce
const updateCSSCalls = updateCSSSpy.mock.calls;
const callWithNonce = updateCSSCalls.find(
call => call[2]?.csp?.nonce === testNonce,
);

expect(callWithNonce).toBeDefined();
});
Comment on lines +64 to +104
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

测试标题与实际测试内容不一致。

测试标题提到 "when provided in getScrollBarSize",但实际上测试调用的是 getTargetScrollBarSize(第 91 行)。虽然注释解释了原因,但标题仍然具有误导性。

建议将标题更新为更准确的描述,例如:

-    it('should pass nonce to updateCSS when provided in getScrollBarSize', () => {
+    it('should pass nonce to updateCSS when provided to getTargetScrollBarSize', () => {

或者,如果想保持原标题意图,可以添加一个独立的测试来验证 getScrollBarSize 的 nonce 参数(即使它不会触发 updateCSS)。

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('should pass nonce to updateCSS when provided in getScrollBarSize', () => {
const testNonce = 'test-nonce-123';
// We need to test with getTargetScrollBarSize since getScrollBarSize
// without an element doesn't trigger updateCSS
const mockElement = document.createElement('div');
document.body.appendChild(mockElement);
// Mock getComputedStyle to return webkit scrollbar dimensions
const originalGetComputedStyle = window.getComputedStyle;
window.getComputedStyle = jest
.fn()
.mockImplementation((element, pseudoElement) => {
if (pseudoElement === '::-webkit-scrollbar') {
return {
width: '10px',
height: '10px',
};
}
return {
scrollbarColor: '',
scrollbarWidth: '',
overflow: 'scroll',
};
}) as any;
// This will trigger updateCSS with webkit styles
getTargetScrollBarSize(mockElement, testNonce);
// Restore original
window.getComputedStyle = originalGetComputedStyle;
document.body.removeChild(mockElement);
// Check if updateCSS was called with nonce
const updateCSSCalls = updateCSSSpy.mock.calls;
const callWithNonce = updateCSSCalls.find(
call => call[2]?.csp?.nonce === testNonce,
);
expect(callWithNonce).toBeDefined();
});
it('should pass nonce to updateCSS when provided to getTargetScrollBarSize', () => {
const testNonce = 'test-nonce-123';
// We need to test with getTargetScrollBarSize since getScrollBarSize
// without an element doesn't trigger updateCSS
const mockElement = document.createElement('div');
document.body.appendChild(mockElement);
// Mock getComputedStyle to return webkit scrollbar dimensions
const originalGetComputedStyle = window.getComputedStyle;
window.getComputedStyle = jest
.fn()
.mockImplementation((element, pseudoElement) => {
if (pseudoElement === '::-webkit-scrollbar') {
return {
width: '10px',
height: '10px',
};
}
return {
scrollbarColor: '',
scrollbarWidth: '',
overflow: 'scroll',
};
}) as any;
// This will trigger updateCSS with webkit styles
getTargetScrollBarSize(mockElement, testNonce);
// Restore original
window.getComputedStyle = originalGetComputedStyle;
document.body.removeChild(mockElement);
// Check if updateCSS was called with nonce
const updateCSSCalls = updateCSSSpy.mock.calls;
const callWithNonce = updateCSSCalls.find(
call => call[2]?.csp?.nonce === testNonce,
);
expect(callWithNonce).toBeDefined();
});
🤖 Prompt for AI Agents
tests/getScrollBarSize.test.ts around lines 64 to 104: the test title states
"when provided in getScrollBarSize" but the test actually calls
getTargetScrollBarSize; update the test to accurately describe what it does by
changing the title to something like "should pass nonce to updateCSS when
provided in getTargetScrollBarSize", or alternatively add a separate test that
calls getScrollBarSize to verify its nonce behavior while keeping this test
focused on getTargetScrollBarSize.


it('should pass nonce to updateCSS when provided in getTargetScrollBarSize', () => {
const testNonce = 'target-nonce-456';
const mockElement = document.createElement('div');
document.body.appendChild(mockElement);

// Mock getComputedStyle to return webkit scrollbar dimensions
const originalGetComputedStyle = window.getComputedStyle;
window.getComputedStyle = jest
.fn()
.mockImplementation((element, pseudoElement) => {
if (pseudoElement === '::-webkit-scrollbar') {
return {
width: '12px',
height: '12px',
};
}
return {
scrollbarColor: '',
scrollbarWidth: '',
overflow: 'scroll',
};
}) as any;

getTargetScrollBarSize(mockElement, testNonce);

// Restore original
window.getComputedStyle = originalGetComputedStyle;
document.body.removeChild(mockElement);

// Check if updateCSS was called with nonce
const updateCSSCalls = updateCSSSpy.mock.calls;
const callWithNonce = updateCSSCalls.find(
call => call[2]?.csp?.nonce === testNonce,
);

expect(callWithNonce).toBeDefined();
});

it('should not pass nonce to updateCSS when not provided', () => {
const mockElement = document.createElement('div');
document.body.appendChild(mockElement);

// Mock getComputedStyle to return webkit scrollbar dimensions
const originalGetComputedStyle = window.getComputedStyle;
window.getComputedStyle = jest
.fn()
.mockImplementation((element, pseudoElement) => {
if (pseudoElement === '::-webkit-scrollbar') {
return {
width: '8px',
height: '8px',
};
}
return {
scrollbarColor: '',
scrollbarWidth: '',
overflow: 'scroll',
};
}) as any;

// Clear previous calls
updateCSSSpy.mockClear();

// Call without nonce
getTargetScrollBarSize(mockElement);

// Restore original
window.getComputedStyle = originalGetComputedStyle;
document.body.removeChild(mockElement);

// Check if updateCSS was called without nonce (undefined as third parameter)
const updateCSSCalls = updateCSSSpy.mock.calls;
if (updateCSSCalls.length > 0) {
const lastCall = updateCSSCalls[updateCSSCalls.length - 1];
expect(lastCall[2]).toBeUndefined();
}
});
Comment on lines +144 to +182
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

测试断言可能会隐藏失败情况。

第 178-181 行的条件检查 if (updateCSSCalls.length > 0) 意味着如果 updateCSS 未被调用,测试会静默通过而不进行任何断言。这可能会隐藏实际问题。

建议明确断言调用次数,然后检查参数:

🔎 建议的改进
      // Check if updateCSS was called without nonce (undefined as third parameter)
      const updateCSSCalls = updateCSSSpy.mock.calls;
-     if (updateCSSCalls.length > 0) {
-       const lastCall = updateCSSCalls[updateCSSCalls.length - 1];
-       expect(lastCall[2]).toBeUndefined();
-     }
+     expect(updateCSSCalls.length).toBeGreaterThan(0);
+     const lastCall = updateCSSCalls[updateCSSCalls.length - 1];
+     expect(lastCall[2]).toBeUndefined();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('should not pass nonce to updateCSS when not provided', () => {
const mockElement = document.createElement('div');
document.body.appendChild(mockElement);
// Mock getComputedStyle to return webkit scrollbar dimensions
const originalGetComputedStyle = window.getComputedStyle;
window.getComputedStyle = jest
.fn()
.mockImplementation((element, pseudoElement) => {
if (pseudoElement === '::-webkit-scrollbar') {
return {
width: '8px',
height: '8px',
};
}
return {
scrollbarColor: '',
scrollbarWidth: '',
overflow: 'scroll',
};
}) as any;
// Clear previous calls
updateCSSSpy.mockClear();
// Call without nonce
getTargetScrollBarSize(mockElement);
// Restore original
window.getComputedStyle = originalGetComputedStyle;
document.body.removeChild(mockElement);
// Check if updateCSS was called without nonce (undefined as third parameter)
const updateCSSCalls = updateCSSSpy.mock.calls;
if (updateCSSCalls.length > 0) {
const lastCall = updateCSSCalls[updateCSSCalls.length - 1];
expect(lastCall[2]).toBeUndefined();
}
});
it('should not pass nonce to updateCSS when not provided', () => {
const mockElement = document.createElement('div');
document.body.appendChild(mockElement);
// Mock getComputedStyle to return webkit scrollbar dimensions
const originalGetComputedStyle = window.getComputedStyle;
window.getComputedStyle = jest
.fn()
.mockImplementation((element, pseudoElement) => {
if (pseudoElement === '::-webkit-scrollbar') {
return {
width: '8px',
height: '8px',
};
}
return {
scrollbarColor: '',
scrollbarWidth: '',
overflow: 'scroll',
};
}) as any;
// Clear previous calls
updateCSSSpy.mockClear();
// Call without nonce
getTargetScrollBarSize(mockElement);
// Restore original
window.getComputedStyle = originalGetComputedStyle;
document.body.removeChild(mockElement);
// Check if updateCSS was called without nonce (undefined as third parameter)
const updateCSSCalls = updateCSSSpy.mock.calls;
expect(updateCSSCalls.length).toBeGreaterThan(0);
const lastCall = updateCSSCalls[updateCSSCalls.length - 1];
expect(lastCall[2]).toBeUndefined();
});
🤖 Prompt for AI Agents
In tests/getScrollBarSize.test.ts around lines 144 to 182 the test silently
passes when updateCSSSpy was never called because of the if
(updateCSSCalls.length > 0) guard; replace that conditional with explicit
assertions: assert that updateCSSSpy was called (e.g.,
expect(updateCSSSpy).toHaveBeenCalled() or
expect(updateCSSSpy.mock.calls.length).toBeGreaterThan(0)) and then assert the
third argument of the last call is undefined (e.g., inspect
updateCSSSpy.mock.calls[updateCSSSpy.mock.calls.length - 1][2] and expect it
toBeUndefined()); this ensures the test fails if updateCSS was not invoked and
still verifies the nonce parameter when it was.

});
Comment on lines +53 to +183

Choose a reason for hiding this comment

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

medium

The tests within this describe block have significant code duplication for setting up mocks and DOM elements. This can be refactored using beforeEach and afterEach hooks to improve readability and maintainability.

Additionally, the first two tests are functionally identical, testing that a nonce is passed to updateCSS via getTargetScrollBarSize. They can be consolidated into a single test. The first test's name is also misleading as it refers to getScrollBarSize but tests getTargetScrollBarSize.

  describe('nonce support', () => {
    let updateCSSSpy: jest.SpyInstance;
    let mockElement: HTMLDivElement;
    const originalGetComputedStyle = window.getComputedStyle;

    beforeEach(() => {
      updateCSSSpy = jest.spyOn(dynamicCSS, 'updateCSS');
      mockElement = document.createElement('div');
      document.body.appendChild(mockElement);

      // Mock getComputedStyle to return webkit scrollbar dimensions
      window.getComputedStyle = jest
        .fn()
        .mockImplementation((element, pseudoElement) => {
          if (pseudoElement === '::-webkit-scrollbar') {
            return {
              width: '10px',
              height: '10px',
            };
          }
          return {
            scrollbarColor: '',
            scrollbarWidth: '',
            overflow: 'scroll',
          };
        }) as any;
    });

    afterEach(() => {
      updateCSSSpy.mockRestore();
      window.getComputedStyle = originalGetComputedStyle;
      document.body.removeChild(mockElement);
    });

    it('should pass nonce to updateCSS when provided to getTargetScrollBarSize', () => {
      const testNonce = 'test-nonce-123';

      getTargetScrollBarSize(mockElement, testNonce);

      const updateCSSCalls = updateCSSSpy.mock.calls;
      const callWithNonce = updateCSSCalls.find(
        call => call[2]?.csp?.nonce === testNonce,
      );

      expect(callWithNonce).toBeDefined();
    });

    it('should not pass nonce to updateCSS when not provided', () => {
      // Clear previous calls
      updateCSSSpy.mockClear();

      // Call without nonce
      getTargetScrollBarSize(mockElement);

      // Check if updateCSS was called without nonce (undefined as third parameter)
      const updateCSSCalls = updateCSSSpy.mock.calls;
      if (updateCSSCalls.length > 0) {
        const lastCall = updateCSSCalls[updateCSSCalls.length - 1];
        expect(lastCall[2]).toBeUndefined();
      }
    });
  });

});