bito-code-review[bot] commented on code in PR #36388: URL: https://github.com/apache/superset/pull/36388#discussion_r2673444032
########## superset-frontend/packages/superset-ui-core/test/components/SafeMarkdown.test.tsx: ########## @@ -0,0 +1,167 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render } from '@testing-library/react'; +import { + getOverrideHtmlSchema, + SafeMarkdown, +} from '../../src/components/SafeMarkdown/SafeMarkdown'; + +describe('getOverrideHtmlSchema', () => { + it('should append the override items', () => { + const original = { + attributes: { + '*': ['size'], + }, + clobberPrefix: 'original-prefix', + tagNames: ['h1', 'h2', 'h3'], + }; + const result = getOverrideHtmlSchema(original, { + attributes: { '*': ['src'], h1: ['style'] }, + clobberPrefix: 'custom-prefix', + tagNames: ['iframe'], + }); + expect(result.clobberPrefix).toEqual('custom-prefix'); + expect(result.attributes).toEqual({ '*': ['size', 'src'], h1: ['style'] }); + expect(result.tagNames).toEqual(['h1', 'h2', 'h3', 'iframe']); + }); +}); + +describe('SafeMarkdown', () => { + describe('remark-gfm compatibility tests', () => { + /** + * Critical regression test for remark-gfm v3.0.1 compatibility. + * + * CONTEXT: + * - remark-gfm v4+ requires unified v11 (react-markdown v9+, React 18+) + * - react-markdown v8 uses unified v10 (compatible with React 17) + * - Mixing remark-gfm v4 with react-markdown v8 causes: + * "Cannot set properties of undefined (setting 'inTable')" error + * + * HISTORY: + * - PR #32420 (March 2025): Fixed by pinning remark-gfm to v3 + * - PR #32945 (July 2025): Dependabot auto-upgraded to v4, breaking tables + * - This test prevents future auto-upgrades from breaking functionality + * + * This test will FAIL if remark-gfm is upgraded to v4+ without upgrading + * react-markdown to v9+ (which requires React 18). + */ + it('should render GitHub Flavored Markdown tables without errors', () => { Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Async test synchronization removed</b></div> <div id="fix"> The SafeMarkdown component uses dynamic imports and setState for lazy loading, making its rendering asynchronous. Removing async/await and waitFor assumes synchronous behavior, but the component initially renders null and updates after imports resolve. This will cause tests to fail as container.innerHTML will be empty on first render. </div> </div> <small><i>Code Review Run #0e3032</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/packages/superset-ui-core/test/components/SafeMarkdown.test.tsx: ########## @@ -0,0 +1,167 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render } from '@testing-library/react'; +import { + getOverrideHtmlSchema, + SafeMarkdown, +} from '../../src/components/SafeMarkdown/SafeMarkdown'; + +describe('getOverrideHtmlSchema', () => { + it('should append the override items', () => { + const original = { + attributes: { + '*': ['size'], + }, + clobberPrefix: 'original-prefix', + tagNames: ['h1', 'h2', 'h3'], + }; + const result = getOverrideHtmlSchema(original, { + attributes: { '*': ['src'], h1: ['style'] }, + clobberPrefix: 'custom-prefix', + tagNames: ['iframe'], + }); + expect(result.clobberPrefix).toEqual('custom-prefix'); + expect(result.attributes).toEqual({ '*': ['size', 'src'], h1: ['style'] }); + expect(result.tagNames).toEqual(['h1', 'h2', 'h3', 'iframe']); + }); +}); + +describe('SafeMarkdown', () => { + describe('remark-gfm compatibility tests', () => { + /** + * Critical regression test for remark-gfm v3.0.1 compatibility. + * + * CONTEXT: + * - remark-gfm v4+ requires unified v11 (react-markdown v9+, React 18+) + * - react-markdown v8 uses unified v10 (compatible with React 17) + * - Mixing remark-gfm v4 with react-markdown v8 causes: + * "Cannot set properties of undefined (setting 'inTable')" error + * + * HISTORY: + * - PR #32420 (March 2025): Fixed by pinning remark-gfm to v3 + * - PR #32945 (July 2025): Dependabot auto-upgraded to v4, breaking tables + * - This test prevents future auto-upgrades from breaking functionality + * + * This test will FAIL if remark-gfm is upgraded to v4+ without upgrading + * react-markdown to v9+ (which requires React 18). + */ + it('should render GitHub Flavored Markdown tables without errors', () => { + const markdownWithTable = ` +| Header 1 | Header 2 | Header 3 | +|----------|----------|----------| +| Cell 1 | Cell 2 | Cell 3 | +| Value A | Value B | Value C | + `.trim(); + + // This will throw "Cannot set properties of undefined (setting 'inTable')" + // if remark-gfm v4+ is used with react-markdown v8 + const { container } = render(<SafeMarkdown source={markdownWithTable} />); + + // Verify table was rendered (basic check that component doesn't throw) + expect(container.innerHTML).toBeTruthy(); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Weakened test assertions</b></div> <div id="fix"> Assertions weakened from checking specific DOM elements (e.g., table existence, header text content) to basic innerHTML contains checks. This reduces test reliability as it doesn't verify proper element structure or content accuracy. </div> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/fc123c8/.cursor/rules/dev-standard.mdc#L38";>dev-standard.mdc:38</a> </li> </ul> </details> <small><i>Code Review Run #0e3032</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
