vasceannie/guide
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

.

Browse Source
This commit is contained in:
Travis Vasceannie
2025-11-22 11:40:31 +00:00
parent 9474f81ce9
commit 64d19d07f4
7 changed files with 293 additions and 459 deletions
Download Patch File Download Diff File Expand all files Collapse all files

219
docs/rolling_feedback.md
View File

@@ -1,184 +1,73 @@
This is an exceptionally high-quality prototype. You have moved past "scripting" into proper **software engineering for automation**. The separation of concerns, type safety, and architectural patterns used here are usually only seen in mature automation frameworks, not early scaffolds.
This is a high-quality prototype. You have moved beyond simple scripting and built a structured **Automation-as-a-Service** architecture. The use of FastAPI to wrap Playwright, combined with a Command Pattern for actions and a sophisticated browser pool, indicates a strong engineering mindset.
Here is a detailed review across your requested dimensions:
Here is my review of the architecture, scalability, consistency, organization, and comprehensiveness.
### 1. Architecture & Organization
**Verdict:** Strong, Modular, and Professional.
---
* **The "Strings" Registry (`src/guide/app/strings/`):**
This is the standout feature of your architecture. Instead of scattering CSS selectors and magic strings throughout your logic, you have a centralized, type-safe registry (`AppStrings`).
* *Why it’s good:* If a `data-test` attribute changes in the frontend, you update it in one place. The nested class structure (`app_strings.intake.selectors...`) provides IDE autocompletion, which drastically reduces developer error.
* **The Action Pattern (`src/guide/app/actions/`):**
Using a Command Pattern (via the `DemoAction` protocol) combined with a generic Registry is excellent. It decouples the *execution* of a demo step from the *API trigger*.
* *Recommendation:* Currently, `ActionContext` is passed in. Ensure that if you need to chain actions (e.g., Login -> Navigate -> Submit), the context allows state to persist or pass between actions.
* **Dependency Injection:**
Leveraging FastAPI’s `Depends` system to inject the `ActionRegistry`, `BrowserClient`, and `AppSettings` is the correct way to handle state in a web application. It makes unit testing significantly easier because you can mock these dependencies.
### 1. Architecture & Design Patterns
### 2. Scalability (The Browser Pool)
**Verdict:** Designed for Performance.
**Strengths:**
* **Command Pattern (`DemoAction`):** The decision to treat automation steps as discrete, self-contained "Actions" (`src/guide/app/actions/base.py`) is excellent. It decouples the *execution logic* from the *trigger mechanism* (API).
* **Composite Actions:** The `CompositeAction` class allows you to chain atomic actions (Login -> Intake -> Supplier) while maintaining shared state (`ActionContext`). This is crucial for complex user flows without monolithic scripts.
* **Dependency Injection:** Implementing your own DI container in `ActionRegistry` (`src/guide/app/actions/registry.py`) allows you to inject dependencies like `PersonaStore` or `LoginUrl` dynamically. This makes testing individual actions much easier.
* **Separation of Concerns:**
* **Models:** Pure data (Pydantic).
* **Browser:** Low-level Playwright management.
* **Actions:** Business logic.
* **Raindrop:** External API integration.
* **Connection Reuse (`src/guide/app/browser/pool.py`):**
Most automation prototypes launch a new browser per request, which is slow and resource-heavy. Your `BrowserPool` implementation maintains persistent connections (CDP or Headless) and simply allocates pages (tabs) or contexts. This is critical for low-latency responses.
* **CDP vs. Headless:**
Supporting both Remote CDP (for debugging or connecting to existing sessions) and Headless (for server-side execution) via configuration (`HostKind`) is a mature design choice.
* **Risk Area - Page Reclaiming:**
In `_pick_raindrop_page`, you look for an existing page with "raindrop.io".
* *Critique:* In a concurrent environment (multiple requests hitting the same browser host), "picking the first page" is dangerous. You might grab a page currently being used by another request.
* *Fix:* You should likely use **Browser Contexts** strictly. Every API request should spin up a `browser.new_context()`, do its work, and then `context.close()`. This isolates cookies/storage per request and prevents cross-contamination.
**Critique / Risks:**
* **Hybrid Browser/Context Management (`pool.py`):**
* In `BrowserInstance.allocate_page`, you distinguish between "CDP" (reuse existing pages) and "Headless" (new context).
* **Risk:** Reusing a `Page` object (CDP mode) across different "Personas" or requests is dangerous. Even with your locking mechanism, data leakage (cookies, local storage, session tokens) is highly likely unless you are strictly wiping the browser context between `acquire` calls.
* **Recommendation:** In Playwright, the unit of isolation is the **`BrowserContext`**, not the `Page`. Even over CDP, you should ideally create a new *Context* for every request to ensure full isolation, rather than recycling Pages.
### 3. Consistency & Code Quality
**Verdict:** High Discipline.
### 2. Scalability
* **Type Safety:** You are using `typing.Protocol`, `TypeVar`, and Pydantic models (`src/guide/app/models/`) extensively. This makes the codebase robust and self-documenting.
* **Config Management:** The layered configuration (Env Vars -> YAML -> JSON overrides) in `core/config.py` is production-ready. It allows you to deploy this container anywhere without code changes.
* **Error Handling:** You have a specific exception hierarchy (`GuideError`) and a centralized handler. This is much better than generic `try/except Exception` blocks.
**Strengths:**
* **Connection Pooling:** The `BrowserPool` avoids the heavy startup cost of launching a browser for every request.
* **Async First:** The entire stack is `async`/`await`, allowing the Python web server to handle concurrent incoming API requests efficiently while waiting on browser I/O.
### 4. Comprehensiveness
**Verdict:** Good Scaffold, missing Logic "Glue".
**Bottlenecks:**
* **Stateful Service:** Your service is stateful. The `BrowserPool` and `PersonaStore` live in memory.
* *Issue:* If you deploy this to Kubernetes with 2+ replicas, they will not share browser connections. If a user flow requires step 1 on Node A and step 2 on Node B, it will fail because the `shared_state` and browser session are local to Node A.
* *Fix:* Ensure sticky sessions (if using a load balancer) or design the API to be stateless (passing full context/cookies back and forth to the client), though the latter is hard with browser automation.
* **Resource Contention:** Browsers are memory hogs. The `MAX_CONTEXTS_PER_BROWSER = 10` limit is a good guardrail, but 10 concurrent Chromium contexts can easily consume 2-4GB of RAM.
* **Auth:** You have the "Happy Path" scaffolding (`DummyMfaCodeProvider`). The logic to scrape the "Current User" from the DOM to verify login status (`detect_current_persona`) is a smart, resilient touch.
* **External API (Raindrop):** You have a clean separation between UI actions (Playwright) and API actions (GraphQL).
* *Observation:* The `GraphQLClient` currently doesn't seem to share auth state with the Browser session. In many apps, you need the Browser's cookies to authorize the direct GraphQL API calls. You might need a bridge to extract cookies from Playwright and inject them into `httpx` headers.
### 3. Organization & File Structure
### 5. Specific Recommendations for Improvement
**Strengths:**
* **`src/guide/app/strings/registry.py`:** This is a standout feature. Centralizing selectors, labels, and texts into a typed registry acts as a "Page Object Model" layer. It prevents "magic strings" scattered across logic files and makes refactoring UI changes trivial.
* **`raindrop/generated` & `.graphql` files:** Storing GraphQL queries in `.graphql` files (`src/guide/app/raindrop/queries/`) is much cleaner than embedding strings in Python code.
#### A. Refine Browser Concurrency
In `src/guide/app/browser/pool.py`, the current logic for CDP hosts tries to reuse pages:
```python
# Current
pages = list(self.browser.contexts[0].pages)
```
**Recommendation:** Even for CDP, try to create ephemeral contexts if the browser supports it. If you must reuse a specific page (e.g., "The user is watching this specific tab"), ensure your `BrowserInstance` has a locking mechanism so two API requests don't drive the same page simultaneously.
**Critique:**
* **GraphQL Parsing:** In `queries.py`, you are using Regex (`re.findall`) to parse GraphQL files.
* *Risk:* Regex parsing of code is fragile. If a query contains comments or complex nesting, this might break.
* *Recommendation:* Use a proper GraphQL code generator (like `ariadne-codegen` or `gql`) to generate Pydantic models and query strings at build time.
* **Action Auto-discovery:** The logic in `_discover_action_modules` works by scanning the file system. While convenient, this can cause issues if the directory structure changes or if run in environments (like PyInstaller/Docker optimized builds) where file walking behaves differently.
#### B. Action Chaining / Orchestration
Currently, the API executes **one** action per request (`POST /actions`).
**Recommendation:** As the demo gets complex, you will want "Playbooks". You might need a `CompositeAction` that takes a list of Action IDs and executes them in sequence.
```python
# Future Concept
class OnboardingFlowAction(DemoAction):
async def run(self, page, context):
await self.registry.get("auth.login").run(page, context)
await self.registry.get("intake.basic").run(page, context)
```
### 4. Consistency & Code Quality
#### C. Resilience Utilities
You have `utils/retry.py`, which is good.
**Recommendation:** Add a specific `DOMRetry` or `Wait` utility. Playwright has auto-waiting, but often for demos, you need "visual stability" checks (waiting for animations to stop) which are distinct from "element is present" checks.
**Strengths:**
* **Type Hinting:** Extensive use of `typing` and `Pydantic` ensures data passing is rigid and predictable.
* **Error Handling:** The custom exception hierarchy (`GuideError`, `BrowserConnectionError`, etc.) in `errors/exceptions.py` is clean and allows for specific HTTP error responses.
* **Diagnostics:** The `DebugInfo` capture (screenshot/HTML/logs) in `diagnostics.py` is essential for headless automation.
#### D. Logging / Observability
**Recommendation:** Since this runs headless, when it fails, you have zero visibility.
* Add a mechanism in your `ActionEnvelope` to capture a **screenshot** or **HTML dump** if `status == "error"`.
* Return this (base64 encoded) or a link to it in the API response for easier debugging.
**Consistency Nits:**
* **Configuration Loading:** The config loader supports YAML, Env Vars, and JSON overrides. This is comprehensive but complex. Ensure you have strict precedence rules (which you seem to have) to avoid debugging nightmares where a setting is coming from an unexpected JSON env var.
### Summary
This is **not** a throwaway script; it is a microservice designed for automation. The foundations (Config, Registry, Pydantic, DI) are solid. Focus your next steps on **concurrency safety** (browser contexts) and **observability** (screenshots on failure).
### 5. Comprehensiveness
Yes, there are a few areas of **redundancy** and **duplicity** in the codebase. Some of it appears to be intentional boilerplate (to support strict typing), but some of it creates unnecessary maintenance overhead where you have to update two files to change one thing.
**Missing Elements (for a production-ready system):**
1. **Testing:** I see no unit or integration tests in the file list. Since you have Dependency Injection, you should be able to mock the `Page` object and test your Actions.
2. **Observability:** You have `logging.py`, but for an automation system, **Tracing** (e.g., OpenTelemetry) is vital. You want to see a trace span for "API Request" -> "Composite Action" -> "Child Action" -> "Playwright Click".
3. **Security (MFA):** `DummyMfaCodeProvider` is fine for a prototype. However, if this is for a demo, ensure the "Prod" implementation handles MFA secrets securely (e.g., via AWS Secrets Manager or Vault), not just environment variables.
Here are the specific areas of redundancy:
### Summary & Recommendations
### 1. The "Strings Registry" Double-Wrapping (High Boilerplate)
You have defined your selectors/labels in constant classes, and then you **re-define** pointers to them in `registry.py`.
**Verdict:** The scaffold is **excellent**. It is over-engineered for a simple script but perfectly engineered for a robust, long-term automation platform.
* **Source:** `src/guide/app/strings/selectors/intake.py`
* **Redundancy:** `src/guide/app/strings/registry.py`
**The Issue:**
In `registry.py`, you have this pattern:
```python
class IntakeStrings:
class _Selectors:
# You are manually mapping the variable AGAIN
description_field: ClassVar[str] = IntakeSelectors.DESCRIPTION_FIELD
```
If you add a new selector to `IntakeSelectors`, you must also open `registry.py` and add the pointer to expose it via `app_strings`.
**Fix:**
You can eliminate the `registry.py` mapping classes by simply importing the original classes and aliasing them in the root `__init__.py` or `registry.py`.
```python
# src/guide/app/strings/registry.py
from .selectors.intake import IntakeSelectors
from .labels.intake import IntakeLabels
class IntakeNamespace:
selectors = IntakeSelectors
labels = IntakeLabels
class AppStrings:
intake = IntakeNamespace
```
*Result: You get the same autocomplete usage (`app_strings.intake.selectors.DESCRIPTION_FIELD`), but you only define the variable once.*
### 2. Configuration vs. Domain Models (Data Mirroring)
You have two Pydantic models that represent the exact same data structure.
* **File 1:** `src/guide/app/core/config.py` -> `class PersonaConfig`
* **File 2:** `src/guide/app/models/personas/models.py` -> `class DemoPersona`
**The Issue:**
Both classes have `id`, `role`, `email`, `login_method`, `browser_host_id`.
In `src/guide/app/models/personas/store.py`, you explicitly map one to the other:
```python
# Redundant mapping logic
DemoPersona(
id=p.id,
role=PersonaRole(p.role), # Enum conversion is the only real work here
email=p.email,
...
)
```
**Fix:**
Since `DemoPersona` is the domain object, it can inherit from `PersonaConfig` or you can use `PersonaConfig` directly until you actually need domain-specific logic that doesn't belong in the config.
```python
# src/guide/app/models/personas/models.py
from guide.app.core.config import PersonaConfig
class DemoPersona(PersonaConfig):
# Add any runtime-only fields here
pass
```
### 3. GraphQL Definitions (Manual Sync)
This is a common issue in Python GraphQL implementations.
* **Queries:** `src/guide/app/strings/graphql/*.py` (The strings)
* **Types:** `src/guide/app/raindrop/types.py` (The Pydantic models)
**The Issue:**
If you add a field to the `GET_INTAKE_REQUEST` string, you **must** manually update `IntakeRequestData` in `types.py`. This is "Process Redundancy" and is error-prone.
**Fix:**
In a prototype, this is fine. In production, use a tool like **Ariadne Code Gen** or **Turms**. These tools read your `.graphql` query files and *generate* the Pydantic models automatically. This removes `types.py` from manual maintenance entirely.
### 4. Action Registration Pathways (Dual Logic)
You have two ways to register actions, which creates architectural ambiguity.
* **Pathway A (Auto):** `@register_action` decorator in `base.py`.
* **Pathway B (Manual):** Instantiating actions in `registry.py` inside `default_registry`.
**The Issue:**
In `default_registry()`, you have logic to load `@register_action` classes, but you *also* manually instantiate `LoginAsPersonaAction` because it requires dependencies (`PersonaStore`).
This means you have two "sources of truth" for what actions exist in the system.
**Fix:**
Standardize on **Dependency Injection via Factory**.
Remove the distinction. Make *all* actions registered via the class map. Modify the `ActionRegistry.get` method to inspect the class constructor; if the class needs `PersonaStore`, inject it automatically from the context (similar to how `pytest` fixtures work, or simpler manual DI).
Or, simpler: Just register the *factories* for everything, even the simple ones.
### 5. Browser Host Definitions
* **File:** `src/guide/app/models/domain/models.py` defines `BrowserHostsResponse`
* **File:** `src/guide/app/core/config.py` defines `BrowserHostConfig`
`BrowserHostsResponse` essentially wraps a dict of `BrowserHostConfig`.
```python
# models.py
class BrowserHostsResponse(BaseModel):
browser_hosts: dict[str, BrowserHostConfig]
```
This isn't strictly "bad" redundancy (it's a DTO wrapping a Config object), but strictly speaking, `BrowserHostConfig` is being used as both an internal configuration schema and a public API schema. If you ever want to hide a field (like an internal password in the host config) from the API response, you will accidentally leak it.
**Recommendation:** Create a specific `BrowserHostDTO` for the API response that *only* includes the fields the frontend needs (id, kind), and map to it. Currently, you are reusing the Config object, which is efficient but couples your backend config structure to your frontend API contract.
### Summary
The only "Must Fix" to reduce typing effort is **#1 (Strings Registry)**. The rest are architectural trade-offs typical in robust systems, which you can likely live with for now.
**Top 3 Recommendations:**
1. **Fix Browser Isolation:** Review `BrowserInstance.allocate_page`. Move towards creating a fresh `BrowserContext` for every single Action Context/Session, even when using CDP. Do not reuse Pages to avoid state pollution between demo users.
2. **Replace Regex GraphQL:** Switch from regex parsing in `queries.py` to a standard library or code generator.
3. **Add Structured Logging:** Implement JSON logging with `correlation_id` (which you already generate in `ActionContext`) included in every log line. This will be a lifesaver when debugging failed parallel demos.