fix: use authenticated API for remote config polling#8266
Merged
Conversation
Fixes an issue where the cloud extension's polling interval was using unauthenticated fetch, causing it to receive only default feature flags instead of user-specific configurations. Changes: - Consolidate loadRemoteConfig into refreshRemoteConfig with options - Add useAuth option (defaults to true) to control auth usage - Bootstrap uses useAuth: false (pre-auth initialization) - Polling uses default authenticated API calls - Add unit tests for both auth modes
dosubot
bot
added
the
size:L
🎭 Playwright Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/23/2026, 06:12:40 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Contributor
📝 WalkthroughWalkthroughThis PR refactors the remote config loading mechanism by replacing Changes
Possibly related PRs
Suggested reviewers
✨ Finishing touches
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 |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.8 kB (baseline 22.7 kB) • 🔴 +108 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 949 kB (baseline 949 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 440 kB (baseline 440 kB) • 🟢 -131 BConfiguration panels, inspectors, and settings screens
Status: 24 added / 23 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.83 kB (baseline 2.83 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 9 added / 9 removed Data & Services — 3.17 MB (baseline 3.17 MB) • 🔴 +1 BStores, services, APIs, and repositories
Status: 9 added / 9 removed Utilities & Hooks — 24 kB (baseline 24 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 11 added / 11 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 8 added / 8 removed Other — 6.36 MB (baseline 6.36 MB) • 🟢 -153 BBundles that do not match a named category
Status: 86 added / 86 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/platform/remoteConfig/refreshRemoteConfig.ts (1)
34-38: Consider clearing config for all non-ok responses, not just 401/403.When the response is non-ok but not 401/403 (e.g., 500, 502), the existing
remoteConfig.valueremains stale. This could lead to the app operating with outdated feature flags until the next successful refresh.If the intent is to preserve the last known good config on transient server errors, consider documenting this behavior. Otherwise, clearing on all failures may be more consistent with the catch block behavior.
♻️ Optional: Clear config on all non-ok responses
console.warn('Failed to load remote config:', response.statusText) - if (response.status === 401 || response.status === 403) { - window.__CONFIG__ = {} - remoteConfig.value = {} - } + // Clear config on auth errors; consider if transient errors should also clear + if (response.status === 401 || response.status === 403) { + window.__CONFIG__ = {} + remoteConfig.value = {} + } + // Alternatively, clear on all failures for consistency: + // window.__CONFIG__ = {} + // remoteConfig.value = {}src/extensions/core/cloudRemoteConfig.ts (1)
19-29: Consider storing the interval reference for potential cleanup.The
setIntervalreference is discarded, making cleanup impossible if ever needed. While this cloud extension likely runs for the app's lifetime, storing the interval ID is a good defensive practice.♻️ Optional: Store interval reference
watchDebounced( [isLoggedIn, isActiveSubscription], () => { if (!isLoggedIn.value) return void refreshRemoteConfig() }, { debounce: 256, immediate: true } ) // Poll for config updates every 10 minutes (with auth) - setInterval(() => void refreshRemoteConfig(), 600_000) + const pollIntervalId = setInterval(() => void refreshRemoteConfig(), 600_000) + // Note: pollIntervalId could be cleared if extension lifecycle management is added
🤖 Fix all issues with AI agents
In `@src/platform/remoteConfig/refreshRemoteConfig.test.ts`:
- Around line 73-107: Add a test that verifies non-auth server errors do not
clear config: set remoteConfig.value and window.__CONFIG__ to a sample object,
mock api.fetchApi to resolve with a Response-like object where ok: false and
status: 500 (statusText 'Internal Server Error'), call refreshRemoteConfig(),
and assert that remoteConfig.value and window.__CONFIG__ remain equal to the
original object; reference refreshRemoteConfig, remoteConfig, window.__CONFIG__,
and api.fetchApi when locating where to add the test.
Comment on lines
+73
to
+107
| describe('error handling', () => { | ||
| it('clears config on 401 response', async () => { | ||
| vi.mocked(api.fetchApi).mockResolvedValue({ | ||
| ok: false, | ||
| status: 401, | ||
| statusText: 'Unauthorized' | ||
| } as Response) | ||
|
|
||
| await refreshRemoteConfig() | ||
|
|
||
| expect(remoteConfig.value).toEqual({}) | ||
| expect(window.__CONFIG__).toEqual({}) | ||
| }) | ||
|
|
||
| it('clears config on 403 response', async () => { | ||
| vi.mocked(api.fetchApi).mockResolvedValue({ | ||
| ok: false, | ||
| status: 403, | ||
| statusText: 'Forbidden' | ||
| } as Response) | ||
|
|
||
| await refreshRemoteConfig() | ||
|
|
||
| expect(remoteConfig.value).toEqual({}) | ||
| expect(window.__CONFIG__).toEqual({}) | ||
| }) | ||
|
|
||
| it('clears config on fetch error', async () => { | ||
| vi.mocked(api.fetchApi).mockRejectedValue(new Error('Network error')) | ||
|
|
||
| await refreshRemoteConfig() | ||
|
|
||
| expect(remoteConfig.value).toEqual({}) | ||
| expect(window.__CONFIG__).toEqual({}) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for non-auth error statuses (e.g., 500).
The current tests cover 401 and 403 (which clear config) and fetch errors. However, the implementation preserves the existing config for other non-ok statuses like 500. Adding a test for this scenario would document the expected behavior and prevent regressions.
📝 Suggested additional test case
it('preserves config on 500 response', async () => {
remoteConfig.value = { existingFeature: true }
window.__CONFIG__ = { existingFeature: true }
vi.mocked(api.fetchApi).mockResolvedValue({
ok: false,
status: 500,
statusText: 'Internal Server Error'
} as Response)
await refreshRemoteConfig()
// Config should NOT be cleared on transient server errors
expect(remoteConfig.value).toEqual({ existingFeature: true })
expect(window.__CONFIG__).toEqual({ existingFeature: true })
})🤖 Prompt for AI Agents
In `@src/platform/remoteConfig/refreshRemoteConfig.test.ts` around lines 73 - 107,
Add a test that verifies non-auth server errors do not clear config: set
remoteConfig.value and window.__CONFIG__ to a sample object, mock api.fetchApi
to resolve with a Response-like object where ok: false and status: 500
(statusText 'Internal Server Error'), call refreshRemoteConfig(), and assert
that remoteConfig.value and window.__CONFIG__ remain equal to the original
object; reference refreshRemoteConfig, remoteConfig, window.__CONFIG__, and
api.fetchApi when locating where to add the test.
christian-byrne
added
needs-backport
christian-byrne
added
the
preview-cpu
DrJKL
reviewed
Jan 23, 2026
Contributor
There was a problem hiding this comment.
Nit: I know it's not coming from this PR, but this feels like it should be a part of the remoteConfig module, not in a separate file.
DrJKL
reviewed
Jan 23, 2026
Comment on lines
+26
to
+30
| it('uses api.fetchApi when useAuth is true', async () => { | ||
| vi.mocked(api.fetchApi).mockResolvedValue({ | ||
| ok: true, | ||
| json: async () => mockConfig | ||
| } as Response) |
Contributor
There was a problem hiding this comment.
Does this need the type assertion?
Could you get away with satisfies Partial<Response>?
DrJKL
reviewed
Jan 23, 2026
| } | ||
| })) | ||
|
|
||
| global.fetch = vi.fn() |
Contributor
There was a problem hiding this comment.
Very nit: I usually like stubGlobal for consistency and control
DrJKL
reviewed
Jan 23, 2026
Comment on lines
+57
to
+60
| vi.mocked(global.fetch).mockResolvedValue({ | ||
| ok: true, | ||
| json: async () => mockConfig | ||
| } as Response) |
Contributor
There was a problem hiding this comment.
Nit: This pattern seems repeated enough times in here to be worth extracting.
DrJKL
approved these changes
Jan 23, 2026
github-actions bot
pushed a commit
that referenced
this pull request
Jan 23, 2026
## Summary
- Fixes remote config polling to use authenticated API
- Consolidates `loadRemoteConfig` into `refreshRemoteConfig` with auth
control
- Adds unit tests for both auth modes
## Problem
The cloud extension's polling interval was using unauthenticated
`fetch`, causing it to receive only default feature flags instead of
user-specific configurations.
**Root cause:**
1. Bootstrap called `loadRemoteConfig()` (raw `fetch`, no auth) -
correct, auth not initialized yet
2. Extension watch called `refreshRemoteConfig()` (`api.fetchApi`, with
auth) - correct
3. Extension interval called `loadRemoteConfig()` (raw `fetch`, no auth)
- **bug**
## Solution
- Consolidate into single `refreshRemoteConfig()` with optional
`useAuth` parameter (defaults to `true`)
- Bootstrap: `refreshRemoteConfig({ useAuth: false })`
- Polling: `refreshRemoteConfig()` (authenticated by default)
## Test Plan
- Unit tests verify both auth modes
- `pnpm typecheck`, `pnpm lint`, `pnpm test:unit` all pass
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-8266-fix-use-authenticated-API-for-remote-config-polling-2f16d73d3650817ea7b0e3a7e3ccf12a)
by [Unito](https://www.unito.io)
Member
|
@christian-byrne Successfully backported to #8284 |
github-actions
bot
removed
the
needs-backport
DrJKL
pushed a commit
that referenced
this pull request
Jan 24, 2026
…lling (#8284) Backport of #8266 to `cloud/1.37` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8284-backport-cloud-1-37-fix-use-authenticated-API-for-remote-config-polling-2f16d73d365081d2898ff4fc1af441f0) by [Unito](https://www.unito.io) Co-authored-by: Christian Byrne <cbyrne@comfy.org>
christian-byrne
added a commit
that referenced
this pull request
Jan 28, 2026
Addresses review feedback from #8266: - Use vi.stubGlobal for global fetch mock - Extract mockSuccessResponse and mockErrorResponse helpers to reduce duplication - Add test verifying 500 responses preserve existing config (only 401/403 clear config) Related: #8266 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8356-test-improve-refreshRemoteConfig-test-quality-2f66d73d365081f7b207c2fd2b8a8179) by [Unito](https://www.unito.io)
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
3 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
loadRemoteConfigintorefreshRemoteConfigwith auth controlProblem
The cloud extension's polling interval was using unauthenticated
fetch, causing it to receive only default feature flags instead of user-specific configurations.Root cause:
loadRemoteConfig()(rawfetch, no auth) - correct, auth not initialized yetrefreshRemoteConfig()(api.fetchApi, with auth) - correctloadRemoteConfig()(rawfetch, no auth) - bugSolution
refreshRemoteConfig()with optionaluseAuthparameter (defaults totrue)refreshRemoteConfig({ useAuth: false })refreshRemoteConfig()(authenticated by default)Test Plan
pnpm typecheck,pnpm lint,pnpm test:unitall pass┆Issue is synchronized with this Notion page by Unito