You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2026/05/20 13:13:42 UTC
[PR] fix(oauth2): make OAuth2 completion idempotent and listener resilient [superset]
betodealmeida opened a new pull request, #40292:
URL: https://github.com/apache/superset/pull/40292
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
The OAuth2 callback page always emitted both `BroadcastChannel` and `localStorage` signals when `BroadcastChannel` was supported, so a single completion re-triggered `reRunQuery` / `triggerQuery` / `onRefresh` twice in the original tab. This PR makes the sender's storage path a true fallback (only when `BroadcastChannel` is unavailable or its construction throws), and guard the receiver against duplicate dispatches as a safety net.
Additionally, it wraps channel/storage operations in `try/catch` so restricted contexts can't abort the flow, and stabilize the listener's effect by reading volatile selector values (`chartList`, `query`, etc.) from a `ref` so `chartList` changes don't churn the channel and open a race window.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-4498878145
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40292?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 55.34%. Comparing base ([`6e8b3bf`](https://app.codecov.io/gh/apache/superset/commit/6e8b3bf9761eea0c7ac7a75d32f8fc6145b53ffd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3331146`](https://app.codecov.io/gh/apache/superset/commit/3331146643fc59aaaa1dd37a60cf83d195f009b3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (6e8b3bf) and HEAD (3331146). Click for more details.
>
> <details><summary>HEAD has 56 uploads less than BASE</summary>
>
>| Flag | BASE (6e8b3bf) | HEAD (3331146) |
>|------|------|------|
>|hive|5|1|
>|python|50|2|
>|presto|5|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40292 +/- ##
==========================================
- Coverage 64.13% 55.34% -8.80%
==========================================
Files 2591 2591
Lines 138367 138354 -13
Branches 32094 32087 -7
==========================================
- Hits 88747 76570 -12177
- Misses 48090 61120 +13030
+ Partials 1530 664 -866
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40292/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40292/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.41% <ø> (ø)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40292/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40292/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40292/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.10% <ø> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/40292/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.17% <ø> (-19.43%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40292/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40292/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/40292?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-4499216366
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4129010739
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -106,21 +106,60 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
const lastHandledTabIdRef = useRef<string>();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ };
Review Comment:
latestStateRef.current gets reassigned on every render (lines 120-127, outside the effect), so handleOAuthComplete always reads whatever's current when it fires, not what was captured at mount. That's the whole point of the ref instead of a dep array. Don't think this one's real.
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -106,21 +106,60 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
const lastHandledTabIdRef = useRef<string>();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ };
+
useEffect(() => {
+ // Guard against duplicate dispatches if both the BroadcastChannel and the
+ // storage fallback ever deliver the same completion.
+ let handled = false;
const handleOAuthComplete = (tabId?: string) => {
- if (tabId !== extra.tab_id || tabId === lastHandledTabIdRef.current) {
+ if (
+ tabId !== extra.tab_id ||
+ tabId === lastHandledTabIdRef.current ||
+ handled
+ ) {
return;
}
-
- if (errorMitigationFunction) {
- errorMitigationFunction();
- } else if (source === 'sqllab' && query) {
- dispatch(reRunQuery(query));
- } else if (source === 'explore') {
- dispatch(triggerQuery(true, chartId));
- } else if (source === 'dashboard') {
- dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId));
- } else if (source === 'crud') {
+ const {
+ source: src,
+ query: q,
+ chartId: cId,
+ chartList: cList,
+ dashboardId: dId,
+ errorMitigationFunction: mitigate,
+ } = latestStateRef.current;
+ // `handled`/`lastHandledTabIdRef` are only set below, after this whole
+ // chain, and only reached when a branch actually dispatches — so a
+ // signal that arrives before state is ready (e.g. `query` still null
+ // in SQL Lab) falls through to the catch-all `return` and a later
+ // fallback signal can still succeed.
+ if (mitigate) {
+ mitigate();
+ } else if (src === 'sqllab' && q) {
+ dispatch(reRunQuery(q));
+ } else if (src === 'explore') {
+ dispatch(triggerQuery(true, cId));
+ } else if (src === 'dashboard') {
+ dispatch(onRefresh(cList.map(Number), true, 0, dId));
Review Comment:
dashboardId can't actually be undefined here in practice. refreshChart unconditionally reads dashboardInfo.common.conf before it does anything, so nothing in this chain can fire before dashboardInfo's loaded. Think this one's a false positive too.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-4514764334
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4151870499
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -99,29 +99,61 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ };
+
useEffect(() => {
+ // Guard against duplicate dispatches if both the BroadcastChannel and the
+ // storage fallback ever deliver the same completion.
+ let handled = false;
const handleOAuthComplete = (tabId?: string) => {
- if (tabId !== extra.tab_id) {
+ if (tabId !== extra.tab_id || handled) {
return;
}
- if (source === 'sqllab' && query) {
- dispatch(reRunQuery(query));
- } else if (source === 'explore' && chartId) {
- dispatch(triggerQuery(true, chartId));
- } else if (source === 'dashboard') {
- dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId));
+ handled = true;
+ const {
+ source: src,
+ query: q,
+ chartId: cId,
+ chartList: cList,
+ dashboardId: dId,
+ } = latestStateRef.current;
+ if (src === 'sqllab' && q) {
+ dispatch(reRunQuery(q));
+ } else if (src === 'explore' && cId) {
+ dispatch(triggerQuery(true, cId));
+ } else if (src === 'dashboard') {
+ dispatch(onRefresh(cList.map(Number), true, 0, dId));
Review Comment:
Confirmed, `handled`/`lastHandledTabIdRef` are only set after a branch actually dispatches now, so a signal that lands before state's ready falls through and a later one can still land. This one's done.
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx:
##########
@@ -180,4 +181,78 @@ describe('OAuth2RedirectMessage Component', () => {
expect(reRunQuery).not.toHaveBeenCalled();
});
+
+ test('dispatches only once when both BroadcastChannel and storage signals arrive', async () => {
+ render(setup());
+
+ simulateBroadcastMessage({ tabId: 'tabId' });
+ simulateStorageMessage({ tabId: 'tabId' });
+
+ await waitFor(() => {
+ expect(reRunQuery).toHaveBeenCalledTimes(1);
+ });
+ });
+
+ test('falls back to storage events when BroadcastChannel construction throws', async () => {
+ (global as any).BroadcastChannel = jest.fn().mockImplementation(() => {
Review Comment:
Fixed in d038eb3, matches the `global as unknown as {...}` pattern already used in RightMenu.test.tsx and chartActions.test.ts.
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx:
##########
@@ -180,4 +181,78 @@ describe('OAuth2RedirectMessage Component', () => {
expect(reRunQuery).not.toHaveBeenCalled();
});
+
+ test('dispatches only once when both BroadcastChannel and storage signals arrive', async () => {
+ render(setup());
+
+ simulateBroadcastMessage({ tabId: 'tabId' });
+ simulateStorageMessage({ tabId: 'tabId' });
+
+ await waitFor(() => {
+ expect(reRunQuery).toHaveBeenCalledTimes(1);
+ });
+ });
+
+ test('falls back to storage events when BroadcastChannel construction throws', async () => {
+ (global as any).BroadcastChannel = jest.fn().mockImplementation(() => {
+ throw new Error('blocked');
+ });
+
+ render(setup());
+
+ simulateStorageMessage({ tabId: 'tabId' });
+
+ await waitFor(() => {
+ expect(reRunQuery).toHaveBeenCalledWith({ sql: 'SELECT * FROM table' });
+ });
+ });
+
+ test('re-processes a later signal when the first arrived before state was ready', async () => {
+ const initialState = {
+ sqlLab: {
+ queries: {},
+ queryEditors: [{ id: 'editor-id' }],
+ tabHistory: ['editor-id'],
+ },
+ explore: { slice: { slice_id: 123 } },
+ charts: { '1': {}, '2': {} },
+ dashboardInfo: { id: 'dashboard-id' },
+ };
+ const dynamicStore = createStore(
+ (state: any = initialState, action: any) => {
Review Comment:
Also fixed in d038eb3, typed as `DynamicStoreState`/`DynamicStoreAction` instead.
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -106,21 +106,60 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
const lastHandledTabIdRef = useRef<string>();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ };
Review Comment:
Bito's reply doesn't actually engage with the point, it just restates the original concern. Re-read the file to make sure: `latestStateRef.current` is reassigned in the component body on every render, not inside the effect, so there's nothing to re-subscribe to, `handleOAuthComplete` always reads whatever the latest render produced when it fires. Closing this as a false positive.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-6307593147
## CodeAnt PR Risk: Low Risk
- The PR appears safe to merge; the completion handler preserves later signals when state is not ready, and the tests cover deduplication and storage fallback.
**Assessed commit:** `cb98e497d8f4`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-5944818702
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r3536654808
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx:
##########
@@ -180,4 +181,78 @@ describe('OAuth2RedirectMessage Component', () => {
expect(reRunQuery).not.toHaveBeenCalled();
});
+
+ test('dispatches only once when both BroadcastChannel and storage signals arrive', async () => {
+ render(setup());
+
+ simulateBroadcastMessage({ tabId: 'tabId' });
+ simulateStorageMessage({ tabId: 'tabId' });
+
+ await waitFor(() => {
+ expect(reRunQuery).toHaveBeenCalledTimes(1);
+ });
+ });
+
+ test('falls back to storage events when BroadcastChannel construction throws', async () => {
+ (global as any).BroadcastChannel = jest.fn().mockImplementation(() => {
Review Comment:
**Suggestion:** Replace the `any` cast on `global` with a typed interface for `BroadcastChannel` on `globalThis` so the assignment remains type-safe. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The file introduces a direct `any` cast on `global`, which violates the no-any-types rule. A more specific global typing should be used instead.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4ec6d1fc543949688badd6ec787c2a98&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4ec6d1fc543949688badd6ec787c2a98&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx
**Line:** 197:197
**Comment:**
*Custom Rule: Replace the `any` cast on `global` with a typed interface for `BroadcastChannel` on `globalThis` so the assignment remains type-safe.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40292&comment_hash=b91250814894baf3440b9d957d17b2b7c38ab5fe34a4cc3498b8a03b6e93791b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40292&comment_hash=b91250814894baf3440b9d957d17b2b7c38ab5fe34a4cc3498b8a03b6e93791b&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx:
##########
@@ -180,4 +181,78 @@ describe('OAuth2RedirectMessage Component', () => {
expect(reRunQuery).not.toHaveBeenCalled();
});
+
+ test('dispatches only once when both BroadcastChannel and storage signals arrive', async () => {
+ render(setup());
+
+ simulateBroadcastMessage({ tabId: 'tabId' });
+ simulateStorageMessage({ tabId: 'tabId' });
+
+ await waitFor(() => {
+ expect(reRunQuery).toHaveBeenCalledTimes(1);
+ });
+ });
+
+ test('falls back to storage events when BroadcastChannel construction throws', async () => {
+ (global as any).BroadcastChannel = jest.fn().mockImplementation(() => {
+ throw new Error('blocked');
+ });
+
+ render(setup());
+
+ simulateStorageMessage({ tabId: 'tabId' });
+
+ await waitFor(() => {
+ expect(reRunQuery).toHaveBeenCalledWith({ sql: 'SELECT * FROM table' });
+ });
+ });
+
+ test('re-processes a later signal when the first arrived before state was ready', async () => {
+ const initialState = {
+ sqlLab: {
+ queries: {},
+ queryEditors: [{ id: 'editor-id' }],
+ tabHistory: ['editor-id'],
+ },
+ explore: { slice: { slice_id: 123 } },
+ charts: { '1': {}, '2': {} },
+ dashboardInfo: { id: 'dashboard-id' },
+ };
+ const dynamicStore = createStore(
+ (state: any = initialState, action: any) => {
Review Comment:
**Suggestion:** Type the reducer parameters explicitly instead of using `any`, for example by defining a concrete state shape and action union for this test store. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The reducer callback explicitly types both `state` and `action` as `any`, which is a direct violation of the no-any-types rule in new TypeScript code.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d765ea029a064128a1ebe2e01dc0f03f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d765ea029a064128a1ebe2e01dc0f03f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx
**Line:** 222:222
**Comment:**
*Custom Rule: Type the reducer parameters explicitly instead of using `any`, for example by defining a concrete state shape and action union for this test store.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40292&comment_hash=9af9517b701e56b0d81f5d222b943b53b6fb39e51f598bdf2a8988709bb6ab49&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40292&comment_hash=9af9517b701e56b0d81f5d222b943b53b6fb39e51f598bdf2a8988709bb6ab49&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): make OAuth2 completion idempotent and listener resilient [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-4498753774
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4333114619
##########
superset-frontend/src/components/ErrorMessage/oauth2CallbackScript.test.ts:
##########
@@ -0,0 +1,113 @@
+/**
+ * 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 fs from 'fs';
+import path from 'path';
+
+// Runs the actual inline script from the OAuth2 popup callback template
+// (not a reimplementation of it), so a future edit to the template can't
+// silently drift from this regression coverage.
+const TEMPLATE_PATH = path.join(
Review Comment:
This test reads `superset/templates/superset/oauth2.html`, but `scripts/change_detector.py` only maps `^superset-frontend/` (and a few workflow files) to the `frontend` flag that gates Jest. A PR that touches only the template, for example restoring an unconditional `window.close()`, would run the Python jobs but skip this test, and the regression would surface on a later unrelated frontend PR. The Dockerfile `COPY` makes the file available but doesn't schedule the test. Should the template path be added to the `frontend` patterns so edits to it run these tests?
##########
superset-frontend/src/components/ErrorMessage/oauth2CallbackScript.test.ts:
##########
@@ -0,0 +1,113 @@
+/**
+ * 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 fs from 'fs';
+import path from 'path';
+
+// Runs the actual inline script from the OAuth2 popup callback template
+// (not a reimplementation of it), so a future edit to the template can't
+// silently drift from this regression coverage.
+const TEMPLATE_PATH = path.join(
+ __dirname,
+ '../../../../superset/templates/superset/oauth2.html',
+);
+
+function extractCallbackScript(): string {
+ const html = fs.readFileSync(TEMPLATE_PATH, 'utf-8');
+ const match = html.match(/<script[^>]*>([\s\S]*?)<\/script>/i);
+ if (!match) {
+ throw new Error('Could not find the callback <script> in oauth2.html');
+ }
+ return match[1].replace('{{ tab_id }}', 'test-tab-id');
+}
+
+function runCallbackScript({
+ broadcastChannel,
+ setItem,
+}: {
+ broadcastChannel?: () => BroadcastChannel;
+ setItem?: () => void;
+}) {
+ const script = extractCallbackScript();
+ const closeMock = jest.fn();
+ const globalWithBroadcastChannel = global as unknown as {
+ BroadcastChannel?: () => BroadcastChannel;
+ };
+ const originalClose = window.close;
+ const originalBroadcastChannel = globalWithBroadcastChannel.BroadcastChannel;
+ const originalSetItem = Storage.prototype.setItem;
+ const originalRemoveItem = Storage.prototype.removeItem;
+
+ window.close = closeMock;
+ if (broadcastChannel) {
+ globalWithBroadcastChannel.BroadcastChannel = broadcastChannel;
+ } else {
+ delete globalWithBroadcastChannel.BroadcastChannel;
+ }
+ if (setItem) {
+ Storage.prototype.setItem = setItem;
+ }
+
+ try {
+ // eslint-disable-next-line no-new-func
+ new Function(script)();
+ } finally {
+ window.close = originalClose;
+ globalWithBroadcastChannel.BroadcastChannel = originalBroadcastChannel;
+ Storage.prototype.setItem = originalSetItem;
+ Storage.prototype.removeItem = originalRemoveItem;
+ }
+
+ return closeMock;
+}
+
+test('closes the popup once the storage signal is sent', () => {
+ const closeMock = runCallbackScript({});
+
+ expect(closeMock).toHaveBeenCalledTimes(1);
+});
+
+test('closes the popup once the BroadcastChannel signal is sent', () => {
Review Comment:
None of the three cases has both transports available, so the main contract of the template (post to BroadcastChannel and also write the storage signal, then close once) isn't asserted. If the storage write were skipped whenever BroadcastChannel succeeds, every test here would still pass, and the original tab would lose its fallback whenever its channel listener misses the broadcast. There's also no case where `postMessage` throws while storage works, so a change that returns from that `catch` would pass too. Could you add a case with both mocks working that asserts `postMessage` and `setItem` get the expected tab ID/key and `window.close` is called once, and one where `postMessage` throws but `setItem` still runs?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-4512020325
Huh, I fixed and pushed, but my commit is not showing up here.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-5924799255
@sadpandajoe mind giving this a look when you get a chance? All the open review threads are cleaned up now, the last couple were CodeAnt `any`-type nits in the test file, fixed in d038eb3. CI's re-running off that push but it was green before.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4086849718
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -99,29 +99,61 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ };
+
useEffect(() => {
+ // Guard against duplicate dispatches if both the BroadcastChannel and the
+ // storage fallback ever deliver the same completion.
+ let handled = false;
const handleOAuthComplete = (tabId?: string) => {
- if (tabId !== extra.tab_id) {
+ if (tabId !== extra.tab_id || handled) {
return;
}
- if (source === 'sqllab' && query) {
- dispatch(reRunQuery(query));
- } else if (source === 'explore' && chartId) {
- dispatch(triggerQuery(true, chartId));
- } else if (source === 'dashboard') {
- dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId));
+ handled = true;
+ const {
+ source: src,
+ query: q,
+ chartId: cId,
+ chartList: cList,
+ dashboardId: dId,
+ } = latestStateRef.current;
+ if (src === 'sqllab' && q) {
+ dispatch(reRunQuery(q));
+ } else if (src === 'explore' && cId) {
+ dispatch(triggerQuery(true, cId));
+ } else if (src === 'dashboard') {
+ dispatch(onRefresh(cList.map(Number), true, 0, dId));
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `acaec3d`.
The `handled` and `lastHandledTabIdRef` flags are now set only after a dispatch or mitigation branch runs. When SQL Lab state is unavailable, the handler falls through without marking the signal handled, allowing a later fallback signal to succeed.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-5926938473
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-4519596877
Richard's agent here: I agree with the review bot’s underlying edge case, but I’d rate the severity lower than Major for the normal flow. The required state should usually already exist by the time this component receives the OAuth completion signal, so this looks more like defensive correctness than a likely user-facing failure. That said, the fix is small and makes the dedupe semantics more precise: set `handled = true` only after one of the dispatch branches actually runs, so a later fallback signal is not dropped if the first matching event arrives before `query`/`chartId` is ready.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-4905852454
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-5804517284
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-5782723865
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r3289169742
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -99,29 +99,61 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ };
+
useEffect(() => {
+ // Guard against duplicate dispatches if both the BroadcastChannel and the
+ // storage fallback ever deliver the same completion.
+ let handled = false;
const handleOAuthComplete = (tabId?: string) => {
- if (tabId !== extra.tab_id) {
+ if (tabId !== extra.tab_id || handled) {
return;
}
- if (source === 'sqllab' && query) {
- dispatch(reRunQuery(query));
- } else if (source === 'explore' && chartId) {
- dispatch(triggerQuery(true, chartId));
- } else if (source === 'dashboard') {
- dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId));
+ handled = true;
+ const {
+ source: src,
+ query: q,
+ chartId: cId,
+ chartList: cList,
+ dashboardId: dId,
+ } = latestStateRef.current;
+ if (src === 'sqllab' && q) {
+ dispatch(reRunQuery(q));
+ } else if (src === 'explore' && cId) {
+ dispatch(triggerQuery(true, cId));
+ } else if (src === 'dashboard') {
+ dispatch(onRefresh(cList.map(Number), true, 0, dId));
Review Comment:
Is this relevant?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4151907526
##########
superset/templates/superset/oauth2.html:
##########
@@ -25,13 +25,23 @@
<body>
<script nonce="{{ macros.get_nonce() }}">
const message = { tabId: '{{ tab_id }}' };
+ // Emit on both channels: postMessage success doesn't guarantee the original
+ // tab's listener was attached. The receiver dedupes via a `handled` flag.
if (typeof BroadcastChannel !== 'undefined') {
- const channel = new BroadcastChannel('oauth');
- channel.postMessage(message);
- channel.close();
+ try {
+ const channel = new BroadcastChannel('oauth');
+ channel.postMessage(message);
+ channel.close();
+ } catch (e) {
+ // ignore; storage path below still fires
+ }
+ }
+ try {
+ localStorage.setItem('oauth2_auth_complete', JSON.stringify(message));
+ localStorage.removeItem('oauth2_auth_complete');
+ } catch (e) {
Review Comment:
If BroadcastChannel and localStorage are both blocked, neither completion signal is sent, but the popup now closes and hides the instruction to re-run the query manually. Could we leave it open when both sends fail, and cover that path in a callback-script regression test?
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx:
##########
@@ -304,4 +312,82 @@ describe('OAuth2RedirectMessage Component', () => {
});
expect(api.util.invalidateTags).not.toHaveBeenCalled();
});
+
+ test('dispatches only once when both BroadcastChannel and storage signals arrive', async () => {
+ render(setup());
+
+ simulateBroadcastMessage({ tabId: 'tabId' });
+ simulateStorageMessage({ tabId: 'tabId' });
+
+ await waitFor(() => {
+ expect(reRunQuery).toHaveBeenCalledTimes(1);
+ });
+ });
+
+ test('falls back to storage events when BroadcastChannel construction throws', async () => {
+ globalWithBroadcastChannel.BroadcastChannel = jest
+ .fn()
+ .mockImplementation(() => {
+ throw new Error('blocked');
+ });
+
+ render(setup());
+
+ simulateStorageMessage({ tabId: 'tabId' });
+
+ await waitFor(() => {
+ expect(reRunQuery).toHaveBeenCalledWith({ sql: 'SELECT * FROM table' });
+ });
+ });
+
+ test('re-processes a later signal when the first arrived before state was ready', async () => {
+ const initialState = {
+ sqlLab: {
+ queries: {},
+ queryEditors: [{ id: 'editor-id' }],
+ tabHistory: ['editor-id'],
+ },
+ explore: { slice: { slice_id: 123 } },
+ charts: { '1': {}, '2': {} },
+ dashboardInfo: { id: 'dashboard-id' },
+ };
+ type DynamicStoreState = typeof initialState;
+ type DynamicStoreAction = { type: string };
+ const dynamicStore = createStore(
+ (state: DynamicStoreState = initialState, action: DynamicStoreAction) => {
+ if (action.type === 'SET_READY') {
+ return {
+ ...state,
+ sqlLab: {
+ ...state.sqlLab,
+ queries: { 'query-id': { sql: 'SELECT * FROM table' } },
+ queryEditors: [{ id: 'editor-id', latestQueryId: 'query-id' }],
+ },
+ };
+ }
+ return state;
+ },
+ );
+
+ render(
+ <Provider store={dynamicStore}>
+ <OAuth2RedirectMessage {...defaultProps} />
+ </Provider>,
+ );
+
+ // First signal arrives before the SQL Lab query state is populated;
+ // nothing dispatches and `handled` must NOT be flipped.
+ simulateBroadcastMessage({ tabId: 'tabId' });
+ expect(reRunQuery).not.toHaveBeenCalled();
+
+ // Query state becomes available, then the storage fallback signal fires.
+ act(() => {
+ dynamicStore.dispatch({ type: 'SET_READY' });
Review Comment:
This still passes if the old effect dependencies are restored: the channel can be closed and recreated on the Redux update, and the later storage event still succeeds. Could we also assert that an unrelated store update neither closes nor recreates the channel, and that the original listener handles the completion?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4151871630
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -106,21 +106,60 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
const lastHandledTabIdRef = useRef<string>();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ };
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding stale state is addressed by the fact that `latestStateRef.current` is updated on every render, ensuring that `handleOAuthComplete` always accesses the most recent values. Since the ref is updated outside the effect, the effect does not need to re-subscribe to maintain access to the latest state. This pattern is a standard approach for avoiding unnecessary effect re-runs while maintaining access to current component state.
**superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx**
```
latestStateRef.current = {
source,
query,
chartId,
chartList,
dashboardId,
errorMitigationFunction,
};
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4074011786
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -99,29 +99,61 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ };
+
useEffect(() => {
+ // Guard against duplicate dispatches if both the BroadcastChannel and the
+ // storage fallback ever deliver the same completion.
+ let handled = false;
const handleOAuthComplete = (tabId?: string) => {
- if (tabId !== extra.tab_id) {
+ if (tabId !== extra.tab_id || handled) {
return;
}
- if (source === 'sqllab' && query) {
- dispatch(reRunQuery(query));
- } else if (source === 'explore' && chartId) {
- dispatch(triggerQuery(true, chartId));
- } else if (source === 'dashboard') {
- dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId));
+ handled = true;
+ const {
+ source: src,
+ query: q,
+ chartId: cId,
+ chartList: cList,
+ dashboardId: dId,
+ } = latestStateRef.current;
+ if (src === 'sqllab' && q) {
+ dispatch(reRunQuery(q));
+ } else if (src === 'explore' && cId) {
+ dispatch(triggerQuery(true, cId));
+ } else if (src === 'dashboard') {
+ dispatch(onRefresh(cList.map(Number), true, 0, dId));
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `3167529`.
The `handled` and `lastHandledTabIdRef` flags are now set only after a dispatch or mitigation branch runs. When SQL Lab state is unavailable, the handler falls through without marking the signal handled, allowing a later fallback signal to succeed.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4129012194
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -106,21 +106,60 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
const lastHandledTabIdRef = useRef<string>();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ };
+
useEffect(() => {
+ // Guard against duplicate dispatches if both the BroadcastChannel and the
+ // storage fallback ever deliver the same completion.
+ let handled = false;
const handleOAuthComplete = (tabId?: string) => {
- if (tabId !== extra.tab_id || tabId === lastHandledTabIdRef.current) {
+ if (
+ tabId !== extra.tab_id ||
+ tabId === lastHandledTabIdRef.current ||
+ handled
+ ) {
return;
}
-
- if (errorMitigationFunction) {
- errorMitigationFunction();
- } else if (source === 'sqllab' && query) {
- dispatch(reRunQuery(query));
- } else if (source === 'explore') {
- dispatch(triggerQuery(true, chartId));
- } else if (source === 'dashboard') {
- dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId));
- } else if (source === 'crud') {
+ const {
+ source: src,
+ query: q,
+ chartId: cId,
+ chartList: cList,
+ dashboardId: dId,
+ errorMitigationFunction: mitigate,
+ } = latestStateRef.current;
+ // `handled`/`lastHandledTabIdRef` are only set below, after this whole
+ // chain, and only reached when a branch actually dispatches — so a
+ // signal that arrives before state is ready (e.g. `query` still null
+ // in SQL Lab) falls through to the catch-all `return` and a later
+ // fallback signal can still succeed.
+ if (mitigate) {
+ mitigate();
+ } else if (src === 'sqllab' && q) {
+ dispatch(reRunQuery(q));
+ } else if (src === 'explore') {
+ dispatch(triggerQuery(true, cId));
+ } else if (src === 'dashboard') {
+ dispatch(onRefresh(cList.map(Number), true, 0, dId));
Review Comment:
<!-- Bito Reply -->
The observation is correct. Since `refreshChart` is only invoked after `dashboardInfo` is loaded, `dashboardId` will be defined in the `dashboard` source branch. You can safely ignore this suggestion as a false positive.
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -106,21 +106,60 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
const lastHandledTabIdRef = useRef<string>();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ };
Review Comment:
<!-- Bito Reply -->
The reviewer's concern is that the effect at line 212 depends on `[extra.tab_id, dispatch]` but uses values from `latestStateRef` (like `source`, `query`, `chartList`, `errorMitigationFunction`) that are not in the dependency array. Because the effect does not re-subscribe when these values change, it may continue to use stale values captured at the time of the initial subscription, even though `latestStateRef.current` is updated on every render. To ensure the effect uses the most recent state, you should either include these dependencies in the effect's dependency array or use a pattern that ensures the effect always accesses the current values.
**superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx**
```
// `chartList` is rebuilt from `Object.keys` on every store update; keeping
// the listener state in a ref avoids tearing down/recreating the
// BroadcastChannel on each render and the race window that comes with it.
const latestStateRef = useRef({
source,
query,
chartId,
chartList,
dashboardId,
errorMitigationFunction,
});
latestStateRef.current = {
source,
query,
chartId,
chartList,
dashboardId,
errorMitigationFunction,
};
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-4513139881
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | eb66ee8df9685e9d41d717a40d7dafa671581071 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a0f80a325290e00079861f8 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40292--superset-docs-preview.netlify.app](https://deploy-preview-40292--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AstMyK3TzE0tLMnTT8nNy8stLC4CyKakFOfmVnilAebNEgzQLg0RjI1MjS4NUAwMDc0sLM8M0C6VaAA) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40292--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r3289181368
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -99,29 +99,61 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ };
+
useEffect(() => {
+ // Guard against duplicate dispatches if both the BroadcastChannel and the
+ // storage fallback ever deliver the same completion.
+ let handled = false;
const handleOAuthComplete = (tabId?: string) => {
- if (tabId !== extra.tab_id) {
+ if (tabId !== extra.tab_id || handled) {
return;
}
- if (source === 'sqllab' && query) {
- dispatch(reRunQuery(query));
- } else if (source === 'explore' && chartId) {
- dispatch(triggerQuery(true, chartId));
- } else if (source === 'dashboard') {
- dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId));
+ handled = true;
+ const {
+ source: src,
+ query: q,
+ chartId: cId,
+ chartList: cList,
+ dashboardId: dId,
+ } = latestStateRef.current;
+ if (src === 'sqllab' && q) {
+ dispatch(reRunQuery(q));
+ } else if (src === 'explore' && cId) {
+ dispatch(triggerQuery(true, cId));
+ } else if (src === 'dashboard') {
+ dispatch(onRefresh(cList.map(Number), true, 0, dId));
Review Comment:
Yes — this comment is **relevant**.
The current logic can mark the event as `handled` **before** confirming that any refresh/re-run actually happened. That means an early signal arriving before `query`, `chartId`, or dashboard state is ready could permanently block the later fallback signal.
### Recommended fix
Move `handled = true` so it is set **only after** a dispatch path succeeds.
For example:
```ts
const handleOAuthComplete = (tabId?: string) => {
if (tabId !== extra.tab_id || handled) {
return;
}
const {
source: src,
query: q,
chartId: cId,
chartList: cList,
dashboardId: dId,
} = latestStateRef.current;
if (src === 'sqllab' && q) {
handled = true;
dispatch(reRunQuery(q));
} else if (src === 'explore' && cId) {
handled = true;
dispatch(triggerQuery(true, cId));
} else if (src === 'dashboard') {
handled = true;
dispatch(onRefresh(cList.map(Number), true, 0, dId));
}
};
```
This preserves the deduplication behavior, but avoids dropping a later valid signal when the first one arrives too early.
If you want, I can also review the remaining comments on this PR and help fix those too.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4161665784
##########
superset/templates/superset/oauth2.html:
##########
@@ -25,13 +25,23 @@
<body>
<script nonce="{{ macros.get_nonce() }}">
const message = { tabId: '{{ tab_id }}' };
+ // Emit on both channels: postMessage success doesn't guarantee the original
+ // tab's listener was attached. The receiver dedupes via a `handled` flag.
if (typeof BroadcastChannel !== 'undefined') {
- const channel = new BroadcastChannel('oauth');
- channel.postMessage(message);
- channel.close();
+ try {
+ const channel = new BroadcastChannel('oauth');
+ channel.postMessage(message);
+ channel.close();
+ } catch (e) {
+ // ignore; storage path below still fires
+ }
+ }
+ try {
+ localStorage.setItem('oauth2_auth_complete', JSON.stringify(message));
+ localStorage.removeItem('oauth2_auth_complete');
+ } catch (e) {
Review Comment:
Good catch, fixed in b0cc385: `window.close()` now only runs once at least one of the two sends actually succeeded, otherwise the window stays open with the re-run instructions visible. Added `oauth2CallbackScript.test.ts` covering it, it runs the actual inline script from the template rather than a copy of it.
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx:
##########
@@ -304,4 +312,82 @@ describe('OAuth2RedirectMessage Component', () => {
});
expect(api.util.invalidateTags).not.toHaveBeenCalled();
});
+
+ test('dispatches only once when both BroadcastChannel and storage signals arrive', async () => {
+ render(setup());
+
+ simulateBroadcastMessage({ tabId: 'tabId' });
+ simulateStorageMessage({ tabId: 'tabId' });
+
+ await waitFor(() => {
+ expect(reRunQuery).toHaveBeenCalledTimes(1);
+ });
+ });
+
+ test('falls back to storage events when BroadcastChannel construction throws', async () => {
+ globalWithBroadcastChannel.BroadcastChannel = jest
+ .fn()
+ .mockImplementation(() => {
+ throw new Error('blocked');
+ });
+
+ render(setup());
+
+ simulateStorageMessage({ tabId: 'tabId' });
+
+ await waitFor(() => {
+ expect(reRunQuery).toHaveBeenCalledWith({ sql: 'SELECT * FROM table' });
+ });
+ });
+
+ test('re-processes a later signal when the first arrived before state was ready', async () => {
+ const initialState = {
+ sqlLab: {
+ queries: {},
+ queryEditors: [{ id: 'editor-id' }],
+ tabHistory: ['editor-id'],
+ },
+ explore: { slice: { slice_id: 123 } },
+ charts: { '1': {}, '2': {} },
+ dashboardInfo: { id: 'dashboard-id' },
+ };
+ type DynamicStoreState = typeof initialState;
+ type DynamicStoreAction = { type: string };
+ const dynamicStore = createStore(
+ (state: DynamicStoreState = initialState, action: DynamicStoreAction) => {
+ if (action.type === 'SET_READY') {
+ return {
+ ...state,
+ sqlLab: {
+ ...state.sqlLab,
+ queries: { 'query-id': { sql: 'SELECT * FROM table' } },
+ queryEditors: [{ id: 'editor-id', latestQueryId: 'query-id' }],
+ },
+ };
+ }
+ return state;
+ },
+ );
+
+ render(
+ <Provider store={dynamicStore}>
+ <OAuth2RedirectMessage {...defaultProps} />
+ </Provider>,
+ );
+
+ // First signal arrives before the SQL Lab query state is populated;
+ // nothing dispatches and `handled` must NOT be flipped.
+ simulateBroadcastMessage({ tabId: 'tabId' });
+ expect(reRunQuery).not.toHaveBeenCalled();
+
+ // Query state becomes available, then the storage fallback signal fires.
+ act(() => {
+ dynamicStore.dispatch({ type: 'SET_READY' });
Review Comment:
Fair, fixed in b0cc385: the test now asserts the BroadcastChannel mock and the storage listener spy both stay at a call count of 1 across the Redux dispatch, so a regression back to the old state-dependent deps would actually fail this test instead of sliding through on the final outcome alone.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4161685789
##########
superset-frontend/src/components/ErrorMessage/oauth2CallbackScript.test.ts:
##########
@@ -0,0 +1,113 @@
+/**
+ * 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 fs from 'fs';
+import path from 'path';
+
+// Runs the actual inline script from the OAuth2 popup callback template
+// (not a reimplementation of it), so a future edit to the template can't
+// silently drift from this regression coverage.
+const TEMPLATE_PATH = path.join(
+ __dirname,
+ '../../../../superset/templates/superset/oauth2.html',
+);
+
+function extractCallbackScript(): string {
+ const html = fs.readFileSync(TEMPLATE_PATH, 'utf-8');
+ const match = html.match(/<script[^>]*>([\s\S]*?)<\/script>/);
Review Comment:
## CodeQL / Bad HTML filtering regexp
This regular expression does not match upper case <SCRIPT> tags.
[Show more details](https://github.com/apache/superset/security/code-scanning/3916)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-6307583985
@sadpandajoe pushed a fix for exactly what you flagged: `superset-node-ci` now copies `superset/templates/superset/oauth2.html` into the image, so shard 3 should stop hitting that `ENOENT`.
Also fixed CodeQL's catch while I was in there, the extraction regex was case-sensitive so it'd miss an upper-cased `<SCRIPT>` tag. Added the `i` flag.
Should be green across the board now.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4332941535
##########
superset-frontend/src/components/ErrorMessage/oauth2CallbackScript.test.ts:
##########
@@ -0,0 +1,113 @@
+/**
+ * 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 fs from 'fs';
+import path from 'path';
+
+// Runs the actual inline script from the OAuth2 popup callback template
+// (not a reimplementation of it), so a future edit to the template can't
+// silently drift from this regression coverage.
+const TEMPLATE_PATH = path.join(
+ __dirname,
+ '../../../../superset/templates/superset/oauth2.html',
+);
+
+function extractCallbackScript(): string {
+ const html = fs.readFileSync(TEMPLATE_PATH, 'utf-8');
+ const match = html.match(/<script[^>]*>([\s\S]*?)<\/script>/i);
Review Comment:
## CodeQL / Bad HTML filtering regexp
This regular expression does not match script end tags like </script >.
[Show more details](https://github.com/apache/superset/security/code-scanning/3957)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r3274586086
##########
superset/templates/superset/oauth2.html:
##########
@@ -25,13 +25,25 @@
<body>
<script nonce="{{ macros.get_nonce() }}">
const message = { tabId: '{{ tab_id }}' };
+ let broadcasted = false;
if (typeof BroadcastChannel !== 'undefined') {
- const channel = new BroadcastChannel('oauth');
- channel.postMessage(message);
- channel.close();
+ try {
+ const channel = new BroadcastChannel('oauth');
+ channel.postMessage(message);
+ channel.close();
+ broadcasted = true;
+ } catch (e) {
+ // fall through to the localStorage fallback below
+ }
+ }
+ if (!broadcasted) {
Review Comment:
Richard's agent here: the receiver-side `handled` guard and the `BroadcastChannel` construction `try/catch` both make sense to me.
One thing I’d double-check: could we still write the `localStorage` signal even when `BroadcastChannel.postMessage()` succeeds? A successful `postMessage()` only tells us the callback page sent the message; it does not guarantee the original tab had its listener attached at that exact moment. The storage event gives us a second delivery path for that race.
Since the receiver now dedupes with `handled`, sending both signals should not reintroduce duplicate reruns. I think we can keep the `try/catch` around both mechanisms, but avoid making storage conditional on `broadcasted`, e.g.:
```js
try {
const channel = new BroadcastChannel('oauth');
channel.postMessage(message);
channel.close();
} catch (e) {}
try {
localStorage.setItem('oauth2_auth_complete', JSON.stringify(message));
localStorage.removeItem('oauth2_auth_complete');
} catch (e) {}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r3284630109
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -99,29 +99,61 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ };
+
useEffect(() => {
+ // Guard against duplicate dispatches if both the BroadcastChannel and the
+ // storage fallback ever deliver the same completion.
+ let handled = false;
const handleOAuthComplete = (tabId?: string) => {
- if (tabId !== extra.tab_id) {
+ if (tabId !== extra.tab_id || handled) {
return;
}
- if (source === 'sqllab' && query) {
- dispatch(reRunQuery(query));
- } else if (source === 'explore' && chartId) {
- dispatch(triggerQuery(true, chartId));
- } else if (source === 'dashboard') {
- dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId));
+ handled = true;
+ const {
+ source: src,
+ query: q,
+ chartId: cId,
+ chartList: cList,
+ dashboardId: dId,
+ } = latestStateRef.current;
+ if (src === 'sqllab' && q) {
+ dispatch(reRunQuery(q));
+ } else if (src === 'explore' && cId) {
+ dispatch(triggerQuery(true, cId));
+ } else if (src === 'dashboard') {
+ dispatch(onRefresh(cList.map(Number), true, 0, dId));
Review Comment:
**Suggestion:** The deduplication flag is set before confirming any action was actually dispatched. If the first matching signal arrives when the required state is not ready (for example `source === 'sqllab'` but `query` is still null), the handler marks the event as handled and drops later duplicate/fallback signals that could have succeeded, causing the OAuth completion to be missed. Set the handled flag only after a dispatch path runs successfully. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SQL Lab queries may not auto re-run after OAuth.
- ⚠️ Explore chart reload can be skipped in rare timing windows.
- ⚠️ Dashboard auto-refresh after OAuth may silently fail.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In an OAuth2-enabled database connection, a query path wrapped in `check_for_oauth2()`
at `superset/utils/oauth2.py:311-320` raises an `OAuth2RedirectError`
(`superset/exceptions.py:341-367`), which is surfaced to the frontend as an
`OAUTH2_REDIRECT` error rendered by `OAuth2RedirectMessage` at
`superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:64-98`.
2. When `OAuth2RedirectMessage` mounts with `source="sqllab"`, it derives `query` from
Redux via `queries`, `queryEditors`, and `tabHistory` selectors at
`OAuth2RedirectMessage.tsx:71-86`, and stores that state in `latestStateRef.current` at
`OAuth2RedirectMessage.tsx:105-118`; if for any reason `qe?.latestQueryId` is unset or
`queries[qe.latestQueryId]` is missing, `query` is `null`.
3. After the user completes OAuth2 in the popup, the backend callback
`DatabasesRestApi.oauth2_authorized` at `superset/databases/api.py:10-21` renders
`superset/templates/superset/oauth2.html`, whose inline script posts the `{ tabId }`
message on the `'oauth'` `BroadcastChannel` and emits a localStorage change at
`superset/templates/superset/oauth2.html:27-44`.
4. On the original tab, the `useEffect` in `OAuth2RedirectMessage` registers both a
`BroadcastChannel` listener and a `storage` listener (lines `145-172`) that call
`handleOAuthComplete` (lines `123-143`); on the first matching event `handleOAuthComplete`
sets `handled = true` at `128` before reading `latestStateRef.current` (lines `129-135`,
where `q` may still be `null`), so the `if (src === 'sqllab' && q)` branch at `136-137` is
skipped and no dispatch occurs, and because `handled` is already `true`, any later
duplicate/fallback signal is ignored by the early guard `if (tabId !== extra.tab_id ||
handled)` at `125-127`, causing the OAuth completion re-run to be permanently dropped.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=25b01e26bbee4aeebdc0f7397d68ddc3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=25b01e26bbee4aeebdc0f7397d68ddc3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx
**Line:** 128:141
**Comment:**
*Incorrect Condition Logic: The deduplication flag is set before confirming any action was actually dispatched. If the first matching signal arrives when the required state is not ready (for example `source === 'sqllab'` but `query` is still null), the handler marks the event as handled and drops later duplicate/fallback signals that could have succeeded, causing the OAuth completion to be missed. Set the handled flag only after a dispatch path runs successfully.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40292&comment_hash=56a2085dbae82feacc5dcce149222da72c6afefcb87e1b8a4e9998994c7d515d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40292&comment_hash=56a2085dbae82feacc5dcce149222da72c6afefcb87e1b8a4e9998994c7d515d&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-4513125458
Had to push an empty commit to make it work! :D
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40292:
URL: https://github.com/apache/superset/pull/40292#issuecomment-4999050884
Mind if I tackle the rebase here, @betodealmeida? Just trying to get some PRs across the finish line :) Thanks, as always, for the excellent work.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4088363108
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -106,21 +106,60 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
const lastHandledTabIdRef = useRef<string>();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ };
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Stale state in long-lived effect</b></div>
<div id="fix">
`latestStateRef` decouples `handleOAuthComplete` from renders, but the effect (deps `[extra.tab_id, dispatch]`, line 212) never re-subscribes when `source`/`query`/`chartList`/`errorMitigationFunction` change. A signal arriving after a state change is handled with stale values, and `handled` (set from that stale decision) blocks later signals for the effect's lifetime. Consider re-subscribing on state changes or versioning the ref snapshot.
</div>
</div>
<small><i>Code Review Run #ebcdd2</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/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -106,21 +106,60 @@ export function OAuth2RedirectMessage({
const dispatch = useDispatch();
const lastHandledTabIdRef = useRef<string>();
+ // `chartList` is rebuilt from `Object.keys` on every store update; keeping
+ // the listener state in a ref avoids tearing down/recreating the
+ // BroadcastChannel on each render and the race window that comes with it.
+ const latestStateRef = useRef({
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ });
+ latestStateRef.current = {
+ source,
+ query,
+ chartId,
+ chartList,
+ dashboardId,
+ errorMitigationFunction,
+ };
+
useEffect(() => {
+ // Guard against duplicate dispatches if both the BroadcastChannel and the
+ // storage fallback ever deliver the same completion.
+ let handled = false;
const handleOAuthComplete = (tabId?: string) => {
- if (tabId !== extra.tab_id || tabId === lastHandledTabIdRef.current) {
+ if (
+ tabId !== extra.tab_id ||
+ tabId === lastHandledTabIdRef.current ||
+ handled
+ ) {
return;
}
-
- if (errorMitigationFunction) {
- errorMitigationFunction();
- } else if (source === 'sqllab' && query) {
- dispatch(reRunQuery(query));
- } else if (source === 'explore') {
- dispatch(triggerQuery(true, chartId));
- } else if (source === 'dashboard') {
- dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId));
- } else if (source === 'crud') {
+ const {
+ source: src,
+ query: q,
+ chartId: cId,
+ chartList: cList,
+ dashboardId: dId,
+ errorMitigationFunction: mitigate,
+ } = latestStateRef.current;
+ // `handled`/`lastHandledTabIdRef` are only set below, after this whole
+ // chain, and only reached when a branch actually dispatches — so a
+ // signal that arrives before state is ready (e.g. `query` still null
+ // in SQL Lab) falls through to the catch-all `return` and a later
+ // fallback signal can still succeed.
+ if (mitigate) {
+ mitigate();
+ } else if (src === 'sqllab' && q) {
+ dispatch(reRunQuery(q));
+ } else if (src === 'explore') {
+ dispatch(triggerQuery(true, cId));
+ } else if (src === 'dashboard') {
+ dispatch(onRefresh(cList.map(Number), true, 0, dId));
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unguarded optional dashboardId</b></div>
<div id="fix">
`dId` comes from `state.dashboardInfo?.id` (lines 102-104), so it can be `undefined`; `onRefresh`'s `dashboardId` parameter is optional (`dashboardState.ts:869-876`). With `src === 'dashboard'` before `dashboardInfo` is loaded, the refresh dispatches with `undefined` and silently no-ops. Guard the branch on `dId !== undefined` or skip dispatch when absent.
</div>
</div>
<small><i>Code Review Run #ebcdd2</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): idempotent OAuth2 completion and resilient listener [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #40292:
URL: https://github.com/apache/superset/pull/40292#discussion_r4170685315
##########
superset-frontend/src/components/ErrorMessage/oauth2CallbackScript.test.ts:
##########
@@ -0,0 +1,113 @@
+/**
+ * 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 fs from 'fs';
+import path from 'path';
+
+// Runs the actual inline script from the OAuth2 popup callback template
+// (not a reimplementation of it), so a future edit to the template can't
+// silently drift from this regression coverage.
+const TEMPLATE_PATH = path.join(
Review Comment:
The frontend CI image only copies `superset-frontend`, so this backend-template path is absent there; shard 3 currently fails all three new tests with `ENOENT: /app/superset/templates/superset/oauth2.html`. Could we make the actual template available in the `superset-node-ci` image so these regression tests run in CI too?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org