Add send logs#648
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a support log submission feature, allowing users to scrub and upload their server logs to a private repository. It adds the SendLogsDialog component, PII scrubbing utilities, Cloudflare Worker integration, and comprehensive tests. The code review highlights several critical issues and improvement opportunities: the IPv6 validation regex incorrectly matches standard timestamps and fails on compressed addresses; potential JSON parsing failures in sendLogs could leave the UI stuck; navigator.clipboard needs a guard to prevent crashes in non-secure contexts; and passing a date generator prop would improve component purity and testability.
| function isIpv6(value: string): boolean { | ||
| if (!value.includes(":")) return false; | ||
|
|
||
| const compressedGroups = value.split("::"); | ||
| if (compressedGroups.length > 2) return false; | ||
|
|
||
| const groups = value.split(":"); | ||
| if (groups.length < 3 || groups.length > 8) return false; | ||
|
|
||
| return groups.every((group) => group === "" || /^[0-9a-fA-F]{1,4}$/.test(group)); | ||
| } |
There was a problem hiding this comment.
The current isIpv6 implementation incorrectly identifies standard HH:MM:SS timestamps (e.g., 12:34:56) as valid IPv6 addresses because they contain 3 groups of valid hex characters separated by colons. This leads to timestamps in the logs being erroneously replaced with [ip]. To fix this, we should enforce that an IPv6 address without a double colon (::) must have exactly 8 groups.
| function isIpv6(value: string): boolean { | |
| if (!value.includes(":")) return false; | |
| const compressedGroups = value.split("::"); | |
| if (compressedGroups.length > 2) return false; | |
| const groups = value.split(":"); | |
| if (groups.length < 3 || groups.length > 8) return false; | |
| return groups.every((group) => group === "" || /^[0-9a-fA-F]{1,4}$/.test(group)); | |
| } | |
| function isIpv6(value: string): boolean { | |
| if (!value.includes(":")) return false; | |
| const compressedGroups = value.split("::"); | |
| if (compressedGroups.length > 2) return false; | |
| const groups = value.split(":"); | |
| if (groups.length < 3 || groups.length > 8) return false; | |
| // If not compressed, it must have exactly 8 groups to be a valid IPv6 address | |
| if (compressedGroups.length === 1 && groups.length !== 8) return false; | |
| return groups.every((group) => group === "" || /^[0-9a-fA-F]{1,4}$/.test(group)); | |
| } |
| export async function sendLogs(payload: SendLogsPayload): Promise<SendLogsResult> { | ||
| if (SUPPORT_WORKER_URL.includes("REPLACE_ME")) { | ||
| return { | ||
| ok: false, | ||
| status: 0, | ||
| message: "Log upload is not configured for this build.", | ||
| }; | ||
| } | ||
|
|
||
| let response: Response; | ||
| try { | ||
| response = await fetch(SUPPORT_WORKER_URL, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(payload), | ||
| }); | ||
| } catch (err) { | ||
| return { | ||
| ok: false, | ||
| status: 0, | ||
| message: err instanceof Error ? err.message : "Network error — check your connection.", | ||
| }; | ||
| } | ||
|
|
||
| if (response.ok) { | ||
| const data = (await response.json()) as { code: string; issueNumber: number }; | ||
| return { ok: true, code: data.code, issueNumber: data.issueNumber }; | ||
| } | ||
|
|
||
| const errorMessages: Record<number, string> = { | ||
| 413: "Log payload is too large (> 500 KB). Try clearing old logs first.", | ||
| 429: "Rate limit reached (5 submissions per hour). Try again later.", | ||
| 502: "Log server could not reach GitHub. Try again in a moment.", | ||
| }; | ||
|
|
||
| let message = errorMessages[response.status] ?? `Unexpected error (HTTP ${response.status}).`; | ||
| try { | ||
| const body = (await response.json()) as { error?: string }; | ||
| if (body.error) message = body.error; | ||
| } catch { | ||
| // ignore — use the default message | ||
| } | ||
|
|
||
| return { ok: false, status: response.status, message }; | ||
| } |
There was a problem hiding this comment.
If response.json() fails on the success path (e.g., due to an empty or invalid JSON response from the server), it will throw an unhandled exception and reject the promise returned by sendLogs. Since the caller handleSend does not wrap sendLogs in a try/catch, this will leave the dialog stuck in the "sending" state forever. Wrapping the entire body of sendLogs in a try/catch block ensures any unexpected errors or parsing failures are caught and returned gracefully as a failure result.
export async function sendLogs(payload: SendLogsPayload): Promise<SendLogsResult> {
if (SUPPORT_WORKER_URL.includes("REPLACE_ME")) {
return {
ok: false,
status: 0,
message: "Log upload is not configured for this build.",
};
}
try {
const response = await fetch(SUPPORT_WORKER_URL, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify(payload),
});
if (response.ok) {
const data = (await response.json()) as { code: string; issueNumber: number };
return { ok: true, code: data.code, issueNumber: data.issueNumber };
}
const errorMessages: Record<number, string> = {
413: "Log payload is too large (> 500 KB). Try clearing old logs first.",
429: "Rate limit reached (5 submissions per hour). Try again later.",
502: "Log server could not reach GitHub. Try again in a moment.",
};
let message = errorMessages[response.status] ?? `Unexpected error (HTTP ${response.status}).`;
try {
const body = (await response.json()) as { error?: string };
if (body.error) message = body.error;
} catch {
// ignore — use the default message
}
return { ok: false, status: response.status, message };
} catch (err) {
return {
ok: false,
status: 0,
message: err instanceof Error ? err.message : "Network error — check your connection.",
};
}
}| const IPV4_RE = /\b(?:\d{1,3}\.){3}\d{1,3}\b/g; | ||
|
|
||
| /** IPv6 candidates are validated after matching to avoid an overly complex regex */ | ||
| const IPV6_RE = /\b[A-Fa-f0-9:]{2,}\b/g; |
There was a problem hiding this comment.
Using \b (word boundary) with a pattern that can start or end with a colon : (like IPv6 addresses ::1 or fe80::) prevents matching when the address is preceded or followed by non-word characters (such as spaces or equal signs). Replacing \b with lookarounds ensures compressed IPv6 addresses starting or ending with colons are correctly matched and scrubbed.
| const IPV6_RE = /\b[A-Fa-f0-9:]{2,}\b/g; | |
| const IPV6_RE = /(?<![A-Fa-f0-9:])[A-Fa-f0-9:]{2,}(?![A-Fa-f0-9:])/g; |
| interface SendLogsDialogProps { | ||
| open: boolean; | ||
| onOpenChange: (open: boolean) => void; | ||
| /** Raw NDJSON lines currently visible in the log viewer */ | ||
| logLines: string[]; | ||
| } |
There was a problem hiding this comment.
To ensure component purity and testability, pass non-deterministic values like new Date() as props rather than creating them directly within the component. Adding an optional getCurrentDate prop allows the parent component or tests to inject a deterministic date generator.
| interface SendLogsDialogProps { | |
| open: boolean; | |
| onOpenChange: (open: boolean) => void; | |
| /** Raw NDJSON lines currently visible in the log viewer */ | |
| logLines: string[]; | |
| } | |
| interface SendLogsDialogProps { | |
| open: boolean; | |
| onOpenChange: (open: boolean) => void; | |
| /** Raw NDJSON lines currently visible in the log viewer */ | |
| logLines: string[]; | |
| /** Optional function to get the current date, useful for testing and purity */ | |
| getCurrentDate?: () => Date; | |
| } |
References
- To ensure component purity and testability, pass non-deterministic values like
new Date()as props rather than creating them directly within the component.
| const handleSend = useCallback(async () => { | ||
| setStep("sending"); | ||
|
|
||
| const scrubbedLogs = scrubLogLines(logLines); | ||
| const outcome = await sendLogs({ | ||
| logs: scrubbedLogs, | ||
| appVersion: APP_VERSION, | ||
| platform: detectPlatform(), | ||
| timestamp: new Date().toISOString(), | ||
| }); | ||
|
|
||
| setResult(outcome); | ||
| setStep(outcome.ok ? "success" : "error"); | ||
| }, [logLines]); |
There was a problem hiding this comment.
Use the optional getCurrentDate prop to obtain the current timestamp instead of hardcoding new Date(), adhering to the component purity and testability guidelines.
| const handleSend = useCallback(async () => { | |
| setStep("sending"); | |
| const scrubbedLogs = scrubLogLines(logLines); | |
| const outcome = await sendLogs({ | |
| logs: scrubbedLogs, | |
| appVersion: APP_VERSION, | |
| platform: detectPlatform(), | |
| timestamp: new Date().toISOString(), | |
| }); | |
| setResult(outcome); | |
| setStep(outcome.ok ? "success" : "error"); | |
| }, [logLines]); | |
| const handleSend = useCallback(async () => { | |
| setStep("sending"); | |
| const scrubbedLogs = scrubLogLines(logLines); | |
| const outcome = await sendLogs({ | |
| logs: scrubbedLogs, | |
| appVersion: APP_VERSION, | |
| platform: detectPlatform(), | |
| timestamp: (getCurrentDate ? getCurrentDate() : new Date()).toISOString(), | |
| }); | |
| setResult(outcome); | |
| setStep(outcome.ok ? "success" : "error"); | |
| }, [logLines, getCurrentDate]); |
References
- To ensure component purity and testability, pass non-deterministic values like
new Date()as props rather than creating them directly within the component.
| const handleCopyCode = useCallback(() => { | ||
| if (!result?.ok) return; | ||
| navigator.clipboard | ||
| .writeText(result.code) | ||
| .then(() => { | ||
| toast({ title: "Copied", description: `Code ${result.code} copied to clipboard` }); | ||
| }) | ||
| .catch(() => { | ||
| toast({ | ||
| title: "Copy failed", | ||
| description: "Clipboard access denied", | ||
| variant: "destructive", | ||
| }); | ||
| }); | ||
| }, [result, toast]); |
There was a problem hiding this comment.
navigator.clipboard can be undefined in older browsers or in non-secure contexts (HTTP). Attempting to call writeText on it directly will throw a TypeError and crash the component. Adding a guard prevents this runtime error and allows showing a helpful fallback toast.
const handleCopyCode = useCallback(() => {
if (!result?.ok) return;
if (!navigator.clipboard) {
toast({
title: "Copy failed",
description: "Clipboard API not supported in this browser",
variant: "destructive",
});
return;
}
navigator.clipboard
.writeText(result.code)
.then(() => {
toast({ title: "Copied", description: `Code ${result.code} copied to clipboard` });
})
.catch(() => {
toast({
title: "Copy failed",
description: "Clipboard access denied",
variant: "destructive",
});
});
}, [result, toast]);
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5da70243b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * placeholder below with the URL printed by Wrangler, e.g.: | ||
| * https://questarr-log-collector.<your-subdomain>.workers.dev | ||
| */ | ||
| export const SUPPORT_WORKER_URL = "https://questarr-log-collector.questarr.workers.dev"; |
There was a problem hiding this comment.
When the dialog calls this hardcoded Cloudflare Worker, the browser is still governed by the app's Helmet CSP; I checked server/routes.ts and connect-src only allows 'self', https://raw.githubusercontent.com, and https://api.github.com (plus websockets in non-production). In production (and dev), fetch("https://questarr-log-collector.questarr.workers.dev") will be blocked by CSP before reaching the worker, so the new Send Logs flow cannot upload logs until this origin is added to connect-src or the request is proxied through the app.
Useful? React with 👍 / 👎.
| return text | ||
| .replace(JWT_RE, "[jwt]") // before email — JWTs contain dots | ||
| .replace(EMAIL_RE, "[email]") | ||
| .replace(IPV6_RE, (match) => (isIpv6(match) ? "[ip]" : match)) |
There was a problem hiding this comment.
For unstructured log lines or messages that contain plain clock times like 12:34:56, this IPv6 pass treats the value as a valid three-group IPv6 candidate and replaces it with [ip]. That strips useful timing information from the support bundle even though no IP address was present; the IPv6 detection should require a real IPv6 shape rather than accepting any colon-separated hex groups.
Useful? React with 👍 / 👎.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Doezer
force-pushed
the
feat/send-logs-button-using-external-worker
branch
from
774fc27 to
3e73b78
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
2 participants
This pull request introduces a new
SendLogsDialogcomponent for uploading logs to support, along with comprehensive tests for both the dialog and the underlying log upload utilities. The changes ensure that logs are scrubbed of PII, uploaded securely, and that users receive clear feedback and guidance throughout the process. The most important changes are summarized below.New Feature: Send Logs Dialog
SendLogsDialogcomponent (client/src/components/SendLogsDialog.tsx) for uploading logs to support, including UI for consent, upload progress, success (with support code and GitHub issue link), and error handling. The dialog scrubs PII from logs, detects platform, and provides user feedback via toasts and UI states.Testing: Dialog and Log Upload Utilities
SendLogsDialogcomponent (client/__tests__/SendLogsDialog.test.tsx), covering log submission, error and rate-limit handling, clipboard copy feedback, and UI state transitions.client/__tests__/send-logs.test.ts), verifying PII scrubbing, log submission, error handling, GitHub issue URL generation, and platform detection.Test Improvements and Fixes
LogsPage.remaining.test.tsxto ensure log streaming events are handled insideactfor proper React state updates.actimport toLogsPage.remaining.test.tsxto support the above fix.