✨(backend) add audio transcription#433
Open
maxenceh wants to merge 2 commits into
Open
Conversation
WalkthroughAdds audio transcription support (async STT jobs, webhook handling, transcript storage), integrates transcription into document ingestion and attachment lifecycle, introduces Tilt/local dev updates and a local Helm "extra" chart (Keycloak, MinIO, Postgres, Redis), updates frontend UI to reflect audio state, and adds tests and settings. ChangesTranscription + local dev
Sequence Diagram(s)sequenceDiagram
participant User as User/Client
participant Backend as Backend API
participant DB as Database
participant Storage as Django Storage
participant STT as STT Service
participant Webhook as Webhook Handler
participant AI as AI Agent
User->>Backend: Upload audio attachment
Backend->>Storage: Save audio file
Backend->>DB: Create attachment record (READY)
Backend->>STT: POST /async-jobs/transcribe (presigned URL)
STT-->>Backend: Return job_id
Backend->>DB: Update attachment (TRANSCRIBING + job_id)
STT->>Webhook: POST webhook (job_id, data_url)
Webhook->>DB: Find attachment by job_id
Webhook->>STT: GET transcription JSON (data_url)
STT-->>Webhook: Return WhisperX response
Webhook->>Storage: Save transcript markdown
Webhook->>DB: Create transcript attachment (READY)
Webhook->>DB: Update audio attachment (READY)
User->>Backend: Start conversation (reads attachments)
Backend->>Storage: Retrieve transcript markdown
Backend->>AI: Include transcript in agent context
AI-->>Backend: Return assistant response
Backend-->>User: Stream SSE events (document_parsing, agent output)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
maxenceh
changed the title
maxenceh
force-pushed
the
maxenceh/audio-transcription
branch
7 times, most recently
from
2c76465 to
4751a27
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
bin/Tiltfile (1)
39-42: Ensure kube-secret file exists before running Tilt.The secret generation requires
../env.d/development/kube-secretto exist. If this file is missing, Tilt will fail. Consider documenting this requirement or adding a fallback/error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/Tiltfile` around lines 39 - 42, The Tiltfile currently calls secret_yaml_generic(name='secret-dev', from_env_file='../env.d/development/kube-secret') without validating the file; add a pre-check to ensure '../env.d/development/kube-secret' exists and either (a) provide a clear error or log message and stop Tilt if missing, or (b) use a documented fallback/default secret path or inline defaults before invoking secret_yaml_generic; update documentation to mention the required kube-secret file presence and tie the message to the secret_yaml_generic call for clarity.src/backend/chat/transcription.py (2)
124-134: Consider handling the case where the transcript attachment doesn't exist.If the webhook fails to create the text attachment record but marks the original as
READY, theagetcall at line 124-127 will raiseDoesNotExist. Consider adding explicit error handling with a clearer message.🛡️ Proposed defensive handling
+ try: + text_attachment = await ChatConversationAttachment.objects.aget( + conversation=conversation, + conversion_from=attachment_key, + ) + except ChatConversationAttachment.DoesNotExist as exc: + raise ValueError( + "Transcript attachment not found. The transcription may have completed " + "but the transcript file was not saved correctly." + ) from exc - text_attachment = await ChatConversationAttachment.objects.aget( - conversation=conversation, - conversion_from=attachment_key, - )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/chat/transcription.py` around lines 124 - 134, The current code calls ChatConversationAttachment.objects.aget(conversation=conversation, conversion_from=attachment_key) and will raise ChatConversationAttachment.DoesNotExist if the transcript record is missing; update the logic in the function (around the aget call and the read_transcript helper) to catch that exception, log/raise a clearer error that includes conversation id and attachment_key (or return a well-documented None), and avoid calling default_storage.open when no text_attachment exists; ensure the error path mentions the symbol names ChatConversationAttachment.objects.aget, attachment_key, conversation, read_transcript and uses a clear, descriptive message for downstream callers.
59-61: Consider making the default language configurable.The language fallback
"fr-fr"is hardcoded. For international deployments, this should ideally be configurable via a Django setting (e.g.,settings.STT_DEFAULT_LANGUAGE) to avoid forcing French as the default transcription language.♻️ Proposed refactor
+from django.conf import settings as django_settings + response = requests.post( urljoin(settings.STT_SERVICE_URL, "async-jobs/transcribe"), json={ "user_sub": str(attachment.uploaded_by.sub), - "language": (attachment.uploaded_by.language or "fr-fr").split("-")[0], + "language": (attachment.uploaded_by.language or getattr(django_settings, 'STT_DEFAULT_LANGUAGE', 'en')).split("-")[0], "cloud_storage_url": presigned_url, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/chat/transcription.py` around lines 59 - 61, Replace the hardcoded fallback "fr-fr" with a configurable Django setting: import settings from django.conf and use something like (attachment.uploaded_by.language or getattr(settings, "STT_DEFAULT_LANGUAGE", "fr-fr")).split("-")[0] when building the payload (the code around attachment.uploaded_by.language in transcription.py), so deployments can override the default via settings.STT_DEFAULT_LANGUAGE while keeping "fr-fr" as a safe fallback.src/backend/chat/clients/pydantic_ai.py (1)
606-623: Consider timeout implications for audio transcript waiting.The
wait_for_transcriptfunction can block for up to 20 minutes while polling for the transcription result. While this is an async context, a long-running transcription could significantly delay the user's message processing. Consider whether the UI provides adequate feedback during this wait period or if there should be additional keepalive events emitted while waiting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/chat/clients/pydantic_ai.py` around lines 606 - 623, The current await wait_for_transcript(...) call in the audio branch (seen with is_audio_content_type and subsequent appending to self._audio_document_names) can block processing for up to 20 minutes; change this so long polling does not stall request handling — either run the transcription wait in a background task (e.g., asyncio.create_task) that calls document_store.astore_document when ready and immediately record a placeholder/metadata entry and self._audio_document_names, or wrap wait_for_transcript in asyncio.wait_for with a shorter configurable timeout and, on timeout, store a placeholder and schedule background retry; also emit periodic keepalive/progress events on self.conversation while waiting (or from the background task) so the UI gets feedback. Ensure parse_and_store_document remains used for non-audio paths.src/helm/extra/templates/keycloak-sts.yaml (1)
70-71: Update Keycloak to a more recent version.Keycloak 22.0.5 is significantly outdated (released late 2023). The latest stable version is 26.6.1 (as of April 2026), offering multiple minor version improvements that include security patches and feature enhancements. Update to a current version if compatible with your deployment requirements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helm/extra/templates/keycloak-sts.yaml` around lines 70 - 71, Update the Keycloak container image tag in the StatefulSet template to a newer stable release: locate the container named "keycloak" in the keycloak-sts.yaml template and change the image string from "quay.io/keycloak/keycloak:22.0.5" to a current stable tag such as "quay.io/keycloak/keycloak:26.6.1" (or another approved version), then run your regular validation/helm lint to ensure compatibility.src/helm/env.d/dev/values.conversations.yaml.gotmpl (2)
15-17: Inconsistent secret management pattern.Lines 15-17 use hardcoded placeholder values for
DJANGO_SECRET_KEY,OIDC_RP_CLIENT_SECRET, andAWS_S3_SECRET_ACCESS_KEY, while lines 18-61 referencesecret-devviasecretKeyRef. This inconsistency may cause confusion when onboarding developers.Consider either documenting this pattern clearly or moving all sensitive values to
secretKeyReffor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helm/env.d/dev/values.conversations.yaml.gotmpl` around lines 15 - 17, The three hardcoded placeholders (DJANGO_SECRET_KEY, OIDC_RP_CLIENT_SECRET, AWS_S3_SECRET_ACCESS_KEY) should be converted to the same secretKeyRef pattern used elsewhere: create/ensure keys exist in the Kubernetes secret named secret-dev and replace each hardcoded value with a secretKeyRef referencing that secret (use the exact secret names DJANGO_SECRET_KEY, OIDC_RP_CLIENT_SECRET, AWS_S3_SECRET_ACCESS_KEY as keys in secret-dev) so the template uniformly reads values from secret-dev rather than embedding plaintext in the values file.
163-166: Hardcoded Python version in certificate mount path.The mount path
/app/.venv/lib/python3.13/site-packages/certifi/cacert.pemis tightly coupled to Python 3.13. If the base image or virtual environment upgrades Python, this path will silently break SSL certificate handling.Consider using a symlink or environment variable
Alternatively, mount to a fixed path and set
SSL_CERT_FILEorREQUESTS_CA_BUNDLEenvironment variable:extraVolumeMounts: - name: certs - mountPath: /app/.venv/lib/python3.13/site-packages/certifi/cacert.pem + mountPath: /etc/ssl/certs/custom-ca.pem subPath: cacert.pemThen add to envVars:
SSL_CERT_FILE: /etc/ssl/certs/custom-ca.pem REQUESTS_CA_BUNDLE: /etc/ssl/certs/custom-ca.pem🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helm/env.d/dev/values.conversations.yaml.gotmpl` around lines 163 - 166, The certificate mount is hardcoded to a Python versioned site-packages path (extraVolumeMounts entry with name: certs and mountPath: /app/.venv/lib/python3.13/site-packages/certifi/cacert.pem) which will break when Python is upgraded; change the mountPath to a stable location (e.g. /etc/ssl/certs/custom-ca.pem) and update the chart values to add the corresponding envVars (SSL_CERT_FILE and REQUESTS_CA_BUNDLE pointing to /etc/ssl/certs/custom-ca.pem) so processes use the mounted cert file instead of relying on a Python-specific path.src/backend/chat/views.py (1)
792-798: Consider narrowing the exception handling or adding more context to logs.The broad
except Exceptioncatches all failures from HTTP request, JSON parsing, and Pydantic validation. While logging the exception provides a stack trace, distinguishing between network failures, invalid JSON, and schema violations could help with debugging in production.Optional: More granular error handling
try: transcript_response = http_requests.get(payload.transcription_data_url, timeout=30) transcript_response.raise_for_status() whisper_data = WhisperXResponse.model_validate_json(transcript_response.content) - except Exception: # pylint: disable=broad-except - logger.exception("Failed to fetch or parse transcript for job %s", job_id) + except http_requests.RequestException: + logger.exception("Failed to fetch transcript for job %s", job_id) + return Response({"error": "Failed to fetch transcript"}, status=status.HTTP_200_OK) + except PydanticValidationError: + logger.exception("Failed to parse transcript response for job %s", job_id) return Response({"error": "Failed to fetch transcript"}, status=status.HTTP_200_OK)
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/Tiltfile`:
- Around line 19-22: The trigger path in the run(...) call is incorrect because
the Tiltfile lives in bin/, so update the trigger argument in the run(...)
invocation (the string './src/backend/requirements.txt') to the correct relative
path '../src/backend/requirements.txt' so it points at the root-level
src/backend/requirements.txt and matches the existing sync sources
'../src/backend'.
In `@CHANGELOG.md`:
- Line 35: Move the entry "✨(dev) setup Tilt for local development" out of the
current section and place it under the appropriate changelog header (preferably
"### Added" or "### Changed"); update the surrounding headers so the entry
matches the conventional category for new features/changes, preserving the exact
entry text and formatting.
In `@docs/tilt.md`:
- Around line 84-85: Update the button/label text "Run makemigration" to the
plural form "Run makemigrations" to match the actual command `python manage.py
makemigrations`; find and replace the label string "Run makemigration" (in
docs/tilt.md) so it exactly matches the command and other label styles used for
"Run database migration".
In `@src/backend/chat/authentication.py`:
- Around line 27-34: The Authorization parsing currently assumes a capitalized
"Bearer" prefix; update the logic in the authentication flow (the block that
reads authorization_header and computes token) to split the header once (e.g.,
parts = authorization_header.split(None, 1)), verify that parts[0].lower() ==
"bearer" before using parts[1] as the token, and only then compare that token to
settings.STT_WEBHOOK_API_KEY; preserve the existing warning log
(logger.warning(... request.META.get("REMOTE_ADDR")) ) and raise
AuthenticationFailed() when the scheme is wrong, missing, or the token does not
match.
In `@src/backend/chat/tests/clients/pydantic_ai/test_parse_audio_documents.py`:
- Around line 130-132: The assertion uses the wrong mock attribute: replace
mock_store.store_document.assert_not_called() with
mock_store.astore_document.assert_not_called() (matching the mock configured in
other tests) so the test actually verifies the intended method was not called;
ensure the test's mock setup references astore_document (the async store method)
and keep the existing error_raised is not None check as-is to satisfy the
SonarCloud warning about the nonlocal closure.
In `@src/backend/chat/webhook_models.py`:
- Around line 51-56: The TranscribeWebhookFailurePayload model's error_code
field is too restrictive (error_code: Literal["unknown_error"]) and will reject
new upstream codes; update the error_code definition in
TranscribeWebhookFailurePayload to accept arbitrary strings (e.g., change its
type to str while preserving the Field title/description) so unknown error codes
validate properly; ensure you keep the field name error_code and its metadata
(title/description) so behavior and docs remain consistent.
In `@src/backend/core/file_upload/utils.py`:
- Around line 16-24: The is_audio_content_type function fails when content_type
includes parameters or mixed case (e.g. "audio/ogg; codecs=opus"); update it to
normalize the MIME string before checks by lowercasing and trimming off any
parameters (split on ';' and strip) and then test the normalized value with
startswith("audio/") and against the allowed container set (e.g.
"application/ogg"); ensure you only change the content-type handling inside
is_audio_content_type and keep the semantics for container formats.
In
`@src/frontend/apps/conversations/src/features/chat/components/MessageItem.tsx`:
- Around line 391-408: The memo comparator arePropsEqual used by MessageItem
only compares message.parts.length, which misses state changes inside parts
(e.g., toolInvocationParts transitioning to result.error) so memoization can
skip re-rendering the error UI; update arePropsEqual to also consider each
part's relevant identity/state (for example inspect
message.parts[].toolInvocation.state and
message.parts[].toolInvocation.result?.state or compute a simple derived
key/hash of message.parts) so changes to a part's toolInvocation status will
break memoization and allow MessageItem (and the toolInvocationParts rendering
block) to re-render.
In
`@src/frontend/apps/conversations/src/features/chat/components/ToolInvocationItem.tsx`:
- Around line 25-52: Formatting in the ToolInvocationItem component is off; run
your project's Prettier formatter and reformat the block handling toolInvocation
(the branch checking toolInvocation.state === 'result', the result typing, and
the args/documents/hasAudio declarations) so spacing, commas, and line breaks
match project style; after running Prettier, stage the updated
ToolInvocationItem.tsx changes and re-run the build to ensure Prettier --check
passes.
In `@src/helm/extra/templates/minio.yaml`:
- Around line 85-90: The current Pod template mounts a volatile volume
(volumeMounts -> mountPath: /data, volumes -> name: data, emptyDir: {}) which
will delete all MinIO objects on pod restart; replace the emptyDir volume with a
PVC-backed volume by declaring volumes -> name: data -> persistentVolumeClaim:
{claimName: <minio-pvc-name>} (and ensure a corresponding PersistentVolumeClaim
manifest or Helm pvc template exists or is added to the chart). Update any Helm
values/templates to allow configuring the PVC name/size/storageClass and ensure
the StatefulSet/Deployment uses that claim for the data volume instead of
emptyDir.
---
Nitpick comments:
In `@bin/Tiltfile`:
- Around line 39-42: The Tiltfile currently calls
secret_yaml_generic(name='secret-dev',
from_env_file='../env.d/development/kube-secret') without validating the file;
add a pre-check to ensure '../env.d/development/kube-secret' exists and either
(a) provide a clear error or log message and stop Tilt if missing, or (b) use a
documented fallback/default secret path or inline defaults before invoking
secret_yaml_generic; update documentation to mention the required kube-secret
file presence and tie the message to the secret_yaml_generic call for clarity.
In `@src/backend/chat/clients/pydantic_ai.py`:
- Around line 606-623: The current await wait_for_transcript(...) call in the
audio branch (seen with is_audio_content_type and subsequent appending to
self._audio_document_names) can block processing for up to 20 minutes; change
this so long polling does not stall request handling — either run the
transcription wait in a background task (e.g., asyncio.create_task) that calls
document_store.astore_document when ready and immediately record a
placeholder/metadata entry and self._audio_document_names, or wrap
wait_for_transcript in asyncio.wait_for with a shorter configurable timeout and,
on timeout, store a placeholder and schedule background retry; also emit
periodic keepalive/progress events on self.conversation while waiting (or from
the background task) so the UI gets feedback. Ensure parse_and_store_document
remains used for non-audio paths.
In `@src/backend/chat/transcription.py`:
- Around line 124-134: The current code calls
ChatConversationAttachment.objects.aget(conversation=conversation,
conversion_from=attachment_key) and will raise
ChatConversationAttachment.DoesNotExist if the transcript record is missing;
update the logic in the function (around the aget call and the read_transcript
helper) to catch that exception, log/raise a clearer error that includes
conversation id and attachment_key (or return a well-documented None), and avoid
calling default_storage.open when no text_attachment exists; ensure the error
path mentions the symbol names ChatConversationAttachment.objects.aget,
attachment_key, conversation, read_transcript and uses a clear, descriptive
message for downstream callers.
- Around line 59-61: Replace the hardcoded fallback "fr-fr" with a configurable
Django setting: import settings from django.conf and use something like
(attachment.uploaded_by.language or getattr(settings, "STT_DEFAULT_LANGUAGE",
"fr-fr")).split("-")[0] when building the payload (the code around
attachment.uploaded_by.language in transcription.py), so deployments can
override the default via settings.STT_DEFAULT_LANGUAGE while keeping "fr-fr" as
a safe fallback.
In `@src/helm/env.d/dev/values.conversations.yaml.gotmpl`:
- Around line 15-17: The three hardcoded placeholders (DJANGO_SECRET_KEY,
OIDC_RP_CLIENT_SECRET, AWS_S3_SECRET_ACCESS_KEY) should be converted to the same
secretKeyRef pattern used elsewhere: create/ensure keys exist in the Kubernetes
secret named secret-dev and replace each hardcoded value with a secretKeyRef
referencing that secret (use the exact secret names DJANGO_SECRET_KEY,
OIDC_RP_CLIENT_SECRET, AWS_S3_SECRET_ACCESS_KEY as keys in secret-dev) so the
template uniformly reads values from secret-dev rather than embedding plaintext
in the values file.
- Around line 163-166: The certificate mount is hardcoded to a Python versioned
site-packages path (extraVolumeMounts entry with name: certs and mountPath:
/app/.venv/lib/python3.13/site-packages/certifi/cacert.pem) which will break
when Python is upgraded; change the mountPath to a stable location (e.g.
/etc/ssl/certs/custom-ca.pem) and update the chart values to add the
corresponding envVars (SSL_CERT_FILE and REQUESTS_CA_BUNDLE pointing to
/etc/ssl/certs/custom-ca.pem) so processes use the mounted cert file instead of
relying on a Python-specific path.
In `@src/helm/extra/templates/keycloak-sts.yaml`:
- Around line 70-71: Update the Keycloak container image tag in the StatefulSet
template to a newer stable release: locate the container named "keycloak" in the
keycloak-sts.yaml template and change the image string from
"quay.io/keycloak/keycloak:22.0.5" to a current stable tag such as
"quay.io/keycloak/keycloak:26.6.1" (or another approved version), then run your
regular validation/helm lint to ensure compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d06665bd-94fe-4f52-b852-1285e31d9fa8
📒 Files selected for processing (38)
.dockerignoreCHANGELOG.mdMakefileREADME.mdbin/Tiltfiledocs/examples/conversations.values.yamldocs/tilt.mdenv.d/development/common.distenv.d/development/kube-secret.distsrc/backend/chat/authentication.pysrc/backend/chat/clients/pydantic_ai.pysrc/backend/chat/malware_detection.pysrc/backend/chat/migrations/0007_chatconversationattachment_transcription_job_id.pysrc/backend/chat/models.pysrc/backend/chat/tests/clients/pydantic_ai/test_parse_audio_documents.pysrc/backend/chat/tests/test_malware_detection.pysrc/backend/chat/tests/test_transcription.pysrc/backend/chat/tests/views/chat/conversations/test_conversation_with_audio_upload.pysrc/backend/chat/tests/views/chat/conversations/test_conversation_with_document_upload.pysrc/backend/chat/tests/views/chat/conversations/test_conversation_with_document_url.pysrc/backend/chat/tests/views/test_transcription_webhook.pysrc/backend/chat/transcription.pysrc/backend/chat/views.pysrc/backend/chat/webhook_models.pysrc/backend/conversations/settings.pysrc/backend/core/file_upload/enums.pysrc/backend/core/file_upload/utils.pysrc/backend/core/urls.pysrc/frontend/apps/conversations/src/features/chat/components/MessageItem.tsxsrc/frontend/apps/conversations/src/features/chat/components/ToolInvocationItem.tsxsrc/helm/env.d/dev-staging/values.conversations.yaml.gotmplsrc/helm/env.d/dev/values.conversations.yaml.gotmplsrc/helm/extra/Chart.yamlsrc/helm/extra/templates/kc-postgresql-sts.yamlsrc/helm/extra/templates/keycloak-sts.yamlsrc/helm/extra/templates/minio.yamlsrc/helm/extra/templates/postgresql-sts.yamlsrc/helm/helmfile.yaml.gotmpl
Comment on lines
+19
to
+22
| run( | ||
| 'pip install -r /app/requirements.txt', | ||
| trigger=['./src/backend/requirements.txt'] | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the actual path structure relative to the Tiltfile
fd -t f 'requirements.txt' src/backend
# Check the bin directory location relative to repo root
ls -la bin/Tiltfile 2>/dev/null || echo "Tiltfile not at expected location"Repository: suitenumerique/conversations
Length of output: 1818
🌐 Web query:
Tilt trigger path resolution relative to Tiltfile location
💡 Result:
In Tilt, relative paths in Tiltfile Starlark are evaluated relative to the Tiltfile’s own directory (the file whose Tiltfile is being executed). The result is that a referenced path is resolved against that Tiltfile’s directory, not necessarily against the directory you ran the tilt CLI from. However, for the special case of where the “main/root Tiltfile” lives, Tilt also exposes config.main_dir, which is the directory containing the Tiltfile that started the session. This is commonly used when you have nested Tiltfiles and need a stable anchor for resolving paths relative to the root Tiltfile directory. So, for your question “Tilt trigger path resolution relative to Tiltfile location”: - tilt trigger itself takes a resource name, not a path; it triggers an update for that named resource, and any path resolution behavior comes from how that resource was defined (e.g., via docker_build, k8s_yaml, local_resource, file deps, etc.). - When the resource definition uses relative paths (Tiltfile functions’ path parameters, or any file paths you read/declare as deps), those relative paths are resolved relative to the Tiltfile that defined them. - If you specifically need “relative to the root Tiltfile directory” (useful with nested Tiltfiles), use config.main_dir to build absolute paths from a stable base. Example pattern for root-anchored path building in a nested Tiltfile: repo_root = config.main_dir load_yaml = repo_root + '/path/to/common.yaml' (Constructing the exact string/using it with k8s_yaml/load depends on the API.)
Citations:
- 1: https://docs.tilt.dev/tiltfile_concepts.html
- 2: Specify path relative to (root) Tiltfile directory tilt-dev/tilt#3501
- 3: https://github.com/windmilleng/tilt/blob/master/internal/tiltfile/config/config.go
- 4: https://docs.tilt.dev/multiple_repos.html
- 5: https://docs.tilt.dev/tutorial/4-code-update-repeat.html
- 6: tiltfile: add working directory option to local and local_resource tilt-dev/tilt#4644
Fix trigger path to use correct relative path from Tiltfile location.
The trigger path './src/backend/requirements.txt' is incorrect. Since the Tiltfile is located in the bin/ directory, relative paths are resolved from there. The path './src/backend/requirements.txt' resolves to bin/src/backend/requirements.txt, which does not exist. The sync sources from '../src/backend' (the root-level src/backend/ directory), so the trigger should be '../src/backend/requirements.txt' to reference the correct file and match the sync context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/Tiltfile` around lines 19 - 22, The trigger path in the run(...) call is
incorrect because the Tiltfile lives in bin/, so update the trigger argument in
the run(...) invocation (the string './src/backend/requirements.txt') to the
correct relative path '../src/backend/requirements.txt' so it points at the
root-level src/backend/requirements.txt and matches the existing sync sources
'../src/backend'.
| - 💄(ui) little fix margin top | ||
| - 💄(ui) review ui for part of the project | ||
| - 🐛(fix) Fix streaming crash with OpenAI-compatible APIs | ||
| - ✨(dev) setup Tilt for local development |
There was a problem hiding this comment.
Use the right changelog section for this entry.
At Line 35, this item reads as an addition, not a fix. Moving it to ### Added (or ### Changed) will keep release notes consistent.
✍️ Suggested changelog adjustment
### Fixed
- 🐛(fix) new conversation in project button max size
- 💄(ui) little fix margin top
- 💄(ui) review ui for part of the project
- 🐛(fix) Fix streaming crash with OpenAI-compatible APIs
-- ✨(dev) setup Tilt for local development
+
+### Added
+
+- ✨(dev) setup Tilt for local development📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - ✨(dev) setup Tilt for local development | |
| ### Fixed | |
| - 🐛(fix) new conversation in project button max size | |
| - 💄(ui) little fix margin top | |
| - 💄(ui) review ui for part of the project | |
| - 🐛(fix) Fix streaming crash with OpenAI-compatible APIs | |
| ### Added | |
| - ✨(dev) setup Tilt for local development |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 35, Move the entry "✨(dev) setup Tilt for local
development" out of the current section and place it under the appropriate
changelog header (preferably "### Added" or "### Changed"); update the
surrounding headers so the entry matches the conventional category for new
features/changes, preserving the exact entry text and formatting.
Comment on lines
+84
to
+85
| - **Run makemigration** — runs `python manage.py makemigrations` | ||
| - **Run database migration** — runs `python manage.py migrate --no-input` |
There was a problem hiding this comment.
Fix command label typo for consistency.
makemigrations is plural; the button label text currently says “Run makemigration”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tilt.md` around lines 84 - 85, Update the button/label text "Run
makemigration" to the plural form "Run makemigrations" to match the actual
command `python manage.py makemigrations`; find and replace the label string
"Run makemigration" (in docs/tilt.md) so it exactly matches the command and
other label styles used for "Run database migration".
Comment on lines
+27
to
+34
| authorization_header: str = request.headers.get("Authorization") or "" | ||
| token = authorization_header.removeprefix("Bearer ") | ||
| if not token or token != settings.STT_WEBHOOK_API_KEY: | ||
| logger.warning( | ||
| "Authentication failed: Bad Authorization header (ip: %s)", | ||
| request.META.get("REMOTE_ADDR"), | ||
| ) | ||
| raise AuthenticationFailed() |
There was a problem hiding this comment.
Parse the Authorization scheme case-insensitively.
HTTP auth schemes are case-insensitive, so this rejects valid headers like authorization: bearer <token>. If the STT provider lowercases the scheme, every webhook call will 403. Split once and compare the scheme case-insensitively before checking the token. (httpwg.org)
Possible fix
+import secrets
+
authorization_header: str = request.headers.get("Authorization") or ""
- token = authorization_header.removeprefix("Bearer ")
- if not token or token != settings.STT_WEBHOOK_API_KEY:
+ scheme, _, token = authorization_header.partition(" ")
+ if (
+ scheme.lower() != "bearer"
+ or not token
+ or not secrets.compare_digest(token, settings.STT_WEBHOOK_API_KEY)
+ ):
logger.warning(
"Authentication failed: Bad Authorization header (ip: %s)",
request.META.get("REMOTE_ADDR"),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| authorization_header: str = request.headers.get("Authorization") or "" | |
| token = authorization_header.removeprefix("Bearer ") | |
| if not token or token != settings.STT_WEBHOOK_API_KEY: | |
| logger.warning( | |
| "Authentication failed: Bad Authorization header (ip: %s)", | |
| request.META.get("REMOTE_ADDR"), | |
| ) | |
| raise AuthenticationFailed() | |
| authorization_header: str = request.headers.get("Authorization") or "" | |
| scheme, _, token = authorization_header.partition(" ") | |
| if ( | |
| scheme.lower() != "bearer" | |
| or not token | |
| or not secrets.compare_digest(token, settings.STT_WEBHOOK_API_KEY) | |
| ): | |
| logger.warning( | |
| "Authentication failed: Bad Authorization header (ip: %s)", | |
| request.META.get("REMOTE_ADDR"), | |
| ) | |
| raise AuthenticationFailed() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/chat/authentication.py` around lines 27 - 34, The Authorization
parsing currently assumes a capitalized "Bearer" prefix; update the logic in the
authentication flow (the block that reads authorization_header and computes
token) to split the header once (e.g., parts = authorization_header.split(None,
1)), verify that parts[0].lower() == "bearer" before using parts[1] as the
token, and only then compare that token to settings.STT_WEBHOOK_API_KEY;
preserve the existing warning log (logger.warning(...
request.META.get("REMOTE_ADDR")) ) and raise AuthenticationFailed() when the
scheme is wrong, missing, or the token does not match.
Comment on lines
+85
to
+90
| volumeMounts: | ||
| - mountPath: /data | ||
| name: data | ||
| volumes: | ||
| - name: data | ||
| emptyDir: {} |
There was a problem hiding this comment.
emptyDir will wipe uploads and transcripts on any MinIO pod restart.
This service is the backing store for the app's S3 media storage, so recreating the pod drops every object while attachment rows still point at them. That will make the new transcription flow flaky in dev after any reschedule/restart. Use a PVC-backed volume here instead of emptyDir.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/helm/extra/templates/minio.yaml` around lines 85 - 90, The current Pod
template mounts a volatile volume (volumeMounts -> mountPath: /data, volumes ->
name: data, emptyDir: {}) which will delete all MinIO objects on pod restart;
replace the emptyDir volume with a PVC-backed volume by declaring volumes ->
name: data -> persistentVolumeClaim: {claimName: <minio-pvc-name>} (and ensure a
corresponding PersistentVolumeClaim manifest or Helm pvc template exists or is
added to the chart). Update any Helm values/templates to allow configuring the
PVC name/size/storageClass and ensure the StatefulSet/Deployment uses that claim
for the data volume instead of emptyDir.
4751a27 to
4f85c1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/frontend/apps/conversations/src/features/chat/components/ToolInvocationItem.tsx (1)
48-53: Consider extracting the args type to a named interface.The inline type assertion works but could be clearer with a named interface, especially as this type may be reused elsewhere.
♻️ Optional refactor
+interface DocumentParsingArgs { + documents: unknown; + has_audio?: boolean; +} + // In the component: -const args = toolInvocation.args as { - documents: unknown; - has_audio?: boolean; -}; +const args = toolInvocation.args as DocumentParsingArgs;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/conversations/src/features/chat/components/ToolInvocationItem.tsx` around lines 48 - 53, Extract the inline type assertion used for toolInvocation.args into a named interface (e.g., ToolInvocationArgs) and use it in the ToolInvocationItem component: replace the inline cast "const args = toolInvocation.args as { documents: unknown; has_audio?: boolean }" with a typed declaration using the new interface, then keep the existing uses of documents and hasAudio (args?.documents and args?.has_audio) but update hasAudio to read from args.has_audio if present; this makes the shape reusable and clearer across the component and other modules.src/backend/chat/tests/views/test_transcription_webhook.py (1)
100-171: Good coverage for error and edge cases.Tests for unknown job_id (404), invalid auth (403), missing auth (403), and failure payload (200 with TRANSCRIPTION_FAILED state) provide solid coverage of error paths.
Consider adding a test for the
pendingstatus payload to ensure completeness.➕ Additional test case for pending status
def test_transcription_webhook_pending_payload(client, transcribing_attachment): """Pending webhook payload is acknowledged but state unchanged.""" payload = { "job_id": "job-abc-123", "type": "transcript", "status": "pending", } url = reverse("transcription-webhook") response = client.post( url, data=json.dumps(payload), content_type="application/json", **AUTH_HEADER, ) assert response.status_code == 200 assert response.json() == {"status": "ignored"} transcribing_attachment.refresh_from_db() assert transcribing_attachment.upload_state == AttachmentStatus.TRANSCRIBING🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/chat/tests/views/test_transcription_webhook.py` around lines 100 - 171, Add a new test function named test_transcription_webhook_pending_payload that posts a payload with "job_id": "job-abc-123", "type": "transcript", "status": "pending" to the "transcription-webhook" URL using the same AUTH_HEADER and transcribing_attachment fixture; assert response.status_code == 200, response.json() == {"status": "ignored"}, then refresh transcribing_attachment and assert its upload_state remains AttachmentStatus.TRANSCRIBING so the pending webhook is acknowledged without changing state.src/backend/chat/clients/pydantic_ai.py (1)
547-651: Consider extracting audio handling to reduce cognitive complexity.SonarCloud flagged cognitive complexity of 24 (limit: 15). The audio-specific logic (lines 606-623) could be extracted to a helper method to improve readability and testability.
♻️ Proposed extraction
+ async def _handle_audio_document( + self, document: DocumentUrl, document_store, key: str + ) -> None: + """Handle audio document by waiting for transcript and storing it.""" + parsed_content = await wait_for_transcript(key, self.conversation) + await document_store.astore_document( + name=document.identifier, content=parsed_content + ) + self._audio_document_names.append(document.identifier) async def _parse_input_documents(self, documents: List[BinaryContent | DocumentUrl]): # ... validation code ... for document in documents: key = None if isinstance(document, DocumentUrl): if document.url.startswith(DOCUMENT_URL_PREFIX): key = document.url[len(DOCUMENT_URL_PREFIX) :] if not key.startswith(f"{self.conversation.pk}/"): raise ValueError("Document URL does not belong to the conversation.") if is_audio_content_type(document.media_type): - parsed_content = await wait_for_transcript(key, self.conversation) - await document_store.astore_document( - name=document.identifier, content=parsed_content - ) - self._audio_document_names.append(document.identifier) + await self._handle_audio_document(document, document_store, key) + continue else: # ... existing non-audio logic ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/chat/clients/pydantic_ai.py` around lines 547 - 651, Extract the audio-specific branch inside _parse_input_documents into a new helper async method (e.g., _handle_audio_document) to reduce cognitive complexity: move the logic that checks is_audio_content_type, calls wait_for_transcript(key, self.conversation), stores the transcript via document_store.astore_document, and appends to self._audio_document_names into that helper; have _parse_input_documents call await self._handle_audio_document(document, key, document_store) where the original block was, keeping the same inputs/side-effects (use same document.identifier, key, self.user, and document_store) and ensure the helper is asynchronous and returns the parsed_content used later for markdown attachment creation.src/backend/chat/tests/clients/pydantic_ai/test_parse_audio_documents.py (1)
70-87: Consider using pytest's async test support for cleaner async tests.The manual
asyncio.run(run())pattern works but could be simplified withpytest-asyncio.♻️ Alternative using pytest-asyncio
`@pytest.mark.asyncio` async def test_audio_document_uses_transcript_not_parse_document( conversation, audio_attachment, transcript_attachment ): """Audio DocumentUrl uses transcript from S3, not parse_document().""" mock_store = mock.MagicMock() mock_store.collection_id = "test-collection" mock_store.astore_document = mock.AsyncMock(return_value=None) # ... rest of setup ... svc = AIAgentService.__new__(AIAgentService) # ... configure svc ... with mock.patch(...): await svc._parse_input_documents([document]) # assertions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/chat/tests/clients/pydantic_ai/test_parse_audio_documents.py` around lines 70 - 87, Replace the manual asyncio.run pattern with pytest-asyncio by converting the test into an async test function (decorate with `@pytest.mark.asyncio`) and await svc._parse_input_documents directly; create async-friendly mocks (use mock.AsyncMock for mock_store.astore_document and any async behaviors), keep the same setup for svc (AIAgentService.__new__(AIAgentService), svc.conversation, svc.user, svc._is_document_upload_enabled, svc._audio_document_names) and the same mock.patch calls for chat.clients.pydantic_ai.document_store_backend and default_storage.open, then run assertions as before against the mocked objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/chat/views.py`:
- Around line 792-798: The current try/except in the transcript fetch (using
transcript_response, payload.transcription_data_url, and WhisperXResponse)
returns Response({"error": "Failed to fetch transcript"},
status=status.HTTP_200_OK) which suppresses retries; change the returned status
to a 5xx (e.g., status.HTTP_502_BAD_GATEWAY or
status.HTTP_503_SERVICE_UNAVAILABLE) so webhook delivery systems will retry on
transient failures, or if you intentionally want no retries add an inline
comment above the except explaining that returning 200 is deliberate and why;
ensure you update the Response call where logger.exception("Failed to fetch or
parse transcript for job %s", job_id) is used.
- Line 793: The transcript download uses
http_requests.get(payload.transcription_data_url, timeout=30); update the
timeout value to 60 to improve robustness for larger or slower transcription
downloads—locate the call that assigns transcript_response and change the
timeout argument from 30 to 60 so the request uses a 60-second timeout.
---
Nitpick comments:
In `@src/backend/chat/clients/pydantic_ai.py`:
- Around line 547-651: Extract the audio-specific branch inside
_parse_input_documents into a new helper async method (e.g.,
_handle_audio_document) to reduce cognitive complexity: move the logic that
checks is_audio_content_type, calls wait_for_transcript(key, self.conversation),
stores the transcript via document_store.astore_document, and appends to
self._audio_document_names into that helper; have _parse_input_documents call
await self._handle_audio_document(document, key, document_store) where the
original block was, keeping the same inputs/side-effects (use same
document.identifier, key, self.user, and document_store) and ensure the helper
is asynchronous and returns the parsed_content used later for markdown
attachment creation.
In `@src/backend/chat/tests/clients/pydantic_ai/test_parse_audio_documents.py`:
- Around line 70-87: Replace the manual asyncio.run pattern with pytest-asyncio
by converting the test into an async test function (decorate with
`@pytest.mark.asyncio`) and await svc._parse_input_documents directly; create
async-friendly mocks (use mock.AsyncMock for mock_store.astore_document and any
async behaviors), keep the same setup for svc
(AIAgentService.__new__(AIAgentService), svc.conversation, svc.user,
svc._is_document_upload_enabled, svc._audio_document_names) and the same
mock.patch calls for chat.clients.pydantic_ai.document_store_backend and
default_storage.open, then run assertions as before against the mocked objects.
In `@src/backend/chat/tests/views/test_transcription_webhook.py`:
- Around line 100-171: Add a new test function named
test_transcription_webhook_pending_payload that posts a payload with "job_id":
"job-abc-123", "type": "transcript", "status": "pending" to the
"transcription-webhook" URL using the same AUTH_HEADER and
transcribing_attachment fixture; assert response.status_code == 200,
response.json() == {"status": "ignored"}, then refresh transcribing_attachment
and assert its upload_state remains AttachmentStatus.TRANSCRIBING so the pending
webhook is acknowledged without changing state.
In
`@src/frontend/apps/conversations/src/features/chat/components/ToolInvocationItem.tsx`:
- Around line 48-53: Extract the inline type assertion used for
toolInvocation.args into a named interface (e.g., ToolInvocationArgs) and use it
in the ToolInvocationItem component: replace the inline cast "const args =
toolInvocation.args as { documents: unknown; has_audio?: boolean }" with a typed
declaration using the new interface, then keep the existing uses of documents
and hasAudio (args?.documents and args?.has_audio) but update hasAudio to read
from args.has_audio if present; this makes the shape reusable and clearer across
the component and other modules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 725b5c79-77a1-4997-85ff-d45b367d515c
📒 Files selected for processing (26)
Makefiledocs/tilt.mdenv.d/development/common.distenv.d/development/kube-secret.distsrc/backend/chat/authentication.pysrc/backend/chat/clients/pydantic_ai.pysrc/backend/chat/malware_detection.pysrc/backend/chat/migrations/0007_chatconversationattachment_transcription_job_id.pysrc/backend/chat/models.pysrc/backend/chat/tests/clients/pydantic_ai/test_parse_audio_documents.pysrc/backend/chat/tests/test_malware_detection.pysrc/backend/chat/tests/test_transcription.pysrc/backend/chat/tests/views/chat/conversations/test_conversation_with_audio_upload.pysrc/backend/chat/tests/views/chat/conversations/test_conversation_with_document_upload.pysrc/backend/chat/tests/views/chat/conversations/test_conversation_with_document_url.pysrc/backend/chat/tests/views/test_transcription_webhook.pysrc/backend/chat/transcription.pysrc/backend/chat/views.pysrc/backend/chat/webhook_models.pysrc/backend/conversations/settings.pysrc/backend/core/file_upload/enums.pysrc/backend/core/file_upload/utils.pysrc/backend/core/urls.pysrc/frontend/apps/conversations/src/features/chat/components/MessageItem.tsxsrc/frontend/apps/conversations/src/features/chat/components/ToolInvocationItem.tsxsrc/helm/env.d/dev/values.conversations.yaml.gotmpl
✅ Files skipped from review due to trivial changes (7)
- src/backend/core/file_upload/enums.py
- src/backend/core/file_upload/utils.py
- env.d/development/common.dist
- env.d/development/kube-secret.dist
- src/backend/chat/tests/views/chat/conversations/test_conversation_with_document_upload.py
- docs/tilt.md
- src/backend/core/urls.py
🚧 Files skipped from review as they are similar to previous changes (11)
- src/backend/chat/models.py
- src/backend/chat/malware_detection.py
- src/backend/chat/tests/views/chat/conversations/test_conversation_with_document_url.py
- src/backend/chat/authentication.py
- src/backend/chat/transcription.py
- src/frontend/apps/conversations/src/features/chat/components/MessageItem.tsx
- src/backend/chat/tests/test_transcription.py
- src/backend/conversations/settings.py
- src/backend/chat/tests/views/chat/conversations/test_conversation_with_audio_upload.py
- src/backend/chat/migrations/0007_chatconversationattachment_transcription_job_id.py
- src/backend/chat/tests/test_malware_detection.py
Comment on lines
+792
to
+798
| try: | ||
| transcript_response = http_requests.get(payload.transcription_data_url, timeout=30) | ||
| transcript_response.raise_for_status() | ||
| whisper_data = WhisperXResponse.model_validate_json(transcript_response.content) | ||
| except Exception: # pylint: disable=broad-except | ||
| logger.exception("Failed to fetch or parse transcript for job %s", job_id) | ||
| return Response({"error": "Failed to fetch transcript"}, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
Consider returning an error status code when transcript fetch fails.
Returning HTTP 200 with {"error": "Failed to fetch transcript"} may cause the AI service to consider the webhook delivery successful and not retry. If retries are desired on transient failures, consider returning 5xx.
🛡️ Suggested change for retry behavior
except Exception: # pylint: disable=broad-except
logger.exception("Failed to fetch or parse transcript for job %s", job_id)
- return Response({"error": "Failed to fetch transcript"}, status=status.HTTP_200_OK)
+ return Response(
+ {"error": "Failed to fetch transcript"},
+ status=status.HTTP_502_BAD_GATEWAY
+ )However, if the current behavior is intentional (e.g., to prevent infinite retries on permanently broken transcript URLs), please add a comment explaining the choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/chat/views.py` around lines 792 - 798, The current try/except in
the transcript fetch (using transcript_response, payload.transcription_data_url,
and WhisperXResponse) returns Response({"error": "Failed to fetch transcript"},
status=status.HTTP_200_OK) which suppresses retries; change the returned status
to a 5xx (e.g., status.HTTP_502_BAD_GATEWAY or
status.HTTP_503_SERVICE_UNAVAILABLE) so webhook delivery systems will retry on
transient failures, or if you intentionally want no retries add an inline
comment above the except explaining that returning 200 is deliberate and why;
ensure you update the Response call where logger.exception("Failed to fetch or
parse transcript for job %s", job_id) is used.
| return Response({"status": "ignored"}) | ||
|
|
||
| try: | ||
| transcript_response = http_requests.get(payload.transcription_data_url, timeout=30) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is a reasonable HTTP timeout for downloading JSON transcription data typically under 10MB?
💡 Result:
A reasonable HTTP timeout for downloading JSON transcription data typically under 10MB is 30-60 seconds. This accounts for connection establishment (3-10s), server processing, and data transfer even on slower networks (e.g., 1Mbps takes ~80s for 10MB worst-case, but typical broadband is much faster: <2s as seen in tests). Use separate connect (3-10s) and read timeouts (30-60s total operation). Examples from sources: - Python requests: (3,30) or 60s total for APIs/large files {5,10,19,20,22} - Curl: --max-time 60 for file downloads {1,29} - General: 60s for STT-related, 300s conservative for larger {6,10} For production, combine with retries and streaming (stream=True) to handle chunks without memory issues.
Citations:
- 1: https://curl-to.com/problems/request-timeout/
- 2: https://stackoverflow.com/questions/78915719/how-do-i-download-a-json-file-with-50k-lines-faster
- 3: https://iproyal.com/blog/python-requests-timeout/
- 4: https://dev.to/nareshipme/building-a-robust-real-time-transcription-pipeline-in-nextjs-stt-streaming-and-error-recovery-2io5
- 5: https://iifx.dev/en/articles/460223371/python-requests-streaming-large-files-to-prevent-memory-errors-and-timeouts
- 6: https://stackoverflow.com/questions/70612906/python-requests-post-timeout-specification-with-large-files
- 7: https://docs.python-requests.org/en/master/user/quickstart/%23timeouts
- 8: https://python-requests.org/python-requests-timeout/
- 9: https://everything.curl.dev/usingcurl/timeouts.html
Consider increasing timeout to 60 seconds for transcript downloads.
The current 30-second timeout is on the lower acceptable bound for downloading transcription data. Industry recommendations suggest 60 seconds for transcription-related operations, which provides safer margins for larger files or slower network conditions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/chat/views.py` at line 793, The transcript download uses
http_requests.get(payload.transcription_data_url, timeout=30); update the
timeout value to 60 to improve robustness for larger or slower transcription
downloads—locate the call that assigns transcript_response and change the
timeout argument from 30 to 60 so the request uses a 60-second timeout.
Refactor backend environment variables to use secret references Update LLM configuration file path
Create connection to meet/summary service and webhook to receive transcript Store transcript in RAG
maxenceh
force-pushed
the
maxenceh/audio-transcription
branch
from
4f85c1a to
7772256
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
CHANGELOG.md (1)
43-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTilt entry is still under the wrong section.
This entry describes a new feature addition, not a fix — it belongs under
### Added.✍️ Proposed fix
### Fixed - 💬(onboarding) fix wording step 1 img - 🐛(fix) new conversation in project button max size - 💄(ui) little fix margin top - 💄(ui) review ui for part of the project - 🐛(fix) Fix streaming crash with OpenAI-compatible APIs - 🐛(fix) strip thinking part for models without reasoning support - -- ✨(dev) setup Tilt for local development +### Added + +- ✨(dev) setup Tilt for local development🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` at line 43, The changelog entry "✨(dev) setup Tilt for local development" is under the wrong section; move this line from its current location into the "### Added" section of the changelog so it appears with other new-feature entries, keeping the same formatting and any surrounding blank lines consistent with existing entries under "### Added".bin/Tiltfile (1)
19-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
triggerpath still resolves incorrectly relative tobin/Tiltfile's directory.A trigger is a filepath, either absolute or relative to the Tiltfile. Since this Tiltfile lives in
bin/, the path./src/backend/requirements.txtresolves tobin/src/backend/requirements.txtwhich does not exist. Thesyncstep already correctly uses../src/backend, so the trigger should match.🐛 Proposed fix
- trigger=['./src/backend/requirements.txt'] + trigger=['../src/backend/requirements.txt']🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/Tiltfile` around lines 19 - 22, The trigger path in the run(...) call is relative to the Tiltfile in bin/ and incorrectly points to ./src/backend/requirements.txt; update the trigger argument in the run(...) invocation to use the correct relative path ../src/backend/requirements.txt so it matches the sync step and points to the actual requirements file.src/helm/extra/templates/minio.yaml (1)
85-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMinIO data is still backed by
emptyDir— all stored objects are lost on pod restart.Every pod restart or reschedule wipes the bucket contents while database attachment rows retain their references, which will cause broken attachments and flaky transcription behavior in dev after any pod cycle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helm/extra/templates/minio.yaml` around lines 85 - 90, The MinIO pod uses an ephemeral emptyDir volume (volume name "data" mounted at mountPath "/data") which loses objects on restart; replace the emptyDir volume with a PersistentVolumeClaim-backed volume by changing the volumes entry for name: data to use persistentVolumeClaim: { claimName: <your-claim-name> } and ensure the corresponding PVC is created (either add a PVC template in the chart or reference an existing claim), keeping the volumeMounts name "data" and mountPath "/data" unchanged so MinIO uses persistent storage across pod restarts.
🧹 Nitpick comments (2)
src/backend/chat/tests/views/test_transcription_webhook.py (1)
53-98: ⚡ Quick winAdd a retry/idempotency case for the success webhook.
This proves the first
"success"delivery, but not the normal retry path. Since webhook senders routinely redeliver, it would be worth asserting that a second callback reuses the existing transcript attachment instead of creating duplicates or reprocessing the transcript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/chat/tests/views/test_transcription_webhook.py` around lines 53 - 98, The test only covers the initial "success" webhook delivery; add a second POST to exercise the retry/idempotency path and assert no duplicate transcription processing occurs. Specifically, in test_transcription_webhook_success (and using the same payload, mock.patch on default_storage.save as currently done), send the same client.post a second time and then assert that models.ChatConversationAttachment.objects.filter(conversion_from=transcribing_attachment.key).count() remains 1 (no duplicate), that mock_save was not called again (or total calls stayed at 1), and that the original text_attachment still has upload_state AttachmentStatus.READY; this verifies the webhook handler reuses the existing transcript instead of reprocessing. Ensure you reuse the same payload/job_id and auth header and call transcribing_attachment.refresh_from_db() between posts if needed.src/helm/extra/templates/keycloak-sts.yaml (1)
69-81: ⚡ Quick winAdd readiness and liveness probes for Keycloak.
Without probes, Kubernetes considers the pod ready the moment the container process starts. Keycloak in
start-devmode takes significantly longer to initialize, so Tilt will mark the resource ready — and potentially trigger downstream auth flows — well before Keycloak can actually serve requests.♻️ Suggested probes
ports: - containerPort: 8080 name: tcp-keycloak + readinessProbe: + httpGet: + path: /realms/master + port: 8080 + initialDelaySeconds: 30 + periodSeconds: 10 + failureThreshold: 6 + livenessProbe: + httpGet: + path: /realms/master + port: 8080 + initialDelaySeconds: 60 + periodSeconds: 15 + failureThreshold: 6 env:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helm/extra/templates/keycloak-sts.yaml` around lines 69 - 81, The Keycloak container (name: keycloak, args include start-dev) needs Kubernetes readinessProbe and livenessProbe so the pod isn't marked ready before Keycloak fully serves requests; add a readinessProbe (HTTP GET against the Keycloak HTTP port 8080 / container port name tcp-keycloak) that queries a lightweight Keycloak endpoint (for example /realms/master or the Keycloak health/ready endpoint) with an initialDelaySeconds ~30, periodSeconds ~10 and reasonable failureThreshold (e.g. 3–6), and add a livenessProbe (HTTP GET against the same host/port, e.g. /health or /realms/master) with a longer initialDelaySeconds ~60 and periodSeconds ~20/failureThreshold ~3–6; attach both probes to the keycloak container spec in keycloak-sts.yaml so readiness reflects actual request-serving state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/chat/clients/pydantic_ai.py`:
- Around line 658-660: The synchronous storage read using
default_storage.open(...).read() must be moved off the event loop to avoid
blocking; inside the async method containing this code, wrap the file open/read
in an executor (e.g., asyncio.to_thread or loop.run_in_executor) and assign the
result to document_data rather than calling file.read() directly; update both
occurrences around default_storage.open and any subsequent reads (the block that
sets document_data and the following 662-668 region) to use the same
to_thread/run_in_executor pattern so remote backends like S3 perform IO on a
worker thread instead of blocking the event loop.
- Around line 651-653: Inside the audio-handling branch where you call
parsed_content = await wait_for_transcript(key, self.conversation) (inside the
is_audio_content_type check before document_store.astore_document), ensure you
honor stop_streaming(): don't await wait_for_transcript directly — instead wait
in a way that can be cancelled by the conversation stop signal (e.g., race
wait_for_transcript against self.conversation.stop_streaming() or check the
cache cancel token periodically) and if stop_streaming() fires return/abort
early rather than proceeding to document_store.astore_document; in short, change
the wait so stop_streaming() can cancel the pending STT and skip storing the
transcript when cancelled.
---
Duplicate comments:
In `@bin/Tiltfile`:
- Around line 19-22: The trigger path in the run(...) call is relative to the
Tiltfile in bin/ and incorrectly points to ./src/backend/requirements.txt;
update the trigger argument in the run(...) invocation to use the correct
relative path ../src/backend/requirements.txt so it matches the sync step and
points to the actual requirements file.
In `@CHANGELOG.md`:
- Line 43: The changelog entry "✨(dev) setup Tilt for local development" is
under the wrong section; move this line from its current location into the "###
Added" section of the changelog so it appears with other new-feature entries,
keeping the same formatting and any surrounding blank lines consistent with
existing entries under "### Added".
In `@src/helm/extra/templates/minio.yaml`:
- Around line 85-90: The MinIO pod uses an ephemeral emptyDir volume (volume
name "data" mounted at mountPath "/data") which loses objects on restart;
replace the emptyDir volume with a PersistentVolumeClaim-backed volume by
changing the volumes entry for name: data to use persistentVolumeClaim: {
claimName: <your-claim-name> } and ensure the corresponding PVC is created
(either add a PVC template in the chart or reference an existing claim), keeping
the volumeMounts name "data" and mountPath "/data" unchanged so MinIO uses
persistent storage across pod restarts.
---
Nitpick comments:
In `@src/backend/chat/tests/views/test_transcription_webhook.py`:
- Around line 53-98: The test only covers the initial "success" webhook
delivery; add a second POST to exercise the retry/idempotency path and assert no
duplicate transcription processing occurs. Specifically, in
test_transcription_webhook_success (and using the same payload, mock.patch on
default_storage.save as currently done), send the same client.post a second time
and then assert that
models.ChatConversationAttachment.objects.filter(conversion_from=transcribing_attachment.key).count()
remains 1 (no duplicate), that mock_save was not called again (or total calls
stayed at 1), and that the original text_attachment still has upload_state
AttachmentStatus.READY; this verifies the webhook handler reuses the existing
transcript instead of reprocessing. Ensure you reuse the same payload/job_id and
auth header and call transcribing_attachment.refresh_from_db() between posts if
needed.
In `@src/helm/extra/templates/keycloak-sts.yaml`:
- Around line 69-81: The Keycloak container (name: keycloak, args include
start-dev) needs Kubernetes readinessProbe and livenessProbe so the pod isn't
marked ready before Keycloak fully serves requests; add a readinessProbe (HTTP
GET against the Keycloak HTTP port 8080 / container port name tcp-keycloak) that
queries a lightweight Keycloak endpoint (for example /realms/master or the
Keycloak health/ready endpoint) with an initialDelaySeconds ~30, periodSeconds
~10 and reasonable failureThreshold (e.g. 3–6), and add a livenessProbe (HTTP
GET against the same host/port, e.g. /health or /realms/master) with a longer
initialDelaySeconds ~60 and periodSeconds ~20/failureThreshold ~3–6; attach both
probes to the keycloak container spec in keycloak-sts.yaml so readiness reflects
actual request-serving state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 190996b5-e647-4c8e-af1c-e055fac9a24e
📒 Files selected for processing (38)
.dockerignoreCHANGELOG.mdMakefileREADME.mdbin/Tiltfiledocs/examples/conversations.values.yamldocs/tilt.mdenv.d/development/common.distenv.d/development/kube-secret.distsrc/backend/chat/authentication.pysrc/backend/chat/clients/pydantic_ai.pysrc/backend/chat/malware_detection.pysrc/backend/chat/migrations/0007_chatconversationattachment_transcription_job_id.pysrc/backend/chat/models.pysrc/backend/chat/tests/clients/pydantic_ai/test_parse_audio_documents.pysrc/backend/chat/tests/test_malware_detection.pysrc/backend/chat/tests/test_transcription.pysrc/backend/chat/tests/views/chat/conversations/test_conversation_with_audio_upload.pysrc/backend/chat/tests/views/chat/conversations/test_conversation_with_document_upload.pysrc/backend/chat/tests/views/chat/conversations/test_conversation_with_document_url.pysrc/backend/chat/tests/views/test_transcription_webhook.pysrc/backend/chat/transcription.pysrc/backend/chat/views.pysrc/backend/chat/webhook_models.pysrc/backend/conversations/settings.pysrc/backend/core/file_upload/enums.pysrc/backend/core/file_upload/utils.pysrc/backend/core/urls.pysrc/frontend/apps/conversations/src/features/chat/components/MessageItem.tsxsrc/frontend/apps/conversations/src/features/chat/components/ToolInvocationItem.tsxsrc/helm/env.d/dev-staging/values.conversations.yaml.gotmplsrc/helm/env.d/dev/values.conversations.yaml.gotmplsrc/helm/extra/Chart.yamlsrc/helm/extra/templates/kc-postgresql-sts.yamlsrc/helm/extra/templates/keycloak-sts.yamlsrc/helm/extra/templates/minio.yamlsrc/helm/extra/templates/postgresql-sts.yamlsrc/helm/helmfile.yaml.gotmpl
✅ Files skipped from review due to trivial changes (9)
- README.md
- src/helm/extra/Chart.yaml
- docs/examples/conversations.values.yaml
- env.d/development/kube-secret.dist
- src/backend/chat/migrations/0007_chatconversationattachment_transcription_job_id.py
- src/backend/chat/authentication.py
- .dockerignore
- src/backend/chat/models.py
- src/backend/chat/webhook_models.py
🚧 Files skipped from review as they are similar to previous changes (17)
- env.d/development/common.dist
- src/backend/core/file_upload/utils.py
- src/frontend/apps/conversations/src/features/chat/components/MessageItem.tsx
- src/backend/core/urls.py
- src/backend/chat/tests/views/chat/conversations/test_conversation_with_document_upload.py
- src/frontend/apps/conversations/src/features/chat/components/ToolInvocationItem.tsx
- src/backend/conversations/settings.py
- src/backend/chat/tests/test_malware_detection.py
- docs/tilt.md
- src/backend/chat/malware_detection.py
- src/helm/helmfile.yaml.gotmpl
- src/backend/chat/transcription.py
- src/backend/chat/views.py
- src/backend/chat/tests/views/chat/conversations/test_conversation_with_document_url.py
- src/helm/env.d/dev/values.conversations.yaml.gotmpl
- src/backend/chat/tests/views/chat/conversations/test_conversation_with_audio_upload.py
- src/backend/chat/tests/test_transcription.py
Comment on lines
+651
to
+653
| if is_audio_content_type(document.media_type): | ||
| parsed_content = await wait_for_transcript(key, self.conversation) | ||
| await document_store.astore_document( |
There was a problem hiding this comment.
Honor stop_streaming() while waiting for the transcript.
Once execution enters wait_for_transcript(), the cache-based cancel check is not reached again. If the user stops the response while STT is still pending, this request will keep hanging until the helper returns or times out.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/chat/clients/pydantic_ai.py` around lines 651 - 653, Inside the
audio-handling branch where you call parsed_content = await
wait_for_transcript(key, self.conversation) (inside the is_audio_content_type
check before document_store.astore_document), ensure you honor stop_streaming():
don't await wait_for_transcript directly — instead wait in a way that can be
cancelled by the conversation stop signal (e.g., race wait_for_transcript
against self.conversation.stop_streaming() or check the cache cancel token
periodically) and if stop_streaming() fires return/abort early rather than
proceeding to document_store.astore_document; in short, change the wait so
stop_streaming() can cancel the pending STT and skip storing the transcript when
cancelled.
Comment on lines
+658
to
+660
| # Retrieve the document data | ||
| with default_storage.open(key, "rb") as file: | ||
| document_data = file.read() |
There was a problem hiding this comment.
Move the storage read off the event loop.
default_storage.open(...).read() is still synchronous I/O inside this async method. With S3 or any remote storage backend, that blocks the worker for the whole download before parsing starts.
Possible fix
else:
- # Retrieve the document data
- with default_storage.open(key, "rb") as file:
- document_data = file.read()
+ # Retrieve the document data without blocking the event loop
+ def _read_document_data():
+ with default_storage.open(key, "rb") as file:
+ return file.read()
+
+ document_data = await asyncio.to_thread(_read_document_data)
# Run in thread to avoid blocking the event loop during parsing
parsed_content = await asyncio.to_thread(
document_store.parse_and_store_document,
name=document.identifier,
content_type=document.media_type,Also applies to: 662-668
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/chat/clients/pydantic_ai.py` around lines 658 - 660, The
synchronous storage read using default_storage.open(...).read() must be moved
off the event loop to avoid blocking; inside the async method containing this
code, wrap the file open/read in an executor (e.g., asyncio.to_thread or
loop.run_in_executor) and assign the result to document_data rather than calling
file.read() directly; update both occurrences around default_storage.open and
any subsequent reads (the block that sets document_data and the following
662-668 region) to use the same to_thread/run_in_executor pattern so remote
backends like S3 perform IO on a worker thread instead of blocking the event
loop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
demo-transcript.mov
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores
Tests