Restore admin Event Log; drop warning-level to silence Browserless noise#469
Merged
Conversation
This reverts commit 1593135.
User intent on #461 was to silence the Browserless warning-noise flood only; reverting #461 brought the Event Log back but also re-enabled warning-level writes that produced the original noise. This drops ErrorLogger.warning() entirely and rewrites every call site to console.warn with the same `[source] message` prefix, so: - Transient/noisy warnings still surface in server logs for ops triage - The admin Event Log only shows error- and info-level events - Dedup/aggregation still applies to the remaining error+info rows Test mocks updated to spy on console.warn instead of ErrorLogger.warning.
Adds two assertions to logger.test.ts: - ErrorLogger.warning is undefined (prevents accidental reintroduction) - ErrorLogger.info routes only to console.log, never console.warn/.error
…API filter Phase 2 security findings from /magicwand review: H1/H2 (high): Replace `url: monitor.url` with `hostname: safeHostname(monitor.url)` at every log site on the branch. Monitor URLs are user-supplied and can carry basic-auth credentials (`https://user:pw@host`) or query-string secrets (`?api_key=…`) that the ErrorLogger's sanitizeString regex list does not catch. This re-applies PR #461's CodeRabbit hardening on top of the Event Log revert. Sites patched: - server/services/scraper.ts (5 call sites — error, console.warn, circuit-breaker) - server/services/scheduler.ts (1 — scheduled-check failed) - server/services/email.ts (1 — notification email failed) Also drops `rawBrowserlessMsg` from the site-specific console.warn at scraper.ts:1283 — upstream errors can echo back the requested URL with credentials. classifiedReason already captures enough triage detail. Updates the stale comment claiming logger sanitization applies. M1 (medium): API `level` filter in GET /api/admin/error-logs still accepted `"warning"` after commit 3541155 removed the warning-level write path. Narrows the allowlist (and batch-delete Zod enum) to ["error", "info"] and removes the "Warnings" dropdown option in AdminErrors.tsx. safeHostname helper inlined in email.ts to avoid pulling the full monitorValidation→storage→schema chain into email.test.ts's drizzle-orm mock. All 2475 tests pass; npm run check clean.
Phase 3 architecture findings:
resendTracker: two direct `db.insert(errorLogs).values({level:"warning",...})`
sites bypassed ErrorLogger entirely — producing new warning rows despite the
branch's stated "no more warning writes" invariant. Threshold alerts (80/90/95%
Resend usage) are legitimately triage-worthy, so route them through
ErrorLogger.info instead (preserves admin visibility, sanitization, and
dedup-upsert). The in-DB 6-hour cooldown SELECT is retained for rate-limiting.
safeHostname consolidation: extracted the hostname helper to dependency-free
server/utils/urlUtils.ts. scraper.ts and email.ts dropped their local copies;
scheduler.ts and monitorValidation.ts updated to import from the new location
(monitorValidation re-exports for back-compat with any external callers).
Doc and test cleanup:
- shared/schema.ts:93 level comment now flags 'warning' as historical-only
- Removed twelve dead `warning: vi.fn()` mocks from route test files —
they masked that the ErrorLogger API no longer exposes .warning()
All 2475 tests pass; npm run check clean.
…fixture Phase 4 code-review minor findings: - adminErrorsUtils.test.ts: swap stale `levelFilter: "warning"` fixture for "info" so the test mirrors real UI traffic (the dropdown no longer offers "warning" and the server Zod schema would 400 it). - monitorValidation.ts: drop the `safeHostname` re-export; routes.ts, routes/extension.ts, routes/v1.ts now import directly from ../utils/urlUtils. extension.test.ts mock moved accordingly. One import path per symbol. All 2475 tests pass; npm run check clean.
…atch
Phase 5 skeptic discoveries addressed:
browserlessTracker (H1/H2): Same direct-insert pattern resendTracker had —
raw db.insert(errorLogs).values({level:"info",...}) bypassed the dedup
upsert. After the 6h cooldown elapsed on an unresolved alert row, the
next INSERT would hit the partial unique index and throw, silently
swallowing the alert (no email sent during the threshold event).
Converted to ErrorLogger.info, matching the resendTracker fix.
scraper.ts transient console.warn paths (M1/M2): Two paths on this
branch use console.warn for transient failures. console.warn bypasses
ErrorLogger.sanitizeString/sanitizeContext entirely:
- Line ~1460 (save-failure retry): was logging extractedValue/previousValue
which contain user-scraped page content — could include API tokens,
PII, or internal URLs. Moved those fields to the non-transient
ErrorLogger.error branch only; transient stdout log now contains
only triage metadata. Test updated to assert the redaction.
- Line ~1590 (outer-catch transient): was interpolating raw error.message
into the stdout line. undici/fetch errors echo the full requested URL
(userinfo creds, ?api_key=... query). Now uses the classified
logContext only; the ErrorLogger.error branch keeps the full message
since it runs through the sanitizer.
Count endpoint / UI dropdown mismatch (M4): The nav badge's count
endpoint (GET /api/admin/error-logs/count) counted all unresolved rows
regardless of level, while the UI filter dropdown only offers
"error"/"info" after the warning-level narrowing. Legacy warning rows
inflated the badge but were unreachable from the filter — admin would
see "7 unresolved" then find 4 after clicking through. Narrowed count
query with inArray(level, ["error","info"]) so badge and filter agree.
Browserless-outage stdout flooding (M5): The
"Browserless service unavailable" console.warn used to dedup at DB
level via ErrorLogger.warning's partial-index upsert. After the
warning-level removal, every monitor check during an outage emits a
fresh stdout line — N monitors x M replicas. Added a module-level
per-process throttle (60s cooldown per message key) for the two
infra-wide warnings ("service unavailable" and "circuit breaker open").
Exported _resetInfraWarnThrottleForTests hook; scraper.test.ts
beforeEach resets it so tests observe the first emit deterministically.
Verified #462 fixes survived the revert: SCHEMA_NOT_READY gate,
activeCheckWaiters, claimed_at / claimDeliveryForRetry /
recoverStalledDeliveries all present.
Deferred to Phase 6 bug report (pre-existing / not regressions):
- safeHostname "unknown" fallback ambiguity
- errorLogs.level has no DB CHECK constraint
- Dual warning/info rows during deploy (operator migration note for
PR body: legacy resend_alert_* warning rows should be resolved or
deleted at deploy to avoid apparent duplicate alerts)
All 2475 tests pass; npm run check clean.
… failure Phase 6 bug-reporter reclassified the Phase-5 "unknown" fallback as in-scope (urlUtils.ts is new code on this branch). A monitor URL of http://unknown/path would collide with the fallback, making "unknown" ambiguous in triage. Swap to `<invalid-url>` — angle brackets are not valid in a real hostname, so the sentinel is guaranteed distinguishable from parsed hostnames.
|
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 45 minutes and 56 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 (1)
📝 WalkthroughWalkthroughThis PR introduces a centralized error logging system with database persistence and a corresponding admin UI. It adds an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: The PR spans 60+ files with dense logic across multiple domains (client UI, server routes, database schema, services, and tests). While much of the integration work follows repetitive patterns (mocking logger in tests, adding ErrorLogger calls to existing services), the core components demand careful scrutiny: the AdminErrors page logic (filtering, selection, batch operations with undo), the ErrorLogger deduplication and redaction mechanisms, the admin API authorization and soft-delete semantics, and the transactional Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
New-code coverage gap noted during review: server/utils/urlUtils.ts was added on this branch but had no direct test. Asserts: - well-formed URL → hostname - userinfo credentials stripped from output - query-string secrets not reflected in hostname - parse failures return the `<invalid-url>` sentinel (guard against regression of the previous "unknown" fallback) - real host named `unknown` round-trips cleanly (collision guard)
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
Reverts #461 to bring back the admin Event Log feature (the "admin panel menu entry" that disappeared), then removes
ErrorLogger.warning()entirely so the Browserless infra-noise that motivated #461 never reaches the Event Log in the first place. Former warning call sites now emit viaconsole.warn(stdout only), while error- and info-level entries continue to flow throughErrorLoggerinto the admin UI.The revert replays #461's CodeRabbit URL-redaction hardening on top, consolidates
safeHostnameintoserver/utils/urlUtils.ts, and picks up additional hardening identified during the review pipeline (stdout throttling, admin badge/filter parity, console-bypass redaction).Changes
Event Log restored (revert of #461 —
b36aecf)/admin/errorspage,/api/admin/error-logs*endpoints (count / list / batch-delete / restore / finalize)DashboardNavbadge for Power tier showing unresolved error+info countErrorLoggerservice,error_logstable,ensureErrorLogColumnsbootstrap, partial unique dedup indexPricing.tsxandUpgradeDialog.tsxWarning-level removed (
3541155,c7bb327)ErrorLogger.warning()method deleted;LogLevel = "error" | "info"console.warn([source] message, context)— stdout only, no DB writelogger.test.tsassertErrorLogger.warningis undefined andinforoutes toconsole.logwarning: vi.fn()mocks removed from route test filesSecurity hardening (
9491536,efcd314)url: monitor.url→hostname: safeHostname(monitor.url)at every log site on this branch (scraper / scheduler / email). Monitor URLs can carryhttps://user:pw@hostor?api_key=…secrets that the DSN/Bearer/provider-key regex list does not catch.rawBrowserlessMsgfrom the site-specific Browserless console.warn (upstream errors can echo back the requested URL)levelallowlist + batch-delete Zod enum to["error", "info"]; removed the "Warnings" dropdown option inAdminErrors.tsxresendTrackerdirectlevel: "warning"inserts converted toErrorLogger.info(preserves admin visibility + dedup upsert)safeHostnameconsolidated intoserver/utils/urlUtils.ts;scraper.ts,scheduler.ts,email.ts,routes.ts,routes/extension.ts,routes/v1.tsall import from the shared helperSkeptic-phase fixes (
e48eb4a,2b757b6)browserlessTracker.ts:111direct insert →ErrorLogger.info(would otherwise throwduplicate keyon partial index after 6h cooldown, silently swallowing threshold alerts)console.warnno longer includesextractedValue/previousValue(user-scraped page content — can carry API tokens, PII, internal URLs). Non-transientErrorLogger.errorpath keeps them because it runssanitizeContext.console.warnno longer interpolates rawerror.message(undici/fetch errors echo full URL with credentials)/api/admin/error-logs/count) narrowed toinArray(level, ["error","info"])so the badge matches what the filter dropdown can showconsole.warnsites to prevent stdout flood during infra outagessafeHostnameparse-failure fallback changed from literal"unknown"→"<invalid-url>"so triage can distinguish a malformed URL from a real host namedunknownHow to test
/admin/errors— the filter dropdown should offerAll levels / Errors / Infoonly (no "Warnings" option).BROWSERLESS_TOKEN=badand run a monitor check). Observe stdout: expect[scraper] Browserless service unavailable …lines but at most one per 60s per replica. Confirm no new rows land inerror_logsundersource='scraper', level='warning'.url = "https://example.com?api_key=SECRET123". Force a scheduled-check failure. Confirmerror_logs.contextshowshostname: "example.com"(not the raw URL) and thatSECRET123does not appear in Replit logs or in the Event Log UI.level='info', source='resend', message='resend_alert_95'row lands inerror_logs(notlevel='warning'); subsequent checks within 6h don't duplicate it.npm run check && npm run test— expect 2475 passing across 100 files.Deploy runbook
Legacy
level='warning'rows from pre-deprecation writes (especiallysource='resend', message='resend_alert_*') remain in the DB and render under the "All levels" filter with a yellow icon. They coexist with newlevel='info'rows for the same alert key (different partial-index dedup buckets). Optional one-shot cleanup to avoid apparent duplicate rows in the admin UI:Out-of-scope follow-ups
Filed during Phase 6 bug triage — not required for this PR:
asc(id).limit(500)global scan can skip caller's rowsPipeline
/magicwandexecuted clean: 1 critical + 3 high + 3 medium + 2 low findings fixed in-branch (15 resolved issues across phases 2/3/4/5); 6 pre-existing issues filed as GitHub Issues. All 2475 tests pass;npm run checkclean.Generated by Claude Code
Summary by CodeRabbit
New Features
Updates