feat(extension): configure PII entity types and custom regex from options page#540
feat(extension): configure PII entity types and custom regex from options page#540yjouini wants to merge 1 commit into
Conversation
…ions page Wire the Chrome extension options page to the new server-side PII config endpoints introduced in dataiku#509/dataiku#513: - Entity types: GET/POST /api/pii/entities ({available, disabled}), unchecking a type adds it to the disabled (passthrough) set. - Custom patterns: GET/POST /api/pii/regexes, whole-set replacement on each add/remove; custom names appear as selectable entity types. Config is stored server-side, so /api/pii/check needs no per-request label list and background.js is unchanged. Closes dataiku#458, Closes dataiku#459 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Davidnet
left a comment
Davidnet
left a comment
There was a problem hiding this comment.
Code review of the PII entity-type / custom-regex config wiring. Inline comments below cover the correctness issues found, ranked roughly most-severe first. Two of these (the empty-grid Save and the stale delete index) can silently lose user data.
|
|
||
| saveLabelBtn.addEventListener("click", async () => { | ||
| const checkboxes = labelGrid.querySelectorAll("input[type=checkbox]"); | ||
| const disabled = Array.from(checkboxes) |
There was a problem hiding this comment.
Save can wipe the user's passthrough config when the grid is empty. If Save is clicked before the grid finishes loading, or after an empty/unreachable GET /api/pii/entities, querySelectorAll returns zero checkboxes, so disabled is []. The POST then re-enables (re-masks) every entity type the user had previously disabled — silent data loss.
Suggest bailing out when no entities are loaded, e.g. if (checkboxes.length === 0) return; (ideally gating the button on a successful load).
|
|
||
| patternList.querySelectorAll("[data-action=delete]").forEach((btn) => { | ||
| btn.addEventListener("click", async () => { | ||
| const idx = Number(btn.dataset.idx); |
There was a problem hiding this comment.
Stale captured index deletes the wrong pattern under concurrent removes. data-idx is baked in at render time. With [A,B,C], removing B then quickly removing C before the first POST re-renders means C's idx=2 now indexes the shortened [A,C]: regexes[2] is undefined and filter(i !== 2) removes nothing, so C survives — and the rollback would splice undefined back in.
Match by identity instead of index, e.g. capture const removed = p; in the forEach and regexes = regexes.filter((r) => r !== removed);.
| function validateRegex(value) { | ||
| if (!value) return null; | ||
| try { | ||
| new RegExp(value); |
There was a problem hiding this comment.
Validation/preview uses the JS RegExp engine, but the Go backend uses RE2. Patterns with lookbehind/backreferences (e.g. (?<=ID:)\d+, (\w)\1) validate and preview fine here but are rejected by the backend on Add (confusing "Failed to save"), or — if stored without compiling — silently never mask. Consider rejecting RE2-incompatible constructs up front so the UI matches backend behavior. (Same root cause applies to the preview at line 271 and the submit-path validation at line 356.)
|
|
||
| patternForm.addEventListener("submit", async (e) => { | ||
| e.preventDefault(); | ||
| const name = patternName.value.trim().toUpperCase(); |
There was a problem hiding this comment.
Whitespace-only name bypasses required. HTML required passes for a spaces-only value, then .trim().toUpperCase() yields "", so a blank-named custom entity is POSTed and rendered as a blank label/row. Validate the trimmed value and bail if empty.
| return; | ||
| } | ||
|
|
||
| regexes.push({ name, pattern }); |
There was a problem hiding this comment.
No duplicate-name check on Add. Adding two patterns named e.g. EMAIL POSTs and renders both, producing duplicate entity-type entries in the label grid with ambiguous enable/disable toggling. Reject (or merge) when regexes already contains the name.
| patternForm.reset(); | ||
| patternPreviewResult.textContent = ""; | ||
| } catch (err) { | ||
| regexes.pop(); |
There was a problem hiding this comment.
UI/backend desync on a non-JSON 2xx body. If POST /api/pii/regexes succeeds (200) but returns a non-JSON body, await resp.json() in savePatterns throws after the server persisted the pattern; this catch runs regexes.pop() and shows "Failed to save", so the pattern vanishes from the list but reappears (active) on the next reload. Guard the JSON parse.
| const data = await resp.json(); | ||
| regexes = data.regexes || regexes; | ||
| renderPatterns(); | ||
| loadLabels(); |
There was a problem hiding this comment.
Cleanup: savePatterns calls loadLabels() on every Add/Remove, adding a full GET /api/pii/entities round-trip and rebuilding the entire checkbox grid — which discards any unsaved check/uncheck state the user set in the PII-types card. Consider refreshing the grid only when names actually change, or merging new names into the existing grid without a full reload.
|
Hi @yjouini I have some proposal that cover a few of my comments, let me know if they make sense, and if you can bring to your PR.
Let me know if you have comments or questions |
2 participants
Closes #458, Closes #459
Wires the Chrome extension options page to the server-side PII config endpoints introduced in #509 / #513. The config UI from #470 is brought over and re-pointed at the new contract; the backend changes in #470 are superseded by #509/#513, so this PR is extension-only.
Entity types (
/api/pii/entities)GET({ available, disabled }). Checked = masked.disabled(passthrough) set viaPOST { disabled: [...] }. A "Disable all / Enable all" toggle is included.400and surfaced inline.Custom patterns (
/api/pii/regexes)name+pattern) with a live match preview before saving.POSTreplaces the whole set, so add/remove rebuild the list and re-POST. Each custom name then appears as a selectable entity type in the grid.Notes
GenericGenerator→[REDACTED_<NAME>_<hash>]. Demasking/substitution for custom types remains open.🤖 Generated with Claude Code