Skip to content

Commit cb87358

Browse files
wgordon17wgordon17
committed
fix: monitor-all gate bypass, hot poll guard, and UX
- invalidates notifications gate when monitoredRepos or trackedUsers change so next poll fetches fresh data instead of returning stale 304 - tightens hot poll PR qualifier to require enriched === true, excluding unenriched and null-checkStatus PRs from unnecessary polling - switches monitor-all eyeball icon from opacity toggle to text-info color for better enabled/disabled contrast - adds "Monitoring all" indicator with repo names on Settings page - adds regex constraints to RepoRefSchema for defense-in-depth validation - sets makePullRequest helper default to enriched: true (steady state) - adds gate bypass integration tests and Settings indicator render tests
1 parent 6e3547c commit cb87358

File tree

8 files changed

+145
-28
lines changed

8 files changed

+145
-28
lines changed

‎src/app/components/onboarding/RepoSelector.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -563,7 +563,10 @@ export default function RepoSelector(props: RepoSelectorProps) {
563563
props.onMonitorToggle?.(toRepoRef(repo()), !monitoredSet().has(repo().fullName));
564564
}}
565565
class="btn btn-ghost btn-sm btn-circle mr-2"
566-
classList={{ "opacity-40": !monitoredSet().has(repo().fullName) }}
566+
classList={{
567+
"text-info": monitoredSet().has(repo().fullName),
568+
"text-base-content/20": !monitoredSet().has(repo().fullName),
569+
}}
567570
title={monitoredSet().has(repo().fullName) ? "Stop monitoring all activity" : "Monitor all activity"}
568571
aria-label={monitoredSet().has(repo().fullName) ? "Stop monitoring all activity" : "Monitor all activity"}
569572
aria-pressed={monitoredSet().has(repo().fullName)}

‎src/app/components/settings/SettingsPage.tsx‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { createSignal, Show, onCleanup } from "solid-js";
1+
import { createSignal, createMemo, Show, onCleanup } from "solid-js";
22
import { useNavigate } from "@solidjs/router";
33
import { config, updateConfig, setMonitoredRepo } from "../../stores/config";
44
import { clearAuth } from "../../stores/auth";
@@ -54,6 +54,10 @@ export default function SettingsPage() {
5454
const [localRepos, setLocalRepos] = createSignal<RepoRef[]>(config.selectedRepos);
5555
const [localUpstream, setLocalUpstream] = createSignal<RepoRef[]>(config.upstreamRepos);
5656

57+
const monitoredRepoNames = createMemo(() =>
58+
config.monitoredRepos.map(r => r.fullName).join(", ")
59+
);
60+
5761
// ── Helpers ──────────────────────────────────────────────────────────────
5862

5963
async function mergeNewOrgs() {
@@ -305,6 +309,15 @@ export default function SettingsPage() {
305309
Tracking {localRepos().length} repos will use significant API quota per poll cycle
306310
</p>
307311
</Show>
312+
<Show when={config.monitoredRepos.length > 0}>
313+
<p class="text-xs text-info flex items-center gap-1 mt-0.5">
314+
<svg xmlns="http://www.w3.org/2000/svg" class="h-3 w-3 shrink-0" fill="none" viewBox="0 0 24 24" stroke="currentColor" stroke-width={2} aria-hidden="true">
315+
<path stroke-linecap="round" stroke-linejoin="round" d="M15 12a3 3 0 11-6 0 3 3 0 016 0z" />
316+
<path stroke-linecap="round" stroke-linejoin="round" d="M2.458 12C3.732 7.943 7.523 5 12 5c4.478 0 8.268 2.943 9.542 7-1.274 4.057-5.064 7-9.542 7-4.477 0-8.268-2.943-9.542-7z" />
317+
</svg>
318+
Monitoring all: {monitoredRepoNames()}
319+
</p>
320+
</Show>
308321
</div>
309322
<button
310323
type="button"

‎src/app/services/poll.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ createRoot(() => {
109109
}
110110
if (key !== _trackedUsersKey) {
111111
_trackedUsersKey = key;
112+
_lastSuccessfulFetch = null; // Force next poll to bypass notifications gate
112113
untrack(() => _resetNotificationState());
113114
}
114115
});
@@ -122,6 +123,7 @@ createRoot(() => {
122123
}
123124
if (key !== _monitoredReposKey) {
124125
_monitoredReposKey = key;
126+
_lastSuccessfulFetch = null; // Force next poll to bypass notifications gate
125127
untrack(() => _resetNotificationState());
126128
}
127129
});
@@ -458,7 +460,7 @@ export function rebuildHotSets(data: DashboardData): void {
458460
_hotRuns.clear();
459461

460462
for (const pr of data.pullRequests) {
461-
if ((pr.checkStatus === "pending" || pr.checkStatus === null) && pr.nodeId) {
463+
if (pr.enriched && pr.checkStatus === "pending" && pr.nodeId) {
462464
if (_hotPRs.size >= MAX_HOT_PRS) {
463465
console.warn(`[hot-poll] PR cap reached (${MAX_HOT_PRS}), skipping remaining`);
464466
break;

‎src/app/stores/config.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,12 @@ export function resolveTheme(theme: ThemeId): string {
1818
return prefersDark ? AUTO_DARK_THEME : AUTO_LIGHT_THEME;
1919
}
2020

21+
const REPO_SEGMENT = /^[A-Za-z0-9._-]{1,100}$/;
22+
2123
export const RepoRefSchema = z.object({
22-
owner: z.string(),
23-
name: z.string(),
24-
fullName: z.string(),
24+
owner: z.string().regex(REPO_SEGMENT),
25+
name: z.string().regex(REPO_SEGMENT),
26+
fullName: z.string().regex(/^[A-Za-z0-9._-]{1,100}\/[A-Za-z0-9._-]{1,100}$/),
2527
});
2628

2729
export const TrackedUserSchema = z.object({

‎tests/components/settings/SettingsPage.test.tsx‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -684,6 +684,31 @@ describe("SettingsPage — Manage org access button", () => {
684684
});
685685

686686
describe("SettingsPage — monitor toggle wiring", () => {
687+
it("shows monitored repos indicator when repos are monitored", () => {
688+
updateConfig({
689+
selectedRepos: [
690+
{ owner: "org", name: "repo1", fullName: "org/repo1" },
691+
{ owner: "org", name: "repo2", fullName: "org/repo2" },
692+
],
693+
monitoredRepos: [
694+
{ owner: "org", name: "repo1", fullName: "org/repo1" },
695+
{ owner: "org", name: "repo2", fullName: "org/repo2" },
696+
],
697+
});
698+
renderSettings();
699+
700+
const indicator = screen.getByText(/Monitoring all:/);
701+
expect(indicator.textContent).toContain("org/repo1");
702+
expect(indicator.textContent).toContain("org/repo2");
703+
});
704+
705+
it("hides monitored repos indicator when no repos are monitored", () => {
706+
updateConfig({ monitoredRepos: [] });
707+
renderSettings();
708+
709+
expect(screen.queryByText(/Monitoring all:/)).toBeNull();
710+
});
711+
687712
it("includes monitoredRepos in exported settings JSON", async () => {
688713
updateConfig({
689714
selectedRepos: [{ owner: "org", name: "repo1", fullName: "org/repo1" }],

‎tests/helpers/index.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ export function makePullRequest(overrides: Partial<PullRequest> = {}): PullReque
5353
labels: [],
5454
reviewDecision: null,
5555
totalReviewCount: 0,
56+
enriched: true,
5657
surfacedBy: ["testuser"],
5758
...overrides,
5859
};

‎tests/services/hot-poll.test.ts‎

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ describe("resetPollState", () => {
206206
it("clears hot sets and resets generation", async () => {
207207
rebuildHotSets({
208208
...emptyData,
209-
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", nodeId: "PR_x" })],
209+
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_x" })],
210210
workflowRuns: [makeWorkflowRun({ id: 10, status: "in_progress", conclusion: null, repoFullName: "o/r" })],
211211
});
212212
expect(getHotPollGeneration()).toBe(1);
@@ -235,7 +235,7 @@ describe("rebuildHotSets", () => {
235235
expect(getHotPollGeneration()).toBe(2);
236236
});
237237

238-
it("populates hot PRs for pending/null checkStatus with nodeId", async () => {
238+
it("populates hot PRs for enriched pending checkStatus with nodeId", async () => {
239239
const octokit = makeOctokit(undefined, () => Promise.resolve({
240240
nodes: [],
241241
rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" },
@@ -245,20 +245,20 @@ describe("rebuildHotSets", () => {
245245
rebuildHotSets({
246246
...emptyData,
247247
pullRequests: [
248-
makePullRequest({ id: 1, checkStatus: "pending", nodeId: "PR_a" }),
249-
makePullRequest({ id: 2, checkStatus: null, nodeId: "PR_b" }),
250-
makePullRequest({ id: 3, checkStatus: "success", nodeId: "PR_c" }), // should be skipped
251-
makePullRequest({ id: 4, checkStatus: "pending" }), // no nodeId, should be skipped
248+
makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_a" }),
249+
makePullRequest({ id: 2, checkStatus: null, enriched: true, nodeId: "PR_b" }), // null checkStatus — skipped (not pending)
250+
makePullRequest({ id: 3, checkStatus: "success", enriched: true, nodeId: "PR_c" }), // resolved — skipped
251+
makePullRequest({ id: 4, checkStatus: "pending", enriched: true }), // no nodeId — skipped
252+
makePullRequest({ id: 5, checkStatus: "pending", enriched: false, nodeId: "PR_e" }), // not enriched — skipped
252253
],
253254
});
254255

255256
await fetchHotData();
256-
// Verify graphql was called with only the 2 eligible node IDs
257+
// Verify graphql was called with only the 1 eligible node ID
257258
expect(octokit.graphql).toHaveBeenCalledTimes(1);
258259
const calledIds = (octokit.graphql.mock.calls[0][1] as { ids: string[] }).ids;
259-
expect(calledIds).toHaveLength(2);
260+
expect(calledIds).toHaveLength(1);
260261
expect(calledIds).toContain("PR_a");
261-
expect(calledIds).toContain("PR_b");
262262
});
263263

264264
it("populates hot runs for queued/in_progress, skips completed", async () => {
@@ -377,7 +377,7 @@ describe("fetchHotData", () => {
377377

378378
rebuildHotSets({
379379
...emptyData,
380-
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", nodeId: "PR_x" })],
380+
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_x" })],
381381
});
382382

383383
// First fetch — PR is hot, returns success -> evicts
@@ -603,7 +603,7 @@ describe("createHotPollCoordinator", () => {
603603

604604
rebuildHotSets({
605605
...emptyData,
606-
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", nodeId: "PR_a" })],
606+
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_a" })],
607607
});
608608

609609
const { pushError } = await import("../../src/app/lib/errors");
@@ -661,7 +661,7 @@ describe("createHotPollCoordinator", () => {
661661

662662
rebuildHotSets({
663663
...emptyData,
664-
pullRequests: [makePullRequest({ id: 42, nodeId: "PR_node42", repoFullName: "o/r" })],
664+
pullRequests: [makePullRequest({ id: 42, checkStatus: "pending", enriched: true, nodeId: "PR_node42", repoFullName: "o/r" })],
665665
workflowRuns: [makeWorkflowRun({ id: 7, status: "in_progress", conclusion: null, repoFullName: "o/r" })],
666666
});
667667

@@ -746,7 +746,7 @@ describe("createHotPollCoordinator", () => {
746746

747747
rebuildHotSets({
748748
...emptyData,
749-
pullRequests: [makePullRequest({ id: 42, nodeId: "PR_node42", repoFullName: "o/r" })],
749+
pullRequests: [makePullRequest({ id: 42, checkStatus: "pending", enriched: true, nodeId: "PR_node42", repoFullName: "o/r" })],
750750
});
751751

752752
const { pushError } = await import("../../src/app/lib/errors");
@@ -868,7 +868,7 @@ describe("rebuildHotSets caps", () => {
868868

869869
it("caps hot PRs at MAX_HOT_PRS (200)", async () => {
870870
const prs = Array.from({ length: 250 }, (_, i) =>
871-
makePullRequest({ id: i + 1, checkStatus: "pending", nodeId: `PR_${i}` })
871+
makePullRequest({ id: i + 1, checkStatus: "pending", enriched: true, nodeId: `PR_${i}` })
872872
);
873873

874874
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
@@ -945,8 +945,8 @@ describe("fetchHotData eviction edge cases", () => {
945945
rebuildHotSets({
946946
...emptyData,
947947
pullRequests: [
948-
makePullRequest({ id: 1, checkStatus: "pending", nodeId: "PR_one" }),
949-
makePullRequest({ id: 2, checkStatus: "pending", nodeId: "PR_two" }),
948+
makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_one" }),
949+
makePullRequest({ id: 2, checkStatus: "pending", enriched: true, nodeId: "PR_two" }),
950950
],
951951
});
952952

@@ -979,7 +979,7 @@ describe("fetchHotData eviction edge cases", () => {
979979

980980
rebuildHotSets({
981981
...emptyData,
982-
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", nodeId: "PR_merged" })],
982+
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_merged" })],
983983
});
984984

985985
const first = await fetchHotData();
@@ -1008,7 +1008,7 @@ describe("fetchHotData eviction edge cases", () => {
10081008

10091009
rebuildHotSets({
10101010
...emptyData,
1011-
pullRequests: [makePullRequest({ id: 2, checkStatus: null, nodeId: "PR_closed" })],
1011+
pullRequests: [makePullRequest({ id: 2, checkStatus: "pending", enriched: true, nodeId: "PR_closed" })],
10121012
});
10131013

10141014
await fetchHotData();
@@ -1022,7 +1022,7 @@ describe("clearHotSets", () => {
10221022
it("empties both hot maps so next fetchHotData is a no-op", async () => {
10231023
rebuildHotSets({
10241024
...emptyData,
1025-
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", nodeId: "PR_a" })],
1025+
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_a" })],
10261026
workflowRuns: [makeWorkflowRun({ id: 10, status: "in_progress", conclusion: null, repoFullName: "o/r" })],
10271027
});
10281028

@@ -1060,7 +1060,7 @@ describe("fetchHotData hadErrors", () => {
10601060

10611061
rebuildHotSets({
10621062
...emptyData,
1063-
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", nodeId: "PR_a" })],
1063+
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_a" })],
10641064
workflowRuns: [makeWorkflowRun({ id: 10, status: "in_progress", conclusion: null, repoFullName: "o/r" })],
10651065
});
10661066

@@ -1074,7 +1074,7 @@ describe("fetchHotData hadErrors", () => {
10741074

10751075
rebuildHotSets({
10761076
...emptyData,
1077-
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", nodeId: "PR_a" })],
1077+
pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_a" })],
10781078
});
10791079

10801080
const { hadErrors, prUpdates } = await fetchHotData();

‎tests/services/poll-notification-effects.test.ts‎

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,9 @@ vi.mock("../../src/app/lib/errors", () => ({
5959
import { updateConfig, resetConfig } from "../../src/app/stores/config";
6060

6161
// Import poll.ts — triggers createRoot + createEffect registration at module scope
62-
import "../../src/app/services/poll";
62+
import { fetchAllData, resetPollState } from "../../src/app/services/poll";
63+
import { getClient } from "../../src/app/services/github";
64+
import { fetchIssuesAndPullRequests, fetchWorkflowRuns } from "../../src/app/services/api";
6365

6466
describe("poll.ts — notification reset reactive effects", () => {
6567
beforeEach(() => {
@@ -124,3 +126,72 @@ describe("poll.ts — notification reset reactive effects", () => {
124126
expect(mockResetNotifState).toHaveBeenCalled();
125127
});
126128
});
129+
130+
describe("poll.ts — notifications gate bypass on config change", () => {
131+
const mockRequest = vi.fn();
132+
133+
beforeEach(() => {
134+
resetPollState();
135+
resetConfig();
136+
mockRequest.mockReset();
137+
mockRequest.mockResolvedValue({
138+
data: [],
139+
headers: { "last-modified": "Thu, 20 Mar 2026 12:00:00 GMT" },
140+
});
141+
vi.mocked(getClient).mockReturnValue({
142+
request: mockRequest,
143+
graphql: vi.fn(),
144+
hook: { before: vi.fn() },
145+
} as never);
146+
vi.mocked(fetchIssuesAndPullRequests).mockResolvedValue({
147+
issues: [], pullRequests: [], errors: [],
148+
});
149+
vi.mocked(fetchWorkflowRuns).mockResolvedValue({
150+
workflowRuns: [], errors: [],
151+
} as never);
152+
});
153+
154+
it("bypasses notifications gate after monitoredRepos change", async () => {
155+
// First call — no _lastSuccessfulFetch, gate skipped
156+
await fetchAllData();
157+
expect(mockRequest).not.toHaveBeenCalled();
158+
159+
// Second call — _lastSuccessfulFetch set, gate fires
160+
await fetchAllData();
161+
expect(mockRequest).toHaveBeenCalledWith("GET /notifications", expect.anything());
162+
mockRequest.mockClear();
163+
164+
// Change monitoredRepos — should null _lastSuccessfulFetch
165+
updateConfig({
166+
selectedRepos: [{ owner: "org", name: "repo", fullName: "org/repo" }],
167+
monitoredRepos: [{ owner: "org", name: "repo", fullName: "org/repo" }],
168+
});
169+
170+
// Third call — gate bypassed because _lastSuccessfulFetch was nulled
171+
await fetchAllData();
172+
expect(mockRequest).not.toHaveBeenCalled();
173+
});
174+
175+
it("bypasses notifications gate after trackedUsers change", async () => {
176+
// First call — sets _lastSuccessfulFetch
177+
await fetchAllData();
178+
179+
// Second call — gate fires
180+
await fetchAllData();
181+
mockRequest.mockClear();
182+
183+
// Change trackedUsers — should null _lastSuccessfulFetch
184+
updateConfig({
185+
trackedUsers: [{
186+
login: "octocat",
187+
avatarUrl: "https://avatars.githubusercontent.com/u/583231",
188+
name: "Octocat",
189+
type: "user" as const,
190+
}],
191+
});
192+
193+
// Next call — gate bypassed
194+
await fetchAllData();
195+
expect(mockRequest).not.toHaveBeenCalled();
196+
});
197+
});

0 commit comments

Comments
 (0)