Skip to content

fix(webapp): stop saving global flags from unsetting the locked ones - #4751

Open
d-cs wants to merge 2 commits into
mainfrom
fix-global-flags-locked-deletion
Open

fix(webapp): stop saving global flags from unsetting the locked ones#4751
d-cs wants to merge 2 commits into
mainfrom
fix-global-flags-locked-deletion

Conversation

@d-cs

@d-cs d-cs commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

On a self-hosted instance, saving anything on the global admin feature flags page also deleted the two read-only flags, defaultWorkerInstanceGroupId and taskEventRepository. Losing the first one leaves deployed runs with no default worker group. Neither deletion showed up in the confirm dialog, so the flags disappeared silently.

Root cause

The page submits only the flags its UI is managing, and strips the read-only ones from the payload unless "Unlock read-only flags" is ticked. The action treated every catalog key absent from that payload as "the admin unset this", and protected the locked keys only when the instance was managed cloud. Anywhere else, both locked rows fell straight into the delete sweep.

The protection now keys off what the client says it was editing rather than off the deployment:

const canDeleteLocked = params.unlockLockedFlags && !params.isManagedCloud;
...
} else if (canDeleteLocked || !GLOBAL_LOCKED_FLAGS.includes(key)) {
  keysToDelete.push(key);
}

Exactly one case changes: a locked flag, on a non managed-cloud instance, with the flags not unlocked, is now kept instead of deleted. Managed cloud behaviour is bit for bit identical, and ticking the unlock box still gives a self-hosted instance full control. The write moves into replaceGlobalFeatureFlags so it can be driven directly in tests against a real Postgres.

The admin flags page submits only the flags its UI is managing, and strips
the read-only ones unless they are unlocked. The action read every absent
catalog key as an unset, so on a self-hosted instance any save deleted
defaultWorkerInstanceGroupId and taskEventRepository as well.
@changeset-bot

changeset-bot Bot commented Aug 21, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 0136d39

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

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bedabf36-be31-4c99-b4cc-d44a35e4ee7d

📥 Commits

Reviewing files that changed from the base of the PR and between ccbe4e9 and 0136d39.

📒 Files selected for processing (1)
  • apps/webapp/app/v3/featureFlags.server.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (33)
  • GitHub Check: report
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
  • GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
  • GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
  • GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
  • GitHub Check: typecheck / typecheck
  • GitHub Check: runops-guard / runops-guard
  • GitHub Check: fk-cascade-guard / fk-cascade-guard
  • GitHub Check: code-quality / code-quality
  • GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}

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

**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead

**/*.{ts,tsx}: Prefer static imports over dynamic imports. Only use dynamic import() when:

  • Circular dependencies cannot be resolved otherwise
  • Code splitting is genuinely needed for performance
  • The module must be loaded conditionally at runtime

Files:

  • apps/webapp/app/v3/featureFlags.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}

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

Use zod for validation in packages/core and apps/webapp

Files:

  • apps/webapp/app/v3/featureFlags.server.ts
**/*.{ts,tsx,js,jsx}

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

Use function declarations instead of default exports

Files:

  • apps/webapp/app/v3/featureFlags.server.ts
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)

**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries

Files:

  • apps/webapp/app/v3/featureFlags.server.ts
apps/webapp/**/*.{ts,tsx}

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

apps/webapp/**/*.{ts,tsx}: Access environment variables through the env export of env.server.ts instead of directly accessing process.env
Use subpath exports from @trigger.dev/core package instead of importing from the root @trigger.dev/core path

Files:

  • apps/webapp/app/v3/featureFlags.server.ts
apps/webapp/app/**/*.{ts,tsx}

📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)

apps/webapp/app/**/*.{ts,tsx}: For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
Use useCallback and useMemo only for context provider values, expensive derived data used as a dependency, or stable references required by dependency arrays; do not wrap ordinary event handlers or trivial computations.
Use named constants for sentinel or placeholder values instead of scattering raw string literals across comparisons.

Files:

  • apps/webapp/app/v3/featureFlags.server.ts
apps/webapp/app/**/*.ts

📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)

apps/webapp/app/**/*.ts: Never use request.signal to detect client disconnects. Use getRequestAbortSignal() from app/services/httpAsyncStorage.server.ts, which is wired to Express response close events.
Access environment variables through the env export from app/env.server.ts; never use process.env directly.
Always use Prisma findFirst instead of findUnique.
Always use the $transaction helper from ~/db.server, never call prisma.$transaction or $replica.$transaction directly. Pass isolation levels as strings, use Serializable for correctness-critical read-then-write invariants, and guard possibly undefined helper results when a definite value is required.

Files:

  • apps/webapp/app/v3/featureFlags.server.ts
apps/webapp/app/v3/**/*.ts

📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)

New code must target Run Engine V2 through the singleton in app/v3/runEngine.server.ts; do not reintroduce V1 execution paths. V1 branches may only reject or finalize gracefully with a clean 4xx.

Files:

  • apps/webapp/app/v3/featureFlags.server.ts
**/*

📄 CodeRabbit inference engine (AGENTS.md)

**/*: Add crumbs as you write code — not just when debugging. Mark lines with
// @Crumbs or wrap blocks in `// `#region` `@crumbs. They stay on the
branch throughout development and are stripped by agentcrumbs strip
before merge.
Do not invent new namespaces — pick from this table or ask first.
Do not flag // @Crumbs markers, `#region `@crumbs blocks, or agentcrumbs
imports in reviews. These are temporary debug instrumentation stripped
before merge. Data logged in crumbs (IDs, names, values) never reaches
production.

Files:

  • apps/webapp/app/v3/featureFlags.server.ts
🧠 Learnings (1)
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.

Applied to files:

  • apps/webapp/app/v3/featureFlags.server.ts
🔇 Additional comments (2)
apps/webapp/app/v3/featureFlags.server.ts (2)

3-3: LGTM!

Also applies to: 243-252


254-268: 🗄️ Data Integrity & Integration

Do not add a serialization mechanism. Each replacement either upserts or deletes every editable catalog key. PostgreSQL row locks serialize successful concurrent writes; conflicting schedules can roll back one transaction, but cannot commit a mixed final state.

			> Likely an incorrect or invalid review comment.

Walkthrough

The admin feature-flag route now delegates persistence to replaceGlobalFeatureFlags. The action accepts an optional locked-flag unlock directive, and save requests send the current UI unlock state. The persistence function transactionally upserts requested catalog flags and deletes omitted flags. It preserves locked flags unless self-hosted deployments explicitly unlock them. PostgreSQL integration tests cover self-hosted and managed-cloud behavior, ordinary flag removal, and flag updates.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description explains the issue and fix, but it omits the required issue link, checklist, testing, changelog, and screenshots sections. Add the missing template sections, link the closing issue, record testing steps, summarize the changelog, and state whether screenshots are applicable.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly describes the primary fix: preventing global feature-flag saves from deleting locked flags.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-global-flags-locked-deletion

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.

@d-cs d-cs self-assigned this Aug 21, 2026
coderabbitai[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

…n helper

Use the $transaction helper from ~/db.server instead of calling
client.$transaction directly, so the write gets tracing and infra-error
boundary logging. The helper is callback-only, so the batched upserts
become sequential statements inside one interactive transaction, and an
undefined result is treated as a failure rather than a silent no-op.
@d-cs
d-cs marked this pull request as ready for review August 21, 2026 15:55
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.

1 participant