Add Community Snapshot Videos Command#2948
Conversation
github-actions
bot
added
backend
docker
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds an end-to-end OWASP community snapshot video pipeline: slide/video engine, ElevenLabs audio integration, Jinja slide templates, a Django management command, video-specific Dockerfile and build targets, consolidated Jinja loader, tests, dependency additions, and .gitignore updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-10-26T12:50:50.512ZApplied to files:
📚 Learning: 2025-12-26T06:09:08.868ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
backend/docker/Dockerfile.local (1)
47-53: Update the Alpine version reference in the comment.Line 47 references Alpine 3.17, but the base image
python:3.13.7-alpinelikely uses a much newer Alpine version (3.20 or 3.21). While the WeasyPrint dependencies should still be correct, the comment may be misleading.Apply this diff to make the comment version-agnostic:
-# WeasyPrint dependencies: https://doc.courtbouillon.org/weasyprint/stable/first_steps.html#alpine-3-17 +# WeasyPrint dependencies: https://doc.courtbouillon.org/weasyprint/stable/first_steps.html RUN --mount=type=cache,target=${APK_CACHE_DIR} \Alternatively, verify the actual Alpine version and update the comment accordingly.
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (2)
40-40: Consider using logger instead of print statements.While
print()statements are acceptable in Django management commands (and explicitly allowed by ruff configuration line 99), using the module-levelloggerwould provide better control over log levels and formatting for production use.Example refactor:
- print(f"Rendering slide image: {self.name}") + logger.info(f"Rendering slide image: {self.name}")Apply similar changes to all print statements. However, if you prefer the current approach for command output, it's acceptable to keep the print statements.
Also applies to: 54-54, 58-58, 69-69, 92-92, 95-95
71-74: Reminder: Implement audio generation.The TODO indicates audio generation is not yet implemented. The hardcoded test audio path will need to be replaced with actual audio generation logic (likely using OpenAI TTS API).
Do you want me to help generate a skeleton implementation for audio generation using OpenAI's text-to-speech API, or open a separate issue to track this task?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
backend/apps/owasp/Makefile(1 hunks)backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py(1 hunks)backend/apps/owasp/templates/video/base.html(1 hunks)backend/apps/owasp/templates/video/slides/intro.html(1 hunks)backend/docker/Dockerfile.local(1 hunks)backend/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (2)
backend/apps/common/open_ai.py (4)
OpenAi(13-105)set_prompt(64-76)set_input(36-48)complete(78-105)backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)
🪛 GitHub Actions: Run CI/CD
backend/docker/Dockerfile.local
[error] 51-51: Unknown word (libgobject) detected by spell checker.
[error] 51-51: Unknown word (libpango) detected by spell checker.
[error] 51-51: Unknown word (libharfbuzz) detected by spell checker.
[error] 52-52: Unknown word (libharfbuzz) detected by spell checker.
[error] 52-52: Unknown word (libpangoft) detected by spell checker.
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
[error] 10-10: Unknown word (pdfium) detected by spell checker.
[error] 44-44: Unknown word (pdfium) detected by spell checker.
[error] 47-47: Unknown word (pil) detected by spell checker.
[error] 50-50: Unknown word (pil) detected by spell checker.
[error] 73-73: Unknown word (rudransh) detected by spell checker.
[error] 73-73: Unknown word (shrivastava) detected by spell checker.
[error] 86-86: Unknown word (acodec) detected by spell checker.
[error] 87-87: Unknown word (vcodec) detected by spell checker.
[error] 87-87: Unknown word (libx) detected by spell checker.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
backend/apps/owasp/Makefile (1)
25-27: LGTM!The new Makefile target follows the existing pattern and is correctly structured. The
$(snapshot_key)variable will need to be provided by the caller, consistent with other targets likeowasp-create-project-metadata-file.backend/apps/owasp/templates/video/slides/intro.html (1)
1-7: LGTM!The intro slide template correctly extends the base template and uses the context variables provided by
SlideBuilder.create_intro_slide(). The structure is clean and appropriate for the video generation workflow.backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (3)
98-119: LGTM!The
SlideBuilderclass is well-structured and correctly creates slide instances with appropriate context and paths. The date formatting and file naming conventions are clear and consistent.
122-138: LGTM!The
Generatorclass provides a clean abstraction for managing slides and orchestrating the video generation pipeline. The sequential processing ingenerate_video()is appropriate for the PoC stage.
164-185: LGTM with a note on query pattern.The command handler correctly validates input, queries for a completed snapshot, and orchestrates the video generation workflow. The use of
filter().first()with case-insensitive matching is safer thanget()for handling user input, as it gracefully returnsNonerather than raisingDoesNotExist.
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (1)
57-74: Handle transcript generation failures more robustly.The method catches exceptions but doesn't re-raise them, and it doesn't handle the case where
open_ai.complete()returnsNone(as documented in the OpenAi class). If transcript generation fails,self.transcriptmight not be set, causing downstream failures when it's accessed.Apply this diff to handle failures properly:
def generate_transcript(self, open_ai: OpenAi): """Generate a transcript.""" logger.info("Prompt: %s", self.input) prompt = """ You are a scriptwriter for a tech presentation. Your task is to generate a script for a presentation slide. The script should be simple and direct. Do not add any extra words, introduction or conclusion. """ open_ai.set_prompt(prompt) open_ai.set_input(self.input) try: self.transcript = open_ai.complete() + if self.transcript is None: + logger.error("Failed to generate transcript for %s", self.name) + raise RuntimeError(f"OpenAI API failed to generate transcript for {self.name}") logger.info("Generated slide transcript: %s", self.transcript) except Exception: logger.exception("Error generating transcript for %s:", self.name) + raise
🧹 Nitpick comments (4)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (3)
75-78: Placeholder audio generation needs implementation.The hardcoded audio path will cause all slides to share the same audio file, leading to incorrect video assembly when multiple slides are processed.
Do you want me to generate a skeleton implementation that creates unique audio files per slide, or would you prefer to open an issue to track this TODO?
104-107: Add docstring for consistency.The
__init__method is missing a docstring, while other methods have them.Apply this diff to add the docstring:
def __init__(self, snapshot, output_dir): - """Initialize SlideBuilder.""" + """Initialize SlideBuilder. + + Args: + snapshot: The snapshot object containing data for video generation. + output_dir: Path to the directory where output files will be saved. + + """ self.snapshot = snapshot self.output_dir = output_dir
128-135: Consider adding type hints for better code clarity.The
append_slidemethod and__init__would benefit from parameter type hints matching the rest of the codebase.Apply this diff to improve type annotations:
- def __init__(self): - """Initialize Video Generator.""" + def __init__(self) -> None: + """Initialize Video Generator. + + Creates an empty slide list and initializes the OpenAI client + for transcript generation. + + """ self.slides: list[Slide] = [] self.open_ai = OpenAi() - def append_slide(self, slide: Slide): - """Append a slide to list.""" + def append_slide(self, slide: Slide) -> None: + """Append a slide to list. + + Args: + slide: The Slide instance to add to the generation queue. + + """ self.slides.append(slide)backend/docker/Dockerfile.local (1)
49-53: Alpine package availability confirmed; consider version pinning for reproducible builds.All specified packages are available in Alpine Linux. The WeasyPrint documentation reference is current, and ffmpeg can be installed via
apk add ffmpegon Alpine Linux. Theso:prefix notation for shared libraries is valid Alpine package syntax.For more reproducible builds, consider pinning package versions:
ffmpeg=~6.1(or a specific stable version)ttf-freefont=20120212(or current stable)This prevents unintended package updates between builds.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py(1 hunks)backend/apps/owasp/templates/video/base.html(1 hunks)backend/docker/Dockerfile.local(1 hunks)cspell/custom-dict.txt(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/templates/video/base.html
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (2)
backend/apps/common/open_ai.py (4)
OpenAi(13-105)set_prompt(64-76)set_input(36-48)complete(78-105)backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
cspell/custom-dict.txt (1)
38-38: Alphabetical ordering and relevance confirmed.All nine new dictionary entries are correctly positioned in alphabetical order and directly correspond to the video generation dependencies being introduced (FFmpeg codecs, WeasyPrint rendering libraries, Pillow, and pypdfium2). The additions are appropriate and necessary to prevent false spell-check warnings for these technical identifiers.
Also applies to: 81-85, 110-110, 114-114, 141-141
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (7)
1-18: LGTM!The imports are well-organized and all appear necessary for the video generation workflow.
80-100: LGTM!The video generation logic correctly handles ffmpeg operations with proper error logging and exception propagation. The codec parameters are appropriate for standard video output.
109-124: LGTM!The intro slide creation correctly extracts and formats snapshot data, building a properly configured Slide instance.
137-143: LGTM!The sequential processing of slides (render → transcript → audio → video) is a clear and appropriate workflow for video generation.
169-176: LGTM!Environment variable setup in the command handler is appropriate, and the output directory creation logic is sound.
178-184: Consider more explicit error handling for snapshot lookup.Using
filter().first()with a None check is valid but less explicit than using a try-except withSnapshot.DoesNotExist. The current pattern matches the code snippet fromsnapshot.py, so consistency is maintained.
186-193: LGTM!The video generation workflow correctly initializes the builder and generator, creates the intro slide, and triggers the generation pipeline.
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py(1 hunks)backend/apps/owasp/templates/video/base.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/templates/video/base.html
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T06:36:42.593Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2089
File: backend/apps/nest/models/google_account_authorization.py:61-62
Timestamp: 2025-08-22T06:36:42.593Z
Learning: In the OWASP/Nest project, ruff linting rules discourage using logger.error() immediately before raising exceptions as it creates redundant logging. The preferred approach is to remove the logging call and let the caller handle logging the exception appropriately.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (2)
backend/apps/common/open_ai.py (4)
OpenAi(13-105)set_prompt(64-76)set_input(36-48)complete(78-105)backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (1)
76-81: Add explicit None check for API response.The
try-exceptblock won't catch failures ifopen_ai.complete()returnsNoneinstead of raising an exception. According to theOpenAiclass implementation, thecomplete()method returnsNoneon error.🔎 Add explicit None check:
open_ai.set_prompt(prompt) open_ai.set_input(self.input) - try: - self.transcript = open_ai.complete() - print("Generated slide transcript:", self.transcript) - except Exception: - logger.exception("Error generating transcript for %s", self.name) - raise + self.transcript = open_ai.complete() + if self.transcript is None: + msg = f"Failed to generate transcript for {self.name}" + logger.error(msg) + raise RuntimeError(msg) + + print("Generated slide transcript:", self.transcript)
🧹 Nitpick comments (4)
backend/apps/common/eleven_labs.py (1)
43-55: Validate text parameter in set_text().The method accepts any text value without validation. Empty or
Nonetext will cause API failures later. Consider adding validation to fail fast with a clear error message.🔎 Add parameter validation:
def set_text(self, text: str) -> ElevenLabs: """Set the text to convert to speech. Args: text (str): The text content. Returns: ElevenLabs: The current instance. """ + if not text: + msg = "Text cannot be empty" + raise ValueError(msg) + self.text = text return selfbackend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (3)
42-42: Replace print() with appropriate output methods.Based on learnings, Django management commands should use
self.stdout.write()for user-facing output. However, since these helper classes don't have access to the command's stdout, consider:
- For the
Command.handle()method (line 212): useself.stdout.write()instead ofprint()- For helper classes (
Slide,SlideBuilder,Generator): uselogger.info()for structured loggingBased on learnings, Django management commands should prefer
self.stdout.write()overprint()for user-facing command output.🔎 Example changes:
In the Command class:
- print("Generating video for snapshot:", snapshot_key) + self.stdout.write(f"Generating video for snapshot: {snapshot_key}")In helper classes like Slide:
- print("Rendering slide image") + logger.info("Rendering slide image: %s", self.name)- print("Saved image to path:", self.image_output_path) + logger.info("Saved %s image to path: %s", self.name, self.image_output_path)Also applies to: 54-54, 66-66, 78-78, 99-99, 120-120, 135-135, 212-212
122-124: Remove redundant logging before re-raising exception.Logging immediately before re-raising an exception creates redundant logging. Let the caller handle logging the exception appropriately.
Based on learnings, ruff linting rules discourage logging immediately before raising exceptions.
🔎 Remove the logging call:
except ffmpeg.Error: - logger.exception("Error generating video for %s:", self.name) raise
128-128: Add type hint for snapshot parameter.The
snapshotparameter is missing a type hint. Based on the import at line 18, it should be typed asSnapshot.🔎 Add type hint:
- def __init__(self, snapshot, output_dir: Path) -> None: + def __init__(self, snapshot: Snapshot, output_dir: Path) -> None: """Initialize SlideBuilder."""
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
backend/.env.example(1 hunks)backend/apps/common/eleven_labs.py(1 hunks)backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py(1 hunks)backend/pyproject.toml(1 hunks)backend/settings/base.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/pyproject.toml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T06:36:42.593Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2089
File: backend/apps/nest/models/google_account_authorization.py:61-62
Timestamp: 2025-08-22T06:36:42.593Z
Learning: In the OWASP/Nest project, ruff linting rules discourage using logger.error() immediately before raising exceptions as it creates redundant logging. The preferred approach is to remove the logging call and let the caller handle logging the exception appropriately.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
🪛 GitHub Actions: Run CI/CD
backend/settings/base.py
[error] 219-219: CSpell: Unknown word 'ELEVENLABS'.
[error] 219-219: CSpell: Unknown word 'ELEVENLABS'.
backend/apps/common/eleven_labs.py
[error] 9-9: CSpell: Unknown word 'elevenlabs'.
[error] 22-22: CSpell: Unknown word 'MSU'.
[error] 35-35: CSpell: Unknown word 'ELEVENLABS'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (3)
53-53: Consider replacing print statements with structured logging.Multiple
print()statements throughout the Slide class should use structured logging for consistency. While the learning aboutself.stdout.write()applies to Command class methods, these helper classes don't have access toself.stdout.Based on learnings, consider using
logger.info()for progress messages, which provides better control over log levels and output destinations.🔎 Examples of recommended changes:
- print("Rendering slide image") + logger.info("Rendering slide image: %s", self.name)- print("Saved image to path:", self.image_output_path) + logger.info("Saved %s image to: %s", self.name, self.image_output_path)- print("Using pre-set transcript:", self.transcript) + logger.info("Using pre-set transcript for %s", self.name)- print("Generating transcript from prompt:", self.transcript_input) + logger.info("Generating transcript for %s", self.name)- print("Generated slide transcript:", self.transcript) + logger.info("Generated transcript for %s", self.name)- print("Generated audio at path:", self.audio_output_path) + logger.info("Generated audio for %s at: %s", self.name, self.audio_output_path)- print("Generated video for:", self.name) + logger.info("Generated video for: %s", self.name)Also applies to: 65-65, 83-83, 89-89, 107-107, 125-125, 146-146
154-154: Add missing type hint for snapshot parameter.The
snapshotparameter inSlideBuilder.__init__lacks a type hint. For consistency with the rest of the codebase and to improve IDE support, add theSnapshottype annotation.🔎 Apply this diff:
- def __init__(self, snapshot, output_dir: Path) -> None: + def __init__(self, snapshot: Snapshot, output_dir: Path) -> None: """Initialize SlideBuilder.""" self.snapshot = snapshot self.output_dir = output_dir
161-161: Replace print statements in Command.handle with self.stdout.write.Lines 161 and 238 use
print()for user-facing output in contexts where the Command instance is available.Based on learnings, Django management commands should use
self.stdout.write()instead ofprint()for user-facing output, as it follows Django conventions and improves testability.🔎 Apply these changes:
For line 161, pass the command's stdout writer to SlideBuilder and use it:
# In SlideBuilder.__init__, add a writer parameter def __init__(self, snapshot: Snapshot, output_dir: Path, writer=None) -> None: """Initialize SlideBuilder.""" self.snapshot = snapshot self.output_dir = output_dir self.writer = writer # In create_intro_slide def create_intro_slide(self) -> Slide: """Create an introduction slide.""" if self.writer: self.writer.write("Generating introduction slide for snapshot\n") # ... rest of methodFor line 238 in Command.handle:
- print("Generating video for snapshot:", snapshot_key) + self.stdout.write(f"Generating video for snapshot: {snapshot_key}\n")Then update the Command.handle call to SlideBuilder:
- slide_builder = SlideBuilder(snapshot, output_dir) + slide_builder = SlideBuilder(snapshot, output_dir, self.stdout)Also applies to: 238-238
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T06:36:42.593Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2089
File: backend/apps/nest/models/google_account_authorization.py:61-62
Timestamp: 2025-08-22T06:36:42.593Z
Learning: In the OWASP/Nest project, ruff linting rules discourage using logger.error() immediately before raising exceptions as it creates redundant logging. The preferred approach is to remove the logging call and let the caller handle logging the exception appropriately.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (3)
backend/apps/common/eleven_labs.py (3)
ElevenLabs(17-137)set_text(43-55)generate_to_file(120-137)backend/apps/common/open_ai.py (4)
OpenAi(13-105)set_prompt(64-76)set_input(36-48)complete(78-105)backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/apps/common/eleven_labs.py (1)
168-193: Validate text attribute before API call.The
generate()method accessesself.textwithout verifying it was set viaset_text(). Ifset_text()was never called, this raises anAttributeError.🔎 Add validation:
def generate(self) -> bytes | None: """Generate audio from text. Returns: bytes | None: The audio bytes or None on error. """ + if not hasattr(self, "text") or not self.text: + logger.error("Text not set. Call set_text() before generate().") + return None + try: audio_iterator = self.client.text_to_speech.convert( model_id=self.model_id, output_format=self.output_format, text=self.text, voice_id=self.voice_id, voice_settings={ "similarity_boost": self.similarity_boost, "stability": self.stability, "style": self.style, "use_speaker_boost": self.use_speaker_boost, }, ) return b"".join(audio_iterator) except Exception: logger.exception("An error occurred during ElevenLabs API request.") return Nonebackend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (1)
53-53: Replace print() with self.stdout.write() for command output.Multiple
print()statements are used throughout the command. In Django management commands, useself.stdout.write()for user-facing output to follow Django conventions and improve testability.Based on learnings, Django management commands should prefer
self.stdout.write()for stdout output.🔎 Example changes:
In the
Slidemethods, you'll need to pass the command instance or use a different approach since these are not methods ofCommand. Consider:Option 1: Add logging instead of print for Slide methods:
- print("Rendering slide image") + logger.info("Rendering slide image: %s", self.name)Option 2: Pass stdout writer to Slide methods as a parameter.
For Command.handle():
- print("Generating video for snapshot:", snapshot_key) + self.stdout.write(f"Generating video for snapshot: {snapshot_key}")For SlideBuilder methods, similar approach needed (pass stdout or use logging).
Also applies to: 65-65, 83-83, 89-89, 107-107, 125-125, 146-146, 161-161, 179-179, 293-293
🧹 Nitpick comments (4)
backend/apps/common/eleven_labs.py (1)
43-46: Validate API key before initializing client.The ElevenLabsClient is instantiated with
settings.ELEVENLABS_API_KEYwithout checking if the key exists or is non-empty. If misconfigured, API calls will fail later with unclear errors.🔎 Add validation:
+ api_key = settings.ELEVENLABS_API_KEY + if not api_key: + msg = "ELEVENLABS_API_KEY is not configured in settings" + raise ValueError(msg) + self.client = ElevenLabsClient( - api_key=settings.ELEVENLABS_API_KEY, + api_key=api_key, timeout=30, )backend/apps/owasp/templates/video/slides/projects.html (1)
8-38: Inconsistent project display between layouts.The template switches layouts at 15 projects but has inconsistencies:
- Dense layout (≤15): shows name, leaders, and created_at
- Sparse layout (>15): shows only name, capped at 25 projects via
slice:":25"This means 16-24 projects are shown with less information, and project 26+ are silently omitted without indication to the viewer.
Consider:
- Adding a count indicator when projects are truncated:
"Showing 25 of {{ project_count }} projects"- Making the data shown consistent across both layouts
- Documenting the 15/25 thresholds or extracting them as template context variables
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (2)
238-250: Handle potential ffmpeg errors in video concatenation.The
_combine_videos()method calls ffmpeg operations without try-except blocks. If concatenation fails (e.g., due to incompatible formats or missing files), the exception will propagate uncaught.🔎 Add error handling:
def _combine_videos(self, output_path: Path) -> None: """Combine all slide videos into final video.""" + try: inputs = [ffmpeg.input(str(slide.video_output_path)) for slide in self.slides] video_streams = [inp.video for inp in inputs] audio_streams = [inp.audio for inp in inputs] joined_video = ffmpeg.concat(*video_streams, v=1, a=0) joined_audio = ffmpeg.concat(*audio_streams, v=0, a=1) ffmpeg.output(joined_video, joined_audio, str(output_path)).overwrite_output().run( capture_stdout=True, capture_stderr=True ) + except ffmpeg.Error: + logger.exception("Error combining videos into final output") + raise
256-274: Add type hint for parser parameter.The
add_argumentsmethod parameterparserlacks a type hint, reducing IDE support and type safety.🔎 Add type hint:
+ from argparse import ArgumentParser + - def add_arguments(self, parser) -> None: + def add_arguments(self, parser: ArgumentParser) -> None: """Add command-line arguments to the parser. Args: - parser (argparse.ArgumentParser): The argument parser instance. + parser: The argument parser instance. """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/common/eleven_labs.py(1 hunks)backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py(1 hunks)backend/apps/owasp/templates/video/base.html(1 hunks)backend/apps/owasp/templates/video/slides/projects.html(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T06:36:42.593Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2089
File: backend/apps/nest/models/google_account_authorization.py:61-62
Timestamp: 2025-08-22T06:36:42.593Z
Learning: In the OWASP/Nest project, ruff linting rules discourage using logger.error() immediately before raising exceptions as it creates redundant logging. The preferred approach is to remove the logging call and let the caller handle logging the exception appropriately.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (3)
backend/apps/common/eleven_labs.py (3)
ElevenLabs(17-212)set_text(126-138)generate_to_file(195-212)backend/apps/common/open_ai.py (4)
OpenAi(13-105)set_prompt(64-76)set_input(36-48)complete(78-105)backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/owasp/templates/video/base.html (1)
1-139: Base template structure looks good.The fixed viewport (1920×1080), inline utility CSS, and Arial font choice are appropriate for WeasyPrint-based video slide rendering. The template provides a solid foundation for the video generation pipeline.
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (1)
201-208: Add guard against empty projects list.Lines 204-207 format project names but don't handle the case when
project_namesis empty. If a snapshot has no new projects, accessingproject_names[-1]will raise anIndexError.In a previous review thread, you mentioned skipping the projects section when there are no new projects. However, the current code still calls
create_projects_slide()unconditionally (line 308). You should either:
- Add a guard in
Command.handle()to skip creating the projects slide whensnapshot.new_projects.count() == 0, or- Add validation here to handle the empty case gracefully.
🔎 Option 1: Skip projects slide in Command.handle() (recommended)
# In Command.handle() around line 308 generator.append_slide(slide_builder.create_intro_slide()) # Only add projects slide if there are new projects if snapshot.new_projects.exists(): generator.append_slide(slide_builder.create_projects_slide())🔎 Option 2: Guard in create_projects_slide()
project_names = [ p.name.replace("OWASP ", "") for p in new_projects[:MAX_TRANSCRIPT_PROJECTS] ] + if not project_names: + raise ValueError("Cannot create projects slide: no projects available") + formatted_project_names = ( f"{', '.join(project_names[:-1])}, and {project_names[-1]}" if len(project_names) > 1 else project_names[0] )Based on learnings, prefer Option 1 as it aligns with your stated intention to skip the section entirely when there are no projects.
🧹 Nitpick comments (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (1)
108-115: Consider raising exception immediately when transcript generation fails.When
open_ai.complete()returnsNone, the method logs an error and returns early, leavingself.transcriptasNone. Later,generate_audio()will detect this and raise aRuntimeError. This deferred error handling makes debugging harder because the error message won't indicate that transcript generation failed.For consistency with
generate_audio()and clearer error reporting, consider raising immediately:🔎 Proposed refactor for immediate error raising
transcript = open_ai.complete() if not transcript: logger.error("Error generating transcript for %s", self.name) - return + raise RuntimeError(f"Failed to generate transcript for slide: {self.name}") self.transcript = transcript print("Generated slide transcript:", self.transcript)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py(1 hunks)backend/apps/owasp/templates/video/base.html(1 hunks)backend/apps/owasp/templates/video/slides/projects.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/templates/video/base.html
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T06:36:42.593Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2089
File: backend/apps/nest/models/google_account_authorization.py:61-62
Timestamp: 2025-08-22T06:36:42.593Z
Learning: In the OWASP/Nest project, ruff linting rules discourage using logger.error() immediately before raising exceptions as it creates redundant logging. The preferred approach is to remove the logging call and let the caller handle logging the exception appropriately.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (4)
backend/apps/common/eleven_labs.py (3)
ElevenLabs(17-212)set_text(126-138)generate_to_file(195-212)backend/apps/common/open_ai.py (4)
OpenAi(13-105)set_prompt(64-76)set_input(36-48)complete(78-105)backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)backend/apps/owasp/api/internal/nodes/snapshot.py (1)
new_projects(45-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (6)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (6)
30-52: LGTM!The dataclass is well-structured with proper type hints and validation logic. The
__post_init__method correctly enforces that exactly one oftranscriptortranscript_inputmust be provided, preventing invalid states.
53-81: LGTM!The resource cleanup pattern is well-implemented. Initializing
pagetoNonebefore the try block and using guarded cleanup in the finally block ensures resources are properly released even if errors occur during rendering.
116-136: LGTM!The error handling is robust. The method validates that a transcript exists before attempting audio generation and raises meaningful exceptions when operations fail, ensuring errors are caught early with clear context.
137-158: LGTM!The FFmpeg integration properly handles errors by catching
ffmpeg.Error, logging with context, and re-raising to ensure failures aren't silently swallowed. The video generation parameters (aac audio codec, libx264 video codec, yuv420p pixel format) are appropriate for broad compatibility.
226-260: LGTM!The Generator orchestrates the video pipeline effectively. The sequence (render → transcript → audio → video → combine) is logical, and the FFmpeg concatenation properly handles both video and audio streams separately before merging them into the final output.
285-310: LGTM with one note.The command structure follows Django conventions properly. The environment variable setup is correctly placed in
handle()rather than at module level, and the snapshot fetching includes appropriate error handling.Note: Line 308 unconditionally creates the projects slide. See the comment on lines 201-208 regarding handling empty projects lists.
rudransh-shrivastava
force-pushed
the
feature/community-snapshot-videos
branch
from
3a54683 to
9c15eda
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
backend/apps/owasp/templates/video/slides/projects.html (1)
1-22: LGTM!The template structure is clean and follows Django best practices. The previously flagged
w-76class is now properly defined inbase.html(lines 154-156), resolving the earlier concern.backend/apps/owasp/templates/video/base.html (1)
1-172: LGTM!The base template is well-structured for video rendering with WeasyPrint. All utility classes are properly defined inline, including the custom
w-76class. The use of Arial font is appropriate for PDF/image rendering.backend/apps/common/eleven_labs.py (2)
43-46: Validate API key before initialization.The code accesses
settings.ELEVENLABS_API_KEYwithout checking if it's configured. If the key is missing or empty, the ElevenLabsClient initialization may fail with an unclear error message.🔎 Add validation before client initialization
+ api_key = settings.ELEVENLABS_API_KEY + if not api_key: + msg = "ELEVENLABS_API_KEY is not configured in settings" + raise ValueError(msg) + self.client = ElevenLabsClient( - api_key=settings.ELEVENLABS_API_KEY, + api_key=api_key, timeout=30, )
168-193: Validate text is set before generating audio.The
generate()method accessesself.textwithout checking ifset_text()was called first. If the text attribute doesn't exist, this will raise anAttributeError.🔎 Add validation before API call
def generate(self) -> bytes | None: """Generate audio from text. Returns: bytes | None: The audio bytes or None on error. """ + if not hasattr(self, "text") or not self.text: + logger.error("Text not set. Call set_text() before generate().") + return None + try: audio_iterator = self.client.text_to_speech.convert( model_id=self.model_id, output_format=self.output_format, text=self.text, voice_id=self.voice_id, voice_settings={ "similarity_boost": self.similarity_boost, "stability": self.stability, "style": self.style, "use_speaker_boost": self.use_speaker_boost, }, ) return b"".join(audio_iterator) except Exception: logger.exception("An error occurred during ElevenLabs API request.") return Nonebackend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (1)
201-208: Handle the case when no new projects exist.Lines 204-208 will raise an
IndexErrorifproject_namesis empty (when there are no new projects or fewer thanMAX_TRANSCRIPT_PROJECTS). While you mentioned this is "quite rare," it's better to handle gracefully to avoid runtime failures.🔎 Add validation for empty project list
project_names = [ p.name.replace("OWASP ", "") for p in new_projects[:MAX_TRANSCRIPT_PROJECTS] ] + if not project_names: + # Skip or handle the no-projects case + logger.warning("No projects available for transcript generation") + formatted_project_names = "" + elif len(project_names) > 1: + formatted_project_names = f"{', '.join(project_names[:-1])}, and {project_names[-1]}" + else: + formatted_project_names = project_names[0] - formatted_project_names = ( - f"{', '.join(project_names[:-1])}, and {project_names[-1]}" - if len(project_names) > 1 - else project_names[0] - )Alternatively, you could skip creating the projects slide entirely in
Command.handle()whensnapshot.new_projects.exists()returnsFalse, which would be cleaner.
🧹 Nitpick comments (2)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (2)
60-60: Consider usingself.stdout.write()for command output.The
print()statements could be replaced withself.stdout.write()to align with Django management command conventions. However, since this is WIP and you've indicated these will be addressed later, this is just a reminder.Based on learnings, Django commands should use
self.stdout.write()for user-facing output instead ofprint().Also applies to: 72-72
307-308: Consider checking if new projects exist before creating the slide.To avoid the potential
IndexErrorincreate_projects_slide()when there are no new projects, you could add a conditional check here:generator.append_slide(slide_builder.create_intro_slide()) - generator.append_slide(slide_builder.create_projects_slide()) + if snapshot.new_projects.exists(): + generator.append_slide(slide_builder.create_projects_slide()) + else: + logger.info("Skipping projects slide - no new projects in snapshot") generator.generate_video(output_dir / f"{snapshot_key}_snapshot.mp4")This pairs with the earlier suggestion to handle empty project lists and provides a cleaner solution.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
backend/.env.examplebackend/apps/common/eleven_labs.pybackend/apps/owasp/Makefilebackend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.pybackend/apps/owasp/templates/video/base.htmlbackend/apps/owasp/templates/video/slides/intro.htmlbackend/apps/owasp/templates/video/slides/projects.htmlbackend/docker/Dockerfile.localbackend/pyproject.tomlbackend/settings/base.pycspell/custom-dict.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/docker/Dockerfile.local
- backend/.env.example
- backend/settings/base.py
- backend/apps/owasp/templates/video/slides/intro.html
- backend/pyproject.toml
- cspell/custom-dict.txt
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T06:36:42.593Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2089
File: backend/apps/nest/models/google_account_authorization.py:61-62
Timestamp: 2025-08-22T06:36:42.593Z
Learning: In the OWASP/Nest project, ruff linting rules discourage using logger.error() immediately before raising exceptions as it creates redundant logging. The preferred approach is to remove the logging call and let the caller handle logging the exception appropriately.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
backend/apps/owasp/Makefile (1)
25-27: LGTM!The new Makefile target follows the established pattern and correctly invokes the Django management command with the required
--snapshot-keyparameter.backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (4)
30-52: LGTM!The
Slidedataclass is well-structured with proper validation in__post_init__to ensure exactly one oftranscriptortranscript_inputis provided. Type hints are clear and complete.
82-114: LGTM!The transcript generation logic properly handles the case where
open_ai.complete()returnsNone, with appropriate error logging and early return. The validation logic for pre-set transcripts and transcript input is also correct.
116-157: LGTM!Both
generate_audio()andgenerate_video()methods have proper validation and error handling. The audio generation checks for transcript existence before proceeding, and video generation properly re-raises exceptions after logging. FFmpeg parameters are appropriate for video encoding.
226-259: LGTM!The
Generatorclass orchestrates the video generation pipeline correctly. The slide processing loop and video concatenation logic using ffmpeg are well-structured. Type hints are properly included.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (2)
212-212: Remove or utilize unused OpenAi instance.
self.open_aiis instantiated but never used anywhere in theGeneratorclass. Since transcripts are currently hardcoded inSlideBuildermethods, this instance serves no purpose and should either be removed to reduce unnecessary dependencies, or integrated if dynamic transcript generation is planned.🔎 If not needed, remove it
class Generator: def __init__(self) -> None: """Initialize Video Generator.""" self.eleven_labs = ElevenLabs(stability=ELEVENLABS_STABILITY, style=ELEVENLABS_STYLE) - self.open_ai = OpenAi() self.slides: list[Slide] = []
285-285: Clarify output directory behavior and consider cleanup of intermediate files.Line 285 uses
OUTPUT_DIR(global temp directory) for slide intermediate files, while the user-providedoutput_diris only used for the final merged video (line 295). This means:
- User's
--output-diraffects only the final.mp4location- Intermediate files (images, audio, per-slide videos) remain in the system temp directory indefinitely
While using temp for intermediates is reasonable to avoid cluttering the user's directory, these files are never cleaned up after
generate_video()completes. Consider either documenting this behavior or implementing cleanup of intermediate files after the merge succeeds.🔎 Optional: Clean up intermediate files after merge
def handle(self, *args, **options) -> None: """Handle the command execution.""" # Required to cache fonts os.environ["XDG_CACHE_HOME"] = str(Path(tempfile.gettempdir()) / "cache") snapshot_key = options["snapshot_key"] output_dir = options["output_dir"] output_dir.mkdir(parents=True, exist_ok=True) + + # Create unique temp directory for intermediate files + import tempfile + temp_dir = Path(tempfile.mkdtemp(prefix=f"snapshot_{snapshot_key}_")) snapshot = Snapshot.objects.filter( key__iexact=snapshot_key, status=Snapshot.Status.COMPLETED ).first() if not snapshot: logger.error("No completed snapshot found for key: %s", snapshot_key) return print("Generating video for snapshot:", snapshot_key) - slide_builder = SlideBuilder(snapshot, OUTPUT_DIR) + slide_builder = SlideBuilder(snapshot, temp_dir) generator = Generator() generator.append_slide(slide_builder.create_intro_slide()) generator.append_slide(slide_builder.create_sponsors_slide()) if snapshot.new_projects.exists(): generator.append_slide(slide_builder.create_projects_slide()) else: print("Skipping projects slide - No new projects in snapshot.") generator.generate_video(output_dir / f"{snapshot_key}_snapshot.mp4") + + # Cleanup intermediate files + import shutil + shutil.rmtree(temp_dir, ignore_errors=True)This creates a unique temporary directory per run (avoiding conflicts) and cleans it up after the final video is generated.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T06:36:42.593Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2089
File: backend/apps/nest/models/google_account_authorization.py:61-62
Timestamp: 2025-08-22T06:36:42.593Z
Learning: In the OWASP/Nest project, ruff linting rules discourage using logger.error() immediately before raising exceptions as it creates redundant logging. The preferred approach is to remove the logging call and let the caller handle logging the exception appropriately.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (1)
244-244: Add type hint for the parser parameter.The
parserparameter lacks a type annotation. Adding it improves type safety and IDE support.🔎 Proposed improvement
+from argparse import ArgumentParser + # ... existing imports ... - def add_arguments(self, parser) -> None: + def add_arguments(self, parser: ArgumentParser) -> None: """Add command-line arguments to the parser.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T06:36:42.593Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2089
File: backend/apps/nest/models/google_account_authorization.py:61-62
Timestamp: 2025-08-22T06:36:42.593Z
Learning: In the OWASP/Nest project, ruff linting rules discourage using logger.error() immediately before raising exceptions as it creates redundant logging. The preferred approach is to remove the logging call and let the caller handle logging the exception appropriately.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py (3)
backend/apps/common/eleven_labs.py (3)
ElevenLabs(17-212)set_text(126-138)generate_to_file(195-212)backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)backend/apps/owasp/api/internal/queries/sponsor.py (1)
sponsors(14-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/apps/common/template_loader.py:
- Around line 11-12: The Slack Jinja environment currently sets autoescape=True
which escapes Slack link syntax and breaks rendering; update the Environment
instantiation that assigns slack_env (the call using
FileSystemLoader(SLACK_TEMPLATES_DIR)) to use autoescape=False so Slack Block
Kit plain-text templates render without HTML-escaping, and remove any redundant
|safe workarounds in those templates afterward.
🧹 Nitpick comments (2)
backend/tests/apps/owasp/video_test.py (1)
15-25: Consider using.jinjaextension for consistency.The fixture uses
template_name="video/slides/intro.html"but the actual templates use.jinjaextension (as verified intest_add_intro_slideat line 165 which assertsslide.template_name == "slides/intro.jinja"). While this doesn't affect test correctness since rendering is mocked, aligning the extension would improve consistency.Suggested fix
@pytest.fixture def slide(self, tmp_path): """Fixture to provide a sample slide.""" return Slide( context={"title": "Test Slide"}, name="test_slide", output_dir=tmp_path, - template_name="video/slides/intro.html", + template_name="video/slides/intro.jinja", transcript="Test transcript", )backend/apps/owasp/video.py (1)
264-305: Consider optimizing the N+1 query pattern.Lines 271-276 iterate through releases and execute a separate database query for each release to find its project. This could be optimized by prefetching the relationship or restructuring the query.
💡 Suggested optimization using prefetch_related
def add_releases_slide(self) -> Slide | None: """Add a releases slide.""" - releases = Release.objects.filter(snapshots__in=self.snapshots).distinct() + releases = Release.objects.filter(snapshots__in=self.snapshots).distinct().select_related('repository') if not releases.exists(): return None project_counts: Counter = Counter() + # Fetch all projects with their repositories in one query + repository_ids = [r.repository_id for r in releases if r.repository_id] + project_map = { + repo.id: project + for project in Project.objects.filter(repositories__id__in=repository_ids).prefetch_related('repositories') + for repo in project.repositories.all() + } + for release in releases: if not release.repository: continue - project = Project.objects.filter(repositories=release.repository).first() + project = project_map.get(release.repository_id) if project: project_counts[project.name] += 1
Minor: Redundant defensive check on line 282.
The ternary
else 1is unreachable since line 280 already returns None whentop_projectsis empty.🔧 Simplify the defensive check
- max_count = top_projects[0][1] if top_projects else 1 + max_count = top_projects[0][1]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/apps/common/template_loader.pybackend/apps/owasp/templates/video/base.jinjabackend/apps/owasp/templates/video/slides/chapters.jinjabackend/apps/owasp/templates/video/slides/intro.jinjabackend/apps/owasp/templates/video/slides/projects.jinjabackend/apps/owasp/templates/video/slides/releases.jinjabackend/apps/owasp/templates/video/slides/sponsors.jinjabackend/apps/owasp/templates/video/slides/thank_you.jinjabackend/apps/owasp/video.pybackend/apps/slack/commands/command.pybackend/apps/slack/events/event.pybackend/apps/slack/template_loader.pybackend/tests/apps/owasp/video_test.py
💤 Files with no reviewable changes (1)
- backend/apps/slack/template_loader.py
✅ Files skipped from review due to trivial changes (1)
- backend/apps/owasp/templates/video/slides/sponsors.jinja
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:296-296
Timestamp: 2025-12-28T15:20:59.650Z
Learning: In `backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py`, the SlideBuilder is intentionally initialized with the module constant OUTPUT_DIR (temp directory) rather than the user-specified output_dir flag. This is by design: intermediate files (images, audio, per-slide videos) are generated in temp, while the final merged video is saved to the user-specified output directory.
📚 Learning: 2025-12-28T15:20:59.650Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:296-296
Timestamp: 2025-12-28T15:20:59.650Z
Learning: In `backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py`, the SlideBuilder is intentionally initialized with the module constant OUTPUT_DIR (temp directory) rather than the user-specified output_dir flag. This is by design: intermediate files (images, audio, per-slide videos) are generated in temp, while the final merged video is saved to the user-specified output directory.
Applied to files:
backend/apps/owasp/templates/video/slides/chapters.jinjabackend/apps/owasp/templates/video/slides/releases.jinjabackend/apps/owasp/templates/video/slides/thank_you.jinjabackend/apps/owasp/templates/video/slides/intro.jinjabackend/tests/apps/owasp/video_test.pybackend/apps/owasp/video.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/apps/slack/commands/command.pybackend/apps/slack/events/event.pybackend/tests/apps/owasp/video_test.pybackend/apps/common/template_loader.pybackend/apps/owasp/video.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/apps/slack/commands/command.pybackend/apps/slack/events/event.pybackend/tests/apps/owasp/video_test.pybackend/apps/common/template_loader.pybackend/apps/owasp/video.py
📚 Learning: 2026-01-01T18:57:05.007Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/video.py:189-215
Timestamp: 2026-01-01T18:57:05.007Z
Learning: In the OWASP backend area, maintain the established pattern: when dealing with sponsors, include all entries from Sponsor.objects.all() (including NOT_SPONSOR) and perform in-memory sorting using the same criteria/pattern used by the GraphQL sponsor query implemented in backend/apps/owasp/api/internal/queries/sponsor.py. Apply this behavior consistently to files in backend/apps/owasp (not just video.py), and ensure code paths that render sponsor lists follow this in-code sorting approach rather than pre-filtering NOT_SPONSOR entries before sorting.
Applied to files:
backend/apps/owasp/video.py
📚 Learning: 2025-06-18T20:00:23.899Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1634
File: frontend/src/app/api/auth/[...nextauth]/route.ts:30-55
Timestamp: 2025-06-18T20:00:23.899Z
Learning: The OWASP Nest application has logging disabled, so avoid suggesting console.log, console.error, or any other logging statements in code review suggestions.
Applied to files:
backend/apps/owasp/video.py
🧬 Code graph analysis (1)
backend/apps/owasp/video.py (1)
backend/apps/common/eleven_labs.py (4)
ElevenLabs(18-225)save(216-225)set_text(151-163)generate(193-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (21)
backend/apps/slack/events/event.py (1)
13-13: LGTM: Centralized template loader migration.The import change correctly migrates to the new centralized template loader while maintaining the same interface.
backend/apps/common/template_loader.py (1)
7-9: LGTM: Path configuration is correct.The BASE_DIR calculation and template directory paths are properly configured.
backend/apps/slack/commands/command.py (1)
10-10: LGTM: Centralized template loader migration.The import change correctly migrates to the new centralized template loader while maintaining the same interface.
backend/apps/owasp/templates/video/slides/releases.jinja (1)
1-18: LGTM!The template structure is clean and consistent with other slide templates. The use of
release.bar_widthin the inline style is appropriate for numeric percentage values, and Jinja2's default autoescaping handles the text content safely.backend/apps/owasp/templates/video/slides/chapters.jinja (1)
1-17: LGTM!The template correctly limits display to 24 chapters with an overflow indicator. The slice syntax and length filter usage are idiomatic Jinja2.
backend/apps/owasp/templates/video/slides/intro.jinja (1)
1-7: LGTM!Clean and minimal intro slide template with proper centering using flexbox utilities.
backend/apps/owasp/templates/video/slides/thank_you.jinja (1)
1-11: LGTM!The thank you slide is appropriately simple with static content. The AI-generated audio disclosure at the bottom-right is a good practice for transparency.
backend/apps/owasp/templates/video/slides/projects.jinja (1)
1-20: LGTM!The template follows the same consistent pattern as the chapters template, with appropriate limits (16 projects) and conditional leaders display. The structure is clean and maintainable.
backend/apps/owasp/templates/video/base.jinja (1)
1-252: LGTM!The base template is well-structured for video slide rendering with fixed 1920x1080 dimensions. The inline CSS utility classes provide a self-contained styling system appropriate for PDF-to-image rendering, avoiding external dependencies. The
@pagerule ensures proper PDF output dimensions.backend/tests/apps/owasp/video_test.py (3)
42-71: LGTM!Thorough test coverage for
render_and_save_imagewith proper mocking of the PDF rendering pipeline. The test correctly verifies template rendering, PDF generation, and resource cleanup (closecalls).
123-158: LGTM!Good use of
@pytest.mark.parametrizeforformat_names_for_transcriptcovering edge cases including empty list, single name, and truncation at 3 names. The fixture setup for mock snapshots is clean.
317-394: LGTM!Comprehensive tests for
VideoGeneratorcovering the slide accumulation, video generation workflow, and merge operations. The exception handling tests properly verify that errors are logged before re-raising.backend/apps/owasp/video.py (9)
1-22: LGTM! Clean module organization.The imports are well-organized, and the module docstring clearly describes the purpose.
24-33: LGTM! Well-defined constants.The module-level constants are appropriately named and provide clear configuration points.
36-116: LGTM! Excellent resource management and error handling.The Slide class demonstrates:
- Proper cleanup of PDF/page resources in the finally block
- Well-documented methods with Args and Raises sections
- Appropriate error handling with logging and re-raising FFmpeg errors
118-149: LGTM! Clear initialization and formatting logic.The SlideBuilder initialization and name formatting are well-implemented with comprehensive docstrings.
151-195: LGTM! Correct sponsor sorting implementation.The sponsor slide correctly includes all sponsors and applies in-memory sorting by sponsor type, consistent with the established pattern.
Based on learnings, this approach is the correct pattern for sponsor lists in the OWASP backend.
197-231: LGTM! Proper QuerySet existence check.The method correctly uses
.exists()for the emptiness check (line 204), avoiding unnecessary database queries.
233-262: LGTM! Proper QuerySet existence check.The method correctly uses
.exists()for the emptiness check (line 238).
307-316: LGTM! Simple and clear.The thank you slide implementation is straightforward.
319-388: LGTM! Well-structured video generation orchestration.The VideoGenerator class demonstrates:
- Clear separation of concerns (rendering, audio, video generation, merging, cleanup)
- Comprehensive docstrings with Args, Returns, and Raises sections
- Proper error handling with logging and re-raising FFmpeg errors
- Safe cleanup with existence checks before unlinking files
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docker/backend/Dockerfile.video (4)
1-2: Avoid:latestbase tag for reproducible builds.Using
FROM nest-backend:latestcan silently change dependencies/OS packages between builds; consider pinning to a digest (or at least a versioned tag) and letting the Makefile choose it.
20-24: Make WORKDIR explicit before COPY / Poetry install to avoid base-image coupling.Right now
COPY poetry.lock pyproject.toml ./depends on whatever WORKDIR the base image set; making it explicit here reduces accidental breakage ifdocker/backend/Dockerfilechanges.Proposed diff
FROM nest-backend:latest USER root +WORKDIR /home/owasp + ENV APK_CACHE_DIR="/home/owasp/.cache/apk-backend-video" \ APK_SYMLINK_DIR="/etc/apk/cache" \ OWASP_GID=1000 \ OWASP_UID=1000 \ PIP_CACHE_DIR="/home/owasp/.cache/pip" \ POETRY_CACHE_DIR="/home/owasp/.cache/pypoetry" \ POETRY_VIRTUALENVS_IN_PROJECT=true \ PYTHONUNBUFFERED=1 @@ -COPY --chmod=444 --chown=root:root poetry.lock pyproject.toml ./ +COPY --chmod=444 --chown=root:root poetry.lock pyproject.toml ./ @@ -WORKDIR /home/owasp - USER owaspAlso applies to: 32-34
17-24: Double-check venv/cache ownership when switching toUSER owasp.Because Poetry runs as root and
POETRY_VIRTUALENVS_IN_PROJECT=true,.venv(and potentially some caches) may end up root-owned; usually fine for reads, but can cause runtime friction if anything needs to write (bytecode, font caches, etc.). Considerchown -R ${OWASP_UID}:${OWASP_GID} /home/owasp/.venv /home/owasp/.cacheafter install if you hit permission issues.
25-31: Consider droppingapk upgradefor deterministic builds (unless you explicitly want “latest packages”).
apk upgradecan change the image output over time even with the same inputs;apk add ...is usually sufficient in Docker builds unless you’re intentionally rolling forward.backend/tests/apps/owasp/video_test.py (4)
15-121: Good external-dependency isolation; consider asserting call args where it matters.E.g., in
test_generate_and_save_audio_success, you already assertset_text(transcript); similarly, forgenerate_and_save_video, it may be worth assertingffmpeg.inputwas called with the expected audio/image paths (and thatffmpeg.output(...).run(...)got the expected flags).
144-158: Avoid hard-coding the transcript name limit in the test.
(["OWASP A", "OWASP B", "OWASP C", "OWASP D"], "A, B, and C")implicitly assumes the cap is 3; importing and usingTRANSCRIPT_NAMES_LIMIT(or constructing the expectation based on it) will keep the test stable if the limit changes.
210-227: MagicMock iteration: prefer__iter__.return_value = iter([...])over replacing__iter__.Replacing
mock_qs.__iter__works, but settingmock_qs.__iter__.return_valuetends to behave more predictably with MagicMock and reduces surprises if other magic methods get exercised.Also applies to: 246-260, 280-307
336-349: Strengthengenerate_videoassertions: verify ElevenLabs instance is passed through.Since
VideoGenerator.generate_video()callsslide.generate_and_save_audio(self.eleven_labs), it’s useful to assertslideX.generate_and_save_audio.assert_called_once_with(generator.eleven_labs)(not just “called”).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/Makefilebackend/apps/owasp/Makefilebackend/tests/apps/owasp/video_test.pydocker/backend/Dockerfile.video
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/Makefile
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:296-296
Timestamp: 2025-12-28T15:20:59.650Z
Learning: In `backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py`, the SlideBuilder is intentionally initialized with the module constant OUTPUT_DIR (temp directory) rather than the user-specified output_dir flag. This is by design: intermediate files (images, audio, per-slide videos) are generated in temp, while the final merged video is saved to the user-specified output directory.
📚 Learning: 2025-10-26T12:50:50.512Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2429
File: backend/Makefile:30-32
Timestamp: 2025-10-26T12:50:50.512Z
Learning: The `exec-backend-e2e-command` and `exec-db-e2e-command` Makefile targets in the backend/Makefile are intended for local development and debugging only, not for CI/CD execution, so the `-it` flags are appropriate.
Applied to files:
backend/Makefile
📚 Learning: 2025-12-26T06:09:08.868Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 3041
File: .github/workflows/run-ci-cd.yaml:233-243
Timestamp: 2025-12-26T06:09:08.868Z
Learning: For the OWASP/Nest repository, Redis image versions should remain consistent across all environments (production, staging, local, E2E, and CI/CD E2E tests). When upgrading Redis, update all docker-compose files and CI/CD workflow configurations together to maintain environment parity.
Applied to files:
backend/Makefiledocker/backend/Dockerfile.video
📚 Learning: 2025-12-28T15:20:59.650Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:296-296
Timestamp: 2025-12-28T15:20:59.650Z
Learning: In `backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py`, the SlideBuilder is intentionally initialized with the module constant OUTPUT_DIR (temp directory) rather than the user-specified output_dir flag. This is by design: intermediate files (images, audio, per-slide videos) are generated in temp, while the final merged video is saved to the user-specified output directory.
Applied to files:
backend/tests/apps/owasp/video_test.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/owasp/video_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/owasp/video_test.py
📚 Learning: 2025-10-17T15:25:55.624Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/providers.tf:1-3
Timestamp: 2025-10-17T15:25:55.624Z
Learning: The infrastructure code in the OWASP/Nest repository (infrastructure/ directory) is intended for quick testing purposes only, not for production deployment.
Applied to files:
docker/backend/Dockerfile.video
🧬 Code graph analysis (1)
backend/tests/apps/owasp/video_test.py (1)
backend/apps/owasp/video.py (18)
Slide(37-115)VideoGenerator(319-388)audio_path(47-49)image_path(52-54)video_path(57-59)render_and_save_image(61-79)generate_and_save_audio(81-90)generate_and_save_video(92-115)format_names_for_transcript(133-149)add_intro_slide(151-167)add_sponsors_slide(169-195)add_projects_slide(197-231)add_chapters_slide(233-262)add_releases_slide(264-305)add_thank_you_slide(307-316)append_slide(327-334)generate_video(336-355)merge_videos(357-381)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/Makefile (1)
15-25: Nice cleanup addition; verify whether a named “video” container also needs removal.Removing
nest-backendandnest-video-backendimages fits the new workflow, but if the video generator is ever run with a stable--name ...,clean-backend-dockershould alsodocker container rm -f <that-name>to avoid dangling containers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @docker/backend/Dockerfile.video:
- Around line 1-24: Poetry is unpinned and the video extras group isn’t being
installed; update the Dockerfile so the Poetry install is pinned when invoking
python -m pip install poetry (use a specific version) and change the poetry
install invocation (the line running poetry install --no-root --without dev
--without test) to explicitly include the video extras (add --with video) so the
video dependencies (elevenlabs, ffmpeg-python, pillow, pypdfium2, weasyprint)
are installed.
🧹 Nitpick comments (4)
backend/apps/owasp/Makefile (3)
30-44: Consider aligning with the established command execution pattern.All other targets in this Makefile follow a consistent pattern:
@CMD="..." $(MAKE) exec-backend-command. This new target manually orchestrates Docker image builds and container execution, which deviates from the established convention and may impact maintainability.While the video generation workflow requires a specialized Docker image, consider whether the
exec-backend-commandinfrastructure could be extended to support this use case, or whether a helper target could abstract the build/run logic to maintain consistency.
31-36: Consider extracting image builds to separate targets for efficiency.The target rebuilds both Docker images on every invocation. While Docker's layer caching helps, extracting the builds into separate prerequisite targets would allow Make to track when rebuilds are actually needed and improve developer experience by avoiding unnecessary build steps.
Example refactor to separate concerns
.PHONY: build-video-images build-video-images: @docker build -f docker/backend/Dockerfile backend/ -t nest-backend:base @docker build -f docker/backend/Dockerfile.video backend/ -t nest-video-backend owasp-generate-community-snapshot-video: build-video-images @mkdir -p backend/generated_videos @docker run \ --env-file backend/.env \ --mount type=bind,src="$(PWD)/backend/generated_videos",dst=/home/owasp/generated_videos \ --network nest-local_nest-network \ --rm \ nest-video-backend \ python manage.py owasp_generate_community_snapshot_video $(snapshot_key) /home/owasp/generated_videos
30-44: Add echo statement and validate required argument for better UX.This target lacks the
@echostatement present in other targets, reducing visibility during execution. Additionally, the required$(snapshot_key)argument has no validation, which could lead to unclear errors if omitted.Suggested improvements
owasp-generate-community-snapshot-video: + @echo "Generating community snapshot video for key: $(snapshot_key)" + @test -n "$(snapshot_key)" || (echo "Error: snapshot_key is required" && exit 1) @docker build \ -f docker/backend/Dockerfile backend/ \ -t nest-backend:basecspell/custom-dict.txt (1)
11-12: Align custom dictionary entries with actual tokens to avoid future cspell noise.A few additions look like they’re meant to cover WeasyPrint/ffmpeg tokens (good), but ensure the dict matches what actually appears in code/docs (e.g.,
libpangoft2vslibpangoft, and any otherso:lib…tokens you expect to keep). Otherwise you’ll still get cspell hits or end up over-whitelisting unrelated words.Also applies to: 39-40, 65-66, 87-92, 107-108, 119-124, 151-152
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
backend/apps/owasp/Makefilecspell/custom-dict.txtdocker/backend/Dockerfile.video
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:296-296
Timestamp: 2025-12-28T15:20:59.650Z
Learning: In `backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py`, the SlideBuilder is intentionally initialized with the module constant OUTPUT_DIR (temp directory) rather than the user-specified output_dir flag. This is by design: intermediate files (images, audio, per-slide videos) are generated in temp, while the final merged video is saved to the user-specified output directory.
📚 Learning: 2025-12-26T06:09:08.868Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 3041
File: .github/workflows/run-ci-cd.yaml:233-243
Timestamp: 2025-12-26T06:09:08.868Z
Learning: For the OWASP/Nest repository, Redis image versions should remain consistent across all environments (production, staging, local, E2E, and CI/CD E2E tests). When upgrading Redis, update all docker-compose files and CI/CD workflow configurations together to maintain environment parity.
Applied to files:
docker/backend/Dockerfile.video
📚 Learning: 2025-10-17T15:25:55.624Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/providers.tf:1-3
Timestamp: 2025-10-17T15:25:55.624Z
Learning: The infrastructure code in the OWASP/Nest repository (infrastructure/ directory) is intended for quick testing purposes only, not for production deployment.
Applied to files:
docker/backend/Dockerfile.video
📚 Learning: 2025-12-28T15:20:59.650Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:296-296
Timestamp: 2025-12-28T15:20:59.650Z
Learning: In `backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py`, the SlideBuilder is intentionally initialized with the module constant OUTPUT_DIR (temp directory) rather than the user-specified output_dir flag. This is by design: intermediate files (images, audio, per-slide videos) are generated in temp, while the final merged video is saved to the user-specified output directory.
Applied to files:
backend/apps/owasp/Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (2)
backend/apps/owasp/Makefile (1)
41-41: Network namenest-local_nest-networkis undefined and will cause the docker run command to fail.The network name is hard-coded on line 41 but does not match any defined network. The docker-compose/local/compose.yaml defines a network named
nest-network, notnest-local_nest-network. Change--network nest-local_nest-networkto--network nest-networkto align with the actual Docker network created by the local compose configuration.⛔ Skipped due to learnings
Learnt from: ahmedxgouda Repo: OWASP/Nest PR: 3041 File: .github/workflows/run-ci-cd.yaml:233-243 Timestamp: 2025-12-26T06:09:08.868Z Learning: For the OWASP/Nest repository, Redis image versions should remain consistent across all environments (production, staging, local, E2E, and CI/CD E2E tests). When upgrading Redis, update all docker-compose files and CI/CD workflow configurations together to maintain environment parity.Learnt from: rudransh-shrivastava Repo: OWASP/Nest PR: 3205 File: docker-compose/local.yaml:32-32 Timestamp: 2026-01-05T16:20:47.532Z Learning: In the OWASP/Nest repository, feature branches use unique volume name suffixes in docker-compose files to prevent volume clashes across parallel development efforts. For example, the feature/nest-zappa-migration branch uses `-zappa` suffix for all volume names (backend-venv-zappa, cache-data-zappa, db-data-zappa, etc.) to ensure isolated environments when switching between branches.docker/backend/Dockerfile.video (1)
25-31: Re-check Alpine/WeasyPrint runtime deps + BuildKit requirement.This relies on WeasyPrint system libs (Line 28-30) and BuildKit-only syntax (
RUN --mount,COPY --chmod). If CI/build users don’t have BuildKit enabled, or if the WeasyPrint dependency set is incomplete for your Alpine/base-image combination, the image will fail at build/runtime.Suggested verifications:
- Confirm CI always builds this Dockerfile with BuildKit enabled.
- Confirm the minimal required libs for the WeasyPrint version you’re using are present in this image (either added here or already in
nest-backend:base).
|
2 participants
Proposed change
Resolves #2924
Checklist
make check-testlocally and all tests passed