codeant-ai-for-open-source[bot] commented on code in PR #36924:
URL: https://github.com/apache/superset/pull/36924#discussion_r2665142551
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -238,6 +251,18 @@ export async function embedDashboard({
setTimeout(refreshGuestToken, getGuestTokenRefreshTiming(guestToken));
+ // Register the resolvePermalinkUrl method for the iframe to call
+ // Returns null if no callback provided, allowing iframe to use default URL
+ ourPort.defineMethod(
+ 'resolvePermalinkUrl',
+ async ({ key }: { key: string }) => {
+ if (!resolvePermalinkUrl) {
+ return null;
+ }
+ return resolvePermalinkUrl({ key });
Review Comment:
**Suggestion:** The host-provided `resolvePermalinkUrl` callback may throw
or reject; the defined method currently forwards exceptions to the comms
boundary. Wrap the call in a try/catch and `await` the result so errors are
handled and a safe `null` is returned on failure. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
try {
const url = await resolvePermalinkUrl({ key });
return url;
} catch (err) {
log('Error in resolvePermalinkUrl callback', err);
return null;
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Wrapping the host-provided callback with try/catch and awaiting the result
prevents exceptions or rejected promises in user code from propagating across
the comms boundary. Returning null (and logging) is a safer failure mode than
letting the RPC reject unexpectedly.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-embedded-sdk/src/index.ts
**Line:** 262:262
**Comment:**
*Possible Bug: The host-provided `resolvePermalinkUrl` callback may
throw or reject; the defined method currently forwards exceptions to the comms
boundary. Wrap the call in a try/catch and `await` the result so errors are
handled and a safe `null` is returned on failure.
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.
```
</details>
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -177,10 +177,11 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug
}: PageProps) => {
// the currently stored value when hydrating
let activeTabs: string[] | undefined;
let chartStates: DashboardChartStates | undefined;
+ let anchor: string | undefined;
if (permalinkKey) {
const permalinkValue = await getPermalinkValue(permalinkKey);
if (permalinkValue) {
- ({ dataMask, activeTabs, chartStates } = permalinkValue.state);
+ ({ dataMask, activeTabs, chartStates, anchor } =
permalinkValue.state);
Review Comment:
**Suggestion:** Destructuring `permalinkValue.state` directly assumes
`state` exists; if `permalinkValue.state` is undefined this will throw a
TypeError at runtime. Provide a safe default (empty object) before
destructuring so the assignment never attempts to read properties from
undefined. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
const permalinkState = permalinkValue.state ?? {};
({ dataMask, activeTabs, chartStates, anchor } = permalinkState as
any);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code will throw if permalinkValue exists but its `state`
property is undefined. The proposed guard (use a default object) prevents a
runtime TypeError and is a safe, minimal hardening. This is a real, verifiable
failure mode given the loose typing of the API response.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/containers/DashboardPage.tsx
**Line:** 181:184
**Comment:**
*Null Pointer: Destructuring `permalinkValue.state` directly assumes
`state` exists; if `permalinkValue.state` is undefined this will throw a
TypeError at runtime. Provide a safe default (empty object) before
destructuring so the assignment never attempts to read properties from
undefined.
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.
```
</details>
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -67,14 +68,28 @@ const getDashboardPermalink = async ({
chartStates &&
Object.keys(chartStates).length > 0;
- return getDashboardPermalinkUtil({
+ const { key, url } = await getDashboardPermalinkUtil({
dashboardId,
dataMask,
activeTabs,
anchor,
chartStates: includeChartState ? chartStates : undefined,
includeChartState,
});
+
+ // Ask the SDK to resolve the permalink URL
+ // Returns null if no callback was provided by the host
+ const resolvedUrl = await Switchboard.get<string | null>(
+ 'resolvePermalinkUrl',
+ { key },
+ );
+
+ // If callback returned a URL, use it; otherwise use Superset's default URL
+ if (resolvedUrl) {
Review Comment:
**Suggestion:** Unsafe return of unvalidated host-provided value:
`resolvedUrl` is returned without verifying it's actually a string URL, which
can lead to callers receiving non-string values (object/function) and
subsequently throwing. Ensure `resolvedUrl` is a string before returning (and
otherwise fall back to the default `url`). [type error]
**Severity Level:** Minor ⚠️
```suggestion
const resolvedUrl = await Switchboard.get<unknown | null>(
'resolvePermalinkUrl',
{ key },
);
// Only accept a string result from the host; otherwise fall back to
Superset's default URL
if (typeof resolvedUrl === 'string') {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Validating the host-provided value is sensible: ensure it's a string before
returning to avoid surprising consumers. This prevents type/runtime errors when
hosts return unexpected shapes.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/embedded/api.tsx
**Line:** 82:88
**Comment:**
*Type Error: Unsafe return of unvalidated host-provided value:
`resolvedUrl` is returned without verifying it's actually a string URL, which
can lead to callers receiving non-string values (object/function) and
subsequently throwing. Ensure `resolvedUrl` is a string before returning (and
otherwise fall back to the default `url`).
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.
```
</details>
##########
superset-frontend/src/dashboard/components/URLShortLinkButton/index.tsx:
##########
@@ -69,7 +69,7 @@ export default function URLShortLinkButton({
chartStates &&
Object.keys(chartStates).length > 0;
- const url = await getDashboardPermalink({
+ const { url } = await getDashboardPermalink({
Review Comment:
**Suggestion:** Defensively handle different return shapes from
`getDashboardPermalink`: the current code destructures `url` assuming the
helper returns an object, but if it returns a plain string (or undefined) that
destructuring will throw a runtime TypeError; instead, capture the raw result,
detect whether it's a string or an object, and only call `setShortUrl` when a
string URL is available. [type error]
**Severity Level:** Minor ⚠️
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -83,6 +86,15 @@ export type ObserveDataMaskCallbackFn = (
) => void;
export type ThemeMode = 'default' | 'dark' | 'system';
+/**
+ * Callback to resolve permalink URLs.
+ * Receives the permalink key and returns the full URL to use for the
permalink.
+ */
+export type ResolvePermalinkUrlFn = (params: {
+ /** The permalink key (e.g., "xyz789") */
+ key: string;
+}) => string | Promise<string>;
+
export type EmbeddedDashboard = {
getScrollSize: () => Promise<Size>;
unmount: () => void;
Review Comment:
**Suggestion:** Type mismatch: the handler returns `null` when no callback
is provided, but `ResolvePermalinkUrlFn` is currently typed to only return
`string | Promise<string>`. Make the type allow `null` (and promise-wrapped
`null`) so consumers and TypeScript agree about the possible return values.
[type error]
**Severity Level:** Minor ⚠️
```suggestion
}) => string | null | Promise<string | null>;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The code path currently returns null when no callback is provided (and other
suggestions also return null on failure). The exported type should reflect that
null is a possible return value so TypeScript consumers handle it correctly.
This fixes a real type/contract mismatch.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-embedded-sdk/src/index.ts
**Line:** 100:100
**Comment:**
*Type Error: Type mismatch: the handler returns `null` when no callback
is provided, but `ResolvePermalinkUrlFn` is currently typed to only return
`string | Promise<string>`. Make the type allow `null` (and promise-wrapped
`null`) so consumers and TypeScript agree about the possible return values.
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.
```
</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: [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]
