fix: resolve open bug backlog #433–#444 with hardening pass#447
Merged
Conversation
Sweeps the remaining open bugs on the tracker. Issues #428/#429/#431/#432 were already fixed in ed68bd2 and are untouched here. Campaign / DB hardening: - #434 cancelCampaign + reconcileCampaignCounters now build their terminal- status IN-lists from TERMINAL_RECIPIENT_STATUSES instead of re-listing 'sent','delivered','opened','clicked' inline. Adding a new terminal status now flows through every call site instead of silently diverging. - #433 adds a partial index campaigns_type_automated_idx ON campaigns(id) WHERE type='automated' so resolveRecipients' welcome-exclusion anti-join can probe automated campaigns via an index scan instead of a seq scan. - #436 adds a partial composite index campaign_recipients_active_user_idx ON (user_id, campaign_id) WHERE status IN active-set so the anti-join's per-user probe hits only the active-status subset. Client / UX: - #435 Dashboard handleRefresh now inspects refetch()'s error field and surfaces a destructive toast for real network/HTTP failures instead of bucketing them as 'No monitors'. - #437 useCheckMonitor and useCheckMonitorSilent now track an AbortController per call in a shared Set and abort all in-flight fetches on component unmount, freeing browserless/scraper quota when the user navigates away mid-check. React Query v5 mutationFn doesn't receive a signal, so we plumb one in via a useAbortableFetchers hook. - #441 new usePageTitle hook sets document.title on authenticated pages (Dashboard, MonitorDetails, AdminErrors, AdminCampaigns, AdminCampaignDetail, ExtensionAuth) so tab labels don't stay stuck on the previous public page after an internal navigation. SEO / head: - #439 LandingPage accepts an optional path prop; ProtectedRoute passes window.location.pathname when rendering it as the unauthed fallback on /dashboard or /monitors/:id, so canonical/og:url/JSON-LD @id reflect the real URL instead of lying as '/'. - #440 new server-side crawler-UA middleware (shared/pageMetadata.ts + server/crawlerMeta.ts) detects Facebook/X/Slack/LinkedIn/Discord/Google/ Bing/etc bots and rewrites index.html's title/description/canonical/OG/ Twitter tags with per-route metadata before sending. Wired into both the prod static handler and the dev Vite middleware so link unfurls preview the actual page, not the landing page. Covered by crawlerMeta.test.ts. - #442 defines --font-sans / --font-serif / --font-mono in :root so the Tailwind font-mono class actually resolves to a monospace stack instead of falling through to Inter; API keys, CSS selectors, and error codes now render in ui-monospace rather than a proportional sans. - #443 drops the render-blocking @import url(fonts.googleapis.com…) from client/src/index.css in favor of a direct <link rel="stylesheet"> in client/index.html, parallelizing the font fetch with index.css. - #444 removes maximum-scale=1 from the viewport meta so low-vision users can pinch-zoom on mobile (WCAG 2.1 SC 1.4.4, Level AA). Tests: 2374 pass on vitest, npm run check clean, npm run build succeeds with only pre-existing warnings. New crawlerMeta.test.ts covers bot-UA detection and per-route rewriting. https://claude.ai/code/session_04BM7
Phase 1 — Write Tests: - use-page-title.test.ts: 5 tests covering set-on-mount, restore-on-unmount, undefined no-op, prop change, nested mount restoration. - use-monitors.test.ts: 2 tests asserting useCheckMonitor + useCheckMonitorSilent abort their in-flight fetches on component unmount (issue #437 regression guard). - campaignEmail.test.ts: 2 tests asserting cancelCampaign and reconcileCampaignCounters raw SQL builds its terminal-status IN-list from TERMINAL_RECIPIENT_STATUSES (issue #434 regression guard), not hardcoded literals. Phase 2 — Security Review (1 Medium, 1 Low): - Medium: server/static.ts + server/vite.ts built baseUrl from req.get("host") for crawler-UA responses, which reflected a spoofed Host header into canonical/og:url. Swapped to the pinned getAppUrl() (already driven by REPLIT_DOMAINS elsewhere in the codebase) so a Googlebot spoof can't poison SEO or unfurl previews with an attacker origin. - Low: server/static.ts now caches the index.html template at module scope instead of fs.readFileSync per crawler request. Template is immutable between deploys (Replit restarts the process), so this is the natural invalidation point. Phase 3 — Architecture Review (2 suggestions): - PAGE_METADATA ↔ SEOHead drift was unenforced. Added client/src/pages/page-metadata-parity.test.ts which statically extracts every <SEOHead path="X" title="Y" description="Z" /> from the page components and asserts a matching PAGE_METADATA[path] entry with the same title and description. Realigned 16 PAGE_METADATA entries to match each page's SEOHead copy word-for-word; added /developer which was missing from the map. - crawlerMeta regex rewriting was only tested against a synthetic HTML template. Added a test that loads the real client/index.html and asserts every expected head tag is actually mutated, so silent no-ops surface the moment someone reorders attributes or switches quote style. Tests: 2412 passing (99 files), npm run check clean. No in-scope regressions — out-of-scope findings filed into the Phase 6 bug report. https://claude.ai/code/session_04BM7
Phase 4 — Code Review: - Corrected stale comment in client/index.html that referenced a removed @import. - Added multi-line attribute test to crawlerMeta to lock in newline tolerance of \s+ (not actually broken, but now covered). - Switched the abort-on-unmount tests from setTimeout(20) sleeps to waitFor(() => expect(...)) to eliminate CI timing flakes. - Froze DEFAULT_PAGE_METADATA so callers that later mutate the returned PageMeta object can't corrupt the landing fallback for every subsequent request. - Clarified useAbortableFetchers effect-body capture comment. Phase 5 — Skeptic Review (7 discoveries, all remediated): - Gotcha #1: blog pages used path={BLOG_PATH} and bypassed the Phase-3 parity test because the regex only matched string literals. Extended extractSEOPath to resolve one level of `const X = "..."` indirection in the same file; parity test went from 28 → 49 cases and every blog post is now covered. - Gotcha #2: unknown paths (typos, stale links) returned 200 to bots with landing-page content and no robots directive, which Googlebot would index as soft-404s. crawlerMeta now injects `<meta name="robots" content="noindex" />` into the rewritten head when the path has no PAGE_METADATA entry. - Pattern #3: the partial-index predicate in shared/schema.ts (campaign_recipients_active_user_idx) was coupled to ACTIVE_RECIPIENT_STATUSES by convention, not by code. A new test file server/partial-index-invariants.test.ts reads the service constant and the schema source and asserts every status appears in the predicate, so adding a status to the constant without updating the index fails CI. - Gotcha #4: SPA fallback had no per-request rate limit, so a UA-spoofed flood against 1000s of paths would repeat the regex rewrite N times. server/static.ts now caches the rewritten HTML per baseUrl+path (Map, max 200 entries, cleared when full) so flood requests after the first land in cache. - Gotcha #5: useAbortableFetchers aborted in-flight mutations on React 18 StrictMode remount because the effect body didn't re-initialize. Added a mountedRef that flips true on mount and false on cleanup, mirroring the pattern already used in Dashboard.tsx. - Gotcha #6: trailing-slash paths (/pricing/, /blog/slug/) bypassed PAGE_METADATA and fell back to landing. crawlerMeta now normalizes the trailing slash (except for root) before lookup, so /pricing/ resolves to /pricing and emits the canonical URL without the slash. - Pattern #7: the regex rewrite replaces only the first occurrence, so a future duplicate meta tag would silently leak landing-page metadata. Added a test asserting client/index.html contains exactly one of each rewritten tag (title, canonical, og:*, twitter:*). Phase 6 — Bug Report: filed #446 for the Dashboard bulk-refresh AbortController gap (pre-existing from #438, out-of-scope for this PR). Tests: 2454 passing (was 2413), npm run check clean, build succeeds. https://claude.ai/code/session_04BM7
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces crawler-specific HTML metadata rewriting for social link unfurling, adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Server (static.ts)
participant Crawler as Crawler Detector
participant Meta as Metadata Lookup
participant Rewrite as HTML Rewriter
participant Response
Client->>Server: HTTP GET /{path}
Server->>Crawler: Check User-Agent
alt Is Crawler User Agent
Crawler-->>Server: true
Server->>Meta: getPageMetadata(path)
Meta-->>Server: PageMeta or DEFAULT
Server->>Rewrite: rewriteIndexHtmlForCrawler(template, path, baseUrl)
Rewrite->>Rewrite: Replace <title>, OG/Twitter tags, canonical
Rewrite->>Rewrite: Inject <meta name="robots" noindex> if unknown path
Rewrite-->>Server: Rewritten HTML
Server->>Response: 200 + rewritten HTML
else Not Crawler User Agent
Crawler-->>Server: false
Server->>Response: Send index.html (SPA)
end
Response-->>Client: HTML Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/src/hooks/use-monitors.ts (1)
221-235:⚠️ Potential issue | 🟠 MajorAbortError from unmount will surface as a destructive error toast on the next page.
When the user navigates away mid-check,
controller.abort()causesfetchto reject with aDOMException(AbortError). That rejection propagates out ofmutationFn→ React Query callsonError→ a destructive toast ("Error",err.message = "The operation was aborted."or similar) fires on whatever page the user just landed on. That's precisely the UX theuseAbortableFetchershook was introduced to avoid (#437), but the toast path undoes half the win.Suggest swallowing abort-caused errors in
onError(and filtering the thrown error inmutationFnso React Query sees a silent cancellation):🛡️ Proposed guard
onError: (err) => { + if (err?.name === "AbortError") return; toast({ title: "Error", description: err.message, variant: "destructive" }); },Or, inside the
finally/catchinmutationFn, re-throw only non-abort errors so the mutation settles silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/hooks/use-monitors.ts` around lines 221 - 235, The onError handler in use-monitors.ts is showing destructive toasts for AbortError caused by controller.abort(), so update both the mutationFn and the onError callback to ignore aborts: inside the mutation implementation (the function passed as mutationFn) catch DOMExceptions with name === "AbortError" (or use an isAbortError helper) and do not rethrow them so React Query treats them as silent cancellations; additionally, change the onError handler to check err.name === "AbortError" (or use the same helper) and return early without calling toast for aborts, while preserving toast behavior for other errors.client/index.html (1)
5-43: 🧹 Nitpick | 🔵 TrivialLGTM — viewport a11y fix and parallel font fetch both correct.
- Removing
maximum-scale=1restores pinch-zoom per WCAG 2.1 SC 1.4.4 (#444). ✅- Direct
<link rel="stylesheet">with matchingpreconnecthints (includingcrossoriginonfonts.gstatic.com) is the right pattern to parallelize font-CSS fetch againstindex.css(#443).display=swapkeeps first paint unblocked by font download.One small nit: the comment at Lines 34-39 says to keep this in sync with
client/src/index.csswhen changing fonts — consider adding a brief code comment inindex.csspointing back toindex.htmlas well so the coupling is visible from either side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/index.html` around lines 5 - 43, Add a reciprocal comment in client/src/index.css that points back to client/index.html and notes the font families and display strategy are kept in sync (e.g., mention the Outfit and Inter families and display=swap), so future font changes are visible from CSS as well as HTML; update the top of index.css (or where fonts are referenced) with this short note referencing index.html and the fonts to maintain the coupling between index.css and index.html.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/hooks/use-monitors.ts`:
- Around line 17-33: Remove the dead StrictMode defense: delete the mountedRef
declaration and its associated comment inside the effect (the const mountedRef =
useRef(true) and any references/commentary in the useEffect block), and leave
the controllersRef-based cleanup intact; no other code needs to change in
useCheckMonitor or useCheckMonitorSilent because they don't read mountedRef, so
remove the unused symbol to avoid misleading future readers.
In `@client/src/hooks/use-page-title.test.ts`:
- Around line 28-32: Add a unit test to explicitly cover the empty-string case
for usePageTitle so the current `!title` guard remains protected: in the test
file use-page-title.test.ts add a test like the existing undefined case but call
renderHook(() => usePageTitle("")); set document.title to "unchanged" before
rendering and assert expect(document.title).toBe("unchanged") after to ensure
empty string does not clobber the title. This references the usePageTitle hook
and mirrors the existing undefined test to prevent future regressions.
In `@client/src/index.css`:
- Around line 21-23: The stylelint false positives for font name casing affect
the CSS custom properties --font-sans, --font-serif, and --font-mono; fix by
adding an inline suppression comment immediately above those declarations (use
/* stylelint-disable-next-line value-keyword-case */) so the TitleCase vendor
font identifiers (e.g., BlinkMacSystemFont, Roboto, Georgia, SFMono-Regular) are
preserved, or alternatively relax the value-keyword-case rule in the stylelint
config scoped to font-family values if you prefer a config change.
In `@client/src/pages/page-metadata-parity.test.ts`:
- Around line 32-38: The current regex in extractSEOProp/extractSEOPath only
matches self-closing <SEOHead .../> and thus skips pages using <SEOHead
...>children</SEOHead>; update both functions to first locate the opening tag
(e.g. match /<SEOHead\b[\s\S]*?>/ to get the opening tag string) and then
extract the prop from that opening-tag substring (so attributes across lines or
non-self-closing tags are handled). Also add a defensive assertion in the test
that the number of pages returned by your extractor equals the number of files
that contain the literal string "<SEOHead" to fail loudly if extraction
regresses.
In `@server/crawlerMeta.test.ts`:
- Around line 215-218: The test currently reads client/index.html at collection
time via the clientIndex = fs.readFileSync(...) call, which can crash test
collection; move that synchronous file read into a beforeAll hook so it runs
during suite setup instead of module load. Locate the clientIndex variable and
the fs.readFileSync(...) usage in the test file (server/crawlerMeta.test.ts) and
change the code to initialize clientIndex inside a beforeAll(async () => { ...
}) (or beforeAll(() => { ... })) so any ENOENT or read error is reported as a
suite setup failure rather than a collection-time crash.
- Around line 99-102: The current test uses ogDescMatch =
out.match(/<meta\s+property="og:description"\s+content="([^"]*)"/) which makes
the subsequent not.toMatch check tautological; instead, capture the full meta
tag or inspect the raw output substring so you can assert that literal "
characters are replaced by ". Concretely, feed a PAGE_METADATA.description
that contains a literal double-quote, run the rewriter/escapeAttr path, then
assert the resulting output (or the full matched tag from out.match without
excluding quotes) contains '"' and does not contain a raw '"' inside the
content attribute; update the assertions around ogDescMatch/out and reference
escapeAttr and PAGE_METADATA in the test to validate proper escaping.
In `@server/crawlerMeta.ts`:
- Around line 93-97: The ogImage assignment’s predicate using
meta.image.startsWith("http") misclassifies protocol-relative URLs and can
falsely match nonstandard schemes; update the condition around meta.image in the
ogImage computation to detect absolute URLs correctly (e.g., treat strings
starting with '//' or matching a proper http/https scheme) before prefixing with
baseUrl; specifically, in the ogImage logic validate meta.image with a strict
check such as a regex for ^(https?:)?// or use URL parsing to decide if
meta.image is already absolute, otherwise prepend baseUrl.
- Around line 109-152: The series of out.replace(...) calls in crawlerMeta.ts
are order-sensitive and can silently no-op if attributes are reordered; update
each replacement to verify it actually changed the string (e.g., capture const
before = out, const after = before.replace(/.../, replacement); if (after ===
before) { in non-prod throw new Error(`Meta replace for ${name} failed`) } else
out = after) so failures surface loudly; apply this check for each replace that
updates title, meta description, canonical, og:* and twitter:* (look for the
out.replace calls and variables titleAttr, descAttr, urlA, typeA, titleA,
imageA) and ensure the check only throws/logs in non-production.
- Around line 154-162: The current literal /<head>/i replacement in
crawlerMeta.ts will miss <head> tags with attributes; update the replacement to
match the opening head tag with attributes (e.g., use a regex like
/<head\b[^>]*>/i) and inject the meta tag immediately after the matched opening
tag while preserving any attributes and spacing — modify the code path that
assigns to out (when !isKnown) to perform a replace with the new regex and
insertion that keeps the original match intact so attributes aren’t lost.
- Around line 87-92: The canonical/og:url concatenation currently uses raw
baseUrl which allows a trailing slash to produce double slashes (e.g.
https://site.com//path); update the function that builds canonicalUrl (the
function with parameters baseUrl and path in server/crawlerMeta.ts) to
defensively normalize baseUrl by removing any trailing slash before
concatenation (e.g., derive a sanitizedBaseUrl or normalizeBaseUrl and use
`${sanitizedBaseUrl}${normalized}`), and apply the same sanitized value when
emitting og:url; also add the suggested regression test using
rewriteIndexHtmlForCrawler to assert a trailing-slash baseUrl yields a single
slash in the canonical href and does not contain "//pricing".
In `@server/partial-index-invariants.test.ts`:
- Around line 54-63: The test name claims it checks all terminal statuses but
only asserts against two hardcoded ones; update the test to generate the
forbidden list from the full recipient status set minus
ACTIVE_RECIPIENT_STATUSES (e.g. derive forbidden = new
Set([...ALL_RECIPIENT_STATUSES].filter(s => !ACTIVE_RECIPIENT_STATUSES.has(s))))
and then assert predicate does not contain any `'${status}'` for each status in
forbidden, or alternatively rename the test to explicitly state it only checks
"bounced" and "complained"; locate the predicate via indexBlock![1] and adjust
the loop to iterate the computed forbidden set or change the test name
accordingly.
- Around line 39-41: The current regex extraction into indexBlock from
SCHEMA_SRC is brittle and can return null silently; update the test to
explicitly guard and fail fast: after computing indexBlock (the capture from
/activeUserCampaignIdx[\s\S]*?\.where\(sql`([^`]+)`\)/) check if (!indexBlock)
and throw a descriptive Error explaining that the index predicate could not be
found and suggesting to inspect shared/schema.ts (or alternatively move the
regex+capture into a helper function used by extractStatusList that throws the
same descriptive error), so subsequent uses of indexBlock or extractStatusList
fail with a clear message instead of a TypeError.
In `@server/services/campaignEmail.test.ts`:
- Around line 433-440: The negative assertion is too brittle; instead of
checking the serialized template string for a specific literal, fetch the
bound-parameter array from the first execute call and assert it contains the
four terminal statuses. Locate the mock invocation at
mockDbExecute.mock.calls[0] (the query object is mock.calls[0][0] and some
SQL-mocks put bound params as mock.calls[0][1] or under mock.calls[0][0].values)
and replace the expect(serialized).not.toContain(...) line with assertions that
the bound values array includes "sent","delivered","opened","clicked" (e.g.,
extract params = mockDbExecute.mock.calls[0][1] ||
mockDbExecute.mock.calls[0][0].values and assert params contains those four
strings).
In `@server/static.ts`:
- Around line 17-35: The comment is misleading: getCachedTemplate() lazily reads
index.html on first use rather than at startup; update the comment to reflect
lazy initialization or make it eager by reading into cachedTemplate at module
init (use indexPath and cachedTemplate) so failures occur at boot; specifically
either change the text around cachedTemplate/getCachedTemplate to say "lazy
cache on first request" or assign cachedTemplate = fs.readFileSync(indexPath,
'utf-8') immediately after indexPath is defined to perform an eager read and
surface errors at startup.
In `@shared/pageMetadata.ts`:
- Around line 28-148: PAGE_METADATA's per-path PageMeta objects are mutable and
must be frozen to prevent accidental runtime mutation; update the module so that
after constructing PAGE_METADATA each value is Object.freeze(...) (freeze each
PageMeta object) and then freeze the PAGE_METADATA map itself, keeping
DEFAULT_PAGE_METADATA as Object.freeze({ ...PAGE_METADATA["/"]! }) intact;
ensure callers like getPageMetadata still return the frozen references but
cannot mutate them.
In `@shared/schema.ts`:
- Around line 163-169: Confirm that the partial index typeAutomatedIdx is
actually used by the anti-join in resolveRecipients: run EXPLAIN (ANALYZE,
BUFFERS) for the resolveRecipients query (the join on campaigns c ON c.id =
cr.campaign_id with filter c.type = 'automated') against production-sized data
and verify the planner chooses an index scan on campaigns using the partial
predicate; if the plan still seq-scans or uses a hash/merge join (no index
benefit), change the indexing strategy (for example, create a non-partial index
on campaigns(id, type) or a separate index that supports the join predicate) or
adjust the query to be index-friendly, then re-run EXPLAIN to confirm the index
is used.
- Around line 193-199: Add a one-line comment next to activeUserCampaignIdx
explaining that the hardcoded predicate duplicates ACTIVE_RECIPIENT_STATUSES in
server/services/campaignEmail.ts and pointing maintainers to
server/partial-index-invariants.test.ts to verify both locations are updated if
statuses change; reference activeUserCampaignIdx, ACTIVE_RECIPIENT_STATUSES,
server/services/campaignEmail.ts, and server/partial-index-invariants.test.ts in
the comment so future editors know where to update and how to validate.
---
Outside diff comments:
In `@client/index.html`:
- Around line 5-43: Add a reciprocal comment in client/src/index.css that points
back to client/index.html and notes the font families and display strategy are
kept in sync (e.g., mention the Outfit and Inter families and display=swap), so
future font changes are visible from CSS as well as HTML; update the top of
index.css (or where fonts are referenced) with this short note referencing
index.html and the fonts to maintain the coupling between index.css and
index.html.
In `@client/src/hooks/use-monitors.ts`:
- Around line 221-235: The onError handler in use-monitors.ts is showing
destructive toasts for AbortError caused by controller.abort(), so update both
the mutationFn and the onError callback to ignore aborts: inside the mutation
implementation (the function passed as mutationFn) catch DOMExceptions with name
=== "AbortError" (or use an isAbortError helper) and do not rethrow them so
React Query treats them as silent cancellations; additionally, change the
onError handler to check err.name === "AbortError" (or use the same helper) and
return early without calling toast for aborts, while preserving toast behavior
for other errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27861377-d46a-4fef-a08d-142ecd777985
📒 Files selected for processing (25)
client/index.htmlclient/src/App.tsxclient/src/hooks/use-monitors.test.tsclient/src/hooks/use-monitors.tsclient/src/hooks/use-page-title.test.tsclient/src/hooks/use-page-title.tsclient/src/index.cssclient/src/pages/AdminCampaignDetail.tsxclient/src/pages/AdminCampaigns.tsxclient/src/pages/AdminErrors.tsxclient/src/pages/Dashboard.tsxclient/src/pages/ExtensionAuth.tsxclient/src/pages/LandingPage.tsxclient/src/pages/MonitorDetails.tsxclient/src/pages/page-metadata-parity.test.tsclient/src/pages/seo-integrity.test.tsserver/crawlerMeta.test.tsserver/crawlerMeta.tsserver/partial-index-invariants.test.tsserver/services/campaignEmail.test.tsserver/services/campaignEmail.tsserver/static.tsserver/vite.tsshared/pageMetadata.tsshared/schema.ts
Comment on lines
+21
to
+23
| --font-sans: 'Inter', ui-sans-serif, system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, sans-serif; | ||
| --font-serif: ui-serif, Georgia, Cambria, 'Times New Roman', Times, serif; | ||
| --font-mono: ui-monospace, SFMono-Regular, 'SF Mono', Menlo, Consolas, 'Liberation Mono', monospace; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stylelint value-keyword-case complaints here are false positives — fonts are proper nouns.
BlinkMacSystemFont, Roboto, Georgia, Cambria, Times, SFMono-Regular, Menlo, Consolas are font family identifiers whose canonical spelling is camel/title-cased. Lowercasing them as Stylelint suggests would still render, but it conflicts with vendor documentation (Apple's BlinkMacSystemFont, MDN's system font stack examples) and with the existing 'Outfit'/'Inter' casing in this same file. Recommend either adding an inline stylelint-disable-next-line value-keyword-case or relaxing the rule in Stylelint config for font-family values.
Fonts/variables themselves look good and correctly fix issue #442 (so font-mono actually renders monospaced for API keys / stack traces where O/0 ambiguity matters).
🧰 Tools
🪛 Stylelint (17.7.0)
[error] 21-21: Expected "BlinkMacSystemFont" to be "blinkmacsystemfont" (value-keyword-case)
(value-keyword-case)
[error] 21-21: Expected "Roboto" to be "roboto" (value-keyword-case)
(value-keyword-case)
[error] 22-22: Expected "Georgia" to be "georgia" (value-keyword-case)
(value-keyword-case)
[error] 22-22: Expected "Cambria" to be "cambria" (value-keyword-case)
(value-keyword-case)
[error] 22-22: Expected "Times" to be "times" (value-keyword-case)
(value-keyword-case)
[error] 23-23: Expected "SFMono-Regular" to be "sfmono-regular" (value-keyword-case)
(value-keyword-case)
[error] 23-23: Expected "Menlo" to be "menlo" (value-keyword-case)
(value-keyword-case)
[error] 23-23: Expected "Consolas" to be "consolas" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/index.css` around lines 21 - 23, The stylelint false positives for
font name casing affect the CSS custom properties --font-sans, --font-serif, and
--font-mono; fix by adding an inline suppression comment immediately above those
declarations (use /* stylelint-disable-next-line value-keyword-case */) so the
TitleCase vendor font identifiers (e.g., BlinkMacSystemFont, Roboto, Georgia,
SFMono-Regular) are preserved, or alternatively relax the value-keyword-case
rule in the stylelint config scoped to font-family values if you prefer a config
change.
Comment on lines
+99
to
+102
| const ogDescMatch = out.match(/<meta\s+property="og:description"\s+content="([^"]*)"/); | ||
| expect(ogDescMatch).not.toBeNull(); | ||
| expect(ogDescMatch?.[1]).not.toMatch(/[^&]".*"/); // no unescaped quote pairs | ||
| }); |
There was a problem hiding this comment.
The "no unescaped quote" assertion is effectively a tautology.
ogDescMatch captures group 1 with ([^"]*), which by construction cannot contain any " character. The follow-up expect(ogDescMatch?.[1]).not.toMatch(/[^&]".*"/) therefore can never fail — it's asserting against a character class the prior regex already excluded. If the goal is to prove escapeAttr turns " into ", assert against the raw output substring (before regex extraction) or feed a PAGE_METADATA description containing a literal " through the rewriter and verify " appears and " does not.
🧪 Suggested stronger assertion
- const ogDescMatch = out.match(/<meta\s+property="og:description"\s+content="([^"]*)"/);
- expect(ogDescMatch).not.toBeNull();
- expect(ogDescMatch?.[1]).not.toMatch(/[^&]".*"/); // no unescaped quote pairs
+ // Feed a description containing HTML-hostile chars through the rewriter
+ // and assert the attribute value is properly escaped.
+ const hostile = '"><script>alert(1)</script>';
+ const escaped = hostile
+ .replace(/&/g, "&")
+ .replace(/"/g, """)
+ .replace(/</g, "<")
+ .replace(/>/g, ">");
+ // (Would require injecting a fixture meta; alternatively verify that the
+ // captured group contains no bare `"` or `<` characters.)
+ const ogDescMatch = out.match(/<meta\s+property="og:description"\s+content="([^"]*)"/);
+ expect(ogDescMatch).not.toBeNull();
+ expect(ogDescMatch?.[1]).not.toContain("<");
+ expect(ogDescMatch?.[1]).not.toContain(">");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/crawlerMeta.test.ts` around lines 99 - 102, The current test uses
ogDescMatch = out.match(/<meta\s+property="og:description"\s+content="([^"]*)"/)
which makes the subsequent not.toMatch check tautological; instead, capture the
full meta tag or inspect the raw output substring so you can assert that literal
" characters are replaced by ". Concretely, feed a
PAGE_METADATA.description that contains a literal double-quote, run the
rewriter/escapeAttr path, then assert the resulting output (or the full matched
tag from out.match without excluding quotes) contains '"' and does not
contain a raw '"' inside the content attribute; update the assertions around
ogDescMatch/out and reference escapeAttr and PAGE_METADATA in the test to
validate proper escaping.
Comment on lines
+433
to
+440
| // First execute call is the pre-transaction COUNT with the terminal IN list | ||
| const serialized = JSON.stringify(mockDbExecute.mock.calls[0][0]); | ||
| // All four terminal statuses are injected as bound values (not hardcoded literals) | ||
| for (const status of ["sent", "delivered", "opened", "clicked"]) { | ||
| expect(serialized).toContain(status); | ||
| } | ||
| // And the literal old-style inline list should no longer appear | ||
| expect(serialized).not.toContain("'sent', 'delivered', 'opened', 'clicked'"); |
There was a problem hiding this comment.
Weak negative assertion — consider tightening.
expect(serialized).not.toContain("'sent', 'delivered', 'opened', 'clicked'") only catches a literally concatenated legacy string in that exact order with that exact spacing. Since the sql mock serializes template strings as JSON, a regression that reintroduced a hardcoded IN-list with different whitespace (e.g., 'sent','delivered',...) would slip past. A stronger assertion would verify the statuses appear in the values array of the bound parameters rather than the static template strings. Low-priority since the positive assertion already confirms presence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/services/campaignEmail.test.ts` around lines 433 - 440, The negative
assertion is too brittle; instead of checking the serialized template string for
a specific literal, fetch the bound-parameter array from the first execute call
and assert it contains the four terminal statuses. Locate the mock invocation at
mockDbExecute.mock.calls[0] (the query object is mock.calls[0][0] and some
SQL-mocks put bound params as mock.calls[0][1] or under mock.calls[0][0].values)
and replace the expect(serialized).not.toContain(...) line with assertions that
the bound values array includes "sent","delivered","opened","clicked" (e.g.,
extract params = mockDbExecute.mock.calls[0][1] ||
mockDbExecute.mock.calls[0][0].values and assert params contains those four
strings).
Comment on lines
+163
to
+169
| // Partial index on type='automated' so the welcome-exclusion anti-join in | ||
| // resolveRecipients can satisfy `c.type = 'automated'` via an index scan | ||
| // instead of a sequential scan over all campaigns. Partial because the | ||
| // automated set is small and stable. See GitHub issue #433. | ||
| typeAutomatedIdx: index("campaigns_type_automated_idx") | ||
| .on(table.id) | ||
| .where(sql`type = 'automated'`), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify the partial index is actually used by the anti-join.
The anti-join in resolveRecipients joins campaigns c ON c.id = cr.campaign_id and filters c.type = 'automated'. A partial index on campaigns(id) WHERE type = 'automated' will only help if the planner picks an index-only scan for the id-lookup-with-predicate path; for a hash/merge join over the small automated set, the planner may just seq-scan campaigns (which is already small). Worth an EXPLAIN (ANALYZE, BUFFERS) against prod-sized data to confirm the new index is chosen — otherwise this is index maintenance overhead with no payoff.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shared/schema.ts` around lines 163 - 169, Confirm that the partial index
typeAutomatedIdx is actually used by the anti-join in resolveRecipients: run
EXPLAIN (ANALYZE, BUFFERS) for the resolveRecipients query (the join on
campaigns c ON c.id = cr.campaign_id with filter c.type = 'automated') against
production-sized data and verify the planner chooses an index scan on campaigns
using the partial predicate; if the plan still seq-scans or uses a hash/merge
join (no index benefit), change the indexing strategy (for example, create a
non-partial index on campaigns(id, type) or a separate index that supports the
join predicate) or adjust the query to be index-friendly, then re-run EXPLAIN to
confirm the index is used.
Comment on lines
+193
to
+199
| // Partial composite index scoped to the active-status set used by the | ||
| // welcome-exclusion anti-join in resolveRecipients. Lets the planner answer | ||
| // the per-user probe from just the active subset instead of every historical | ||
| // recipient row for the user. See GitHub issue #436. | ||
| activeUserCampaignIdx: index("campaign_recipients_active_user_idx") | ||
| .on(table.userId, table.campaignId) | ||
| .where(sql`status IN ('pending', 'sent', 'delivered', 'opened', 'clicked')`), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded status list in partial-index predicate — acceptable, but duplication risk is real.
The predicate status IN ('pending', 'sent', 'delivered', 'opened', 'clicked') necessarily duplicates ACTIVE_RECIPIENT_STATUSES from server/services/campaignEmail.ts because Drizzle migrations need a literal SQL fragment. The new server/partial-index-invariants.test.ts guards the drift, which is the right mitigation. Worth a one-line comment here pointing at that test so future editors know to update both sites and rerun it.
📝 Suggested inline note
// Partial composite index scoped to the active-status set used by the
// welcome-exclusion anti-join in resolveRecipients. Lets the planner answer
// the per-user probe from just the active subset instead of every historical
// recipient row for the user. See GitHub issue `#436`.
+ // NOTE: the status list below must stay in lock-step with ACTIVE_RECIPIENT_STATUSES
+ // in server/services/campaignEmail.ts. The invariant is enforced by
+ // server/partial-index-invariants.test.ts.
activeUserCampaignIdx: index("campaign_recipients_active_user_idx")📝 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
| // Partial composite index scoped to the active-status set used by the | |
| // welcome-exclusion anti-join in resolveRecipients. Lets the planner answer | |
| // the per-user probe from just the active subset instead of every historical | |
| // recipient row for the user. See GitHub issue #436. | |
| activeUserCampaignIdx: index("campaign_recipients_active_user_idx") | |
| .on(table.userId, table.campaignId) | |
| .where(sql`status IN ('pending', 'sent', 'delivered', 'opened', 'clicked')`), | |
| // Partial composite index scoped to the active-status set used by the | |
| // welcome-exclusion anti-join in resolveRecipients. Lets the planner answer | |
| // the per-user probe from just the active subset instead of every historical | |
| // recipient row for the user. See GitHub issue `#436`. | |
| // NOTE: the status list below must stay in lock-step with ACTIVE_RECIPIENT_STATUSES | |
| // in server/services/campaignEmail.ts. The invariant is enforced by | |
| // server/partial-index-invariants.test.ts. | |
| activeUserCampaignIdx: index("campaign_recipients_active_user_idx") | |
| .on(table.userId, table.campaignId) | |
| .where(sql`status IN ('pending', 'sent', 'delivered', 'opened', 'clicked')`), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shared/schema.ts` around lines 193 - 199, Add a one-line comment next to
activeUserCampaignIdx explaining that the hardcoded predicate duplicates
ACTIVE_RECIPIENT_STATUSES in server/services/campaignEmail.ts and pointing
maintainers to server/partial-index-invariants.test.ts to verify both locations
are updated if statuses change; reference activeUserCampaignIdx,
ACTIVE_RECIPIENT_STATUSES, server/services/campaignEmail.ts, and
server/partial-index-invariants.test.ts in the comment so future editors know
where to update and how to validate.
Applies 11 of 17 CodeRabbit findings. Skipped 6 that were style
false-positives (stylelint on font-family proper nouns), informational
(partial-index-usage verification), or duplicated existing coverage
(attribute-order lock-in already has real-template test, weak negative
assertion has a stronger positive companion).
Applied:
- client/src/hooks/use-monitors.ts: removed dead mountedRef — it was
declared and written but never read, so the StrictMode defense the
comment promised was not actually implemented. CodeRabbit correctly
flagged the scenario as unreachable today (mutations require user
events, which can't fire between StrictMode's mount → cleanup → mount).
Hook is now strictly the Set<AbortController> cleanup.
- client/src/hooks/use-page-title.test.ts: added an explicit
empty-string test so a future tightening of the `!title` guard to
`=== undefined` can't silently clobber document.title with "".
- client/src/pages/page-metadata-parity.test.ts: SEO_BLOCK_RE now
matches both <SEOHead .../> and <SEOHead ...> forms so a page with
SEOHead children doesn't silently bypass parity. Added a "no silent
bypass" assertion comparing publicPageFiles.length against every
PublicNav page that contains `<SEOHead`, excluding LandingPage
(intentionally dynamic — documented in INTENTIONALLY_DYNAMIC_PAGES).
- server/crawlerMeta.test.ts: replaced the tautology assertion (the
regex's own capture class already excluded `"`, so the follow-up
`not.toMatch(/[^&]".*"/)` could never fail) with direct checks that
the captured attribute and title text contain neither `<` nor `>`.
Also added a regression test asserting baseUrl with a trailing slash
doesn't produce `//pricing`.
- server/crawlerMeta.ts:
* baseUrl normalization: strip trailing slash before concatenation so
`getAppUrl()` (or future callers) passing `https://host/` no longer
yields a broken `https://host//pricing` canonical URL.
* isAbsolute(): changed `meta.image.startsWith("http")` to a
`/^https?:\/\//i` test plus a `u.startsWith("//")` branch for
protocol-relative URLs. Old check accepted `httpfoo:` and treated
`//cdn.example.com/x.png` as site-relative.
* <head> noindex injection: regex now tolerates attributes on the
head tag (`<head lang="en">`, `<head prefix="og: …">`) so adding
one doesn't silently turn off noindex for unknown paths.
- server/partial-index-invariants.test.ts: refactored indexBlock
extraction into extractActiveUserIdxPredicate() that throws a
descriptive error if the shared/schema.ts regex fails to match, so a
formatting change there surfaces as an actionable test failure
instead of a TypeError on indexBlock![1]. Also renamed the test from
the overly broad "predicate does not reference any terminal status
outside ACTIVE_RECIPIENT_STATUSES" to the specific "predicate does
not reference known non-active recipient statuses (bounced,
complained)" that matches the narrow check it actually performs.
- server/static.ts: corrected stale comment — the template is lazily
cached on first crawler hit, not read at startup. Clarified the
trade-off (fail-fast on first unfurl rather than at boot).
- shared/pageMetadata.ts: Object.freeze every PAGE_METADATA entry, not
just DEFAULT_PAGE_METADATA. getPageMetadata returns references
directly — a consumer that mutated a returned PageMeta would have
silently corrupted the map for every subsequent crawler request.
Tests: 2456 passing (up from 2454), npm run check clean.
https://claude.ai/code/session_04BM7
This was referenced Apr 19, 2026
Closed
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundles fixes for 11 open bug issues (#433–#444) and a full magicwand pipeline pass on top (tests, security, architecture, code review, skeptic review). All 2454 tests pass,
npm run checkclean, production build succeeds.Issues #428, #429, #431, #432 were already resolved in commit ed68bd2 on main — this branch does not revisit them.
Changes
Campaign / DB hardening
cancelCampaignandreconcileCampaignCountersraw SQL now build terminal-status IN-lists fromTERMINAL_RECIPIENT_STATUSESviasql.join, eliminating the dual-source-of-truth drift againstresolveRecipients.campaigns.typecolumn #433: Added partial indexcampaigns_type_automated_idx ON campaigns(id) WHERE type = 'automated'so the welcome-exclusion anti-join probes via index scan rather than seq scan.campaign_recipients_active_user_idx ON (user_id, campaign_id) WHERE status IN (active-set)so the per-user probe hits only the active-status subset.Client / UX
handleRefreshnow inspectsrefetch()'serrorfield and surfaces a destructive toast for real network/HTTP failures instead of bucketing them as "No monitors".useAbortableFetchershook — tracks aSet<AbortController>inuseRefand aborts all in-flight fetches on component unmount. Applied touseCheckMonitoranduseCheckMonitorSilentso browserless/Resend quota isn't burnt on orphaned work. StrictMode-safe viamountedRefre-init.usePageTitlehook setsdocument.titleon authenticated pages (Dashboard, MonitorDetails, AdminErrors, AdminCampaigns, AdminCampaignDetail, ExtensionAuth) so browser tab labels don't stay stuck on the previous public page.SEO / head
LandingPageaccepts an optionalpathprop;ProtectedRoutepasseswindow.location.pathnamewhen rendering it as the unauthed fallback on/dashboardor/monitors/:id, so canonical / og:url / JSON-LD@idreflect the actual URL instead of lying about it being/.server/crawlerMeta.ts+shared/pageMetadata.ts) rewritesindex.html<head>for known bot UAs (Facebook, X, Slack, LinkedIn, Discord, Google, Bing, etc.) with per-route title/description/canonical/og/twitter tags. Hooked into both the prod static handler (server/static.ts) and the dev Vite middleware (server/vite.ts). Template cached at module scope; rewrite cached per path (bounded, max 200 entries). Host header is NOT reflected intobaseUrl— uses pinnedgetAppUrl()fromREPLIT_DOMAINSto prevent SEO poisoning. Unknown paths inject<meta name="robots" content="noindex">to prevent soft-404 indexation. Trailing-slash paths normalize to the canonical form.font-monoTailwind class maps to an undefined CSS variable (--font-mono) #442: Defined--font-sans,--font-serif,--font-monoin:rootso the Tailwindfont-monoclass actually resolves to a monospace stack instead of inheriting Inter — matters for API keys, CSS selectors, error codes, stack traces.@importof Google Fonts in index.css #443: Moved Google Fonts from CSS@import(render-blocking, serial) to<link rel="stylesheet">inclient/index.html, parallelizing the font fetch withindex.css.maximum-scale=1, disabling pinch-to-zoom (WCAG 1.4.4 violation) #444: Droppedmaximum-scale=1from the viewport meta to restore pinch-zoom on mobile (WCAG 2.1 SC 1.4.4 Level AA compliance).Test coverage
client/src/hooks/use-page-title.test.ts— 5 tests (set-on-mount, restore-on-unmount, undefined no-op, prop change, nested mount).client/src/hooks/use-monitors.test.ts— 2 tests assertinguseCheckMonitor/useCheckMonitorSilentabort in-flight fetches on unmount.server/services/campaignEmail.test.ts— 2 tests assertingcancelCampaignandreconcileCampaignCountersbind terminal statuses from the constant.server/crawlerMeta.test.ts— 29 tests (bot-UA detection, real-template integration, noindex for unknown paths, trailing-slash normalization, multi-line attribute tolerance, exact-one-of-each-tag invariant).client/src/pages/page-metadata-parity.test.ts— 49 tests enforcingPAGE_METADATA ↔ <SEOHead>agreement; resolves one-levelconst BLOG_PATH = "..."indirection so all blog posts are covered.server/partial-index-invariants.test.ts— 4 tests asserting thecampaign_recipients_active_user_idxpredicate contains every status inACTIVE_RECIPIENT_STATUSES.Magicwand pipeline findings
req.get("host"), now uses pinnedgetAppUrl()) and added a per-path rewrite cache to cap UA-spoofed flood amplification.PAGE_METADATA ↔ SEOHeadparity test and a real-template crawler test so the two sources-of-truth can't silently drift.How to test
https://<your-deploy>/blog/slack-webpage-change-alertsinto a Slack DM or Facebook Sharing Debugger. Preview should show the blog post title/description, not the landing page./dashboardor an error code on/admin/errors. Computedfont-familyshould start withui-monospace.GET /api/monitors, click the refresh button, confirm you see a destructive "Refresh failed" toast (not "No monitors")./api/monitors/:id/checkrequests started from the hook-based paths should show as(cancelled)./pricing→/dashboard→/monitors/X→/admin/errors. Each should update the tab label to something descriptive./dashboard. View source — canonical link and og:url should point to/dashboard, not/.curl -H "User-Agent: Googlebot" https://<deploy>/no-such-route | grep robots— expect<meta name="robots" content="noindex">.https://claude.ai/code/session_04BM7
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Performance
UX Improvements