Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ apiTest.describe(
expect(response.body.steps).toBeDefined();
expect(Array.isArray(response.body.steps)).toBe(true);

// Registered ⊆ approved: every step returned by the server must be listed in the fixture
// with a matching handler hash (catches new or changed handlers not yet approved).
for (const step of response.body.steps) {
const approvedStep = APPROVED_STEP_DEFINITIONS.find(({ id }) => id === step.id);

Expand All @@ -57,6 +59,22 @@ apiTest.describe(
message: `Step "${step.id}" has an invalid handler hash`,
}).toBe(approvedStep?.handlerHash);
}
Comment on lines 51 to 61
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

As with triggers, the two loops do repeated .find(...) lookups, leading to O(n²) behavior. Consider indexing response.body.steps (and/or APPROVED_STEP_DEFINITIONS) into a Map once to make both the forward and reverse checks O(n) and reduce duplicated lookup logic.

Copilot uses AI. Check for mistakes.

// Approved ⊆ registered: every fixture row must still exist on the server with the same hash
// (catches stale rows left after a step was removed or the list was not updated).
for (const approved of APPROVED_STEP_DEFINITIONS) {
const registeredStep = response.body.steps.find(
(step: { id: string; handlerHash: string }) => step.id === approved.id
);
Comment on lines +65 to +68
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

As with triggers, the two loops do repeated .find(...) lookups, leading to O(n²) behavior. Consider indexing response.body.steps (and/or APPROVED_STEP_DEFINITIONS) into a Map once to make both the forward and reverse checks O(n) and reduce duplicated lookup logic.

Copilot uses AI. Check for mistakes.

expect(registeredStep, {
message: `Approved list contains stale entry "${approved.id}" that is not registered`,
}).toBeDefined();

expect(registeredStep?.handlerHash, {
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

Similar to the trigger spec, optional chaining in the second assertion (registeredStep?.handlerHash) is unnecessary after asserting registeredStep is defined and can obscure intent/type safety. Prefer direct access after the defined-assertion (e.g., using non-null assertion or control flow that guarantees presence).

Suggested change
expect(registeredStep?.handlerHash, {
expect(registeredStep!.handlerHash, {

Copilot uses AI. Check for mistakes.
message: `Approved entry "${approved.id}" has an invalid handler hash (does not match registered step)`,
}).toBe(approved.handlerHash);
}
}
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ apiTest.describe(
).toBe(true);
expect(Array.isArray(response.body.triggers)).toBe(true);

// Registered ⊆ approved: every trigger returned by the server must be listed in the fixture
// with a matching schema hash (catches new or changed schemas not yet approved).
for (const trigger of response.body.triggers) {
const approvedTrigger = APPROVED_TRIGGER_DEFINITIONS.find(({ id }) => id === trigger.id);

Expand All @@ -59,6 +61,22 @@ apiTest.describe(
message: `Trigger "${trigger.id}" has an invalid schema hash`,
}).toBe(trigger.schemaHash);
}
Comment on lines 53 to 63
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

Both directions currently do linear .find(...) inside loops, which makes the test O(n²) over the number of triggers. Consider building a Map/object index once for response.body.triggers (and optionally for APPROVED_TRIGGER_DEFINITIONS) and doing constant-time lookups; this improves runtime and avoids duplicating lookup logic in two loops.

Copilot uses AI. Check for mistakes.

// Approved ⊆ registered: every fixture row must still exist on the server with the same hash
// (catches stale rows left after a trigger was removed or the list was not updated).
for (const approved of APPROVED_TRIGGER_DEFINITIONS) {
const registeredTrigger = response.body.triggers.find(
(trigger: { id: string; schemaHash: string }) => trigger.id === approved.id
);
Comment on lines +67 to +70
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

Both directions currently do linear .find(...) inside loops, which makes the test O(n²) over the number of triggers. Consider building a Map/object index once for response.body.triggers (and optionally for APPROVED_TRIGGER_DEFINITIONS) and doing constant-time lookups; this improves runtime and avoids duplicating lookup logic in two loops.

Copilot uses AI. Check for mistakes.

expect(registeredTrigger, {
message: `Approved list contains stale entry "${approved.id}" that is not registered`,
}).toBeDefined();

expect(registeredTrigger?.schemaHash, {
message: `Approved entry "${approved.id}" has an invalid schema hash (does not match registered trigger)`,
}).toBe(approved.schemaHash);
Comment on lines +72 to +78
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

After asserting registeredTrigger is defined, the follow-up assertion still uses optional chaining (registeredTrigger?.schemaHash). This can make failures harder to interpret and weakens type safety. Prefer asserting defined and then accessing the value directly (e.g., via non-null assertion or by restructuring so the second expect only runs when defined).

Copilot uses AI. Check for mistakes.
}
}
);
}
Expand Down
Loading