noteflow/docs/triage.md

This is a comprehensive code review of the NoteFlow repository.

Overall, this codebase demonstrates a high level of engineering maturity. It effectively utilizes Clean Architecture concepts (Entities, Use Cases, Ports/Adapters), leveraging strong typing, Pydantic for validation, and SQLAlchemy/Alembic for persistence. The integration test setup using testcontainers is particularly robust.

However, there are critical performance bottlenecks regarding async/sync bridging in the ASR engine, potential concurrency issues in the UI state management, and specific security considerations regarding the encryption implementation.

Below is the review categorized into actionable feedback, formatted to be convertible into Git issues.

---

1. Critical Architecture & Performance Issues

Issue 1: Blocking ASR Inference in Async gRPC Server

Severity: Critical Location: src/noteflow/grpc/service.py, src/noteflow/infrastructure/asr/engine.py

The Problem: The NoteFlowServer uses grpc.aio (AsyncIO), but the FasterWhisperEngine.transcribe method is blocking (synchronous CPU-bound operation). In NoteFlowServicer._maybe_emit_partial and _process_audio_segment, the code calls:

# src/noteflow/grpc/service.py
partial_text = " ".join(result.text for result in self._asr_engine.transcribe(combined))

Since transcribe performs heavy computation, executing it directly within an async def method freezes the entire Python AsyncIO event loop. This blocks heartbeats, other RPC calls, and other concurrent meeting streams until inference completes.

Actionable Solution: Offload the transcription to a separate thread pool executor.

1. Modify FasterWhisperEngine to remain synchronous (it wraps CTranslate2 which releases the GIL often, but it is still blocking from an asyncio perspective).
2. Update NoteFlowServicer to run transcription in an executor.

# In NoteFlowServicer
from functools import partial

# Helper method
async def _run_transcription(self, audio):
    loop = asyncio.get_running_loop()
    # Use a ThreadPoolExecutor specifically for compute-heavy tasks
    return await loop.run_in_executor(
        None, 
        partial(list, self._asr_engine.transcribe(audio))
    )

# Usage in _maybe_emit_partial
results = await self._run_transcription(combined)
partial_text = " ".join(r.text for r in results)

Issue 2: Synchronous sounddevice Callbacks in Async Client App

Severity: High Location: src/noteflow/infrastructure/audio/capture.py

The Problem: The sounddevice library calls the python callback from a C-level background thread. In SoundDeviceCapture._stream_callback, you are invoking the user-provided callback:

self._callback(audio_data, timestamp)

In app.py, this callback (_on_audio_frames) interacts with self._audio_activity.update and self._client.send_audio. While queue.put is thread-safe, any heavy logic or object allocation here happens in the real-time audio thread. If Python garbage collection pauses this thread, audio artifacts (dropouts) will occur.

Actionable Solution: The callback should strictly put bytes into a thread-safe queue and return immediately. A separate consumer thread/task should process the VAD, VU meter logic, and network sending.

Issue 3: Encryption Key Material in Memory

Severity: Medium Location: src/noteflow/infrastructure/security/crypto.py

The Problem: The AesGcmCryptoBox keeps the master key in memory via _get_master_cipher. While inevitable for operation, secrets.token_bytes creates immutable bytes objects which cannot be zeroed out (wiped) from memory when no longer needed. Python's GC handles cleanup, but the key lingers in RAM.

Actionable Solution: While strict memory zeroing is hard in Python, you should minimize the lifespan of the dek (Data Encryption Key).

1. In MeetingAudioWriter, the dek is stored as an instance attribute: self._dek. This keeps the unencrypted key in memory for the duration of the meeting.
2. Consider refactoring ChunkedAssetWriter to store the cipher object (the AESGCM context) rather than the raw bytes of the dek if the underlying C-library handles memory better, though strictly speaking, the key is still in RAM.
3. Critical: Ensure writer.close() sets self._dek = None immediately (it currently does, which is good practice).

---

2. Domain & Infrastructure Logic

Issue 4: Fallback Logic in SummarizationService

Severity: Low Location: src/noteflow/application/services/summarization_service.py

The Problem: The method _get_provider_with_fallback iterates through a hardcoded fallback_order = [SummarizationMode.LOCAL, SummarizationMode.MOCK]. This ignores the configuration order or user preference if they added new providers.

Actionable Solution: Allow SummarizationServiceSettings to define a fallback_chain: list[SummarizationMode].

Issue 5: Race Condition in MeetingStore (In-Memory)

Severity: Medium Location: src/noteflow/grpc/meeting_store.py

The Problem: The MeetingStore uses threading.RLock. However, the methods return the actual Meeting object reference.

def get(self, meeting_id: str) -> Meeting | None:
    with self._lock:
        return self._meetings.get(meeting_id)

The caller gets a reference to the mutable Meeting entity. If two threads get the meeting and modify it (e.g., meeting.state = ...), the MeetingStore lock does nothing to protect the entity itself, only the dictionary lookups.

Actionable Solution:

1. Return deep copies of the Meeting object (performance impact).
2. Or, implement specific atomic update methods on the Store (e.g., update_status(id, status)), rather than returning the whole object for modification.

Issue 6: pgvector Dependency Management

Severity: Low Location: src/noteflow/infrastructure/persistence/migrations/versions/6a9d9f408f40_initial_schema.py

The Problem: The migration blindly executes CREATE EXTENSION IF NOT EXISTS vector. On managed database services (like RDS or standard Docker Postgres images), the user might not have superuser privileges to install extensions, or the extension binaries might be missing.

Actionable Solution: Wrap the extension creation in a try/catch block or check capabilities. For the integration tests, ensure the pgvector/pgvector:pg16 image is strictly pinned (which you have done, good job).

---

3. Client & UI (Flet)

Issue 7: Massive app.py File Size

Severity: Medium Location: src/noteflow/client/app.py

The Problem: app.py is orchestrating too much. It handles UI layout, audio capture orchestration, gRPC client events, and state updates. It serves as a "God Class" Controller.

Actionable Solution: Refactor into a ClientController class separate from the UI layout construction.

1. src/noteflow/client/controller.py: Handles NoteFlowClient, SoundDeviceCapture, and updates AppState.
2. src/noteflow/client/views.py: Accepts AppState and renders UI.

Issue 8: Re-rendering Efficiency in Transcript

Severity: Medium Location: src/noteflow/client/components/transcript.py

The Problem: _render_final_segment appends controls to self._list_view.controls. In Flet, modifying a large list of controls can become slow as the transcript grows (hundreds of segments).

Actionable Solution:

1. Implement a "virtualized" list or pagination if Flet supports it efficiently.
2. If not, implement a sliding window rendering approach where only the last N segments + visible segments are rendered in the DOM, though this is complex in Flet.
3. Immediate fix: Ensure auto_scroll is handled efficiently. The current implementation clears and re-adds specific rows during search, which is heavy.

---

4. Specific Code Feedback (Nitpicks & Bugs)

1. Hardcoded Audio Constants

File: src/noteflow/infrastructure/asr/segmenter.py The SegmenterConfig defaults to sample_rate=16000. The SoundDeviceCapture defaults to 16000. Risk: If the server is configured for 44.1kHz, the client currently defaults to 16kHz hardcoded in several places. Fix: Ensure DEFAULT_SAMPLE_RATE from src/noteflow/config/constants.py is used everywhere.

2. Exception Swallowing in Audio Writer

File: src/noteflow/grpc/service.py -> _write_audio_chunk_safe

except Exception as e:
    logger.error("Failed to write audio chunk: %s", e)

If the disk fills up or permissions change, the audio writer fails silently (just logging), but the meeting continues. The user might lose the audio recording entirely while thinking it's safe. Fix: This error should probably trigger a circuit breaker that stops the recording or notifies the client via a gRPC status update or a metadata stream update.

3. Trigger Service Rate Limiting Logic

File: src/noteflow/application/services/trigger_service.py In _determine_action:

if self._last_prompt is not None:
    elapsed = now - self._last_prompt
    if elapsed < self._settings.rate_limit_seconds:
        return TriggerAction.IGNORE

This logic ignores all triggers if within the rate limit. If a high confidence trigger (Auto-start) comes in 10 seconds after a low confidence prompt, it gets ignored. Fix: The rate limit should likely apply to NOTIFY actions, but AUTO_START might need to bypass the rate limit or have a shorter one.

4. Database Session Lifecycle in UoW

File: src/noteflow/infrastructure/persistence/unit_of_work.py The __init__ does not create the session, __aenter__ does. This is correct. However, SqlAlchemyUnitOfWork caches repositories:

self._annotations_repo = SqlAlchemyAnnotationRepository(self._session)

If __aenter__ is called, __aexit__ closes the session. If the same UoW instance is reused (calling async with uow: again), it creates a new session but overwrites the repo references. This is generally safe, but verify that SqlAlchemyUnitOfWork instances are intended to be reusable or disposable. Currently, they look reusable, which is fine.

5. Frontend Polling vs Events

File: src/noteflow/client/components/playback_sync.py POSITION_POLL_INTERVAL = 0.1. Using a thread to poll self._state.playback.current_position every 100ms is CPU inefficient in Python (due to GIL). Suggestion: Use the sounddevice stream callback time info to update the position state only when audio is actually playing, rather than a separate while True loop.

---

5. Security Review

1. Keyring Headless Failure

File: src/noteflow/infrastructure/security/keystore.py Risk: The app crashes if keyring cannot find a backend (common in Docker/Headless Linux servers). Fix:

except keyring.errors.KeyringError:
    logger.warning("Keyring unavailable, falling back to environment variable or temporary key")
    # Implement a fallback strategy or explicit failure

Currently, it raises RuntimeError, which crashes the server startup.

2. DEK Handling

Analysis: You generate a DEK, wrap it, and store wrapped_dek in the DB. The dek stays in memory during the stream. Verdict: This is standard envelope encryption practice. Acceptable for this application tier.

---

6. Generated Issues for Git

Issue: Asynchronous Transcription Processing

Title: Refactor ASR Engine to run in ThreadPoolExecutor Description: The gRPC server uses asyncio, but FasterWhisperEngine.transcribe is blocking. This freezes the event loop during transcription segments. Task:

1. Inject asyncio.get_running_loop() into NoteFlowServicer.
2. Wrap self._asr_engine.transcribe calls in loop.run_in_executor.

Issue: Client Audio Callback Optimization

Title: Optimize Audio Capture Callback Description: SoundDeviceCapture callback executes application logic (network sending, VAD updates) in the audio thread. Task:

1. Change callback to only queue.put_nowait().
2. Move logic to a dedicated consumer worker thread.

Issue: Handle Write Errors in Audio Stream

Title: Critical Error Handling for Audio Writer Description: _write_audio_chunk_safe catches exceptions and logs them, potentially resulting in data loss without user feedback. Task:

1. If writing fails, update the meeting state to ERROR.
2. Send an error message back to the client via the Transcript stream if possible, or terminate the connection.

Issue: Database Extension Installation Check

Title: Graceful degradation for pgvector Description: Migration script 6a9d9f408f40 attempts to create an extension. This fails if the DB user isn't superuser. Task:

1. Check if extension exists or if user has permissions.
2. If not, fail with a clear message about required database setup steps.

Issue: Foreground App Window Detection on Linux/Headless

Title: Handle pywinctl dependencies Description: pywinctl requires X11/display headers on Linux. The server might run headless. Task:

1. Wrap ForegroundAppProvider imports in try/except blocks.
2. Ensure the app doesn't crash if pywinctl fails to load.

---

7. Packaging & Deployment (Future)

Since you mentioned packaging is a WIP:

1. Dependencies: Separating server deps (torch, faster-whisper) from client deps (flet, sounddevice) is crucial. Use pyproject.toml extras: pip install noteflow[server] vs noteflow[client].
2. Model Management: The Docker image for the server will be huge due to Torch/Whisper. Consider a build stage that pre-downloads the "base" model so the container starts faster.

Conclusion

The code is high quality, well-typed, and structurally sound. Fixing the Blocking ASR issue is the only mandatory change before any serious load testing or deployment. The rest are robustness and architectural improvements.