codeant-ai-for-open-source[bot] commented on PR #36209: URL: https://github.com/apache/superset/pull/36209#issuecomment-3642408096
## 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/36209/files#diff-451f09a751938fa38073ad6506c2fe0cfc0824c91fe7cde4c067f8ddc9a7bf71R466-R508'><strong>Memo dependency bug</strong></a><br>The filters useMemo has an empty dependency array but references variables from the component scope (for example `user` and `isReportEnabled` indirectly). This can lead to stale filter config if props change. Verify and update the dependencies so the memo recalculates when relevant inputs change.<br> - [ ] <a href='https://github.com/apache/superset/pull/36209/files#diff-479e2e2fcdb82617b76b383985798f2a753629801d3af815d856063174ceb523R96-R96'><strong>Ensure DOM passthrough</strong></a><br>The `Input` component is from a shared component library; confirm that `autoComplete` is forwarded to the actual DOM <input>. If that wrapper doesn't forward unknown props, the `autoComplete` setting won't reach the browser. If not forwarded, update the wrapper or explicitly set props on the underlying input element.<br> - [ ] <a href='https://github.com/apache/superset/pull/36209/files#diff-479e2e2fcdb82617b76b383985798f2a753629801d3af815d856063174ceb523R96-R96'><strong>Missing anti-autofill attribute</strong></a><br>The PR title mentions adding a `data-1p-ignore` attribute to the search input to help avoid password-manager autofill. The file change only adds an `autoComplete` prop but does not add the `data-1p-ignore` attribute to the rendered Input. Verify whether the data attribute should be added here (or intentionally omitted and implemented elsewhere) to reliably prevent 1Password/other managers from showing autofill icons when fields use common names.<br> - [ ] <a href='https://github.com/apache/superset/pull/36209/files#diff-083e28c539e0738336b113f5390324dc75c7d3ff75b7ec83cb15ea5519d63536R126-R136'><strong>Prop forwarding</strong></a><br>The new props (`inputName` and `autoComplete`) are passed into `SearchFilter` here, but it must be verified that `SearchFilter` accepts and forwards those props to the underlying <input>. If not forwarded, the changes will have no effect (particularly the `autoComplete` behavior and any data attribute used to suppress password-manager behavior).<br> - [ ] <a href='https://github.com/apache/superset/pull/36209/files#diff-083e28c539e0738336b113f5390324dc75c7d3ff75b7ec83cb15ea5519d63536R126-R136'><strong>Effective mitigation for 1Password</strong></a><br>The PR description mentions adding `data-1p-ignore` to inputs, but this file only changes the `name`/`autoComplete` props. Confirm that `SearchFilter` (and other consumers) set the `data-1p-ignore` attribute (or an equivalent approach) on the DOM input; otherwise 1Password may still behave undesirably.<br> - [ ] <a href='https://github.com/apache/superset/pull/36209/files#diff-a2e7448595725f7b2454dca3d3bdd40e415fd58d3b77e484c8d46eec348e6992R269-R269'><strong>Behavior verification</strong></a><br>Ensure the SearchFilter/ListView pipeline actually uses `inputName` to set the final input `name` attribute (and/or `data-1p-ignore`) on the rendered <input>. If SearchFilter still favors `id` or ignores `inputName`, the change will have no effect.<br> - [ ] <a href='https://github.com/apache/superset/pull/36209/files#diff-c41901af1854fbb16ba92eee449c031cb9c077a2f6eef86f89431eb73a54bc1aR294-R303'><strong>Propagation check</strong></a><br>Verify that the new `inputName` field is actually consumed downstream (Filters/index.tsx -> SearchFilter) so that the input element receives the custom `name` attribute and the `data-1p-ignore` attribute required to suppress password-manager UI. If `inputName` is added here but not used by the filter rendering component, the change will have no effect.<br> - [ ] <a href='https://github.com/apache/superset/pull/36209/files#diff-92b545c602f29d88bcc67f27d7efd7cdda32819e37f7d5f288e89ca2bb3d1fcbR303-R303'><strong>Attribute propagation risk</strong></a><br>Adding `inputName` assumes the downstream ListView/SearchFilter implementation consumes and applies it to the rendered input element. If the component that renders the search input doesn't read this property, this change will have no effect. Verify the prop is passed through and applied as the `name` attribute on the actual <input>.<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]
