Refactor Repomix configuration and update documentation
- Removed the `support/` and `tests/` directories from the Repomix configuration to streamline included files. - Updated `repomix-output.md` to reflect the new file structure and clarify included patterns. - Deleted the outdated `triage.md` document to reduce clutter and improve project organization. - Introduced a new `ui.md` document detailing the migration plan for transitioning to a Rust/Tauri application, including architecture overview and feature parity checklist.
This commit is contained in:
382
docs/triage.md
382
docs/triage.md
@@ -1,382 +0,0 @@
|
||||
According to a document from December 19, 2025 (your repomix-output.md snapshot), here’s a targeted review focused on duplication/redundancy, bug & race risks, optimization, and DRY/robustness—with concrete excerpts and “what to do next”.
|
||||
|
||||
⸻
|
||||
|
||||
Highest-impact bug & race-condition risks
|
||||
|
||||
1) Streaming diarization looks globally stateful (cross-meeting interference risk)
|
||||
|
||||
In your streaming state init, you reset diarization streaming state for every meeting:
|
||||
|
||||
if self._diarization_engine is not None:
|
||||
self._diarization_engine.reset_streaming()
|
||||
|
||||

|
||||
|
||||
And reset_streaming() resets the underlying streaming pipeline:
|
||||
|
||||
def reset_streaming(self) -> None:
|
||||
if self._streaming_pipeline is not None:
|
||||
self._streaming_pipeline.reset()
|
||||
|
||||

|
||||
|
||||
Why this is risky
|
||||
• If two meeting streams happen at once (multiple clients, or a reconnect edge-case), meeting B can reset meeting A’s diarization pipeline mid-stream.
|
||||
• You also have a global diarization lock in streaming paths (serialization), which suggests the diarization streaming pipeline is not intended to be shared concurrently across meetings. (That’s fine—but then enforce that constraint explicitly.) 
|
||||
|
||||
Recommendation
|
||||
Pick one of these models and make it explicit:
|
||||
• Model A: Only one active diarization stream allowed
|
||||
• Enforce a single-stream invariant when diarization streaming is enabled.
|
||||
• If a second stream begins, abort with FAILED_PRECONDITION (“diarization streaming is single-session”).
|
||||
• Model B: Diarization pipeline is per meeting
|
||||
• Store per-meeting diarization pipelines/state: self._diarization_streams[meeting_id] = DiarizationEngine(...) or engine.create_stream_session().
|
||||
• Remove global .reset_streaming() and instead reset only the per-meeting session.
|
||||
|
||||
Testing idea: add an async test that starts two streams and ensures diarization state for meeting A doesn’t get reset when meeting B starts.
|
||||
|
||||
⸻
|
||||
|
||||
2) StopMeeting closes the audio writer even if the stream might still be writing
|
||||
|
||||
In StopMeeting, you close the writer immediately:
|
||||
|
||||
meeting_id = request.meeting_id
|
||||
if meeting_id in self._audio_writers:
|
||||
self._close_audio_writer(meeting_id)
|
||||
|
||||

|
||||
|
||||
Why this is risky
|
||||
• Your streaming loop writes audio chunks while streaming is active. If StopMeeting can be called while StreamTranscription is still mid-loop, you can get:
|
||||
• “file not open” style exceptions,
|
||||
• partial/corrupted encrypted audio artifacts,
|
||||
• or silent loss depending on how _write_audio_chunk_safe is implemented.
|
||||
|
||||
Recommendation
|
||||
Make writer closure occur in exactly one place, ideally “stream teardown,” and make StopMeeting signal rather than force:
|
||||
• Track active streams and gate closure:
|
||||
• If meeting_id is currently streaming, set a stop_requested[meeting_id] = True and let the stream finally: close the writer.
|
||||
• Or: in StopMeeting, call the same cleanup routine that a stream uses, but only after ensuring the stream is stopped/cancelled.
|
||||
|
||||
Testing idea: a concurrency test that runs StreamTranscription and calls StopMeeting mid-stream, asserting no exceptions + writer is closed exactly once.
|
||||
|
||||
⸻
|
||||
|
||||
3) Naive datetimes + timezone-aware DB columns = subtle bugs and wrong timestamps
|
||||
|
||||
Your domain Meeting uses naive datetimes:
|
||||
|
||||
created_at: datetime = field(default_factory=datetime.now)
|
||||
|
||||

|
||||
|
||||
But your DB models use DateTime(timezone=True) while still defaulting to naive datetime.now:
|
||||
|
||||
created_at: Mapped[datetime] = mapped_column(
|
||||
DateTime(timezone=True),
|
||||
default=datetime.now,
|
||||
)
|
||||
|
||||

|
||||
|
||||
And you convert to proto timestamps using .timestamp():
|
||||
|
||||
created_at=meeting.created_at.timestamp(),
|
||||
|
||||

|
||||
|
||||
Why this is risky
|
||||
• Naive/aware comparisons can explode at runtime (depending on code paths).
|
||||
• .timestamp() on naive datetimes depends on local timezone assumptions.
|
||||
• You already use UTC-aware time elsewhere (example: retention test uses datetime.now(UTC)), so the intent seems to be “UTC everywhere.” 
|
||||
|
||||
Recommendation
|
||||
• Standardize: store and operate on UTC-aware datetimes everywhere.
|
||||
• Domain: default_factory=lambda: datetime.now(UTC)
|
||||
• DB: default=datetime.now(UTC) or server_default=func.now() (and be consistent about timezone)
|
||||
• Add a test that ensures all exported/serialized timestamps are monotonic and consistent (no “timezone drift”).
|
||||
|
||||
⸻
|
||||
|
||||
4) Keyring keystore bypasses your own validation logic
|
||||
|
||||
You have a proper validator:
|
||||
|
||||
def _decode_and_validate_key(self, encoded: str) -> bytes:
|
||||
...
|
||||
if len(decoded) != MASTER_KEY_SIZE:
|
||||
raise WrongKeySizeError(...)
|
||||
|
||||

|
||||
|
||||
…but KeyringKeyStore skips it:
|
||||
|
||||
if stored:
|
||||
return base64.b64decode(stored)
|
||||
|
||||

|
||||
|
||||
Why this is risky
|
||||
• Invalid base64 or wrong key sizes could silently produce bad keys, later causing encryption/decryption failures that look like “random corruption.”
|
||||
|
||||
Recommendation
|
||||
• Use _decode_and_validate_key() in all keystore implementations (file, env, keyring).
|
||||
• Add a test that injects an invalid stored keyring value and asserts a clear, typed failure.
|
||||
|
||||
⸻
|
||||
|
||||
Biggest duplication & DRY pain points (and how to eliminate them)
|
||||
|
||||
1) gRPC mixins repeat “DB path vs memory path” everywhere
|
||||
|
||||
Example in ListMeetings:
|
||||
|
||||
if self._use_database():
|
||||
async with self._create_uow() as uow:
|
||||
meetings = await uow.meetings.list_all()
|
||||
else:
|
||||
store = self._get_memory_store()
|
||||
meetings = list(store._meetings.values())
|
||||
return ListMeetingsResponse(meetings=[meeting_to_proto(m) for m in meetings])
|
||||
|
||||

|
||||
|
||||
Same pattern appears in summarization:
|
||||
|
||||
if self._use_database():
|
||||
summary = await self._generate_summary_db(...)
|
||||
else:
|
||||
summary = self._generate_summary_memory(...)
|
||||
|
||||
And streaming segment persistence is duplicated too (and commits per result; more on perf below). 
|
||||
|
||||
Why this matters
|
||||
• This is a structural duplication multiplier.
|
||||
• Code assistants will mirror the existing pattern and duplicate more.
|
||||
• You’ll fix a bug in one branch and forget the other.
|
||||
|
||||
Recommendation (best ROI refactor)
|
||||
Create a single abstraction layer and make the gRPC layer depend on it:
|
||||
• Define Ports/Protocols (async-friendly):
|
||||
• MeetingRepository, SegmentRepository, SummaryRepository, etc.
|
||||
• Provide two adapters:
|
||||
• SqlAlchemyMeetingRepository
|
||||
• InMemoryMeetingRepository
|
||||
• In the servicer, inject one concrete implementation behind a unified interface:
|
||||
• self.meetings_repo, self.segments_repo, self.summaries_repo
|
||||
|
||||
Then ListMeetings becomes:
|
||||
|
||||
meetings = await self.meetings_repo.list_all()
|
||||
return ListMeetingsResponse(meetings=[meeting_to_proto(m) for m in meetings])
|
||||
|
||||
No branching in every RPC = dramatically less duplication, fewer assistant copy/pastes, and fewer inconsistencies.
|
||||
|
||||
⸻
|
||||
|
||||
Performance / optimization hotspots
|
||||
|
||||
1) DB add_batch is not actually batching
|
||||
|
||||
Your segment repo’s add_batch loops and calls add for each segment:
|
||||
|
||||
async def add_batch(...):
|
||||
for seg in segments:
|
||||
await self.add(meeting_id, seg)
|
||||
|
||||

|
||||
|
||||
Why this hurts
|
||||
• If add() flushes/commits or even just issues INSERTs repeatedly, you get N roundtrips / flushes.
|
||||
|
||||
Recommendation
|
||||
• Build ORM models for all segments and call session.add_all([...]), then flush once.
|
||||
• If you need IDs, flush once and read them back.
|
||||
• Keep commit decision at the UoW/service layer (not inside repo).
|
||||
|
||||
⸻
|
||||
|
||||
2) Streaming persistence commits inside a tight loop
|
||||
|
||||
Inside _process_audio_segment (DB branch):
|
||||
|
||||
for result in results:
|
||||
...
|
||||
await uow.segments.add(meeting.id, segment)
|
||||
await uow.commit()
|
||||
yield segment_to_proto_update(meeting_id, segment)
|
||||
|
||||

|
||||
|
||||
Why this hurts
|
||||
• If ASR returns multiple results for one audio segment, you commit multiple times.
|
||||
• Commits are expensive and can become the bottleneck.
|
||||
|
||||
Recommendation
|
||||
• Commit once per processed audio segment (or once per gRPC request chunk batch), not once per ASR result.
|
||||
• You can still yield after staging; if you must ensure durability before yielding, commit once after staging all results for that audio segment.
|
||||
|
||||
⸻
|
||||
|
||||
3) Transcript search re-renders the entire list every change (likely OK now, painful later)
|
||||
|
||||
**RESOLVED**: Added 200ms debounce timer and visibility toggling instead of full rebuild.
|
||||
- Search input now uses `threading.Timer` with 200ms debounce
|
||||
- All segment rows are created once and visibility is toggled via `container.visible`
|
||||
- No more clearing and rebuilding on each keystroke
|
||||
|
||||
~~On search changes, you do a full rebuild:~~
|
||||
|
||||
~~self._segment_rows.clear()~~
|
||||
~~self._list_view.controls.clear()~~
|
||||
~~for idx, seg in enumerate(self._state.transcript_segments):~~
|
||||
~~...~~
|
||||
~~self._list_view.controls.append(row)~~
|
||||
|
||||

|
||||
|
||||
~~And it is triggered directly by search input changes:~~
|
||||
|
||||
~~self._search_query = (value or "").strip().lower()~~
|
||||
~~...~~
|
||||
~~self._rerender_all_segments()~~
|
||||
|
||||

|
||||
|
||||
~~Recommendation~~
|
||||
~~• Add a debounce (150–250ms) to search updates.~~
|
||||
~~• Consider incremental filtering (track which rows match and only hide/show).~~
|
||||
~~• If transcripts can become large: virtualized list rendering.~~
|
||||
|
||||
⸻
|
||||
|
||||
4) Client streaming stop/join can leak threads
|
||||
|
||||
stop_streaming joins with a timeout and clears the thread reference:
|
||||
|
||||
self._stream_thread.join(timeout=2.0)
|
||||
self._stream_thread = None
|
||||
|
||||
If the thread doesn’t exit in time, you can lose the handle while it’s still running.
|
||||
|
||||
Recommendation
|
||||
• After join timeout, check is_alive() and log + keep the reference (or escalate).
|
||||
• Better: make the worker reliably cancellable and join without timeout in normal shutdown paths.
|
||||
|
||||
⸻
|
||||
|
||||
How to write tests that catch more of these bugs
|
||||
|
||||
You already have a good base: there are stress tests around streaming cleanup / leak prevention (nice). 
|
||||
What’s missing tends to be concurrency + contract + regression tests that mirror real failure modes.
|
||||
|
||||
1) Add “race tests” for the exact dangerous interleavings
|
||||
|
||||
Concrete targets from this review:
|
||||
• StopMeeting vs StreamTranscription write
|
||||
• Start a stream, send some chunks, call StopMeeting mid-flight.
|
||||
• Assert: no exception, audio writer closed once, meeting state consistent.
|
||||
• Two streams with diarization enabled
|
||||
• Start stream A, then start stream B.
|
||||
• Assert: A’s diarization state isn’t reset (or assert server rejects second stream if you enforce single-stream).
|
||||
|
||||
Implementation approach:
|
||||
• Use grpc.aio and asyncio.gather() to force overlap.
|
||||
• Keep tests deterministic by using fixed chunk data and controlled scheduling (await asyncio.sleep(0) strategically).
|
||||
|
||||
2) Contract tests for DB vs memory parity
|
||||
|
||||
As long as you support both backends, enforce “same behavior”:
|
||||
• For each operation (create meeting, add segments, summary generation, deletion):
|
||||
• Run the same test suite twice: once with in-memory store, once with DB store.
|
||||
• Assert responses and side effects match.
|
||||
|
||||
This directly prevents the “fixed in DB path, broken in memory path” class of bugs.
|
||||
|
||||
3) Property-based tests for invariants (catches “weird” edge cases)
|
||||
|
||||
Use Hypothesis (or similar) for:
|
||||
• Random sequences of MeetingState transitions: ensure only allowed transitions occur.
|
||||
• Random segments + word timings:
|
||||
• word start/end are within segment range,
|
||||
• segment durations are non-negative,
|
||||
• transcript ordering invariants.
|
||||
|
||||
These tests are excellent at flushing out hidden assumptions.
|
||||
|
||||
4) Mutation testing (biggest upgrade if “bugs slip through”)
|
||||
|
||||
If you feel like “tests pass but stuff breaks,” mutation testing will tell you where your suite is weak.
|
||||
• Run mutation testing on core modules:
|
||||
• gRPC mixins (streaming/summarization)
|
||||
• repositories
|
||||
• crypto keystore/unwrap logic
|
||||
• You’ll quickly find untested branches and “assert-less” code.
|
||||
|
||||
5) Make regressions mandatory: “bug → test → fix”
|
||||
|
||||
A simple process change that works:
|
||||
• When you fix a bug, first write a failing test that reproduces it.
|
||||
• Only then fix the code.
|
||||
This steadily raises your “bugs can’t sneak back in” floor.
|
||||
|
||||
6) CI guardrails
|
||||
• Enforce coverage thresholds on:
|
||||
• domain + application + grpc layers (UI excluded if unstable)
|
||||
• Run stress/race tests nightly if they’re expensive.
|
||||
|
||||
⸻
|
||||
|
||||
How to get code assistants to stop duplicating code
|
||||
|
||||
1) Remove the architectural duplication magnets
|
||||
|
||||
The #1 driver is your repeated DB-vs-memory branching in every RPC/method. 
|
||||
|
||||
If you refactor to a single repository/service interface, the assistant literally has fewer “duplicate-shaped” surfaces to copy.
|
||||
|
||||
2) Add an “assistant contract” file to your repo
|
||||
|
||||
Create a short, explicit guide your assistants must follow, e.g. ASSISTANT_GUIDELINES.md:
|
||||
|
||||
Include rules like:
|
||||
• “Before writing code, search for existing helpers/converters/repositories and reuse them.”
|
||||
• “No new ‘_db’ / ‘_memory’ functions. Add to the repository interface instead.”
|
||||
• “If adding logic similar to existing code, refactor to shared helper instead of copy/paste.”
|
||||
• “Prefer editing an existing module over creating a new one.”
|
||||
|
||||
3) Prompt pattern that reduces duplication dramatically
|
||||
|
||||
When you ask an assistant to implement something, use a structure like:
|
||||
• Step 1: “List existing functions/files that already do part of this.”
|
||||
• Step 2: “Propose the minimal diff touching the fewest files.”
|
||||
• Step 3: “If code would be duplicated, refactor first (create helper + update callers).”
|
||||
• Step 4: “Show me the final patch.”
|
||||
|
||||
It forces discovery + reuse before generation.
|
||||
|
||||
4) Add duplication detection to CI
|
||||
|
||||
Make duplication a failing signal, not a suggestion:
|
||||
• Add a copy/paste detector (language-agnostic ones exist) and fail above a threshold.
|
||||
• Pair it with lint rules that push toward DRY patterns.
|
||||
|
||||
5) Code review rubric (even if you’re solo)
|
||||
|
||||
A quick checklist:
|
||||
• “Is this new code already present elsewhere?”
|
||||
• “Did we add a second way to do the same thing?”
|
||||
• “Is there now both a DB and memory version of the same logic?”
|
||||
|
||||
⸻
|
||||
|
||||
If you only do 3 things next
|
||||
1. Refactor gRPC mixins to a unified repository/service interface (kills most duplication + reduces assistant copy/paste). 
|
||||
2. Fix diarization streaming state scoping (global reset is the scariest race).  
|
||||
3. Add race tests for StopMeeting vs streaming + enforce UTC-aware datetimes.  
|
||||
|
||||
⸻
|
||||
|
||||
If you want, I can also propose a concrete refactor sketch (new interfaces + how to wire the servicer) that eliminates the DB/memory branching with minimal disruption—using the patterns you already have (UoW + repositories).
|
||||
9030
docs/ui.md
Normal file
9030
docs/ui.md
Normal file
File diff suppressed because it is too large
Load Diff
@@ -26,7 +26,7 @@
|
||||
"includeLogsCount": 50
|
||||
}
|
||||
},
|
||||
"include": ["src/", "tests/", "support/"],
|
||||
"include": ["src/"],
|
||||
"ignore": {
|
||||
"useGitignore": true,
|
||||
"useDefaultPatterns": true,
|
||||
|
||||
Reference in New Issue
Block a user