feat(web): per-device provider settings - #4479
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces a new feature for per-device provider settings with ~1400 lines of new code, new state management, and permission controls. Combined with an unresolved review comment identifying an access control bug in the dialog handling, this warrants human review. You can customize Macroscope's approvability policy. Learn more. |
|
Addressed all three Bugbot findings in 6a822e6. Unknown operate access treated as editable (medium) — Read-only blocked provider viewing (medium) — read-only sessions now render a Loading catalog looked empty (low) — the panel now reads Verification: |
|
Addressed the latest findings in 370174b. Session refresh unmounts editor (high, Bugbot) — Real regression from my previous fix, thanks. Loading copy misstates wait reason (low, Bugbot) — The loading state now carries Read-only names ignore labels (medium, Bugbot) — Non-primary Verification: typecheck passes; 172 files / 1,524 tests pass (4 new cases for the access-resolution paths); lint clean on changed files. |
370174b to
1913ad4
Compare
|
Simplification pass in ca0d588, which also resolves the two open findings:
Session fetch failure showed read-only (medium, Bugbot) — Cleanup from parallel reuse/simplification/efficiency/altitude reviews:
Verification: typecheck passes; 195 files / 1,721 tests pass; lint clean on all touched files. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/web/src/components/settings/ProviderSettingsPanel.tsx (2)
520-556: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the local type declarations to module scope.
InstanceRow(lines 520-526) andLegacyProviderSettings(line 551) are declared inside the component, andLegacyProviderSettingsis re-declared inside the driver loop and again inresetDefaultInstance(line 667). Move both to module scope so the same alias serves all three sites and the component body holds only runtime logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/settings/ProviderSettingsPanel.tsx` around lines 520 - 556, Move the InstanceRow and LegacyProviderSettings type aliases from the component body to module scope, then remove the duplicate LegacyProviderSettings declarations inside the visibleDriverKinds loop and resetDefaultInstance. Reuse the module-level aliases at all existing sites while leaving the runtime logic unchanged.
296-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the optimistic
operateAccess="granted"default for non-primary devices.Line 299 grants operate access to every non-primary environment and to Electron primary sessions. The PR description explains the reason: the client cannot observe remote session scopes, and the server stays authoritative. Add that rationale next to the call so a later reader does not treat it as a client-side authorization decision.
♻️ Proposed comment
if (isPrimary && !isElectron) { return <PrimarySessionGatedProviderSettings environment={environment} />; } + // Remote and desktop-bridge sessions have no observable scope list on the + // client, so operate access is assumed granted; the server rejects writes + // this session is not allowed to make. return <AccessGatedProviderSettings environment={environment} operateAccess="granted" />;As per coding guidelines: "Use comments mainly to describe how a function is used".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/settings/ProviderSettingsPanel.tsx` around lines 296 - 300, Add a concise inline comment beside the operateAccess="granted" prop in the provider-selection function, documenting that this is an optimistic client default because remote session scopes are not observable and the server remains authoritative for authorization.Source: Coding guidelines
apps/web/src/components/settings/AddProviderInstanceDialog.environment.test.tsx (1)
6-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the write, or narrow the test name.
The test name claims that the dialog "reads and writes settings through the supplied environment". The assertions at lines 51-52 only prove that both hooks received
remoteEnvironmentId. No write is exercised, becausehandleSaveis never called.updatealso returns a newvi.fn()on every call (line 8), so the updater the component holds is not observable from the test. Return a stable updater mock and assert that saving reaches it.♻️ Proposed change
const settingsHooks = vi.hoisted(() => ({ read: vi.fn(() => ({ providerInstances: {} })), - update: vi.fn(() => vi.fn()), + updateSettings: vi.fn(), + update: vi.fn(), }));Wire
updateto returnsettingsHooks.updateSettings, reset both inbeforeEach, then invoke the save handler from the rendered element tree and assert thatsettingsHooks.updateSettingsreceived the newproviderInstancesmap.Also applies to: 42-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/settings/AddProviderInstanceDialog.environment.test.tsx` around lines 6 - 9, Update the environment test’s settings hook mocks so update returns a stable settingsHooks.updateSettings mock, and reset both hook mocks in beforeEach. Exercise the dialog’s save handler through the rendered element tree, then assert settingsHooks.updateSettings receives the updated providerInstances map, preserving the test’s existing remoteEnvironmentId assertions.apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx (1)
123-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the fixed microtask wait with a deterministic completion signal.
flushPromisesawaits two microtasks. The count is arbitrary: it matches the current number ofawaitpoints insiderefreshProvidersandrunProviderUpdate. If the component adds one moreawait, the assertions at lines 157 and 168 run before the command dispatch and the test fails for the wrong reason. Await the promise the mocked command returns, or expose the dispatch promise from the handler, so the test waits on a real receipt.As per coding guidelines: "Tests must wait for typed receipts and worker drains in event-sourced async flows; do not use sleeps, polling, or arbitrary timeouts to make tests pass."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx` around lines 123 - 126, Replace the fixed two-microtask implementation of flushPromises in the provider settings tests with a deterministic completion signal tied to the mocked command promise or the handler’s dispatch promise. Update the test setup and assertions around refreshProviders and runProviderUpdate to await that typed receipt, ensuring command completion is observed without arbitrary waits.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/settings/ProviderSettingsPanel.tsx`:
- Around line 557-570: Update the provider-instance setup around driver,
legacyConfig, and defaultLegacyConfig to handle missing legacy provider entries
without non-null assertions. Match resetDefaultInstance’s existing undefined
behavior: only construct the fallback effectiveInstance and compute
legacy-config dirtiness when the corresponding legacy entries exist, while
preserving explicitInstance handling for drivers without legacy configuration.
---
Nitpick comments:
In
`@apps/web/src/components/settings/AddProviderInstanceDialog.environment.test.tsx`:
- Around line 6-9: Update the environment test’s settings hook mocks so update
returns a stable settingsHooks.updateSettings mock, and reset both hook mocks in
beforeEach. Exercise the dialog’s save handler through the rendered element
tree, then assert settingsHooks.updateSettings receives the updated
providerInstances map, preserving the test’s existing remoteEnvironmentId
assertions.
In `@apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx`:
- Around line 123-126: Replace the fixed two-microtask implementation of
flushPromises in the provider settings tests with a deterministic completion
signal tied to the mocked command promise or the handler’s dispatch promise.
Update the test setup and assertions around refreshProviders and
runProviderUpdate to await that typed receipt, ensuring command completion is
observed without arbitrary waits.
In `@apps/web/src/components/settings/ProviderSettingsPanel.tsx`:
- Around line 520-556: Move the InstanceRow and LegacyProviderSettings type
aliases from the component body to module scope, then remove the duplicate
LegacyProviderSettings declarations inside the visibleDriverKinds loop and
resetDefaultInstance. Reuse the module-level aliases at all existing sites while
leaving the runtime logic unchanged.
- Around line 296-300: Add a concise inline comment beside the
operateAccess="granted" prop in the provider-selection function, documenting
that this is an optimistic client default because remote session scopes are not
observable and the server remains authoritative for authorization.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c185c90-e3b8-41da-becd-d207c2b3b28c
📒 Files selected for processing (14)
apps/web/src/components/ConnectionStatusDot.tsxapps/web/src/components/settings/AddProviderInstanceDialog.environment.test.tsxapps/web/src/components/settings/AddProviderInstanceDialog.tsxapps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsxapps/web/src/components/settings/ProviderSettingsPanel.logic.test.tsapps/web/src/components/settings/ProviderSettingsPanel.logic.tsapps/web/src/components/settings/ProviderSettingsPanel.tsxapps/web/src/components/settings/SettingsPanels.logic.tsapps/web/src/components/settings/SettingsPanels.tsxapps/web/src/components/settings/settingsLayout.tsxapps/web/src/routes/settings.providers.tsxapps/web/src/state/server.tsapps/web/src/test/reactElementTree.tsapps/web/src/test/reactHookHarness.ts
ca0d588 to
7a3c281
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 997cfa5. Configure here.
Providers settings were hardwired to the primary backend, so remote boxes reached through T3 Connect or app.t3.codes could not be configured. - List every registered execution environment and let one be selected - Scope provider reads, settings writes, refreshes, updates, and instance creation to the selected environment - Gate controls on raw server config, connection phase, and operate scope - Keep shared model preferences intact when removing per-device config Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review findings on the per-device providers panel. - Treat unresolved primary session scopes as loading instead of editable, so controls are never offered before permissions are known - Render provider status rows for read-only sessions instead of replacing the whole panel with a blocking message - Distinguish a hydrating environment catalog from having no devices Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Extract resolvePrimaryOperateAccess so SWR revalidation no longer reports pending while cached session data is available, which was unmounting the provider editor and discarding in-progress edits - Carry a reason on the loading state so waiting on permissions is not described as waiting on device configuration - Name read-only provider rows with deriveProviderInstanceEntries so multiple instances of one driver stay distinguishable Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cleanup pass over the providers panel from parallel reuse, simplification, efficiency, and altitude reviews: - Share the connection-phase dot/ping mapping from ConnectionStatusDot instead of a third inline copy - Extract the React Compiler hook harness and element-tree walker into src/test and reuse them from both new test files - Move interval/background-activity helpers and PolicyTooltip out of the SettingsPanels component file into SettingsPanels.logic and settingsLayout - Drop the setState-in-useEffect selection sync; the effective selection is derived, so a device that reconnects regains its prior selection - Only mount the primary-session atom when a browser session is actually gated by it; remote devices skip the SWR fetch entirely - Guard runProviderUpdate re-entry with a ref instead of a state-updater flag - Treat a failed session fetch as transport trouble rather than denied access - Reuse EnvironmentConnectionPhase, isElectron, and the shared EMPTY_SERVER_PROVIDERS constant; remove the indefinite loading spinner Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A remote device may run a server version whose settings predate a driver in this build's DRIVER_OPTIONS, so the legacy providers mirror can lack the entry. The default-slot loop asserted it non-null and would throw during render; skip the slot instead when neither an explicit instance nor a legacy blob exists, matching resetDefaultInstance's existing guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nfig The missing-legacy guard used continue, which also skipped the custom instance rows appended later in the same loop iteration. Only the default slot depends on the legacy blob, so skip just that row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6d9de69 to
f43e38e
Compare
|
Rebased onto latest main (f43e38e). Conflicts were confined to imports in Verification after rebase: web typecheck clean, 210 test files / 1,826 tests passing, lint clean on changed files. Resolved the two open threads: the Macroscope non-primary |
Non-primary environments previously assumed operate access, so a narrow-scope credential was offered edit controls whose writes the environment RPC would reject. Each environment's /api/auth/session now answers what this client may change, and sessions without orchestration:operate see the full provider layout greyed out and inert behind a limited-permissions notice instead of a separate stripped view. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
688524b replaces the optimistic non-primary access assumption with real per-environment permissions, per discussion:
Verification: web + client-runtime + mobile typecheck clean, 1,831 web tests and 522 client-runtime tests passing (new coverage for |
| </div> | ||
| </SettingsSection> | ||
|
|
||
| {isAddInstanceDialogOpen ? ( |
There was a problem hiding this comment.
🟠 High settings/ProviderSettingsPanel.tsx:892
When readOnly becomes true while the add-instance dialog is already open, the main provider layout becomes inert but AddProviderInstanceDialog is rendered outside that wrapper and remains fully interactive, so the user can still submit new provider instances via updateSettings despite the session no longer having operate permission. Consider closing the dialog (or gating it) when readOnly transitions to true, since the inert wrapper only covers the settings rows below it.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/settings/ProviderSettingsPanel.tsx around line 892:
When `readOnly` becomes `true` while the add-instance dialog is already open, the main provider layout becomes `inert` but `AddProviderInstanceDialog` is rendered outside that wrapper and remains fully interactive, so the user can still submit new provider instances via `updateSettings` despite the session no longer having operate permission. Consider closing the dialog (or gating it) when `readOnly` transitions to `true`, since the `inert` wrapper only covers the settings rows below it.

Problem
Providers settings were hardwired to the primary backend. When working through T3 Connect or app.t3.codes, remote boxes could not be configured — controls were missing or ineffective.
What this does
Single local-primary setups keep the existing Providers layout with no extra device section.
No contracts, server RPC, relay, or permission-scope changes — the environment-scoped commands already carry
environmentId.Verification
pnpm --filter @t3tools/web typecheck— passedpnpm --filter @t3tools/web test— 172 files / 1,519 tests passed/settings/providers: provider rows render, add-instance dialog opens/cancels, refresh completes, no Providers-specific console errorsNew tests cover environment ordering/selection fallback, all access states, non-primary command routing, add-instance scoping, nullable snapshots, and shared-preference preservation. Interactive multi-device and disconnected-device checks were blocked by single-environment runtime state and are covered by those tests instead.
🤖 Generated with Claude Code
Note
Medium Risk
Large settings UI refactor with new session-scope gating and optimistic permission fallbacks; incorrect access classification could expose controls that RPC rejects, though writes remain server-enforced.
Overview
Providers settings are no longer tied to the primary device. The panel is extracted into
ProviderSettingsPaneland, when more than one execution environment exists, adds a Devices picker (primary, T3 Connect, SSH, etc.) with connection status on each row.All provider actions are scoped to the selected
environmentId: settings read/write, refresh, one-click updates, and the add-instance dialog. Server commands already carriedenvironmentId; the UI now passes it consistently.Access is gated before showing editable controls: connection phase, whether server config is loaded, and whether the session has
orchestration:operate(via new per-environment/api/auth/sessionatoms in client-runtime anduseEnvironmentSessionStateon web). Missing permission shows the real layout read-only (inert) with a limited-permissions notice; disconnected or loading devices get explicit unavailable/loading copy instead of broken controls.Other behavior tweaks: deleting or resetting a provider instance on one device no longer clears shared favorites/model preferences in the same settings patch; default provider slots on older remote servers can omit legacy driver blobs without breaking the panel. Single primary-only setups keep the previous layout without the Devices section.
Reviewed by Cursor Bugbot for commit 688524b. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add per-device provider settings with environment selection to the Providers settings panel
ProviderSettingsPanelwith a multi-device aware version inProviderSettingsPanel.tsxthat lets users select an execution environment and manage providers per device.ProviderSettingsPanel.logic.tsto classify each environment as editable, read-only, loading, unavailable, or error based on connection phase, config availability, and session scopes.createEnvironmentSessionAtomsinpackages/client-runtime/src/state/session.tswithsessionStateAtomandsessionStateValueAtomthat fetch/api/auth/sessionper environment using SWR semantics (30s stale, 5m idle TTL).useEnvironmentSessionStatehook inapps/web/src/state/session.tsexposing auth session data withisPending/hasErrorflags for UI gating.durationToSeconds,normalizeIntervalSeconds,backgroundActivityOverrideSettings) fromSettingsPanels.tsxtoSettingsPanels.logic.tsand removes the oldProviderSettingsPanelimplementation fromSettingsPanels.tsx.ProviderSettingsPanelis no longer exported fromSettingsPanels.tsx; the import path is updated insettings.providers.tsxbut any other consumer importing from the old path will break at runtime.Macroscope summarized 688524b.
Summary by CodeRabbit
New Features
Bug Fixes
Tests