Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ interface WorkspaceHostRow {
workspaceId: string;
organizationId: string;
hostId: string;
name: string;
branch: string;
}

interface HostNotificationSubscriberGroup {
Expand Down Expand Up @@ -43,6 +45,8 @@ export function V2NotificationController() {
workspaceId: v2Workspaces.id,
organizationId: v2Workspaces.organizationId,
hostId: v2Workspaces.hostId,
name: v2Workspaces.name,
branch: v2Workspaces.branch,
})),
[collections],
);
Expand Down Expand Up @@ -118,6 +122,8 @@ function groupWorkspacesByHostUrl({
const group = groups.get(hostUrl) ?? [];
group.push({
workspaceId: workspace.workspaceId,
workspaceName:
workspace.name.trim() || workspace.branch.trim() || "Workspace",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Using a constant fallback label ("Workspace") removes unique workspace identification for unnamed/unbranched workspaces, making notifications ambiguous.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsx, line 126:

<comment>Using a constant fallback label (`"Workspace"`) removes unique workspace identification for unnamed/unbranched workspaces, making notifications ambiguous.</comment>

<file context>
@@ -123,9 +123,7 @@ function groupWorkspacesByHostUrl({
-				workspace.name.trim() ||
-				workspace.branch.trim() ||
-				workspace.workspaceId,
+				workspace.name.trim() || workspace.branch.trim() || "Workspace",
 			paneLayout: paneLayoutsByWorkspaceId.get(workspace.workspaceId) ?? null,
 		});
</file context>
Suggested change
workspace.name.trim() || workspace.branch.trim() || "Workspace",
workspace.name.trim() || workspace.branch.trim() || workspace.workspaceId,

Tip: Review your code locally with the cubic CLI to iterate faster.

paneLayout: paneLayoutsByWorkspaceId.get(workspace.workspaceId) ?? null,
});
groups.set(hostUrl, group);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {

export interface HostNotificationWorkspaceState {
workspaceId: string;
workspaceName: string;
paneLayout: WorkspaceState<PaneViewerData> | null;
}

Expand Down Expand Up @@ -53,6 +54,7 @@ export function HostNotificationSubscriber({
if (!workspace) return;
handleV2AgentLifecycleEvent({
workspaceId,
workspaceName: workspace.workspaceName,
payload,
paneLayout: workspace.paneLayout,
volume,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
useV2NotificationStore,
type V2NotificationSourceInput,
} from "renderer/stores/v2-notifications";
import { getV2NativeNotificationContent } from "./notificationContent";
import {
isV2NotificationTargetVisible,
resolveV2NotificationTarget,
Expand All @@ -27,12 +28,14 @@ import { resolveV2AgentStatusTransition } from "./statusTransitions";
*/
export function handleV2AgentLifecycleEvent({
workspaceId,
workspaceName,
payload,
paneLayout,
volume,
muted,
}: {
workspaceId: string;
workspaceName: string;
payload: AgentLifecyclePayload;
paneLayout: WorkspaceState<PaneViewerData> | null | undefined;
volume: number;
Expand Down Expand Up @@ -61,7 +64,12 @@ export function handleV2AgentLifecycleEvent({
const ringtoneId = useRingtoneStore.getState().selectedRingtoneId;
void playRingtone({ ringtoneId, volume, muted });

showNativeNotification({ payload, workspaceId, target });
showNativeNotification({
payload,
workspaceId,
workspaceName,
target,
});
}

export function handleV2TerminalLifecycleEvent({
Expand Down Expand Up @@ -134,17 +142,18 @@ function shouldSuppress(
function showNativeNotification({
payload,
workspaceId,
workspaceName,
target,
}: {
payload: AgentLifecyclePayload;
workspaceId: string;
workspaceName: string;
target: V2NotificationTarget;
}): void {
const isPermission = payload.eventType === "PermissionRequest";
const title = isPermission ? "Awaiting Response" : "Agent Complete";
const body = isPermission
? "Your agent needs input"
: "Your agent has finished";
const { title, body } = getV2NativeNotificationContent({
workspaceName,
payload,
});

void electronTrpcClient.notifications.showNative
.mutate({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { describe, expect, it } from "bun:test";
import type { AgentLifecyclePayload } from "@superset/workspace-client";
import { getV2NativeNotificationContent } from "./notificationContent";

function payload(
overrides: Partial<AgentLifecyclePayload>,
): AgentLifecyclePayload {
return {
eventType: "Stop",
terminalId: "terminal-1",
occurredAt: 1,
...overrides,
};
}

describe("getV2NativeNotificationContent", () => {
it("uses the agent label in the title and workspace label in the body", () => {
expect(
getV2NativeNotificationContent({
workspaceName: "Improve notifications",
payload: payload({
agent: { agentId: "codex", sessionId: "session-1" },
}),
}),
).toEqual({
title: "Codex - Complete",
body: "Improve notifications",
});
});

it("uses needs-attention copy for permission requests", () => {
expect(
getV2NativeNotificationContent({
workspaceName: "Improve notifications",
payload: payload({
eventType: "PermissionRequest",
agent: { agentId: "claude" },
}),
}),
).toMatchObject({
title: "Claude - Needs Attention",
body: "Improve notifications",
});
});

it("falls back to generic labels", () => {
expect(
getV2NativeNotificationContent({
workspaceName: " ",
payload: payload({ agent: { agentId: "droid" } }),
}),
).toEqual({
title: "Droid - Complete",
body: "Workspace",
});

expect(
getV2NativeNotificationContent({
workspaceName: "",
payload: payload({ agent: undefined }),
}),
).toMatchObject({
title: "Agent - Complete",
body: "Workspace",
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import {
BUILTIN_AGENT_LABELS,
type BuiltinAgentId,
} from "@superset/shared/agent-catalog";
import type {
AgentIdentity,
AgentLifecyclePayload,
} from "@superset/workspace-client";

interface V2NativeNotificationContentOptions {
workspaceName: string;
payload: AgentLifecyclePayload;
}

export function getV2NativeNotificationContent({
workspaceName,
payload,
}: V2NativeNotificationContentOptions): { title: string; body: string } {
const agentLabel = getAgentLabel(payload.agent);
const action =
payload.eventType === "PermissionRequest" ? "Needs Attention" : "Complete";
const workspaceLabel = cleanLabel(workspaceName) ?? "Workspace";

return {
title: `${agentLabel} - ${action}`,
body: workspaceLabel,
};
}

function getAgentLabel(agent: AgentIdentity | undefined): string {
const agentId = cleanLabel(agent?.agentId);
if (!agentId) return "Agent";
if (agentId in BUILTIN_AGENT_LABELS) {
return BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId];
Comment on lines +33 to +34
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify BUILTIN_AGENT_LABELS declaration and whether it uses a null-prototype map.
rg -n -C3 "BUILTIN_AGENT_LABELS"

# Verify AgentIdentity/agentId typing or validation constraints upstream.
rg -n -C3 "interface AgentIdentity|type AgentIdentity|agentId"

Repository: superset-sh/superset

Length of output: 50379


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find cleanLabel function definition
rg -A 10 "function cleanLabel|const cleanLabel|export.*cleanLabel" apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts

# Show the full function context in notificationContent.ts
rg -B 5 -A 15 "function getAgentLabel" apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts

Repository: superset-sh/superset

Length of output: 973


Use Object.prototype.hasOwnProperty.call() for own-property checks.

On line 36, the in operator checks both own properties and inherited prototype keys. While the practical risk is mitigated by type constraints on agentId (BuiltinAgentId | "droid"), using Object.prototype.hasOwnProperty.call() is the correct approach for own-property checks and avoids relying on type safety for this operation.

Proposed fix
-	if (agentId in BUILTIN_AGENT_LABELS) {
+	if (Object.prototype.hasOwnProperty.call(BUILTIN_AGENT_LABELS, agentId)) {
 		return BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId];
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (agentId in BUILTIN_AGENT_LABELS) {
return BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId];
if (Object.prototype.hasOwnProperty.call(BUILTIN_AGENT_LABELS, agentId)) {
return BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId];
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts`
around lines 36 - 37, Replace the use of the `in` operator for own-property
checking with an explicit hasOwnProperty call: instead of `if (agentId in
BUILTIN_AGENT_LABELS)`, use
`Object.prototype.hasOwnProperty.call(BUILTIN_AGENT_LABELS, agentId)` and then
return `BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId]` as before; this change
should be applied where `agentId`, `BUILTIN_AGENT_LABELS`, and `BuiltinAgentId`
are referenced in notificationContent.ts to ensure only own properties are
matched.

}
return humanizeIdentifier(agentId);
}

function cleanLabel(value: string | null | undefined): string | null {
const trimmed = value?.trim();
return trimmed ? trimmed : null;
}

function humanizeIdentifier(value: string): string {
const words = value
.replace(/^custom:/, "")
.split(/[-_:\s]+/)
.filter(Boolean);
if (words.length === 0) return "Agent";
return words
.map((word) => word.charAt(0).toUpperCase() + word.slice(1))
.join(" ");
}
Loading