fix(slack): use environment id for slack connection id with legacy fallback by pfreixes · Pull Request #5559 · NangoHQ/nango

pfreixes · 2026-03-04T10:17:59Z

Renaming an environment broke Slack alerts because the connection id was built using the environment name. Switch to environment id (stable) for new installations and add a fallback to the legacy name-based id for existing ones.

Tests run in local:

Test that new slack alerts are resilient to an environment name change
Test that legacy slack alerts are still working when they did not suffer an environment name change
Test that once legacy slack alerts once renamed they work

The update also applies the dual-lookup behavior across server-side Slack channel resolution, notification sending, and webapp disconnect flows, with connectSlack now constructing IDs from envId, and it expands API typing plus adds integration tests for the current-environment endpoint to validate Slack channel resolution for both new and legacy formats.

Possible Issues

• Legacy disconnect retry in slackDisconnect only triggers on 404; other failure statuses may leave legacy connections undeleted.

This summary was automatically generated by @propel-code-bot

…llback Renaming an environment broke Slack alerts because the connection id was built using the environment name. Switch to environment id (stable) for new installations and add a fallback to the legacy name-based id for existing ones. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

linear · 2026-03-04T10:18:03Z

NAN-3540 Renaming an env breaks the Slack notifications (Cloud + Self-hosted)

…isconnect Only fall back to legacy name-based connection id when the error is specifically unknown_connection, not on any failure. Also handle disconnect for legacy name-based connections by retrying with the old format on 404. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

TBonnin

lgtm. I don't think this endpoint has any integration tests yet. Might be a good time to add some :)

TBonnin · 2026-03-04T15:56:23Z

packages/shared/lib/services/notification/slack.service.ts

+            this.integrationKey,
+            adminRes.environment.id
+        );
+        if (!slackConnectionResult.success && slackConnectionResult.error?.type === 'unknown_connection') {


isn't !slackConnectionResult.success redundant?

Yeps good point! let me remove it!

…tests - Fix getEnvironment fallback condition from !connectionConfig to !connectionConfig['incoming_webhook.channel'] since getConnectionConfig always returns {} on miss, never null - Remove redundant !slackConnectionResult.success check in slack.service.ts - Add integration tests for GET /api/v1/environments/current covering auth, env param enforcement, basic data, and both slack connection ID formats Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

pfreixes · 2026-03-05T09:26:52Z

@TBonnin @rossmcewan thanks for the review, Ive added some new integration tests and thanks to that Ive caught a bug that I was not aware of!

A new review would be very appreciate it!

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

propel-code-bot

No issues found; changes look solid and align with the review focus.

Status: No Issues Found | Risk: Low

Review Details

📁 7 files reviewed | 💬 0 comments

Instruction Files

├── .claude/
│   ├── agents/
│   │   └── nango-docs-migrator.md
│   └── skills/
│       ├── agent-builder-skill/
│       │   ├── EXAMPLES.md
│       │   └── SKILL.md
│       ├── creating-integration-docs/
│       │   └── SKILL.md
│       └── creating-skills-skill/
│           └── SKILL.md
├── AGENTS.md
└── GEMINI.md

@ts-expect-error

… in tests Consistent with other environment endpoint types (PatchEnvironment etc.) which don't define Querystring and use @ts-expect-error in tests instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-authored-by: propel-code-bot[bot] <203372662+propel-code-bot[bot]@users.noreply.github.com>

TBonnin · 2026-03-06T01:06:57Z

packages/server/lib/controllers/v1/environment/getEnvironment.integration.test.ts

+
+        afterEach(() => {
+            (envs as any).NANGO_ADMIN_UUID = originalAdminUUID;
+        });


I would create the admin account and slack config in beforeAll so it doesn't need to be done in each test.

Good point done here.

TBonnin · 2026-03-06T01:08:16Z

packages/server/lib/controllers/v1/environment/getEnvironment.ts

-                connection_id: connectionId
+                connection_id: generateSlackConnectionId(account.uuid, environment.id)
            });
+            if (!connectionConfig || !connectionConfig['incoming_webhook.channel']) {


why !connectionConfig['incoming_webhook.channel']

bc getConnectionConfig can either return an empty object or a null, see this. From my understanding, I could be wrong here since my JS is a bit rusty, when there is no record in the DB we return an empty object, but when there is a record we just return the connection_config column which can be null.

Indeed this was the original pattern, see this

TBonnin · 2026-03-06T01:34:18Z

packages/shared/lib/services/notification/slack.service.ts

+            this.integrationKey,
+            adminRes.environment.id
+        );
+        if (slackConnectionResult.error?.type === 'unknown_connection') {


can we be consistent and use the same fallback condition here and in getEnvironment

Let me know if Im missing something here, but both are not semantically equivalent. While this one just makes a look up to the db for checking if a connection exists for enabling the dual look up (originally there was an expectation that this function should always return a record and should be attributed to the connection slack one, otherwise this would mean that the dataset was corrupted or there was a bug - which is what is happening Today) the getEnvironment one uses the getConnectionConfig which is kinda different.

We could try to use in both cases connectionService.getConnection, but this is what we already had in place before. Also remember that we will remove this dual look up.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

This comment was marked as outdated.

Sign in to view

pfreixes temporarily deployed to development March 4, 2026 10:44 — with GitHub Actions Inactive

pfreixes requested a review from a team March 4, 2026 15:31

rossmcewan approved these changes Mar 4, 2026

View reviewed changes

TBonnin approved these changes Mar 4, 2026

View reviewed changes

pfreixes mentioned this pull request Mar 4, 2026

feat(db): migrate Slack connection IDs from env name to env id #5564

Draft

pfreixes requested review from TBonnin and rossmcewan March 5, 2026 09:24

pfreixes and others added 2 commits March 5, 2026 10:29

fix(types): add GetEnvironment to APIEndpoints union

9b4fa78

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(types): add Querystring to GetEnvironment type

6bb40be

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

This comment was marked as outdated.

Sign in to view

fix(test): add ts-expect-error for intentionally missing query param

1f39216

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

propel-code-bot bot reviewed Mar 5, 2026

View reviewed changes

pfreixes and others added 2 commits March 5, 2026 10:45

Apply suggestion from @propel-code-bot[bot]

fc6f123

Co-authored-by: propel-code-bot[bot] <203372662+propel-code-bot[bot]@users.noreply.github.com>

TBonnin reviewed Mar 6, 2026

View reviewed changes

TBonnin self-requested a review March 6, 2026 02:43

refactor(test): move admin account setup to beforeAll in slack tests

14fb655

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Conversation

pfreixes commented Mar 4, 2026 • edited by propel-code-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear bot commented Mar 4, 2026

Uh oh!

This comment was marked as outdated.

Uh oh!

TBonnin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfreixes commented Mar 5, 2026

Uh oh!

This comment was marked as outdated.

Uh oh!

propel-code-bot bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pfreixes commented Mar 4, 2026 •

edited by propel-code-bot bot

Loading

propel-code-bot bot left a comment •

edited

Loading