codeant-ai-for-open-source[bot] commented on PR #36858: URL: https://github.com/apache/superset/pull/36858#issuecomment-3702126258
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36858/files#diff-1fe9f23910bda67f66ff7b5e9127cdaba0d304153a5246dbcb2d76d70d2e5a4fR1101-R1121'><strong>Race condition</strong></a><br>When expand/collapse calls reach the backend (table.initialized is true) rapid user toggles may cause multiple concurrent post requests that can flip server state unpredictably. Consider debouncing, coalescing, or canceling in-flight requests to keep server and UI state consistent.<br> - [ ] <a href='https://github.com/apache/superset/pull/36858/files#diff-1fe9f23910bda67f66ff7b5e9127cdaba0d304153a5246dbcb2d76d70d2e5a4fR1101-R1108'><strong>Endpoint encoding</strong></a><br>The code builds the backend endpoint using encodeURI with a dynamic `table.id`. If `table.id` can contain characters that need path-segment encoding (e.g., slashes, spaces, unicode), encodeURI may not be sufficient. Use `encodeURIComponent` for the id segment to avoid sending malformed URLs to the backend.<br> - [ ] <a href='https://github.com/apache/superset/pull/36858/files#diff-c489b64153b9a09dc1f0984560850db74bf49af1184bfe68b9f258a44b89105aR1495-R1523'><strong>Mock restore misuse</strong></a><br>The tests call isFeatureEnabled.mockRestore() after using mockImplementation. However, isFeatureEnabled is created via jest.mock(...) as a jest.fn(), and mockRestore() is only valid for mocks created by jest.spyOn or when a restore implementation exists. Calling mockRestore on a plain jest.fn() may throw a TypeError at runtime. There's also an afterEach() in the same describe that calls mockRestore again, increasing risk of double restore. Use mockReset/mockClear or adjust the mocking strategy (spyOn the import) to allow restore.<br> - [ ] <a href='https://github.com/apache/superset/pull/36858/files#diff-c489b64153b9a09dc1f0984560850db74bf49af1184bfe68b9f258a44b89105aR1431-R1440'><strong>Brittle fetchMock call detection</strong></a><br>Several new tests locate calls to the expanded table endpoint by calling fetchMock.calls().filter(...) and using call[0].includes(...). This is brittle: depending on fetch-mock configuration the first element of a call may be a Request object (no .includes) or the URL string; calling .includes without checking type can throw. Also filtering by substring is less precise than using fetchMock.calls(matcher). Prefer using fetchMock.calls(updateTableSchemaExpandedEndpoint) or ensuring the item is a string before using includes to avoid intermittent test failures.<br> - [ ] <a href='https://github.com/apache/superset/pull/36858/files#diff-1fe9f23910bda67f66ff7b5e9127cdaba0d304153a5246dbcb2d76d70d2e5a4fR1101-R1134'><strong>Code duplication</strong></a><br>The expand and collapse handlers duplicate the same persistence logic (building `sync`, posting to the same endpoint). Extracting a small helper will reduce duplication and make future changes (e.g., encoding, retries) easier and less error-prone.<br> </td></tr> </table> -- 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]
