Skip to content
Closed
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
23 changes: 16 additions & 7 deletions apps/webapp/app/services/runsRepository/runsRepository.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -361,13 +361,22 @@ export async function convertRunListInputOptionsToFilterRunsOptions(
period: undefined,
};

// Convert time period to ms
const time = timeFilters({
period: options.period,
from: options.from,
to: options.to,
});
convertedOptions.period = time.period ? (parseDuration(time.period) ?? undefined) : undefined;
// Convert time period to ms.
// Explicit run-id selections target specific, already-existing runs, so we
// don't apply a default time bound (which could otherwise exclude a selected run).
const hasExplicitRunIds = Boolean(options.runId && options.runId.length > 0);
Comment on lines +365 to +367

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Bulk action fix lacks release note

When convertRunListInputOptionsToFilterRunsOptions includes older selected runs, users see a server-only behavior fix without a release note. The release changelog omits this fix.

Learn more

Explicit run-ID selections now include older runs in counts and bulk actions. This is a user-facing change in the webapp, with no package changes. The repository's server-change policy requires a .server-changes/ file for such fixes, but this PR adds none. Users reading the release notes will miss a change they may have encountered.

Example: A user whose selected 30-day-old run was previously skipped can now cancel it through a bulk action, but the server release summary does not mention this correction.

Recommended fix: Add a .server-changes/ Markdown entry with area: webapp, type: fix, and a one-line description of the user-visible behavior.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

const hasExplicitTimeFilter = Boolean(
options.period !== undefined || options.from !== undefined || options.to !== undefined
);

if (!hasExplicitRunIds || hasExplicitTimeFilter) {
const time = timeFilters({
period: options.period,
from: options.from,
to: options.to,
});
convertedOptions.period = time.period ? (parseDuration(time.period) ?? undefined) : undefined;
}

// Cross-DB resolution: BatchTaskRun is a RUN-OPS table. A run-ops batch resident on the
// dedicated run-ops DB must resolve via the store's NEW->LEGACY probe — a single control-plane
Expand Down
114 changes: 114 additions & 0 deletions apps/webapp/test/runsRepositoryConvert.runIds.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import { describe, expect, it, vi } from "vitest";

// Prevent run-store / db / env singletons from constructing at import
vi.mock("~/db.server", () => ({}));
vi.mock("~/v3/runStore.server", () => ({ runStore: {} }));
vi.mock("~/env.server", () => ({ env: {} }));
Comment on lines +4 to +6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Run-filter tests use forbidden mocks

The new run-filter tests use vi.mock to replace three production dependencies. Repository testing rules prohibit mocks and require real dependencies, so these tests violate the prescribed test strategy.

Learn more

The webapp tests replace database, run-store, and environment modules before loading the converter. The repository's testing instructions prohibit mocks, stubs, and fakes; these tests also pass placeholder objects as dependencies. Keeping those replacements means the tests do not exercise the prescribed production dependency setup.

Example: The first case passes {} as the store and Prisma client, then checks only the converted period and IDs. It never runs the resulting filters against a real repository.

Recommended fix: Restructure the tests around real dependencies, using the repository's testcontainers helpers where database services are needed. If the converter needs isolation, extract pure time-filter conversion into a dependency-free helper and test that without mocking imports or dependencies.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.


import { convertRunListInputOptionsToFilterRunsOptions } from "~/services/runsRepository/runsRepository.server";

describe("convertRunListInputOptionsToFilterRunsOptions with explicit runIds vs filters (#5110)", () => {
const mockPrisma = {} as any;
const mockStore = {} as any;

it("leaves period undefined when explicit runIds are supplied without time filters", async () => {
const result = await convertRunListInputOptionsToFilterRunsOptions(
{
organizationId: "org_123",
projectId: "proj_123",
environmentId: "env_123",
runId: ["run_recent", "run_older_than_7_days"],
},
mockPrisma,
mockStore
);

// Period must remain undefined so runs older than 7 days are not silently excluded (#5110)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Run-filter test lacks regression annotation

The new unit test's // Period must remain comment lacks the required // Catches: <bug> regression annotation. Repository rules require that one-line annotation for new unit tests.

Suggested change
// Period must remain undefined so runs older than 7 days are not silently excluded (#5110)
// Catches: explicit run IDs no longer exclude runs older than the default seven-day period.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

expect(result.period).toBeUndefined();
expect(result.runId).toEqual(["run_recent", "run_older_than_7_days"]);
});

it("applies the default 7d period when no explicit runIds are supplied (filter-only)", async () => {
const result = await convertRunListInputOptionsToFilterRunsOptions(
{
organizationId: "org_123",
projectId: "proj_123",
environmentId: "env_123",
statuses: ["COMPLETED_SUCCESSFULLY"],
},
mockPrisma,
mockStore
);

expect(result.period).toBe(7 * 24 * 60 * 60 * 1000); // 604800000 ms
});

it("applies the default 7d period when runId is an empty array", async () => {
const result = await convertRunListInputOptionsToFilterRunsOptions(
{
organizationId: "org_123",
projectId: "proj_123",
environmentId: "env_123",
runId: [],
},
mockPrisma,
mockStore
);

expect(result.period).toBe(7 * 24 * 60 * 60 * 1000);
});

it("honors explicit period when supplied alongside runIds", async () => {
const result = await convertRunListInputOptionsToFilterRunsOptions(
{
organizationId: "org_123",
projectId: "proj_123",
environmentId: "env_123",
runId: ["run_1"],
period: "30d",
},
mockPrisma,
mockStore
);

expect(result.period).toBe(30 * 24 * 60 * 60 * 1000);
expect(result.runId).toEqual(["run_1"]);
});

it("honors explicit from/to when supplied alongside runIds without defaulting period", async () => {
const from = Date.now() - 30 * 24 * 60 * 60 * 1000;
const to = Date.now();
const result = await convertRunListInputOptionsToFilterRunsOptions(
{
organizationId: "org_123",
projectId: "proj_123",
environmentId: "env_123",
runId: ["run_1"],
from,
to,
},
mockPrisma,
mockStore
);

expect(result.period).toBeUndefined();
expect(result.from).toBe(from);
expect(result.to).toBe(to);
expect(result.runId).toEqual(["run_1"]);
});

it("honors custom period on filter-only queries", async () => {
const result = await convertRunListInputOptionsToFilterRunsOptions(
{
organizationId: "org_123",
projectId: "proj_123",
environmentId: "env_123",
period: "14d",
},
mockPrisma,
mockStore
);

expect(result.period).toBe(14 * 24 * 60 * 60 * 1000);
});
});