Comfy-Org / Comfy-Org/ComfyUI_frontend
Narrow ReportErrorOptions.context so the no-PII rule is enforced, not just documented
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
## Problem / Goal
ADR-TELEMETRY-ERRORS-0030 rule 7 says diagnostics must carry no PII and limits `context` to "ids, counts, enums, and booleans". Nothing enforces that:
```ts
// src/platform/telemetry/reportError.ts
context?: Record
```
`unknown` accepts a whole response body, a workflow name, a file path or a token as readily as a boolean. The rule is a convention that reviewers have to catch by eye, on a call site count that keeps growing.
This matters more now that `reportError` writes a console line for every report, because `context` reaches Sentry, RUM and the console.
Current call sites mostly comply — `has_cached_plans`, `attribution_stage`, `workspace_still_current`, `checkout_attempt_id` — so a narrowing is unlikely to be a large migration. `url` on the resource-load report in `App.vue` is one known outlier to decide on.
## Proposed Solution
Narrow the type to what the ADR permits, roughly:
```ts
type DiagnosticValue = string | number | boolean | null
context?: Record
```
`string` still admits PII, so this is a guard rail rather than a proof — but it stops whole objects and response bodies at compile time, which is the case that actually leaks.
Audit existing call sites for outliers first; the change is only worth making if the migration is small.
## Acceptance Criteria
- [ ] `context` rejects a nested object or array of objects at compile time
- [ ] Every existing `reportError` call site compiles or is migrated
- [ ] A decision recorded for `App.vue`'s `url` value (keep, truncate, or drop)
- [ ] ADR rule 7 references the type as the enforcement point
---
Offered as the real fix on #17001 (https://github.com/Comfy-Org/ComfyUI_frontend/pull/17001#discussion_r3962162535), where the alternative suggestion was redacting `context` at the log site — which would remove most of the console line's diagnostic value without addressing the sinks.
Contributor guide
Research direction
Start with src/platform/telemetry/reportError.ts and audit existing reportError call sites, including the resource-load report in App.vue. Read ADR-TELEMETRY-ERRORS-0030 rule 7 and inspect the discussion linked from #17001. Done means nested objects are rejected at compile time, all call sites compile or are migrated, App.vue's url has a recorded decision, and the ADR references the type.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- observability, security
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100