fix(web,server): show real platform connection status in Settings (closes #1031)#1061
Merged
Wirasm merged 1 commit intocoleam00:devfrom Apr 21, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughA mutable Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/adapters/src/chat/telegram/adapter.ts`:
- Around line 196-217: The promise currently resolves inside the telegraf
onLaunch callback (this.bot.launch(..., () => { ... resolve() })), but onLaunch
fires before polling/getUpdates begin so 409 errors surface after resolve and
bypass the retry try/catch; change the logic so the outer promise does not
resolve on onLaunch but only after polling is actually started or the launch()
promise resolves/rejects: e.g., remove resolving from the onLaunch callback and
instead await/attach to the Promise returned by this.bot.launch(), or listen for
an event that confirms startPolling (or handle 'polling_error') and resolve only
when polling is confirmed, ensuring .catch on the launch promise can reject and
trigger the existing retry loop (relate to this.bot.launch, onLaunch,
startPolling, getUpdates).
In `@packages/server/src/index.ts`:
- Around line 490-494: activePlatforms is initialized once but not updated when
other adapters finish starting, causing /api/health to report stale data; after
each adapter successfully starts (e.g., when calling start() on
Telegram/Discord/etc.), immediately push that adapter's platform identifier into
the existing activePlatforms array (use the same array passed into
registerApiRoutes) — for example, in the promise resolution or async/await right
after adapter.start() do activePlatforms.push('Telegram') or
activePlatforms.push(adapter.platformName) so the health endpoint reflects
adapters as soon as they are up (ensure this change is applied in each adapter
startup location that currently only starts the adapter without mutating
activePlatforms).
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 615-619: The settings UI is missing the server-reported platforms
Gitea and GitLab, so update the static platforms list in SettingsPage (the array
that currently contains { name: 'Web', connected: active.has('Web') }, ... {
name: 'GitHub', connected: active.has('GitHub') }) to include { name: 'Gitea',
connected: active.has('Gitea') } and { name: 'GitLab', connected:
active.has('GitLab') }, or replace the hardcoded list with a dynamic map derived
from the server-provided active set (using the existing active variable and
active.has checks) so the UI reflects whatever platforms the server reports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fe1e94b-bdd9-4deb-a021-a4c4d40092e2
📒 Files selected for processing (5)
packages/adapters/src/chat/telegram/adapter.tspackages/server/src/index.tspackages/server/src/routes/api.tspackages/web/src/lib/api.tspackages/web/src/routes/SettingsPage.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/adapters/src/chat/telegram/adapter.test.ts (1)
247-258: Add one regression test for a 409 that happens afteronLaunch.These cases still only exercise immediate
launch()rejection. The new production path is the opposite ordering:onLaunchfires, then polling startup fails during the grace window. Without that case, the core race this PR fixes is still unproven.🧪 Suggested test shape
+ test('should retry when polling fails after onLaunch but before grace settles', async () => { + const adapter = new TelegramAdapter('fake-token-for-testing'); + const conflictError = new Error('409: Conflict: terminated by other getUpdates request'); + let callCount = 0; + + const mockLaunch = mock((_config: unknown, onLaunch?: () => void): Promise<void> => { + callCount++; + if (callCount === 1) { + onLaunch?.(); + return new Promise((_, reject) => setTimeout(() => reject(conflictError), 0)); + } + onLaunch?.(); + return new Promise(() => {}); + }); + + (adapter.getBot() as unknown as { launch: typeof mockLaunch }).launch = mockLaunch; + + await adapter.start({ retryDelayMs: 0, pollingGraceMs: 10 }); + expect(mockLaunch).toHaveBeenCalledTimes(2); + });As per coding guidelines "Prefer reproducible commands in CI-sensitive paths; keep tests deterministic without flaky timing or network dependence; ensure local validation commands map to CI expectations."
Also applies to: 286-296, 305-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/adapters/src/chat/telegram/adapter.test.ts` around lines 247 - 258, Add a deterministic regression test that simulates a 409 Conflict occurring after the telegraf onLaunch callback (to exercise the race where polling fails during the grace window) by updating the mocked launch used in the existing test: have mockLaunch call onLaunch() immediately, then simulate a polling-start failure that rejects with new Error('409: Conflict: terminated by other getUpdates request') at a controlled time (within the pollingGraceMs window) instead of rejecting immediately; use adapter.getBot().launch = mockLaunch and call adapter.start({ retryDelayMs: 0, pollingGraceMs: 0 or small value }) and assert the adapter handles the post-onLaunch 409 the same as the pre-onLaunch case (retries/continues) so the race is exercised deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 608-623: The platforms list currently marks integrations as not
connected whenever activePlatforms is undefined; change the connected value
computation in PlatformConnectionsSection so it is tri-state
(true/false/undefined) by setting connected: activePlatforms ? active.has('Web')
: undefined (and likewise for each platform name) instead of active.has(...),
and update the component rendering logic that consumes platforms (the code
reading platforms[].connected) to display an "Unknown"/"Loading" state when
connected is undefined rather than showing "Not configured".
---
Nitpick comments:
In `@packages/adapters/src/chat/telegram/adapter.test.ts`:
- Around line 247-258: Add a deterministic regression test that simulates a 409
Conflict occurring after the telegraf onLaunch callback (to exercise the race
where polling fails during the grace window) by updating the mocked launch used
in the existing test: have mockLaunch call onLaunch() immediately, then simulate
a polling-start failure that rejects with new Error('409: Conflict: terminated
by other getUpdates request') at a controlled time (within the pollingGraceMs
window) instead of rejecting immediately; use adapter.getBot().launch =
mockLaunch and call adapter.start({ retryDelayMs: 0, pollingGraceMs: 0 or small
value }) and assert the adapter handles the post-onLaunch 409 the same as the
pre-onLaunch case (retries/continues) so the race is exercised
deterministically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bfa8b72-4bd9-4f89-bf0c-c6d27687bf9f
📒 Files selected for processing (4)
packages/adapters/src/chat/telegram/adapter.test.tspackages/adapters/src/chat/telegram/adapter.tspackages/server/src/index.tspackages/web/src/routes/SettingsPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/index.ts
Comment on lines
608
to
623
| function PlatformConnectionsSection({ | ||
| adapter, | ||
| activePlatforms, | ||
| }: { | ||
| adapter: string | undefined; | ||
| activePlatforms: string[] | undefined; | ||
| }): React.ReactElement { | ||
| const active = new Set(activePlatforms ?? []); | ||
| const platforms = [ | ||
| { name: 'Web', connected: adapter === 'web' }, | ||
| { name: 'Slack', connected: false }, | ||
| { name: 'Telegram', connected: false }, | ||
| { name: 'Discord', connected: false }, | ||
| { name: 'GitHub', connected: false }, | ||
| { name: 'Web', connected: active.has('Web') }, | ||
| { name: 'Slack', connected: active.has('Slack') }, | ||
| { name: 'Telegram', connected: active.has('Telegram') }, | ||
| { name: 'Discord', connected: active.has('Discord') }, | ||
| { name: 'GitHub', connected: active.has('GitHub') }, | ||
| { name: 'Gitea', connected: active.has('Gitea') }, | ||
| { name: 'GitLab', connected: active.has('GitLab') }, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Avoid showing “Not configured” before health data exists.
When activePlatforms is still undefined (initial load or health fetch failure), this renders every integration as Not configured. That reintroduces a false status during the exact window this PR is trying to fix. Render an Unknown/Loading state until the health payload arrives.
💡 Suggested tweak
function PlatformConnectionsSection({
activePlatforms,
}: {
activePlatforms: string[] | undefined;
}): React.ReactElement {
+ const hasHealthData = activePlatforms !== undefined;
const active = new Set(activePlatforms ?? []);
const platforms = [
{ name: 'Web', connected: active.has('Web') },
{ name: 'Slack', connected: active.has('Slack') },
{ name: 'Telegram', connected: active.has('Telegram') },
@@
{platforms.map(p => (
<div key={p.name} className="flex items-center justify-between text-sm">
<span>{p.name}</span>
- <Badge variant={p.connected ? 'default' : 'secondary'}>
- {p.connected ? 'Connected' : 'Not configured'}
+ <Badge variant={hasHealthData && p.connected ? 'default' : 'secondary'}>
+ {!hasHealthData ? 'Unknown' : p.connected ? 'Connected' : 'Not configured'}
</Badge>
</div>
))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/routes/SettingsPage.tsx` around lines 608 - 623, The
platforms list currently marks integrations as not connected whenever
activePlatforms is undefined; change the connected value computation in
PlatformConnectionsSection so it is tri-state (true/false/undefined) by setting
connected: activePlatforms ? active.has('Web') : undefined (and likewise for
each platform name) instead of active.has(...), and update the component
rendering logic that consumes platforms (the code reading platforms[].connected)
to display an "Unknown"/"Loading" state when connected is undefined rather than
showing "Not configured".
Collaborator
|
This PR appears to fully address #1031. Consider adding |
The Settings page's Platform Connections section hardcoded every platform except Web to 'Not configured', so users couldn't tell whether their Slack/ Telegram/Discord/GitHub/Gitea/GitLab adapters had actually started. - Server: /api/health now returns an activePlatforms array populated live as each adapter's start() resolves. Passed into registerApiRoutes so the reference stays mutable — Telegram starts after the HTTP listener is already accepting requests, so a snapshot would miss it. - Web: SettingsPage.PlatformConnectionsSection now reads activePlatforms from /api/health and looks each platform up in a Set. Also adds Gitea and GitLab to the list (they already ship as adapters). Closes coleam00#1031
Wirasm
force-pushed
the
fix/platform-connections-status
branch
from
6a816a9 to
c31e481
Compare
Wirasm
changed the title
Collaborator
|
Hi @liorfranko — thanks for this! Rebased onto current Also added |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/api/healthnow returns anactivePlatformsarray populated live as each adapter'sstart()resolves. The live reference is passed intoregisterApiRoutesbecause Telegram starts after the HTTP listener is already accepting requests, so a snapshot would miss it.SettingsPage.PlatformConnectionsSectionreadsactivePlatformsand looks each platform up in aSet. Also adds Gitea and GitLab to the list (they already ship as adapters).devmigrated to grammY and now uses theonStartcallback inpackages/adapters/src/chat/telegram/adapter.ts. That portion of the original PR has been dropped on rebase.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
startServerregisterApiRoutes(activePlatforms)start()activePlatforms.push(...)GET /api/healthHealthResponse.activePlatforms['Web']SettingsPagehealth.activePlatformsfalse)Label Snapshot
risk: lowsize: Sserver,webserver:index,server:routes/api,web:SettingsPageChange Metadata
fixmulti(server + web)Linked Issue
Validation Evidence (required)
Security Impact (required)
NoNoNoNoactivePlatformsexposes only platform names (e.g."Slack") — no tokens or config values.Compatibility / Migration
Yes—activePlatformsis an optional field; clients that ignore it see no change.NoNoHuman Verification (required)
@archon/servertest suite pass locally.registerApiRoutesruns — the live array reference ensures/api/healthreflects its state once it pushes.getHealth()without platforms configured returns['Web']as before.Side Effects / Blast Radius (required)
/api/health, Settings page.*.bot_started,github_webhook_registered, etc.) remain the source of truth for startup observability.Rollback Plan (required)
git revert <commit>— single commit, 4 files, contained.Risks and Mitigations
'Telegram'to the array.tryblock afterawait telegramAdapter.start(); failure skips the push (matches pre-existingtelegram = nullhandling).Rebased from @liorfranko's original PR onto current
dev. The adapter changes were dropped because dev migrated from telegraf to grammY and already resolves thebot.launch()never-resolves issue via theonStartcallback — so that fix is no longer needed. The server + web portions remain and are credited to @liorfranko.Summary by CodeRabbit
New Features
Bug Fixes