Skip to content

Commit 92579ff

Browse files
committed
feat(popup): add main open target for notifications
Add a dedicated open target inside notification-main without changing the current row layout or click behavior. Cover the new interaction with renderer tests.
1 parent f1ceeba commit 92579ff

3 files changed

Lines changed: 223 additions & 3 deletions

File tree

src/popup/notification-renderer.js

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,11 @@ function createNotificationItem(notif, repoHeader, repoFullName) {
275275
li.dataset.id = notif.id;
276276
li.dataset.repo = repoFullName;
277277

278+
async function openNotification() {
279+
await sendMessage(MESSAGE_TYPES.OPEN_NOTIFICATION, { notificationId: notif.id });
280+
window.close();
281+
}
282+
278283
// Build icon class with state information
279284
const iconClass = buildIconClass(notif);
280285

@@ -300,6 +305,10 @@ function createNotificationItem(notif, repoHeader, repoFullName) {
300305
const mainDiv = document.createElement("div");
301306
mainDiv.className = "notification-main";
302307

308+
const openTarget = document.createElement("button");
309+
openTarget.type = "button";
310+
openTarget.className = "notification-open-target";
311+
303312
const titleDiv = document.createElement("div");
304313
titleDiv.className = "notification-title";
305314
const fullTitle = releaseBody ? `${notif.title}\n\n${releaseBody}` : notif.title;
@@ -325,7 +334,8 @@ function createNotificationItem(notif, repoHeader, repoFullName) {
325334
titleDiv.appendChild(previewSpan);
326335
}
327336

328-
mainDiv.appendChild(titleDiv);
337+
openTarget.appendChild(titleDiv);
338+
mainDiv.appendChild(openTarget);
329339
contentDiv.appendChild(mainDiv);
330340

331341
// Create metadata area
@@ -445,8 +455,7 @@ function createNotificationItem(notif, repoHeader, repoFullName) {
445455
if (e.target.closest(".btn-mark-read") || e.target.closest("a")) {
446456
return;
447457
}
448-
await sendMessage(MESSAGE_TYPES.OPEN_NOTIFICATION, { notificationId: notif.id });
449-
window.close();
458+
await openNotification();
450459
});
451460

452461
// Mark as read button with immediate visual feedback

src/popup/popup.css

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -800,6 +800,29 @@ body.dark-theme .notification-hover-card {
800800
color: var(--text-secondary);
801801
}
802802

803+
.notification-open-target {
804+
display: block;
805+
width: 100%;
806+
min-width: 0;
807+
padding: 0;
808+
margin: 0;
809+
border: 0;
810+
background: transparent;
811+
appearance: none;
812+
-webkit-appearance: none;
813+
color: inherit;
814+
font: inherit;
815+
line-height: inherit;
816+
text-align: left;
817+
cursor: pointer;
818+
border-radius: var(--radius-sm);
819+
}
820+
821+
.notification-open-target:focus-visible {
822+
outline: none;
823+
box-shadow: 0 0 0 3px var(--action-focus);
824+
}
825+
803826
/* Issue states */
804827
.notification-icon.issue.open {
805828
color: var(--icon-issue-open);
Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import { describe, it, expect, vi, beforeEach, beforeAll, afterEach } from "vitest";
5+
6+
beforeAll(() => {
7+
if (typeof CSS === "undefined" || !CSS.escape) {
8+
// @ts-ignore
9+
globalThis.CSS = {
10+
escape(value) {
11+
const str = String(value);
12+
const length = str.length;
13+
let result = "";
14+
for (let i = 0; i < length; i++) {
15+
const code = str.charCodeAt(i);
16+
if (code === 0x0000) {
17+
result += "\uFFFD";
18+
continue;
19+
}
20+
if (
21+
(code >= 0x0001 && code <= 0x001f) ||
22+
code === 0x007f ||
23+
(i === 0 && code >= 0x0030 && code <= 0x0039) ||
24+
(i === 1 && code >= 0x0030 && code <= 0x0039 && str.charCodeAt(0) === 0x002d)
25+
) {
26+
result += `\\${code.toString(16)} `;
27+
continue;
28+
}
29+
if (
30+
code >= 0x80 ||
31+
code === 0x2d ||
32+
code === 0x5f ||
33+
(code >= 0x30 && code <= 0x39) ||
34+
(code >= 0x41 && code <= 0x5a) ||
35+
(code >= 0x61 && code <= 0x7a)
36+
) {
37+
result += str[i];
38+
} else {
39+
result += `\\${str[i]}`;
40+
}
41+
}
42+
return result;
43+
},
44+
};
45+
}
46+
});
47+
48+
vi.mock("../src/lib/constants.js", () => ({
49+
GITHUB_SITE_BASE: "https://github.com",
50+
ANIMATION_DURATION: { FADE_OUT: 0, ERROR_BACKGROUND_FADE: 0 },
51+
NOTIFICATION_TYPES: { RELEASE: "Release" },
52+
MESSAGE_TYPES: {
53+
MARK_AS_READ: "markAsRead",
54+
OPEN_NOTIFICATION: "openNotification",
55+
},
56+
TIME_CONVERSION: { MS_PER_MINUTE: 60000 },
57+
}));
58+
59+
vi.mock("../src/lib/format-utils.js", () => ({
60+
formatReason: vi.fn((reason) => reason),
61+
getNotificationStatus: vi.fn(() => "Status"),
62+
}));
63+
64+
vi.mock("../src/lib/icons.js", () => ({
65+
getIconSVGElement: vi.fn(() => {
66+
const svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");
67+
return svg;
68+
}),
69+
}));
70+
71+
const sendMessage = vi.fn();
72+
73+
const { initRenderer, renderNotifications, clearNotificationCache } =
74+
await import("../src/popup/notification-renderer.js");
75+
76+
function makeNotif(id) {
77+
return {
78+
id: String(id),
79+
title: `Notification ${id}`,
80+
reason: "mention",
81+
updated_at: new Date().toISOString(),
82+
icon: "issue",
83+
type: "Issue",
84+
url: "https://github.com/owner/repo/issues/1",
85+
repository: {
86+
full_name: "owner/repo",
87+
html_url: "https://github.com/owner/repo",
88+
},
89+
author: {
90+
login: "octocat",
91+
avatar_url: "https://avatars.githubusercontent.com/u/1?v=4",
92+
},
93+
};
94+
}
95+
96+
describe("notification main open target", () => {
97+
let notificationsList;
98+
let emptyState;
99+
let markAllBtn;
100+
let closeSpy;
101+
102+
beforeEach(() => {
103+
vi.clearAllMocks();
104+
clearNotificationCache();
105+
106+
document.body.innerHTML = `
107+
<ul id="notifications-list"></ul>
108+
<div id="empty-state" hidden></div>
109+
<button id="mark-all-btn"></button>
110+
`;
111+
112+
notificationsList = document.getElementById("notifications-list");
113+
emptyState = document.getElementById("empty-state");
114+
markAllBtn = document.getElementById("mark-all-btn");
115+
closeSpy = vi.spyOn(window, "close").mockImplementation(() => {});
116+
117+
initRenderer({
118+
notificationsList,
119+
emptyState,
120+
markAllBtn,
121+
sendMessage,
122+
getShowHoverCards: () => false,
123+
onUserAction: vi.fn(),
124+
onMarkRepoAsRead: vi.fn(),
125+
});
126+
});
127+
128+
afterEach(() => {
129+
closeSpy.mockRestore();
130+
});
131+
132+
it("renders an explicit main open target inside notification-main", () => {
133+
renderNotifications([makeNotif(1)]);
134+
135+
const openTarget = notificationsList.querySelector(
136+
'.notification-item[data-id="1"] .notification-main .notification-open-target',
137+
);
138+
139+
expect(openTarget).not.toBeNull();
140+
expect(openTarget.tagName).toBe("BUTTON");
141+
});
142+
143+
it("opens the notification from the main target", async () => {
144+
renderNotifications([makeNotif(1)]);
145+
sendMessage.mockResolvedValueOnce({ success: true });
146+
147+
const openTarget = notificationsList.querySelector(
148+
'.notification-item[data-id="1"] .notification-open-target',
149+
);
150+
openTarget.click();
151+
152+
expect(sendMessage).toHaveBeenCalledWith("openNotification", { notificationId: "1" });
153+
await vi.waitFor(() => {
154+
expect(closeSpy).toHaveBeenCalled();
155+
});
156+
});
157+
158+
it("keeps row click behavior unchanged outside the explicit open target", async () => {
159+
renderNotifications([makeNotif(1)]);
160+
sendMessage.mockResolvedValueOnce({ success: true });
161+
162+
const item = notificationsList.querySelector('.notification-item[data-id="1"]');
163+
item.click();
164+
165+
expect(sendMessage).toHaveBeenCalledWith("openNotification", { notificationId: "1" });
166+
await vi.waitFor(() => {
167+
expect(closeSpy).toHaveBeenCalled();
168+
});
169+
});
170+
171+
it("keeps the avatar link and mark-as-read button independent from notification open", () => {
172+
renderNotifications([makeNotif(1)]);
173+
174+
const avatarLink = notificationsList.querySelector(
175+
'.notification-item[data-id="1"] .author-profile-link',
176+
);
177+
avatarLink.click();
178+
expect(sendMessage).not.toHaveBeenCalledWith("openNotification", { notificationId: "1" });
179+
180+
const markReadBtn = notificationsList.querySelector(
181+
'.notification-item[data-id="1"] .btn-mark-read',
182+
);
183+
markReadBtn.click();
184+
185+
expect(sendMessage).toHaveBeenCalledWith("markAsRead", { notificationId: "1" });
186+
expect(sendMessage).not.toHaveBeenCalledWith("openNotification", { notificationId: "1" });
187+
});
188+
});

0 commit comments

Comments
 (0)