fix: scrollbar measurement missing CSP nonce when creating <style>#744
fix: scrollbar measurement missing CSP nonce when creating <style>#744danielthegray wants to merge 3 commits intoreact-component:masterfrom
Conversation
|
@danielthegray is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a Content Security Policy (CSP) compliance issue by integrating nonce support into the scrollbar measurement utility. It ensures that when the component dynamically creates Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough扩展了滚动条测量函数以接受可选的 CSP 配置对象（含 Changes
Sequence Diagram(s)(无) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Content Security Policy (CSP) support, specifically for nonce attributes, to the scrollbar size measurement functions (measureScrollbarSize and getTargetScrollBarSize). This involves passing a csp object to ensure dynamically created <style> elements comply with CSP. The primary feedback is that the new csp functionality lacks test coverage, and a unit test is requested to verify the correct application of the nonce to the created <style> element.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/getScrollBarSize.test.ts (1)
51-71: 测试清理逻辑建议改进当前的清理代码（
mockRestore()和removeChild()）位于测试末尾，如果断言失败，这些清理操作将不会执行，可能影响后续测试的隔离性。建议使用
try/finally或将清理逻辑移至afterEach钩子中以确保资源始终被释放。♻️ 建议的改进方案
it('should pass csp nonce to updateCSS', () => { const updateCSSSpy = jest.spyOn( require('../src/Dom/dynamicCSS'), 'updateCSS', ); const target = document.createElement('div'); document.body.appendChild(target); - const nonce = 'test-nonce-123'; - getTargetScrollBarSize(target, { nonce }); - - expect(updateCSSSpy).toHaveBeenCalledWith( - expect.any(String), - expect.any(String), - { csp: { nonce } }, - ); - - updateCSSSpy.mockRestore(); - document.body.removeChild(target); + try { + const nonce = 'test-nonce-123'; + getTargetScrollBarSize(target, { nonce }); + + expect(updateCSSSpy).toHaveBeenCalledWith( + expect.any(String), + expect.any(String), + { csp: { nonce } }, + ); + } finally { + updateCSSSpy.mockRestore(); + document.body.removeChild(target); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/getScrollBarSize.test.ts` around lines 51 - 71, Move the test cleanup into a guaranteed teardown so resources are always released: ensure the spy created for updateCSS (updateCSSSpy) is restored and the DOM node (target) appended for getTargetScrollBarSize is removed even if assertions fail by wrapping the test body in a try/finally that calls updateCSSSpy.mockRestore() and document.body.removeChild(target), or alternatively register those cleanup steps in an afterEach hook to restore the spy and remove the target element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/getScrollBarSize.test.ts`:
- Around line 51-71: Move the test cleanup into a guaranteed teardown so
resources are always released: ensure the spy created for updateCSS
(updateCSSSpy) is restored and the DOM node (target) appended for
getTargetScrollBarSize is removed even if assertions fail by wrapping the test
body in a try/finally that calls updateCSSSpy.mockRestore() and
document.body.removeChild(target), or alternatively register those cleanup steps
in an afterEach hook to restore the spy and remove the target element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5723034-4c68-4df5-a8bb-7c837a6ab4ff
📒 Files selected for processing (1)
tests/getScrollBarSize.test.ts
1 participant
The CSP needs to be passed in by the calling component (via the ConfigContext or ConfigProvider), but these utility methods need to have a way to pass in this value, to be used when cloning the <style> element and calling
updateCSS().I tried to add this in a way that would not break any of the callers.
Summary by CodeRabbit