Skip to content

fix: prevent silent config loss and misleading post-configure errors (F293433)#302

Merged
samsolaimani merged 3 commits into
masterfrom
fix/developer-configure-bugs-F293433
May 22, 2026
Merged

fix: prevent silent config loss and misleading post-configure errors (F293433)#302
samsolaimani merged 3 commits into
masterfrom
fix/developer-configure-bugs-F293433

Conversation

@samsolaimani
Copy link
Copy Markdown
Contributor

Summary

Fixes two bugs in alks developer configure that together caused users to get stuck in a "Server URL is not configured" loop and see misleading error messages.


US1985865 — Fix updateDeveloper double-loadDatabase orphaning collection reference

Root cause: updateDeveloper obtained a LokiJS collection reference via getCollection(), then called getDeveloper() which internally called getCollection() a second time. Each call invokes db.loadDatabase() — when alks.db exists on disk, LokiJS resets db.collections and rebuilds from disk, orphaning the first reference. When the loaded collection has zero records, the developer object is a fresh {} (not a live reference), so subsequent writes target the orphaned collection and db.save() drops the update silently.

Fix: Call getDeveloper() first so the DB is loaded, then call getCollection() — guaranteeing the reference is attached to the live, post-load db.collections.

Tests added: src/lib/state/developer.test.ts — integration tests against a real LokiJS file using a tmpDir + ALKS_DB env var. Includes a regression test that seeds alks.db with an empty account collection to reproduce the exact failure mode.


US1985867 — Wrap non-critical post-configure steps in try/catch

Root cause: Optional post-save steps (tab completion install, update check) ran inside the same top-level try/catch as the critical configuration writes. Any failure there called errorAndExit('Error configuring developer: ...') — a misleading message since the configuration had already been persisted successfully.

Fix: Moved both optional steps outside the main try/catch, each with its own try/catch that warns and continues. Configuration is always preserved; only the warning message changes.


Test results

All 400 tests pass. tsc --noEmit clean. No new lint warnings.


Refs: F293433, US1985865, US1985867

🤖 Generated with Claude Code

US1985865: Reorder getDeveloper/getCollection in updateDeveloper to
prevent silent data loss when alks.db exists but has an empty account
collection. LokiJS resets db.collections on each loadDatabase call,
orphaning any collection reference obtained before the load. By calling
getDeveloper first (which internally loads the DB), the getCollection
call that follows always returns the live, post-load reference.

Adds integration tests that exercise the actual LokiJS layer, including
a regression test that seeds alks.db with an empty account collection
to confirm the fix.

US1985867: Move optional post-configure steps (tab completion install,
update check) outside the main try/catch block and wrap each in its own
try/catch. Previously, any failure in these non-critical steps called
errorAndExit('Error configuring developer: ...') even though the
configuration had already been persisted successfully. Failures are
now reported as warnings and the command exits cleanly.

Refs: F293433, US1985865, US1985867

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@samsolaimani samsolaimani added the release/patch Indicates an update without breaking changes or new features label May 21, 2026
Copy link
Copy Markdown
Contributor

@DevOpsDave DevOpsDave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve

Both fixes are correct and well-tested.

US1985865 (silent config loss): The root cause — getCollection calling db.loadDatabase() which resets db.collections and orphans a previously-obtained collection reference — is accurately diagnosed. The reorder (call getDeveloper first, then obtain the collection reference) guarantees writes target the live, post-load collection. The removal of the redundant removeDataOnly() is correct. The new integration tests in developer.test.ts cover the regression case (empty account collection on disk) well.

US1985867 (misleading error message): Moving post-configure steps (tabtab.install, checkForUpdate) outside the main try/catch with their own warning-level error handling is the right approach. Config is always preserved; only optional steps degrade gracefully.

Test expectations updated correctly. CI builds pass on Node 20/22/24. SonarQube upload failure is infra-related.

@samsolaimani samsolaimani merged commit e7ab0b5 into master May 22, 2026
4 of 5 checks passed
@samsolaimani samsolaimani deleted the fix/developer-configure-bugs-F293433 branch May 22, 2026 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release/patch Indicates an update without breaking changes or new features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants