codeant-ai-for-open-source[bot] commented on PR #36196: URL: https://github.com/apache/superset/pull/36196#issuecomment-3605111203
## 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/36196/files#diff-c7d5bc27b42b8e45ffc6d9b7773e84b4b785d849653825981ade10eb40d7fd81R99-R176'><strong>Timeout Handling</strong></a><br>The new `waitForLoginSuccess` method enforces its own timeout but delegates to `waitForLoginRequest`, which still relies on Playwright's default timeout. This can cause the overall wait to exceed the caller-provided timeout and lead to flaky or confusing behavior. Consider propagating the same timeout into `waitForResponse` or otherwise aligning these timeouts.<br> - [ ] <a href='https://github.com/apache/superset/pull/36196/files#diff-0fdea19044d6444014a0c32b61cd814ed60e24a73156f1bbeb316fa4431135daR88-R104'><strong>Possible Bug</strong></a><br>The glob patterns used in `chromium`'s `testIgnore` and `chromium-unauth`'s `testMatch` include an extra `tests/` segment (`'**/tests/auth/**/*.spec.ts'`) even though `testDir` is already set to `'./playwright/tests'`. These patterns likely won't match the intended auth spec files, causing auth tests not to be routed correctly between the authenticated and unauthenticated projects. This should be verified and adjusted so the globs match paths relative to `testDir`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36196/files#diff-f52ab7734f71a91be9b766528ad252d58a8c5be235ae5df9af7280e411a11b39R55-R59'><strong>Possible Bug</strong></a><br>getRow is documented as matching the first cell but the implementation matches any cell in the row, which can lead to ambiguous row selection if the same text appears in other columns.<br> - [ ] <a href='https://github.com/apache/superset/pull/36196/files#diff-71657b26b223754c48a57bb862823512a24a03895f26728929beda862b21f2a1R74-R100'><strong>Possible Bug</strong></a><br>`getDatasetByName` calls `apiGet` without overriding `failOnStatusCode`, but then relies on `response.ok()` to decide whether to return `null`. Since `apiGet` defaults `failOnStatusCode` to `true`, 4xx responses (e.g., dataset not found) will likely throw before reaching the `ok()` check, so the helper may throw instead of returning `null` as intended.<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]
