Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
| open={isOpen} | ||
| onClose={() => setIsOpen(false)} | ||
| title="Failure log" | ||
| title={`${monitor.name} - Failure log`} |
There was a problem hiding this comment.
This is an unrelated change. It came up in a recent pod check-in that not including the monitor name could cause confusion when an integration has multiple monitors.
This is a minimal change to add some context, but we can improve it in a future iteration
vcruces
force-pushed
the
ENG-2782
branch
2 times, most recently
from
9ab6ec0 to
935e7ca
Compare
… the integrations tray
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Clean, well-scoped PR. Changes are minimal and correctly implement the performance improvement by passing include_total: false on all three tree fetch call sites in MonitorTree.tsx.
Correctness
- All three tree fetch paths (initial expand, load-more/pagination, root load) are updated consistently — no missed call sites.
monitor.nameis typed as a required non-nullablestringinMonitorStatusResponse, so the template literal in the drawer title is safe.- RTK Query correctly omits
undefinedparams from the request, so omittinginclude_totalat call sites won't break anything today.
Suggestion
The one thing worth hardening: include_total defaults to undefined in the query type, which means a future call site that forgets to pass include_total: false will silently trigger the COUNT query again. Since the intent is always to skip it, consider defaulting include_total to false in the query destructor inside the slice (see inline comment). This makes the safe behavior the default without requiring every caller to opt in explicitly.
Testing
No tests were added, but the changes are a thin pass-through of a query parameter — the logic lives in the backend. The existing test coverage for MonitorTree and the RTK Query endpoint should be sufficient. The drawer title change (MonitorStatusCell.tsx) is equally trivial and matches a required-string field in the type.
| staged_resource_urn?: string; | ||
| include_descendant_details?: boolean; | ||
| diff_status?: DiffStatus[]; | ||
| include_total?: boolean; |
There was a problem hiding this comment.
The parameter is typed as boolean | undefined, so any future call site that omits include_total will silently default to whatever the backend's default is (presumably true, which triggers the COUNT query). Since the entire motivation of this PR is to skip COUNT queries, consider defaulting to false here in the slice instead:
include_total?: boolean; // default: false at call sitesOr alternatively, set the default in the query destructor:
include_total = false,That way new call sites are safe by default without having to remember to pass include_total: false explicitly.
There was a problem hiding this comment.
The backend defaults this to false if it’s not provided
1 participant
…o the integrations tray
Ticket ENG-2782
Description Of Changes
Frontend updates to support the improved cursor pagination in the
/treeendpoint. Passesinclude_total: falseexplicitly to skip the COUNT query on all tree navigation calls, taking advantage of the performance improvement introduced in fidesplus #3305.Also improves the monitor failure log drawer title to include the monitor name for better context.
Code Changes
include_totalparameter to thegetMonitorTreeRTK Query endpoint inaction-center.slice.tsinclude_total: falseon all three tree fetch calls inMonitorTree.tsx"Failure log"to"${monitor.name} - Failure log"inMonitorStatusCell.tsxSteps to Confirm
include_total=false.Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works