Skip to content

Conversation

@GordonSmith
Copy link
Member

  • Updated various dependencies in package.json primarily to fix the btoa polyfill issue with unicode chars
  • Refactored session management in session.ts:
    • Removed credential migration on initialization (replaced with migrate on use).
    • Enhanced session switching logic to handle credentials more effectively.
    • Improved configuration updates to avoid unnecessary triggers.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request updates dependencies (primarily to fix btoa polyfill unicode character issues) and refactors session management to migrate credentials on-demand rather than during initialization. The key changes improve configuration update efficiency and credential handling workflow.

  • Updated @hpcc-js packages and related dependencies for unicode character handling fixes
  • Refactored credential migration from initialization-time to on-demand (during session switch)
  • Optimized configuration updates to prevent unnecessary event triggers

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
package.json Minor version bumps for @hpcc-js packages (3.4.1→3.4.3, 3.5.1→3.5.3, 3.11.1→3.12.1, etc.), esbuild (0.27.0→0.27.1), vitest (4.0.10→4.0.15), and other dev dependencies
package-lock.json Corresponding lockfile updates for dependency version changes
src/util/credentialManager.ts Refactored Credentials.attach() caching logic, renamed migrateExistingCredentials to migrateLaunchConfigIfNeeded (now public), added removePasswordFromLaunchConfig helper
src/hpccplatform/session.ts Removed automatic migration on initialize(), made switchTo async with on-demand migration, added conditional configuration updates, enhanced status bar refresh with credential checks
ecl-sample/.vscode/launch.json Added test launch configurations with credential fields for testing unicode password support

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (1)

src/util/credentialManager.ts:57

  • Potential issue with cache invalidation: When retrieving a password from secrets storage, the code directly modifies the cached Credentials object's _password field. However, if the password retrieval fails (caught in the catch block), the cached object will remain with an empty password. This could lead to stale cache entries.

Recommendation: Consider removing the cached entry if password retrieval fails, or add a flag to indicate the retrieval status:

try {
    const password = await context.secrets.get(storageKey);
    credentials._password = password ?? "";
} catch (e) {
    logger.error(`Failed to get password for ${user}@${baseUrl}: ${e}`);
    credentialManagerCache.delete(storageKey); // Remove stale cache entry
}
        try {
            const password = await context.secrets.get(storageKey);
            credentials._password = password;
        } catch (e) {
            logger.error(`Failed to get password for ${user}@${baseUrl}: ${e}`);
        }

- Updated various dependencies in package.json primarily to fix the btoa polyfill issue with
  unicode chars
- Refactored session management in session.ts:
  - Removed credential migration on initialization (replaced with migrate on use).
  - Enhanced session switching logic to handle credentials more effectively.
  - Improved configuration updates to avoid unnecessary triggers.

Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
@GordonSmith GordonSmith merged commit 56822c2 into hpcc-systems:main Dec 5, 2025
4 checks passed
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

🎉 This change has been included in ecl-v2.33.2 🎉

The release is available on:

Your release-please bot 🚀🙏

@GordonSmith GordonSmith deleted the CREDS_REFACTOR branch December 16, 2025 14:17
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