Sitelet https://github.com/DelegoLabs/Delego/pull/226
Skip to content

feat: add delegation status filter validation (#113) - #226

Closed
Mikkelnice wants to merge 1 commit into
DelegoLabs:mainfrom
Mikkelnice:feat/delegation-status-filter-validation
Closed

Mikkelnice wants to merge 1 commit into
DelegoLabs:mainfrom
Mikkelnice:feat/delegation-status-filter-validation

Conversation

@Mikkelnice

@Mikkelnice Mikkelnice commented Jun 25, 2026 •

Copy link
Copy Markdown

Summary

Validates the ?status= query parameter on GET /api/v1/delegations before querying the database.

Changes

  • apps/backend/gateway/src/models/Delegation.ts — added DelegationStatusFilter type and DELEGATION_STATUS_FILTERS constant
  • apps/backend/gateway/routes/delegations.ts — listDelegationsHandler now rejects unsupported status values with 400 VALIDATION_ERROR; valid values are passed through to the DB query; no-param behavior is unchanged
  • tests/unit/src/delegation-status-filter.test.js — route tests: unfiltered listing (200), all 5 valid statuses (200), two invalid status values (400 VALIDATION_ERROR)
  • tests/unit/package.json — added --experimental-test-module-mocks flag to support ESM module mocking in Node's built-in test runner

Testing

node --test --experimental-test-module-mocks tests/unit/src/delegation-status-filter.test.js
# 4/4 pass

Closes #113

Summary by CodeRabbit

  • New Features

    • You can now filter delegations by status when viewing the delegation list.
    • Supported statuses include pending, active, paused, revoked, and expired.
  • Bug Fixes

    • Invalid status values now return a clear validation error instead of being accepted.
  • Tests

    • Added coverage for delegation status filtering and validation behavior.

- Add DelegationStatusFilter type and DELEGATION_STATUS_FILTERS constant to Delegation.ts
- Validate ?status= query param in listDelegationsHandler before DB query
- Reject unsupported values with 400 VALIDATION_ERROR
- Add route tests covering valid filters, invalid filters, and unfiltered listing
@coderabbitai

coderabbitai Bot commented Jun 25, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds delegation status filtering to the list route. The gateway validates the optional status query parameter against allowed values, returns VALIDATION_ERROR for unsupported values, and applies the filter when querying delegations. Unit tests cover valid, invalid, and unfiltered requests.

Changes

Delegation status filter validation

Layer / File(s) Summary
Status filter contract
apps/backend/gateway/src/models/Delegation.ts
Defines DelegationStatusFilter and DELEGATION_STATUS_FILTERS with the allowed delegation statuses.
List handler validation
apps/backend/gateway/routes/delegations.ts
Parses status from the request URL, rejects unsupported values with VALIDATION_ERROR, and passes the optional filter into Delegation.findAll.
Unit coverage
tests/unit/package.json, tests/unit/src/delegation-status-filter.test.js
Enables module mocks for the unit runner and adds tests for unfiltered, valid, and invalid delegation status requests.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant listDelegationsHandler
  participant Delegation

  Client->>listDelegationsHandler: GET /api/v1/delegations?status=...
  listDelegationsHandler->>listDelegationsHandler: parse status from URL
  listDelegationsHandler->>listDelegationsHandler: validate against DELEGATION_STATUS_FILTERS

  alt unsupported status
    listDelegationsHandler-->>Client: 400 VALIDATION_ERROR
  else supported or absent
    listDelegationsHandler->>Delegation: findAll({ userId, status? })
    Delegation-->>listDelegationsHandler: delegations
    listDelegationsHandler-->>Client: 200 data[]
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A rabbit hopped through filters neat,
With pending, active, all complete.
Bad statuses? Thump! Gone from the burrow,
Good ones hop on in, swift and narrow. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: delegation status filter validation.
Linked Issues check ✅ Passed The route now validates supported status values, preserves unfiltered behavior, and adds tests covering valid and invalid cases.
Out of Scope Changes check ✅ Passed The changes stay focused on delegation status filtering and the minimal test setup needed to support the new route tests.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch feat/delegation-status-filter-validation

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
apps/backend/gateway/src/models/Delegation.ts (1)

4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Derive the union type from the constant to prevent contract drift.

Defining the same status literals twice creates a maintenance footgun. Make the array the source of truth and infer the union type from it.

Proposed refactor
-export type DelegationStatusFilter = 'pending' | 'active' | 'paused' | 'revoked' | 'expired';
-export const DELEGATION_STATUS_FILTERS: readonly DelegationStatusFilter[] = ['pending', 'active', 'paused', 'revoked', 'expired'];
+export const DELEGATION_STATUS_FILTERS = ['pending', 'active', 'paused', 'revoked', 'expired'] as const;
+export type DelegationStatusFilter = (typeof DELEGATION_STATUS_FILTERS)[number];
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/backend/gateway/src/models/Delegation.ts` around lines 4 - 5, The
Delegation status contract is duplicated between DelegationStatusFilter and
DELEGATION_STATUS_FILTERS, so make the constant the source of truth and derive
the union from it. Update DelegationStatusFilter in Delegation.ts to infer from
the literal array (using the existing DELEGATION_STATUS_FILTERS symbol) so the
type and list stay in sync when statuses change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/backend/gateway/routes/delegations.ts`:
- Line 147: The request parsing in delegations.ts uses req.headers.host as the
base for new URL, which is client-controlled and can cause parsing failures.
Update the URL construction near the req.url handling to use a ثابت fixed base
origin instead of req.headers.host, since only the query string needs to be
parsed, and keep the logic in the route handler that reads the URL query
parameters intact.

In `@tests/unit/package.json`:
- Line 7: The unit test command in package.json relies on
--experimental-test-module-mocks, which is not supported on all currently
allowed Node 20 runtimes. Update the repository’s Node version requirement and
CI/test runtime floor to a version that supports this flag (Node 20.18+ or
22.3+), and make sure the engine constraint and any version checks stay
consistent so tests won’t run on 20.0–20.17. Refer to the test script entry and
any engine/version configuration used for Node gating.

In `@tests/unit/src/delegation-status-filter.test.js`:
- Around line 17-25: The delegation status filter tests only verify response
codes, so they can miss a regression in the query filter. Update the relevant
cases in delegation-status-filter.test.js to assert that Delegation.findAll is
called with the expected where clause, using userId for the unfiltered request
and userId plus status for the filtered request, so the behavior is checked
through the Delegation.findAll mock as well as the response.

---

Nitpick comments:
In `@apps/backend/gateway/src/models/Delegation.ts`:
- Around line 4-5: The Delegation status contract is duplicated between
DelegationStatusFilter and DELEGATION_STATUS_FILTERS, so make the constant the
source of truth and derive the union from it. Update DelegationStatusFilter in
Delegation.ts to infer from the literal array (using the existing
DELEGATION_STATUS_FILTERS symbol) so the type and list stay in sync when
statuses change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0a7f8d2f-e85d-43fb-b28a-c1ae37c82275

📥 Commits

Reviewing files that changed from the base of the PR and between c6dd0d9 and 0ea9e2e.

📒 Files selected for processing (4)
  • apps/backend/gateway/routes/delegations.ts
  • apps/backend/gateway/src/models/Delegation.ts
  • tests/unit/package.json
  • tests/unit/src/delegation-status-filter.test.js

return;
}

const url = new url(/sitelet?url=https%3A%2F%2Fgithub.com%2FDelegoLabs%2FDelego%2Fpull%2Freq.url%2520%3F%3F%2520%26quot%3B%2F%26quot%3B%2C%2520%60http%3A%2F%2F%24%7Breq.headers.host%2520%3F%3F%2520%26quot%3Blocalhost%26quot%3B%7D%60);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Avoid using untrusted Host header as URL base in request parsing.

req.headers.host is client-controlled; malformed values can make new url(/sitelet?url=https%3A%2F%2Fgithub.com%2FDelegoLabs%2FDelego%2Fpull%2F...) throw and turn a bad query into a 500. Use a fixed base since only query parsing is needed.

Proposed fix
-    const url = new url(/sitelet?url=https%3A%2F%2Fgithub.com%2FDelegoLabs%2FDelego%2Fpull%2Freq.url%2520%3F%3F%2520%2522%2F%2522%2C%2520%60http%3A%2F%2F%24%7Breq.headers.host%2520%3F%3F%2520%2522localhost%2522%7D%60);
+    const url = new url(/sitelet?url=https%3A%2F%2Fgithub.com%2FDelegoLabs%2FDelego%2Fpull%2Freq.url%2520%3F%3F%2520%2522%2F%2522%2C%2520%2522http%3A%2F%2Flocalhost%2522);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const url = new URL(req.url ?? "/", `http://${req.headers.host ?? "localhost"}`);
const url = new URL(req.url ?? "/", "http://localhost");
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/backend/gateway/routes/delegations.ts` at line 147, The request parsing
in delegations.ts uses req.headers.host as the base for new URL, which is
client-controlled and can cause parsing failures. Update the URL construction
near the req.url handling to use a ثابت fixed base origin instead of
req.headers.host, since only the query string needs to be parsed, and keep the
logic in the route handler that reads the URL query parameters intact.

Comment thread tests/unit/package.json
"type": "module",
"scripts": {
"test": "node --test",
"test": "node --test --experimental-test-module-mocks",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Node version =="
node -v || true

echo "== tests/unit/package.json script =="
cat tests/unit/package.json | sed -n '1,80p'

echo "== engine/toolchain hints =="
fd -i 'package.json|.nvmrc|.node-version|.tool-versions|Dockerfile|ci|workflow' . \
  | sed -n '1,120p'

Repository: DelegoLabs/Delego

Length of output: 1145


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== root package.json (engines/scripts) =="
cat package.json | sed -n '1,220p'

echo "== Node/toolchain files =="
fd -HI '(^\.nvmrc$|^\.node-version$|^\.tool-versions$|^Dockerfile$|^package-lock\.json$|^pnpm-lock\.yaml$|^yarn\.lock$)' . -t f

echo "== CI/workflow files =="
fd -HI '(\.github/workflows/.*\.ya?ml$|ci|workflow)' . -t f

echo "== search for node version references =="
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' -e 'node\s*:?v?(\d+)' -e 'actions/setup-node' -e 'setup-node' -e 'engines' -e '18\.' -e '20\.' -e '22\.' .github . 2>/dev/null || true

Repository: DelegoLabs/Delego

Length of output: 20920


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== .github/workflows/ci.yml =="
cat -n .github/workflows/ci.yml | sed -n '1,260p'

Repository: DelegoLabs/Delego

Length of output: 5452


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== .github/workflows/ci.yml =="
cat -n .github/workflows/ci.yml | sed -n '1,260p'

Repository: DelegoLabs/Delego

Length of output: 5452


🌐 Web query:

Node.js --experimental-test-module-mocks introduced in which version supported Node 20

💡 Result:

The --experimental-test-module-mocks flag was introduced to the Node.js test runner in version 22.0.0 [1][2]. While it was developed and merged into the main branch in May 2024, it is not supported in Node.js 20 [1][2]. The feature, which enables mock.module functionality in the node:test runner, remains an experimental feature in newer releases [3][4][5]. Because module mocking relies on customization hooks that are also experimental, there have been no plans to backport this feature to Node.js 20 [2]. Users attempting to use mock.module in Node.js 20 will find the property undefined [6].

Citations:


🌐 Web query:

Node.js --experimental-test-module-mocks introduced in which version supported Node 20

💡 Result:

The --experimental-test-module-mocks flag was introduced in Node.js v22.3.0 [1][2][3]. While there were discussions regarding backporting this feature to Node.js 20.x, it was explicitly marked as "backport-blocked" in the Node.js repository [4], and the feature is not natively supported in Node.js 20 versions. To use module mocking via the node:test module, you must use a Node.js version that includes the feature, starting from v22.3.0 [5][1].

Citations:


🌐 Web query:

site:nodejs.org --experimental-test-module-mocks Node.js release notes

💡 Result:

The --experimental-test-module-mocks command-line flag is a feature in Node.js that enables module mocking within the built-in test runner [1][2]. Key details regarding this flag: Purpose: It allows developers to mock the exports of ECMAScript modules (ESM), CommonJS modules (CJS), and Node.js built-in modules during testing [2][3]. Stability: This feature is currently marked as having a stability index of 1.0 (Early development) [1]. Availability: The flag was introduced in Node.js v22.3.0 and v20.18.0 [1]. Usage: To utilize module mocking, you must start the Node.js process with this flag enabled [2][3]. If your environment uses the Node.js Permission Model, using this feature also requires the --allow-worker flag [1]. Implementation: Within tests, module mocking is accessed via the mock.module method provided by the test runner [2][4][5]. It is recommended to perform imports of the module to be tested dynamically after the mock has been configured to ensure the mock is correctly applied [4][5]. For detailed usage, you can refer to the official Node.js documentation for the test runner module [6][2].

Citations:


🌐 Web query:

site:nodejs.org v20.18.0 experimental-test-module-mocks node:test

💡 Result:

In Node.js v20.18.0, the --experimental-test-module-mocks command-line flag was introduced to enable experimental module mocking support in the built-in test runner [1][2][3]. Key details regarding this feature include: Experimental Status: As of its introduction, this feature is classified as Early Development (stability index 1.0) [2][4]. Usage: To use this feature, you must explicitly pass the --experimental-test-module-mocks flag when running your Node.js process [2][5]. If you are also using the Node.js Permission Model, this feature requires that you additionally pass the --allow-worker flag [2]. Functionality: This flag enables the mock.module(specifier[, options]) API within the node:test module [4][3]. This API allows for mocking the exports of ECMAScript modules (ESM), CommonJS modules (CJS), and Node.js built-in modules [4]. Important Implementation Note: When mocking modules, the mock must be configured before the module being mocked is imported or required by the code under test [6][5]. Because of this, it is common practice to use dynamic import statements after the mock has been set up to ensure the mocked version of the module is loaded instead of the original [6][5]. The mock.module API supports several configuration options, including: - cache: A boolean indicating whether to cache the mock module (default is false) [4]. - defaultExport: An optional value to serve as the module's default export [4]. - namedExports: An optional object to define the module's named exports [4]. This feature was part of the Node.js v20.18.0 release, which included several related commits to support module detection and refactor the mocking implementation [3][7].

Citations:


🌐 Web query:

site:nodejs.org "experimental-test-module-mocks" "v20.18.0" "v22.3.0"

💡 Result:

The flag --experimental-test-module-mocks is a Node.js command-line option used to enable module mocking functionality within the built-in test runner [1][2]. This feature was added in Node.js version 22.3.0 and backported to version 20.18.0 [1]. When enabled, it allows developers to mock the exports of ECMAScript modules (ESM), CommonJS modules (CJS), JSON modules, and Node.js built-in modules during testing [3][2]. This is typically achieved using the mock.module method provided by the node:test module [3][2]. If you are using the Permission Model in Node.js, using this feature also requires passing the --allow-worker flag [1].

Citations:


Raise the Node floor for --experimental-test-module-mocks

The flag requires Node 20.18+ (or 22.3+), but the repo still advertises >=20.0.0. Tighten the engine/CI version so 20.0–20.17 runtimes don’t break the unit tests.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/package.json` at line 7, The unit test command in package.json
relies on --experimental-test-module-mocks, which is not supported on all
currently allowed Node 20 runtimes. Update the repository’s Node version
requirement and CI/test runtime floor to a version that supports this flag (Node
20.18+ or 22.3+), and make sure the engine constraint and any version checks
stay consistent so tests won’t run on 20.0–20.17. Refer to the test script entry
and any engine/version configuration used for Node gating.

Comment on lines +17 to +25
await mock.module(`${root}/apps/backend/gateway/dist/src/models/index.js`, {
namedExports: {
Delegation: { findAll: async () => [] },
DelegationPolicy: {},
SpendLimit: {},
PermissionLevel: {},
Wallet: {},
},
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add assertions that Delegation.findAll receives the expected where filter.

Right now the suite checks status codes only, so it would still pass if query filtering regressed. Assert where: { userId } for unfiltered and where: { userId, status } for filtered requests.

Also applies to: 62-70

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/src/delegation-status-filter.test.js` around lines 17 - 25, The
delegation status filter tests only verify response codes, so they can miss a
regression in the query filter. Update the relevant cases in
delegation-status-filter.test.js to assert that Delegation.findAll is called
with the expected where clause, using userId for the unfiltered request and
userId plus status for the filtered request, so the behavior is checked through
the Delegation.findAll mock as well as the response.

@ScriptedBro

Copy link
Copy Markdown
Contributor

Resolve conflicts

@ScriptedBro

Copy link
Copy Markdown
Contributor

Will be closed as conflicts weren't resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Gateway] Add Delegation Status Filter Validation

2 participants