Skip to content

fix: provider wizard data loss and startup provider fallback#680

Open
llupRisinglll wants to merge 4 commits into
Nano-Collective:mainfrom
llupRisinglll:rc/provider-robustness
Open

fix: provider wizard data loss and startup provider fallback#680
llupRisinglll wants to merge 4 commits into
Nano-Collective:mainfrom
llupRisinglll:rc/provider-robustness

Conversation

@llupRisinglll

@llupRisinglll llupRisinglll commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Description

  • The provider wizard no longer deletes existing providers when adding a new one — new providers are appended to agents.config.json.
  • Startup no longer freezes when the saved provider is missing from agents.config.json — it warns and falls back to the first configured provider (an explicit --provider CLI arg stays strict and still errors).
  • If every configured provider fails to load, the per-provider errors are shown and the provider wizard opens instead of a dead screen.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (useAppInitialization spec, tsc --noEmit, build — all clean; no behavior removed, only new guard paths added)
  • Tests cover both success and error scenarios

Manual Testing

Verified across four scenarios on a real multi-provider config:

  1. Stale saved provider + valid providers → warning + boots on first provider.
  2. Stale saved provider + broken first provider → provider-order chain skips it, boots on next valid one.
  3. Zero providers configured → setup wizard opens (pre-existing path, confirmed unaffected).
  4. All providers broken → per-provider error list + wizard opens (previously froze on a dead screen).
  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes
  • Appropriate logging added using structured logging

@akramcodez

Copy link
Copy Markdown
Collaborator

Hi @llupRisinglll, thanks for putting this together! Great work tracking down both the provider wizard data loss issue and the startup provider fallback logic. Overall the implementation is heading in the right direction, but I found a couple of blockers regarding test coverage and test hygiene before we can merge this.

1. Missing Integration Test Coverage for Initialization Flow

Where it is wrong: source/hooks/useAppInitialization.spec.tsx

Why it is wrong:
The new startup behavior is implemented inside useAppInitialization.tsx, but the corresponding user-facing flows are not covered by integration tests. While startup-provider.spec.ts does a good job testing the provider resolution logic in isolation, it doesn't verify that the hook performs the expected UI side effects.

Specifically, the following new behaviors are currently untested:

  • Displaying a warning message when a stale provider is detected.
  • Opening the configuration wizard when all configured providers fail to initialize.

These are the primary user-visible behaviors introduced by this PR, so they should be covered to prevent future regressions.

What should be done:

Please add tests to useAppInitialization.spec.tsx that:

  • Mock resolveStartupProvider() returning a staleName and verify that the warning message is added to the chat queue.
  • Mock createLLMClient() throwing the "All configured providers failed" error and verify that the application switches into the configuration wizard (setActiveMode('configWizard')).

2. Temporary Directory Cleanup

Where it is wrong: source/wizards/base-config-wizard.spec.tsx

Why it is wrong:
The tests create a temporary configuration directory using mkdtempSync, but the directory is never cleaned up afterwards. This causes temporary directories to accumulate across local test runs and CI executions.

What should be done:

Please clean up the temporary directory after the tests complete (for example using test.after.always() together with rmSync(..., { recursive: true, force: true })).

3. Please Open an Issue Before Large Behavioral Changes

This PR introduces multiple behavioral changes (provider wizard persistence improvements, startup fallback logic, and provider recovery flow), but there isn't an associated GitHub issue describing the problem, expected behavior, or proposed solution.

For changes of this scope, I'd recommend opening an issue first so we can discuss the architecture and expected user flow before implementation. It also makes future maintenance much easier by providing clear context for why the change was made.

Overall, this is a solid improvement and addresses some important edge cases. Once the missing integration tests and test cleanup are addressed, I'll be happy to take another look. Great work!

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.

2 participants