Skip to content

Conversation

@yaugenst-flex
Copy link
Collaborator

@yaugenst-flex yaugenst-flex commented Oct 31, 2025

Greptile Overview

Updated On: 2025-10-31 12:11:05 UTC

Greptile Summary

This PR fixes a synchronization issue between the legacy Env proxy and the new ConfigManager system, ensuring profile switches are properly reflected in the legacy API.

Key Changes:

  • Added LegacyConfigWrapper.switch_profile() that calls Env._sync_to_manager() after profile switches to keep legacy code in sync
  • Refactored LegacyEnvironmentConfig to use a _pending overrides system that applies changes when the profile becomes active
  • Introduced WebConfig.build_api_url() helper method to centralize URL construction logic
  • Migrated all production code from Env.current to config.web (8 files affected)
  • Added comprehensive regression tests for profile switching, pending overrides, and environment variable restoration

Technical Details:

  • The _sync_to_manager() method ensures Env.current always reflects the active profile
  • Pending overrides queue changes to inactive profiles and apply them upon activation
  • URL building is now centralized through build_api_url() instead of manual string concatenation
  • Environment variables are properly restored when switching between profiles

Confidence Score: 4/5

  • This PR is safe to merge with minor considerations around global state mutation
  • The refactoring is well-structured with comprehensive test coverage. The score reflects one global state mutation concern in the SSL verification fallback logic. All Env.current usages have been successfully migrated from production code, and the synchronization mechanism is properly implemented.
  • Pay attention to tidy3d/plugins/dispersion/web.py which modifies global config state in an exception handler

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/config/legacy.py 4/5 Major refactor to sync legacy Env proxy with ConfigManager, added _sync_to_manager(), _pending overrides, and bidirectional sync logic
tidy3d/config/sections.py 5/5 Added build_api_url() helper to join API endpoint with request paths
tidy3d/web/core/http_util.py 5/5 Migrated from Env.current to config.web for all HTTP session configuration
tidy3d/plugins/dispersion/web.py 4/5 Migrated from Env.current to config.web, including SSL verification updates using config.update_section()

Sequence Diagram

sequenceDiagram
    participant User
    participant config as config.switch_profile()
    participant ConfigManager
    participant LegacyEnv as Env (Legacy)
    participant LegacyConfig as LegacyEnvironmentConfig
    participant WebConfig
    
    User->>config: switch_profile("dev")
    config->>ConfigManager: switch_profile("dev")
    ConfigManager->>ConfigManager: Set active profile to "dev"
    config->>LegacyEnv: _sync_to_manager(apply_env=True)
    LegacyEnv->>LegacyEnv: Get active profile name
    LegacyEnv->>LegacyConfig: _get_config("dev")
    LegacyConfig-->>LegacyEnv: Return dev config
    LegacyEnv->>LegacyConfig: apply_pending_overrides()
    LegacyConfig->>ConfigManager: update_section("web", **pending)
    ConfigManager->>WebConfig: Apply pending overrides
    LegacyConfig->>LegacyConfig: Clear _pending dict
    LegacyEnv->>LegacyEnv: Set _current = dev config
    LegacyEnv->>LegacyEnv: _apply_env_vars(config)
    LegacyEnv->>LegacyEnv: Restore previous env vars
    LegacyEnv->>LegacyEnv: Apply new env vars from config
    LegacyEnv-->>config: Sync complete
    config-->>User: Profile switched, Env synced
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

13 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@yaugenst-flex yaugenst-flex force-pushed the FXC-3929-refactor-profile-switching-to-remove-env-manager-divergence-around-web-endpoints branch from f69ae3f to 41a8541 Compare October 31, 2025 12:18
@github-actions
Copy link
Contributor

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/config/legacy.py (80.0%): Missing lines 120-121,177,179,181,183,192-194,267,275-279,282-284,306,311-313,317,385
  • tidy3d/config/sections.py (100%)
  • tidy3d/plugins/dispersion/fit.py (100%)
  • tidy3d/plugins/dispersion/web.py (54.5%): Missing lines 231-232,252-254
  • tidy3d/web/api/mode.py (100%)
  • tidy3d/web/api/webapi.py (70.0%): Missing lines 83,88,94
  • tidy3d/web/cli/app.py (0.0%): Missing lines 97,100,102
  • tidy3d/web/core/http_util.py (83.3%): Missing lines 203
  • tidy3d/web/core/s3utils.py (100%)
  • tidy3d/web/core/task_core.py (100%)

Summary

  • Total: 163 lines
  • Missing: 36 lines
  • Coverage: 77%

tidy3d/config/legacy.py

Lines 116-125

  116         normalized = normalize_profile_name(profile)
  117         self._manager.switch_profile(normalized)
  118         try:
  119             from tidy3d.config import Env as _legacy_env
! 120         except Exception:
! 121             _legacy_env = None
  122         if _legacy_env is not None:
  123             _legacy_env._sync_to_manager(apply_env=True)
  124 
  125     def __getattr__(self, name: str) -> Any:

Lines 173-187

  173             self._pending["website_endpoint"] = website_endpoint
  174         if s3_region is not None:
  175             self._pending["s3_region"] = s3_region
  176         if ssl_verify is not None:
! 177             self._pending["ssl_verify"] = ssl_verify
  178         if enable_caching is not None:
! 179             self._pending["enable_caching"] = enable_caching
  180         if ssl_version is not None:
! 181             self._pending["ssl_version"] = ssl_version
  182         if env_vars is not None:
! 183             self._pending["env_vars"] = dict(env_vars)
  184 
  185     def reset_manager(self, manager: ConfigManager) -> None:
  186         self._manager = manager

Lines 188-198

  188     @property
  189     def manager(self) -> Optional[ConfigManager]:
  190         if self._manager is not None:
  191             return self._manager
! 192         if self._environment is not None:
! 193             return self._environment._manager
! 194         return None
  195 
  196     def active(self) -> None:
  197         _warn_env_deprecated()
  198         environment = self._environment

Lines 263-271

  263         self._name = normalize_profile_name(value)
  264 
  265     def copy_state_from(self, other: LegacyEnvironmentConfig) -> None:
  266         if not isinstance(other, LegacyEnvironmentConfig):
! 267             raise TypeError("Expected LegacyEnvironmentConfig instance.")
  268         for key, value in other._pending.items():
  269             if key == "env_vars" and value is not None:
  270                 self._pending[key] = dict(value)
  271             else:

Lines 271-288

  271             else:
  272                 self._pending[key] = value
  273 
  274     def get_real_url(self, path: str) -> str:
! 275         manager = self.manager
! 276         if manager is not None and manager.profile == self._name:
! 277             web_section = manager.get_section("web")
! 278             if hasattr(web_section, "build_api_url"):
! 279                 return web_section.build_api_url(path)
  280 
  281         endpoint = self.web_api_endpoint or ""
! 282         if not path:
! 283             return endpoint
! 284         return "/".join([endpoint.rstrip("/"), str(path).lstrip("/")])
  285 
  286     def apply_pending_overrides(self) -> None:
  287         manager = self.manager
  288         if manager is None or manager.profile != self._name:

Lines 302-321

  302 
  303     def _web_section(self) -> dict[str, Any]:
  304         manager = self.manager
  305         if manager is None:
! 306             return {}
  307         profile = normalize_profile_name(self._name)
  308         if manager.profile == profile:
  309             section = manager.get_section("web")
  310             return section.model_dump(mode="python", exclude_unset=False)
! 311         preview = manager.preview_profile(profile)
! 312         source = preview.get("web", {})
! 313         return dict(source) if isinstance(source, dict) else {}
  314 
  315     def _value(self, key: str) -> Any:
  316         if key in self._pending:
! 317             return self._pending[key]
  318         return self._web_section().get(key)
  319 
  320 
  321 class LegacyEnvironment:

Lines 381-389

  381         return config
  382 
  383     def _sync_to_manager(self, *, apply_env: bool = False) -> None:
  384         if self._manager is None:
! 385             return
  386         active = normalize_profile_name(self._manager.profile)
  387         config = self._get_config(active)
  388         config.apply_pending_overrides()
  389         self._current = config

tidy3d/plugins/dispersion/web.py

Lines 227-236

  227         """
  228 
  229         _env = config_env
  230         if _env == "default":
! 231             endpoint = str(config.web.api_endpoint or "")
! 232             _env = "dev" if "dev" in endpoint else "prod"
  233         return URL_ENV[_env]
  234 
  235     @staticmethod
  236     def _setup_server(url_server: str) -> tuple[dict[str, str], bool]:

Lines 248-258

  248             resp = requests.get(f"{url_server}/health", verify=ssl_verify)
  249             resp.raise_for_status()
  250         except (requests.exceptions.SSLError, ssl.SSLError):
  251             log.info("Retrying with SSL verification disabled.")
! 252             ssl_verify = False
! 253             resp = requests.get(f"{url_server}/health", verify=ssl_verify)
! 254             resp.raise_for_status()
  255         except Exception as e:
  256             raise WebError("Connection to the server failed. Please try again.") from e
  257 
  258         return get_headers(), ssl_verify

tidy3d/web/api/webapi.py

Lines 79-92

  79 
  80 
  81 def _get_folder_url(folder_id: str) -> str:
  82     """Get the URL for a task folder on our server."""
! 83     return _build_website_url(f"folders/{folder_id}")
  84 
  85 
  86 def _get_url_rf(resource_id: str) -> str:
  87     """Get the RF GUI URL for a modeler/batch group."""
! 88     return _build_website_url(f"rf?taskId={resource_id}")
  89 
  90 
  91 def _build_website_url(path: str) -> str:
  92     base = str(config.web.website_endpoint or "")

Lines 90-98

  90 
  91 def _build_website_url(path: str) -> str:
  92     base = str(config.web.website_endpoint or "")
  93     if not path:
! 94         return base
  95     return "/".join([base.rstrip("/"), str(path).lstrip("/")])
  96 
  97 
  98 def _is_modeler_batch(resource_id: str) -> bool:

tidy3d/web/cli/app.py

Lines 93-106

   93         current_apikey = get_description()
   94         message = f"Current API key: [{current_apikey}]\n" if current_apikey else ""
   95         apikey = click.prompt(f"{message}Please enter your api key", type=str)
   96 
!  97     target_url = config.web.build_api_url("apikey")
   98 
   99     try:
! 100         resp = requests.get(target_url, auth=auth, verify=config.web.ssl_verify)
  101     except (requests.exceptions.SSLError, ssl.SSLError):
! 102         resp = requests.get(target_url, auth=auth, verify=False)
  103 
  104     if resp.status_code == 200:
  105         click.echo("Configured successfully.")
  106         config.update_section("web", apikey=apikey)

tidy3d/web/core/http_util.py

Lines 199-207

  199 
  200 
  201 class TLSAdapter(HTTPAdapter):
  202     def init_poolmanager(self, *args: Any, **kwargs: Any) -> None:
! 203         context = create_urllib3_context(ssl_version=config.web.ssl_version)
  204         kwargs["ssl_context"] = context
  205         return super().init_poolmanager(*args, **kwargs)
  206 

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Well, I checked that it works... and greptile seems to like it - so ship it.

@yaugenst-flex yaugenst-flex added this pull request to the merge queue Oct 31, 2025
Merged via the queue into develop with commit 1d713a6 Oct 31, 2025
36 checks passed
@yaugenst-flex yaugenst-flex deleted the FXC-3929-refactor-profile-switching-to-remove-env-manager-divergence-around-web-endpoints branch October 31, 2025 13:29
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.

3 participants