Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdds a developer-only Test Monitors page with two UI components (datastore and website), a new route and nav entry, supporting utils, mutation-driven flows to patch connections, put monitors, execute monitors, and a changelog entry documenting the addition. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as TestDatastoreMonitor (Form)
participant Mutations as Mutation Hooks
participant API as Backend API
User->>UI: Fill form & click Run
UI->>Mutations: patchConnection(params)
Mutations->>API: PATCH /connections
API-->>Mutations: connection response
UI->>Mutations: putMonitor(monitorConfig)
Mutations->>API: PUT /monitors
API-->>Mutations: monitor response
UI->>Mutations: executeMonitor(monitorId)
Mutations->>API: POST /monitors/:id/execute
API-->>Mutations: execution result
Mutations-->>UI: success (execution id)
UI->>User: show success & update UI
sequenceDiagram
actor User
participant UI as TestWebsiteMonitor (Form)
participant Mutations as Mutation Hooks
participant API as Backend API
User->>UI: Configure & click Create and run
UI->>Mutations: patchConnection(siteParams)
Mutations->>API: PATCH /connections
API-->>Mutations: connection response
UI->>Mutations: putMonitor(websiteMonitorConfig)
Mutations->>API: PUT /monitors
API-->>Mutations: monitor response
UI->>Mutations: executeMonitor(monitorId)
Mutations->>API: POST /monitors/:id/execute
API-->>Mutations: execution result
Mutations-->>UI: success (execution id)
UI->>User: show success & reset/new monitor name
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 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)
Comment |
Contributor
Greptile SummaryThis PR adds a developer-only UI page at Key findings:
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: b520a5e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+59
to
+69
| function randomizeParams(): Omit<FormValues, "monitor_name"> { | ||
| return { | ||
| num_cookies: randInt(5, 50), | ||
| num_javascript_requests: randInt(5, 30), | ||
| num_image_requests: randInt(1, 20), | ||
| num_iframe_requests: randInt(0, 10), | ||
| num_browser_requests: randInt(2, 20), | ||
| consent_granted_percentage: randInt(30, 90), | ||
| consent_denied_percentage: randInt(0, 30), | ||
| vendor_match_percentage: randInt(20, 90), | ||
| }; |
Contributor
There was a problem hiding this comment.
consent_granted_percentage + consent_denied_percentage can exceed 100%
randomizeParams draws consent_granted_percentage from randInt(30, 90) and consent_denied_percentage independently from randInt(0, 30). Their sum can reach up to 120% (e.g. 90 granted + 30 denied), which is semantically invalid — the two values together with "no consent" should sum to ≤ 100%.
Consider constraining the second value so granted + denied ≤ 100, for example by capping denied at 100 - granted.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
clients/admin-ui/src/features/test-monitors/TestWebsiteMonitor.tsx (1)
82-96: Consider validating the form before proceeding.The
monitor_namefield is typed as optional and accessed with a non-null assertion (monitorName!). While the form has arequired: truerule,handleRundoesn't trigger form validation before proceeding. If validation fails silently, this could lead to a runtime issue.🛡️ Suggested approach to validate before running
const handleRun = async () => { + try { + await form.validateFields(); + } catch { + return; + } const { monitor_name: monitorName, consent_granted_percentage: grantedPct,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/features/test-monitors/TestWebsiteMonitor.tsx` around lines 82 - 96, The handleRun function uses form.getFieldsValue and a non-null assertion on monitor_name (monitorName!) without validating the form; change it to call and await form.validateFields() (or form.validateFields([...required fields])) at the start of handleRun, use the returned validated values instead of form.getFieldsValue, remove the non-null assertion when assigning name, and handle validation errors by aborting the run so you never proceed with missing/invalid fields; reference handleRun, form.getFieldsValue, monitor_name, and the name assignment when making the change.clients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsx (1)
73-83: Consider validating the form before proceeding.Same pattern as
TestWebsiteMonitor— themonitor_namefield uses a non-null assertion without explicit form validation. Consider addingawait form.validateFields()at the start ofhandleRunfor consistency and safety.🛡️ Suggested approach
const handleRun = async () => { + try { + await form.validateFields(); + } catch { + return; + } const { monitor_name: monitorName,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsx` around lines 73 - 83, The handler handleRun should validate the form before reading values: call await form.validateFields() at the start of handleRun (same pattern as TestWebsiteMonitor) so monitor_name and other fields are guaranteed valid; then read values from form.getFieldsValue(), drop the unsafe non-null assertion on monitorName or keep it only after successful validation, and ensure any validation errors are allowed to surface (or caught and returned) so you don't proceed with undefined monitor_name or invalid nested_field_percentage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@clients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsx`:
- Around line 73-83: The handler handleRun should validate the form before
reading values: call await form.validateFields() at the start of handleRun (same
pattern as TestWebsiteMonitor) so monitor_name and other fields are guaranteed
valid; then read values from form.getFieldsValue(), drop the unsafe non-null
assertion on monitorName or keep it only after successful validation, and ensure
any validation errors are allowed to surface (or caught and returned) so you
don't proceed with undefined monitor_name or invalid nested_field_percentage.
In `@clients/admin-ui/src/features/test-monitors/TestWebsiteMonitor.tsx`:
- Around line 82-96: The handleRun function uses form.getFieldsValue and a
non-null assertion on monitor_name (monitorName!) without validating the form;
change it to call and await form.validateFields() (or
form.validateFields([...required fields])) at the start of handleRun, use the
returned validated values instead of form.getFieldsValue, remove the non-null
assertion when assigning name, and handle validation errors by aborting the run
so you never proceed with missing/invalid fields; reference handleRun,
form.getFieldsValue, monitor_name, and the name assignment when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75d7e922-4f37-42f2-8d5f-b3e0ac3f3aaa
📒 Files selected for processing (7)
changelog/7565-test-monitor-developer-tool.yamlclients/admin-ui/src/features/common/nav/nav-config.tsxclients/admin-ui/src/features/common/nav/routes.tsclients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsxclients/admin-ui/src/features/test-monitors/TestWebsiteMonitor.tsxclients/admin-ui/src/features/test-monitors/utils.tsclients/admin-ui/src/pages/poc/test-monitors.tsx
gilluminate
reviewed
Mar 4, 2026
| setIsRunning(false); | ||
| return; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Nit: the three sequential awaits aren't wrapped in try/catch, so if any of them reject (vs returning an error result), setIsRunning(false) never fires and the button gets stuck in a permanent loading state. You'd have to refresh the page.
A try/catch/finally would fix it and also let you drop the three duplicated setIsRunning(false) calls:
const handleRun = async () => {
setIsRunning(true);
try {
// ... existing logic, minus the setIsRunning(false) in each early return ...
} catch (e) {
message.error('Unexpected error');
} finally {
setIsRunning(false);
}
};Same pattern applies in TestWebsiteMonitor.tsx.
gilluminate
reviewed
Mar 4, 2026
| setIsRunning(true); | ||
|
|
||
| const connResult = await patchConnection({ | ||
| key, |
Contributor
There was a problem hiding this comment.
This key logic is a bit hard to follow — if the user edits the name field, this branch generates a new timestamp-based key that differs from both the auto-key and the user's input. So key and name silently diverge, which could be confusing when debugging monitor results.
Would it be simpler to either always use name as the key (slugified if needed), or always generate a fresh key regardless of what the user typed?
Contributor
Author
There was a problem hiding this comment.
Yeah, I think I overengineered this one a bit. I've simplified so it uses the autogenerated key for the connection name and key and the monitor key and the name field just changes the monitor name.
gilluminate
reviewed
Mar 4, 2026
…com/ethyca/fides into jpople/2026-03-04/test-monitor-flow
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clients/admin-ui/src/features/test-monitors/TestWebsiteMonitor.tsx (1)
200-232: Consider adding validation that granted + denied percentages do not exceed 100.The form allows users to manually input values where
consent_granted_percentage + consent_denied_percentage > 100(e.g., 80% granted + 50% denied = 130%). WhilerandomizeParams()prevents this, manual entry does not. This could lead to unexpected behavior when the values are converted to fractions and passed to the API.💡 Suggested approach using form-level validation
Add a custom validator to one of the percentage fields:
<Form.Item label="Denied %" name="consent_denied_percentage" rules={[ { validator: async (_, value) => { const granted = form.getFieldValue("consent_granted_percentage") ?? 0; if (granted + (value ?? 0) > 100) { throw new Error("Granted + Denied cannot exceed 100%"); } }, }, ]} dependencies={["consent_granted_percentage"]} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/features/test-monitors/TestWebsiteMonitor.tsx` around lines 200 - 232, The grant/deny percentage fields (Form.Item names "consent_granted_percentage" and "consent_denied_percentage") need form-level validation so their sum cannot exceed 100; add a custom validator rule to the denied field (or both fields) that uses form.getFieldValue("consent_granted_percentage") and checks granted + (value ?? 0) <= 100, mark the field invalid with an error message if it exceeds 100, and include dependencies={["consent_granted_percentage"]} on the Form.Item so changes revalidate; this mirrors the protection in randomizeParams() but enforces it for manual input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@clients/admin-ui/src/features/test-monitors/TestWebsiteMonitor.tsx`:
- Around line 200-232: The grant/deny percentage fields (Form.Item names
"consent_granted_percentage" and "consent_denied_percentage") need form-level
validation so their sum cannot exceed 100; add a custom validator rule to the
denied field (or both fields) that uses
form.getFieldValue("consent_granted_percentage") and checks granted + (value ??
0) <= 100, mark the field invalid with an error message if it exceeds 100, and
include dependencies={["consent_granted_percentage"]} on the Form.Item so
changes revalidate; this mirrors the protection in randomizeParams() but
enforces it for manual input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ac0d6a4-e799-4f42-a49c-269964ca766e
📒 Files selected for processing (4)
clients/admin-ui/src/features/common/nav/nav-config.tsxclients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsxclients/admin-ui/src/features/test-monitors/TestWebsiteMonitor.tsxclients/admin-ui/src/pages/poc/test-monitors.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- clients/admin-ui/src/pages/poc/test-monitors.tsx
- clients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsx
- clients/admin-ui/src/features/common/nav/nav-config.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
clients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsx (1)
202-208: Clarify reset behavior for monitor name.The button label says “Reset to default” but only numeric params are reset;
monitor_nameis preserved. Either includemonitor_name: keyin reset or rename to “Reset params” for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsx` around lines 202 - 208, The reset button in TestDatastoreMonitor currently calls form.setFieldsValue({ ...DEFAULT_PARAMS }) which leaves monitor_name unchanged; update the onClick to include monitor_name: key (i.e. form.setFieldsValue({ ...DEFAULT_PARAMS, monitor_name: key })) so the monitor name resets too, referencing the DEFAULT_PARAMS constant, the form.setFieldsValue call, and the monitor_name field in the TestDatastoreMonitor component; alternatively, if you prefer preserving the name, rename the Button label from "Reset to default" to "Reset params" to match current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsx`:
- Around line 73-87: handleRun currently only relies on validateFields() for
monitor_name, so numeric inputs like nested_field_percentage can be
undefined/invalid and cause nestedFieldPct / 100 to produce bad payloads; add
required and numeric validation rules to the form schema for fields such as
nested_field_percentage (and any other numeric fields used in params) or
explicitly validate/normalize values after form.getFieldsValue() inside
handleRun (e.g., check that nestedFieldPct is a finite number, provide a default
or show validation error and return) before constructing params; update
references in handleRun and the form field declarations to use these validators
so params only contain valid numeric values.
---
Nitpick comments:
In `@clients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsx`:
- Around line 202-208: The reset button in TestDatastoreMonitor currently calls
form.setFieldsValue({ ...DEFAULT_PARAMS }) which leaves monitor_name unchanged;
update the onClick to include monitor_name: key (i.e. form.setFieldsValue({
...DEFAULT_PARAMS, monitor_name: key })) so the monitor name resets too,
referencing the DEFAULT_PARAMS constant, the form.setFieldsValue call, and the
monitor_name field in the TestDatastoreMonitor component; alternatively, if you
prefer preserving the name, rename the Button label from "Reset to default" to
"Reset params" to match current behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59d1ee2a-78df-4902-b0b2-9629aca5a05e
📒 Files selected for processing (2)
clients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsxclients/admin-ui/src/features/test-monitors/TestWebsiteMonitor.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- clients/admin-ui/src/features/test-monitors/TestWebsiteMonitor.tsx
Comment on lines
+73
to
+87
| const handleRun = async () => { | ||
| try { | ||
| await form.validateFields(); | ||
| } catch { | ||
| return; | ||
| } | ||
| const { | ||
| monitor_name: monitorName, | ||
| nested_field_percentage: nestedFieldPct, | ||
| ...rest | ||
| } = form.getFieldsValue(); | ||
| const params = { | ||
| ...rest, | ||
| nested_field_percentage: nestedFieldPct / 100, | ||
| }; |
There was a problem hiding this comment.
Add required validation for numeric fields before building monitor params.
validateFields() currently enforces only monitor_name (Line 150). If numeric inputs are empty/invalid, Line 86 can produce an invalid payload (nestedFieldPct / 100), causing avoidable API failures.
Proposed fix
<Form.Item
label="Monitor name"
name="monitor_name"
rules={[{ required: true }]}
>
<Input />
</Form.Item>
...
- <Form.Item label="Databases" name="num_databases">
+ <Form.Item
+ label="Databases"
+ name="num_databases"
+ rules={[{ required: true, type: "number", min: 1 }]}
+ >
<InputNumber min={1} className="w-full" />
</Form.Item>
...
- <Form.Item label="Schemas / DB" name="num_schemas_per_db">
+ <Form.Item
+ label="Schemas / DB"
+ name="num_schemas_per_db"
+ rules={[{ required: true, type: "number", min: 1 }]}
+ >
<InputNumber min={1} className="w-full" />
</Form.Item>
...
- <Form.Item label="Tables / schema" name="num_tables_per_schema">
+ <Form.Item
+ label="Tables / schema"
+ name="num_tables_per_schema"
+ rules={[{ required: true, type: "number", min: 1 }]}
+ >
<InputNumber min={1} className="w-full" />
</Form.Item>
...
- <Form.Item label="Fields / table" name="num_fields_per_table">
+ <Form.Item
+ label="Fields / table"
+ name="num_fields_per_table"
+ rules={[{ required: true, type: "number", min: 1 }]}
+ >
<InputNumber min={1} className="w-full" />
</Form.Item>
...
- <Form.Item label="Max nesting level" name="max_nesting_level">
+ <Form.Item
+ label="Max nesting level"
+ name="max_nesting_level"
+ rules={[{ required: true, type: "number", min: 0 }]}
+ >
<InputNumber min={0} className="w-full" />
</Form.Item>
...
- <Form.Item label="Nested field %" name="nested_field_percentage">
+ <Form.Item
+ label="Nested field %"
+ name="nested_field_percentage"
+ rules={[{ required: true, type: "number", min: 0, max: 100 }]}
+ >
<InputNumber
min={0}
max={100}Also applies to: 147-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/features/test-monitors/TestDatastoreMonitor.tsx` around
lines 73 - 87, handleRun currently only relies on validateFields() for
monitor_name, so numeric inputs like nested_field_percentage can be
undefined/invalid and cause nestedFieldPct / 100 to produce bad payloads; add
required and numeric validation rules to the form schema for fields such as
nested_field_percentage (and any other numeric fields used in params) or
explicitly validate/normalize values after form.getFieldsValue() inside
handleRun (e.g., check that nestedFieldPct is a finite number, provide a default
or show validation error and return) before constructing params; update
references in handleRun and the form field declarations to use these validators
so params only contain valid numeric values.
gilluminate
approved these changes
Mar 5, 2026
Contributor
gilluminate
left a comment
gilluminate
left a comment
There was a problem hiding this comment.
Changes look good, thanks.
mfbrown
pushed a commit
that referenced
this pull request
Mar 12, 2026
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.
Ticket
Description Of Changes
I got tired of copy-pasting JSONs into Swagger, so adds a dev-only UI for seeding monitor data using the test-datastore and test-website monitors.
Code Changes
src/pages/poc/test-monitors.tsxwith two monitor cards (TestDatastoreMonitor,TestWebsiteMonitor) and shared utilitiesSteps to Confirm
FIDES__DEV_MODE=truePre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit
New Features
Chore