Skip to content

Conversation

@nicktrn
Copy link
Collaborator

@nicktrn nicktrn commented Oct 28, 2025

Deploys would silently overwrite concurrency settings again. Also fixes a couple of bugs in the override modal.

@changeset-bot
Copy link

changeset-bot bot commented Oct 28, 2025

⚠️ No Changeset found

Latest commit: 0f7b8cd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

These changes refactor queue concurrency limit handling across two files. The first file updates QueueOverrideConcurrencyButton to accept a QueueItem type instead of an inline object, adjusting property access from queue.concurrencyLimit to queue.concurrency?.base. The second file introduces distinction between base and overridden concurrency limits in the queue creation/upsert logic, adding override detection via concurrencyLimitOverriddenAt and new conditional logic to preserve existing overrides while applying limit changes appropriately.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Override detection and preservation logic in createBackgroundWorker.server.ts — verify that concurrencyLimitOverriddenAt checks correctly identify when an override exists and that the payload adjustments appropriately preserve vs. update limits
  • Data model consistency between files — ensure the new QueueItem type structure and property access patterns (queue.concurrency?.base, queue.concurrency.overriddenAt) are applied consistently
  • Conditional logic flow in the route component — confirm that condition text and max attribute constraints correctly reflect the new data structure and override semantics
  • Paused queue handling — verify the new branch for paused queues integrates correctly with existing concurrency limit logic

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description provided is largely incomplete compared to the required template structure. While the description contains useful information about the fix ("Deploys would silently overwrite concurrency settings again") and mentions additional bug fixes in the override modal, it is missing critical required sections including the issue reference (Closes #), the entire checklist with contributing guide confirmation, the Testing section describing test steps, and the properly formatted Changelog section. The description is one brief line rather than following the structured template format expected by the repository. The PR description should be updated to follow the repository template structure. Please add the issue reference at the top (Closes #), complete all three checklist items confirming compliance with the contributing guide and code quality standards, add a Testing section describing the verification steps taken, and provide a Changelog section with a concise summary of the changes. This will ensure the PR metadata is complete and consistent with repository standards.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "fix(webapp): persist concurrency overrides on deploy" directly aligns with the main objective of the changeset. The title clearly and specifically describes the primary fix—preventing concurrency settings from being overwritten during deployment. The changes in both modified files support this objective: the route component now uses the QueueItem type for better type safety, while the worker creation service adds logic to detect and preserve concurrency overrides when upserting queue records. The title is concise, avoids vague terminology, and accurately conveys the core purpose of the PR.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/persist-concurrency-overrides

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8fdbbeb and 0f7b8cd.

📒 Files selected for processing (2)
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx (4 hunks)
  • apps/webapp/app/v3/services/createBackgroundWorker.server.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx
  • apps/webapp/app/v3/services/createBackgroundWorker.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

  • apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx
  • apps/webapp/app/v3/services/createBackgroundWorker.server.ts
apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

  • apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx
  • apps/webapp/app/v3/services/createBackgroundWorker.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly

Files:

  • apps/webapp/app/v3/services/createBackgroundWorker.server.ts
apps/webapp/app/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead

Files:

  • apps/webapp/app/v3/services/createBackgroundWorker.server.ts
🧬 Code graph analysis (2)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx (1)
packages/core/src/v3/schemas/queues.ts (2)
  • QueueItem (16-50)
  • QueueItem (52-52)
apps/webapp/app/v3/services/createBackgroundWorker.server.ts (2)
internal-packages/run-engine/src/run-queue/index.ts (1)
  • updateQueueConcurrencyLimits (314-320)
apps/webapp/app/v3/marqs/index.server.ts (1)
  • updateQueueConcurrencyLimits (177-183)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
apps/webapp/app/v3/services/createBackgroundWorker.server.ts (2)

362-377: Clamp looks correct and defensive.

Good rename to baseConcurrencyLimit and clamp against env/org ceilings with lower bound 0. No issues.


383-396: Resume path properly reapplies concurrency limits to Redis.

The concern has been verified. In pauseQueue.server.ts lines 60–68, when a queue is resumed, the service correctly reapplies the concurrency limit from the database to Redis: if queue.concurrencyLimit exists, it calls updateQueueConcurrencyLimits() with that value; otherwise, it calls removeQueueConcurrencyLimits(). This ensures no stale Redis limits remain after resuming a paused queue. The code in createBackgroundWorker.server.ts correctly skips Redis updates during pause, relying on this resume-time sync.

apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx (3)

15-16: Good: use QueueItem from subpath export.

Type-only import from @trigger.dev/core/v3/schemas matches our webapp import rule.


930-932: Prop now typed to QueueItem — aligns UI with API.

Solid change that reduces drift between presenter data and UI.


969-975: Clearer messaging using concurrency.base.

Conditionally surfacing the original code limit via concurrency.base improves UX. Looks good.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@matt-aitken matt-aitken merged commit 2283ca6 into main Oct 28, 2025
31 checks passed
@matt-aitken matt-aitken deleted the fix/persist-concurrency-overrides branch October 28, 2025 11:28
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.

3 participants