Skip to content

fix(secret-approval): stable pagination for change requests list (V2)#5312

Merged
IgorHorta merged 4 commits intomainfrom
igor/eng-4497-improve-secret-change-request-ui-experience
Jan 29, 2026
Merged

fix(secret-approval): stable pagination for change requests list (V2)#5312
IgorHorta merged 4 commits intomainfrom
igor/eng-4497-improve-secret-change-request-ui-experience

Conversation

@IgorHorta
Copy link
Contributor

@IgorHorta IgorHorta commented Jan 29, 2026 •
edited
Loading

Context

Before: The change requests (CR) list had (1) pagination issues—total count and page size could be wrong because DENSE_RANK() used only createdAt, so ties produced more than limit rows per page and inconsistent totals; (2) search only supported author, environment, and policy path—not folder path or secret name.

After:

  • Pagination: The list is built in two phases: first we get a page of distinct CR IDs using DENSE_RANK() OVER (ORDER BY "createdAt" DESC, id) and a single total count; then we load full rows only for those IDs. Results are capped at limit and totalCount matches the distinct CR count.
  • Search: The list query now exposes folder path (requestFolderPath from SecretFolder.name) and secret key (secretKey from commit rows). The search filter includes path and secret name (e.g. “path”, “policy path or secret name” in the placeholder).

Screenshots

N/A (backend + small placeholder text change).

Steps to verify the change

  1. Open Secret Manager → Change Requests for a V3 project with many CRs.
  2. Pagination: Change page size and page; confirm total count is stable and each page has at most that many requests (no duplicate requests across rows).
  3. Search: Use the search box with path-like terms (e.g. folder name) and secret key names; confirm the list filters by path and by secret name.

Type

  • Fix
  • Feature
  • Improvement
  • Breaking
  • Docs
  • Chore

Checklist

@IgorHorta
fix(secret-approval): stable pagination for change requests list (V2)
2c0a6ca 
- Use two-phase query in findByProjectIdBridgeSecretV2: first get page of
  distinct CR ids (with DENSE_RANK), then fetch full data for those ids
  so each page returns exactly N change requests (not N commit rows).
- Add tie-breaker to DENSE_RANK (ORDER BY createdAt DESC, id) so
  identical createdAt no longer produces duplicate ranks and variable
  page sizes.
- Cap page ids and returned approvals at limit so page size is always
  correct even with ties or replica quirks.
@maidul98
Copy link
Collaborator

maidul98 commented Jan 29, 2026 •
edited
Loading

✅ Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@IgorHorta IgorHorta marked this pull request as ready for review January 29, 2026 19:29
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 29, 2026

Greptile Overview

Greptile Summary

This PR fixes pagination issues and enhances search functionality for the secret approval request list (V2).

Key Changes:

  • Replaced single-phase pagination with two-phase approach: first gets distinct request IDs with stable ranking (DENSE_RANK() OVER (ORDER BY "createdAt" DESC, id)), then loads full data for those IDs only
  • Fixed pagination count inconsistency by ensuring exactly limit requests per page (previous implementation could return more due to ties in createdAt)
  • Added requestFolderPath (from SecretFolder.name) and secretKey to search capabilities
  • Updated frontend placeholder text to reflect new search fields

Issues Found:

  • Column ambiguity in .whereIn("id", pageIds) on line 773 - should specify table name to avoid ambiguity with joined tables

Confidence Score: 3/5

  • Safe to merge after fixing the column ambiguity issue on line 773
  • The pagination logic improvement is sound and addresses real issues. However, there's a column ambiguity bug that could cause runtime errors depending on the database query planner. The OR conditions in the search filter are properly grouped (safe pattern). No security vulnerabilities detected.
  • backend/src/ee/services/secret-approval-request/secret-approval-request-dal.ts needs attention for the column ambiguity fix on line 773

Important Files Changed

Filename Overview
backend/src/ee/services/secret-approval-request/secret-approval-request-dal.ts Refactored pagination logic to use two-phase query with DISTINCT ON and DENSE_RANK; added folder path and secret key to search filters; potential column ambiguity issue in whereIn clause
frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/SecretApprovalRequest.tsx Updated search placeholder text to reflect new search capabilities (path and secret name)

greptile-apps[bot]
greptile-apps bot reviewed Jan 29, 2026
View reviewed changes
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@IgorHorta IgorHorta merged commit 1e9a76e into main Jan 29, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@varonix0 varonix0 varonix0 approved these changes

+1 more reviewer

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@IgorHorta @maidul98 @varonix0