codeant-ai-for-open-source[bot] commented on code in PR #40968:
URL: https://github.com/apache/superset/pull/40968#discussion_r3394882608
##########
superset-frontend/src/core/views/index.ts:
##########
@@ -75,6 +75,11 @@ const registerView: typeof viewsApi.registerView = (
viewRegistry.set(id, { view, location, provider });
const ids = locationIndex.get(location) ?? new Set();
+ // Re-registering an existing id must reflect its new recency. A Set
preserves
+ // first-insertion order and `add` is a no-op for an existing member, so
delete
+ // first to move the id to the end. Consumers that select the most-recently
+ // registered view (e.g. the chatbot resolver) depend on this ordering.
+ ids.delete(id);
ids.add(id);
Review Comment:
**Suggestion:** Re-registration is now a supported path, but this change
only updates ordering and does not make disposal instance-safe. If the same
view id is registered twice, disposing the older `Disposable` will still remove
the newer active registration (because disposal is keyed only by `id`), which
can make the chatbot disappear unexpectedly after hot-reload/re-init flows. Tie
each registration to a unique token/version and make `dispose()` a no-op when
it is not disposing the currently registered instance. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chatbot bubble vanishes after extension reload or teardown.
- ⚠️ Re-registered views can be removed by stale disposables.
- ⚠️ Extensions cannot safely re-register views with same id.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Login with extensions enabled so the frontend mounts `App` and
`ExtensionsStartup`,
which sets `window.superset.views = views` and calls
`ExtensionsLoader.getInstance().initializeExtensions()` when
`FeatureFlag.EnableExtensions` is on
(`superset-frontend/src/views/App.tsx:24` shows
`<ChatbotMount />` under the flag;
`superset-frontend/src/extensions/ExtensionsStartup.tsx:88-113` sets
`window.superset` and
initializes extensions).
2. In an extension module loaded by `ExtensionsLoader.loadModule()`
(`superset-frontend/src/extensions/ExtensionsLoader.ts:118-177`), its
`activate(context)`
implementation registers a chatbot view twice under the same id, pushing
both disposables
into `context.subscriptions`, e.g.
`context.subscriptions.push(window.superset.views.registerView({ id:
'core.chatbot', ...
}, CHATBOT_LOCATION, providerV1));` followed later by another `registerView`
with the same
`id` but a different provider; both calls return `Disposable` instances
created at
`superset-frontend/src/core/views/index.ts:88-93`.
3. The second `registerView` overwrites the first entry for that `id` in
`viewRegistry`
and updates `locationIndex` ordering (`viewRegistry.set(id, { view,
location, provider })`
at `superset-frontend/src/core/views/index.ts:75` and the recency-handling
block at lines
77-84), so the active chatbot now points to the new provider, as consumed by
`getActiveChatbot()` (`superset-frontend/src/core/chatbot/index.ts:56-70`)
and
`ChatbotMount`
(`superset-frontend/src/components/ChatbotMount/index.ts:50-59`).
4. When the extension later tears down its first registration by calling
`dispose()` on
the older `Disposable` from `context.subscriptions`, that `dispose` executes
the callback
defined at `superset-frontend/src/core/views/index.ts:88-93`, which looks up
`viewRegistry.get(id)` using only the shared `id` and then calls
`viewRegistry.delete(id)`
and `locationIndex.get(registeredLocation)?.delete(id)`. Because
`viewRegistry` now holds
the newer registration under the same `id`, this stale disposal removes the
current active
chatbot entry, causing `getRegisteredViewIds(CHATBOT_LOCATION)` to return an
array without
that id (`superset-frontend/src/core/views/index.ts:12-15`),
`getActiveChatbot()` to
return `undefined` (`superset-frontend/src/core/chatbot/index.ts:56-60`), and
`ChatbotMount` to render nothing
(`superset-frontend/src/components/ChatbotMount/index.ts:58-62`), making the
chatbot
bubble disappear even though the extension believes its latest registration
is still
active. This is possible for any view id, not just `core.chatbot`, because
`Disposable`
instances are not tied to a unique registration token.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ecc002a2eaac4e15b13ca248a6d5506e&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=ecc002a2eaac4e15b13ca248a6d5506e&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/core/views/index.ts
**Line:** 78:83
**Comment:**
*Stale Reference: Re-registration is now a supported path, but this
change only updates ordering and does not make disposal instance-safe. If the
same view id is registered twice, disposing the older `Disposable` will still
remove the newer active registration (because disposal is keyed only by `id`),
which can make the chatbot disappear unexpectedly after hot-reload/re-init
flows. Tie each registration to a unique token/version and make `dispose()` a
no-op when it is not disposing the currently registered instance.
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%2F40968&comment_hash=ff1b63a6d24fbe79b0244ee382dac54b62322ed867290c42945cad7c0d35f50e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40968&comment_hash=ff1b63a6d24fbe79b0244ee382dac54b62322ed867290c42945cad7c0d35f50e&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
