Dynamic uploads and doument map for Pixel - #1382
Merged
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe change replaces the static Pixelbot document map with course-scoped database records. It adds category and document administration, S3 lifecycle handling, metadata generation, orphan cleanup, protected routes, and course-scoped RAG and chat context. Pixelbot document management
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 17
🧹 Nitpick comments (6)
test/cadet/chatbot/course_documents_test.exs (1)
64-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
release_datefilter.
build_document_map_json/1andget_documents_by_ids/2both exclude documents whoserelease_dateis in the future. The docstrings at lines 228-230 and 271-272 oflib/cadet/chatbot/course_documents.exdescribe this as the rule that keeps a scheduled document invisible to Pixel. No test exercises it. A test with a futurerelease_dateand one with a pastrelease_datewould protect that rule.🤖 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 `@test/cadet/chatbot/course_documents_test.exs` around lines 64 - 108, Add tests covering future and past release dates for both build_document_map_json/1 and get_documents_by_ids/2. Assert documents scheduled with a future release_date are excluded, while documents with a past release_date remain included, preserving the documented Pixel visibility rule.lib/cadet/chatbot/llm_content_block.ex (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a
@moduledocfor consistency.The other new chatbot modules define
@moduledoc. If the project enables theCredomoduledoc check orwarnings_as_errors, this module fails the check.🤖 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 `@lib/cadet/chatbot/llm_content_block.ex` around lines 1 - 2, Add a module documentation attribute to Cadet.Chatbot.LlmContentBlock, placed immediately after the module declaration and before `@text_media_types`, describing the module’s purpose consistently with the other chatbot modules.lib/cadet/workers/PixelbotOrphanSweeper.ex (1)
55-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSelect only
s3_keyinstead of loading full rows.
Repo.all(PixelbotDocument)loads every document struct for every course into memory on each sweep. Only the key is needed.♻️ Proposed fix
+ import Ecto.Query + defp known_s3_keys do - PixelbotDocument - |> Repo.all() - |> MapSet.new(& &1.s3_key) + from(d in PixelbotDocument, select: d.s3_key) + |> Repo.all() + |> MapSet.new() end🤖 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 `@lib/cadet/workers/PixelbotOrphanSweeper.ex` around lines 55 - 59, Update known_s3_keys to query only the s3_key field from PixelbotDocument rather than loading full document structs, while preserving the resulting MapSet of keys.lib/cadet/chatbot/document_store.ex (1)
79-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared MIME table.
Cadet.Chatbot.DocumentUploader.media_type_for/1(lib/cadet/chatbot/document_uploader.ex Lines 129-141) holds the same extension-to-MIME mapping, andCadet.Chatbot.LlmContentBlock(lib/cadet/chatbot/llm_content_block.ex Line 2) holds the matching text-type list. This PR had to update two copies for.texand.xml. Move the mapping into one module and call it from both places to prevent drift.🤖 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 `@lib/cadet/chatbot/document_store.ex` around lines 79 - 80, Extract the shared extension-to-MIME mapping into a dedicated module or shared symbol, then update Cadet.Chatbot.DocumentStore and Cadet.Chatbot.DocumentUploader.media_type_for/1 to reuse it. Also have Cadet.Chatbot.LlmContentBlock consume the shared text-type data instead of maintaining its own list, preserving the existing MIME and text classification behavior.lib/cadet_web/admin_controllers/admin_pixelbot_documents_controller.ex (1)
47-89: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBound the work performed inside the upload request.
The reduce processes each file sequentially. Each iteration performs an S3 upload, reads the whole file into memory, Base64-encodes it, and waits for an LLM completion. With several large files the request can exceed the proxy timeout, and the memory held per request is roughly twice the total upload size. Add a limit on the number of files per request, and consider moving metadata generation to a background job or running it with bounded concurrency through
Task.async_stream/3.🤖 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 `@lib/cadet_web/admin_controllers/admin_pixelbot_documents_controller.ex` around lines 47 - 89, Bound the upload work in upload by enforcing a maximum number of files per request before the sequential Enum.reduce, with an explicit response for excess files. Keep each file’s upload result handling unchanged; if metadata generation remains inline, replace the unbounded sequential processing with bounded-concurrency Task.async_stream/3, preserving entry order and the existing success/error entries.lib/cadet/chatbot/metadata_generator.ex (1)
37-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet an explicit timeout for the metadata LLM call.
OpenAI.chat_completion/1usesopenai0.6.2, whose globalconfig :openai, http_options: [...]is not set here. Add%OpenAI.Config{http_options: [recv_timeout: 120_000]}ingenerate/4so large uploads do not hold the admin request process on the default HTTPoison timeout.🤖 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 `@lib/cadet/chatbot/metadata_generator.ex` around lines 37 - 47, Update generate/4 at the OpenAI.chat_completion call to pass an explicit %OpenAI.Config with http_options containing recv_timeout: 120_000, while preserving the existing model and messages arguments and success/error handling.
🤖 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 `@lib/cadet_web/admin_controllers/admin_pixelbot_documents_controller.ex`:
- Around line 100-117: Update save/2 so CourseDocuments.create_documents/2 and
update_existing/2 execute within a single Repo.transaction/1, propagating any
insert or update error to roll back all changes together. Preserve the existing
success rendering and error responses, while ensuring retries cannot leave
partially committed new documents.
- Around line 51-55: Update the upload reduction around List.wrap(files) to
pattern-match each upload as %Plug.Upload{} before accessing filename or path.
Route non-upload values, including strings, maps, and arrays, through the
existing bad-request clause so the endpoint returns 400 instead of raising.
In `@lib/cadet_web/controllers/chat_controller.ex`:
- Around line 151-155: Update openai_error_message/1 in
lib/cadet_web/controllers/chat_controller.ex at lines 151-155 and
lib/cadet_web/controllers/rag_chat_controller.ex at lines 187-191 so non-map
provider errors are logged server-side with their diagnostic reason while the
HTTP response always returns a generic fallback message; do not interpolate
inspect(reason) into either client-facing result.
In `@lib/cadet_web/controllers/rag_chat_controller.ex`:
- Around line 82-108: Update build_screen_context/2 to enforce a combined size
limit for the code and question screen context before constructing the prompt.
If the total exceeds the configured budget, propagate a validation result that
makes the controller return HTTP 422; otherwise preserve the existing trimming,
filtering, and formatting behavior.
- Around line 255-256: Update screen_context_message/1 so client-provided
screen_context is emitted as clearly delimited user reference content rather
than a system message. Also update the primary system prompt to explicitly treat
this screen content as untrusted data, while preserving the nil case as an empty
message list.
In `@lib/cadet/chatbot/course_documents.ex`:
- Around line 98-101: Update the entry-processing flow in create_documents/2
before calling Slug.slugify/1 to validate that the title value from
entry["title"] or entry[:title] is present and usable. Reject missing titles
through the existing validation/error path so PixelbotDocument.changeset/2 can
return a validation error instead of allowing slugification to raise.
- Around line 76-81: Update list_documents_for_category/2 to pass the external
category_id through cast_id/1 before interpolating it into the Ecto query,
matching the validation pattern used by the other ID-accepting functions and
avoiding CastError responses for non-numeric binaries.
- Around line 146-164: Update rename_document/3 to coordinate the S3 rename and
metadata persistence transactionally: retain the original S3 key, perform the
database update within the established transaction mechanism, and restore the
original S3 object when the metadata update or transaction fails after a
successful move. Preserve the existing not-found and uploader-error results
while ensuring no row references a deleted object.
In `@lib/cadet/chatbot/document_uploader.ex`:
- Around line 52-59: Update the uniqueness probe in rename/3 so object_exists?
treats old_s3_key as available when checking candidates, allowing Slug.unique/2
to return the unchanged key when appropriate. Preserve the existing new_key ==
old_s3_key short circuit and rename behavior for actual collisions with other
objects.
- Around line 115-127: Update object_exists?/1 to treat only {:error,
{:http_error, 404, _}} from ExAws.S3.head_object as absent; treat all other
error responses as present, preserving the existing true result for successful
responses. Add the requested logging or retry handling for non-404 errors using
the surrounding S3 flow.
In `@lib/cadet/chatbot/llm_content_block.ex`:
- Around line 5-7: Update the text-media `build/3` clause to decode base64
defensively and validate the decoded content as UTF-8 before constructing the
text map. When decoding fails or the bytes are not valid UTF-8, fall back to the
existing file-block behavior instead of raising or returning non-JSON-encodable
text.
In `@lib/cadet/chatbot/pixelbot_document.ex`:
- Around line 39-50: Update the changeset function to coerce a nil description
to an empty string before or during casting, while preserving the existing
optional-field behavior for non-nil descriptions. Ensure update_existing/2
requests containing "description": null validate and persist an empty string
instead of passing nil to the non-null column.
In `@lib/cadet/workers/PixelbotOrphanSweeper.ex`:
- Around line 1-9: Rename the worker file for
Cadet.Workers.PixelbotOrphanSweeper to the snake_case name
pixelbot_orphan_sweeper.ex, and ensure this module is scheduled through the
existing Oban configuration rather than Oban.Plugins.Cron. If no scheduling
mechanism exists, remove the “Daily job” wording from the module documentation.
- Around line 37-47: Update the orphan deletion flow in PixelbotOrphanSweeper to
split orphans into batches of at most 1,000 keys before calling
ExAws.S3.delete_multiple_objects. Execute and handle each batch request
independently, preserving the existing success and error logging for each batch.
In `@priv/repo/migrations/20260804000000_create_pixelbot_documents.exs`:
- Around line 15-16: Update the category_id foreign-key definition in the
migration’s pixelbot_documents table to use on_delete: :nothing instead of
:restrict. Preserve the existing nullability and course_id cascade behavior,
relying on the application-level guard in CourseDocuments.delete_category/2 for
category deletion checks.
In `@test/cadet_web/controllers/rag_chat_controller_test.exs`:
- Around line 35-44: Update the cassette setup in the RAG chat controller test
to return the fixture document key course-<course.id>/l1a.pdf from the routing
response instead of an empty list, ensuring the cassette includes the
corresponding S3 fetch. Extend the test assertions so the final payload confirms
a PDF attachment is selected.
In `@test/cadet/chatbot/course_documents_test.exs`:
- Around line 85-87: Update the assertion in the build_document_map_json/1 test
to inspect the stored document data rather than Jason.OrderedObject struct
fields. Assert that "s3_key" is absent using entry["s3_key"], a value from
entry.values, or the encoded JSON output.
---
Nitpick comments:
In `@lib/cadet_web/admin_controllers/admin_pixelbot_documents_controller.ex`:
- Around line 47-89: Bound the upload work in upload by enforcing a maximum
number of files per request before the sequential Enum.reduce, with an explicit
response for excess files. Keep each file’s upload result handling unchanged; if
metadata generation remains inline, replace the unbounded sequential processing
with bounded-concurrency Task.async_stream/3, preserving entry order and the
existing success/error entries.
In `@lib/cadet/chatbot/document_store.ex`:
- Around line 79-80: Extract the shared extension-to-MIME mapping into a
dedicated module or shared symbol, then update Cadet.Chatbot.DocumentStore and
Cadet.Chatbot.DocumentUploader.media_type_for/1 to reuse it. Also have
Cadet.Chatbot.LlmContentBlock consume the shared text-type data instead of
maintaining its own list, preserving the existing MIME and text classification
behavior.
In `@lib/cadet/chatbot/llm_content_block.ex`:
- Around line 1-2: Add a module documentation attribute to
Cadet.Chatbot.LlmContentBlock, placed immediately after the module declaration
and before `@text_media_types`, describing the module’s purpose consistently with
the other chatbot modules.
In `@lib/cadet/chatbot/metadata_generator.ex`:
- Around line 37-47: Update generate/4 at the OpenAI.chat_completion call to
pass an explicit %OpenAI.Config with http_options containing recv_timeout:
120_000, while preserving the existing model and messages arguments and
success/error handling.
In `@lib/cadet/workers/PixelbotOrphanSweeper.ex`:
- Around line 55-59: Update known_s3_keys to query only the s3_key field from
PixelbotDocument rather than loading full document structs, while preserving the
resulting MapSet of keys.
In `@test/cadet/chatbot/course_documents_test.exs`:
- Around line 64-108: Add tests covering future and past release dates for both
build_document_map_json/1 and get_documents_by_ids/2. Assert documents scheduled
with a future release_date are excluded, while documents with a past
release_date remain included, preserving the documented Pixel visibility rule.
🪄 Autofix
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 Plus
Run ID: 62c8f979-2781-4f4d-9c54-3db2558602d3
📒 Files selected for processing (22)
lib/cadet/chatbot/course_documents.exlib/cadet/chatbot/document_store.exlib/cadet/chatbot/document_uploader.exlib/cadet/chatbot/llm_content_block.exlib/cadet/chatbot/metadata_generator.exlib/cadet/chatbot/pixelbot_category.exlib/cadet/chatbot/pixelbot_document.exlib/cadet/chatbot/prompt_builder.exlib/cadet/chatbot/rag_pipeline.exlib/cadet/chatbot/slug.exlib/cadet/workers/PixelbotOrphanSweeper.exlib/cadet_web/admin_controllers/admin_courses_controller.exlib/cadet_web/admin_controllers/admin_pixelbot_documents_controller.exlib/cadet_web/admin_views/admin_pixelbot_documents_view.exlib/cadet_web/controllers/chat_controller.exlib/cadet_web/controllers/rag_chat_controller.exlib/cadet_web/router.expriv/course_documents/document_map.jsonpriv/repo/migrations/20260804000000_create_pixelbot_documents.exstest/cadet/chatbot/course_documents_test.exstest/cadet/chatbot/rag_pipeline_test.exstest/cadet_web/controllers/rag_chat_controller_test.exs
💤 Files with no reviewable changes (2)
- priv/course_documents/document_map.json
- lib/cadet_web/admin_controllers/admin_courses_controller.ex
|
coverage: 90.475% (+2.1%) from 88.376% — yilin/pixel into master |
sayomaki
requested changes
Aug 10, 2026
sayomaki
left a comment
sayomaki
left a comment
Contributor
There was a problem hiding this comment.
Took a look at the changes, and I do have some comments below.
Just leaving a quick note here on the use of per-course LLM API keys vs global LLM API keys, will discuss more with the team on what should be the direction for this.
sayomaki
requested changes
Aug 11, 2026
sayomaki
left a comment
sayomaki
left a comment
Contributor
There was a problem hiding this comment.
Just some minor changes, and a small clarification
Contributor
Author
|
hi @sayomaki i added a new commit to configure the course specific llm key instead. I refactored part of the code for the llm grading since that feature already set up the infra to use course specific key. This involved some fe changes so i opened a new pr there this is the linked fe mr |
7 tasks
sayomaki
approved these changes
Aug 13, 2026
sayomaki
left a comment
sayomaki
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thanks for the work!
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
4 participants
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.
Description
This PR adds 2 main features for pixel
Type of change
How to test
Checklist