feat: upgrade lesson planner to question-driven walkthroughs with asy… - #14
Merged
Merged
Conversation
…nc video export Make lesson generation grounded in multi-source retrieval, add richer lesson schema/citations, and expand dashboard with question input, presenter narration, and MP4 export polling. Add async lesson render APIs with ffmpeg-backed pipeline plus local render job store and docs updates. Made-with: Cursor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR introduces lesson video rendering with FFmpeg and ElevenLabs text-to-speech, adds task structure normalization with preset playbooks, implements OpenAI fallback for Gemini API, extends lesson schema with metadata (citations, confidence, speaker notes), integrates task-aware chat context, and updates UI for task creation, lesson generation with narration, and async video export with progress tracking. Changes
Sequence DiagramssequenceDiagram
participant Client as Client
participant API as POST /api/lesson/render
participant Store as Render Store
participant BG as Background Job
participant FFmpeg as FFmpeg Pipeline
participant TTS as ElevenLabs/TTS
participant Disk as Disk Storage
Client->>API: POST {lesson}
API->>Store: createLessonRenderJob()
Store->>Disk: Save job (queued)
API-->>Client: Return job + 202
API->>BG: Start async render
BG->>Store: setStatus(running)
BG->>FFmpeg: renderLessonVideo(jobId, lesson)
FFmpeg->>TTS: Synthesize slide text
TTS-->>FFmpeg: Audio stream
FFmpeg->>Disk: Write slide clips + narration
FFmpeg->>Disk: Concatenate to final video
BG->>Store: setStatus(completed, outputUrl)
Client->>API: Poll GET /api/lesson/render/[jobId]
API->>Store: getLessonRenderJob(jobId)
Store-->>API: Job status + progress
API-->>Client: Return status
Client->>API: GET /api/lesson/render/[jobId]/video
API->>Disk: Read video file
API-->>Client: Stream MP4 audio/mpeg
sequenceDiagram
participant Client as Client (Dashboard)
participant ChatAPI as POST /api/chat
participant TaskDB as Task Store
participant AIProvider as AI (Gemini/OpenAI)
participant Retrieval as Lesson Retrieval
Client->>ChatAPI: POST {hireId, question}
ChatAPI->>TaskDB: Retrieve assigned tasks
TaskDB-->>ChatAPI: Tasks (normalized description)
ChatAPI->>Retrieval: Fetch context (if not task-only)
Retrieval-->>ChatAPI: Documents + company context
ChatAPI->>ChatAPI: Format taskContext + contextDocs
ChatAPI->>AIProvider: Generate with task-first prompt
AIProvider-->>ChatAPI: Lesson {slides, sources, confidence}
ChatAPI-->>Client: Return lesson + sourcesUsed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Increase lesson context depth and slide detail requirements, remove confusing lesson source dropdown, and improve slide readability with speaker notes. Fix video export reliability by running render requests synchronously and correcting ffmpeg drawtext path handling. Made-with: Cursor
…fully Show clear inline UI guidance when video export cannot run due to missing ffmpeg, avoid hard throw behavior in dashboard render flow, and render slide body/notes/summary with rich formatting. Also strengthen lesson depth prompt with per-step action and verification requirements. Made-with: Cursor
Inject assigned hire tasks into chat context so "how do I do my task" requests produce concrete step-by-step plans grounded in both tasks and docs. Also tighten lesson prompts/context handling to keep outputs question-specific instead of repeating generic slide templates. Made-with: Cursor
…estion Fix ffmpeg filter syntax for slide rendering so MP4 export runs on local ffmpeg 8.x builds. Improve lesson generation by injecting relevant assigned tasks, scoring tasks by question intent, and hardening model JSON parsing to reduce generic fallback behavior. Made-with: Cursor
Detect drawtext support at runtime and gracefully fall back to plain slide clips when the local ffmpeg build lacks libfreetype/drawtext. This keeps lesson MP4 export working instead of hard-failing on filter-not-found errors. Made-with: Cursor
…ext video output Replace the generic fallback lesson with a question-grounded flow built from assigned tasks and retrieved docs so different questions produce different walkthroughs even when Gemini is unavailable. Improve ffmpeg no-drawtext fallback rendering to use animated patterned visuals instead of flat color-only clips. Made-with: Cursor
Make task guidance consistently actionable across seeded and manager-created tasks, add clickable task deep-dives on the new hire dashboard, and extend lesson narration to ElevenLabs for both exported videos and slide reading. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (12)
README.md (1)
48-59: LGTM — docs accurately mirror the new endpoints and runtime requirements.The endpoint paths match the new routes (
POST /api/lesson/render,GET /api/lesson/render/[jobId],GET /api/lesson/render/[jobId]/video) and the.runbook-data/lesson-renders/storage location is consistent withrenderOutputPathinsrc/lib/lessonRenderStore.ts. One optional follow-up: you may want to mention non-macOS install paths (e.g.,apt install ffmpeg,choco install ffmpeg) so Linux/Windows contributors aren't left guessing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 48 - 59, Update README.md to mention ffmpeg install commands for Linux and Windows in addition to macOS: add brief instructions or examples such as apt-based install for Debian/Ubuntu and Chocolatey for Windows so non-macOS contributors know how to install ffmpeg; ensure the text still references the MP4 export requirement and the existing endpoints (POST /api/lesson/render, GET /api/lesson/render/:jobId, GET /api/lesson/render/:jobId/video) and the storage location .runbook-data/lesson-renders/ (which corresponds to renderOutputPath in src/lib/lessonRenderStore.ts).src/components/ui/ChatMessageBody.tsx (1)
30-38: LGTM — defensive normalization handles the new shape variability.Coercing
unknown→string(with array-join andString()fallbacks) is a sensible response to upstream chat/lesson payloads that may now contain non-string content. Two small notes you can take or leave:
- Widening the prop from
stringtounknownweakens compile-time safety for all callers; if only one or two callsites legitimately pass non-strings, consider keeping the public type asstring | string[] | null | undefinedinstead ofunknown.- For arrays of objects,
.join("\n")will yield"[object Object]"lines. Fine as a guardrail, but worth knowing if such inputs ever reach this component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ChatMessageBody.tsx` around lines 30 - 38, The prop type for ChatMessageBody is currently too broad (text: unknown) and the normalization with normalizedText can produce "[object Object]" for arrays of objects; change the public prop signature on ChatMessageBody to a narrower union (e.g. string | string[] | null | undefined) and update the normalization logic to handle array entries safely by mapping items to strings (using JSON.stringify for objects, otherwise String(item)) before joining, leaving role and blocks logic unchanged.src/app/api/tasks/route.ts (1)
32-40: Minor: numbering preserves user-supplied numbers but doesn't normalize them.If a user pastes steps like
1. foo\n3. bar\n2. baz, the regex^\d+\.keeps the original numbers (1, 3, 2) instead of renumbering. Likely fine since the chat prompt atsrc/app/api/chat/route.tslines 96-101 just pastestask.descriptionverbatim, but worth noting if you ever want strictly canonical step ordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/tasks/route.ts` around lines 32 - 40, The current normalization preserves user-supplied numeric prefixes (using /^\d+\./) which can leave steps out-of-order; update the normalizedSteps logic so every line is renumbered sequentially: in the chain that builds normalizedSteps (operating on steps), strip any existing leading numeric prefix from each line (e.g., remove /^\d+\.\s*/), then always prepend `${idx + 1}. ` to the cleaned line before joining; reference the variables normalizedSteps, steps and parts to locate and replace the existing map() that conditionally keeps /^\d+\./.src/app/dashboard/page.tsx (2)
361-381: Type cast assumes the error response shape matchesLessonRenderJob.When
!res.ok, the response body is most likely{ error: "..." }(per the route atsrc/app/api/lesson/render/route.tslines 18, 40), not aLessonRenderJob. The cast(await res.json()) as LessonRenderJobis unsafe and readingdata.errorhere happens to work only because TS doesn't enforce runtime shape. Tightening:- const data = (await res.json()) as LessonRenderJob; - if (!res.ok) { - const message = data.error || "Failed to render lesson video"; + const data = (await res.json().catch(() => ({}))) as Partial<LessonRenderJob> & { error?: string }; + if (!res.ok) { + const message = data.error || "Failed to render lesson video";Then narrow further before
setRenderJob(data as LessonRenderJob)in the success branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/dashboard/page.tsx` around lines 361 - 381, When handling the fetch response, don't cast the error branch response to LessonRenderJob; instead parse the JSON into a narrower error shape (e.g. { error?: string }) when !res.ok, use that error message to populate setRenderJob and setLessonUiError, and only in the success branch validate/narrow the parsed body before calling setRenderJob(data as LessonRenderJob). Update the code around res.json() usage so the error path uses a separate variable/type for the {error} payload and the success path performs a runtime shape check (or optional property checks) on the parsed body before treating it as a LessonRenderJob in setRenderJob.
433-449: Cleanup effect doesn't null out audio refs and runs only on unmount.Two small issues with this unmount cleanup:
narrationAudioRef.currentandnarrationObjectUrlRef.currentare paused/revoked but never set tonull, so any later code that checksif (narrationAudioRef.current)would still see a stale reference (in practice cleanup runs at unmount so this is benign, but is brittle if the effect is ever reused).- The empty dependency array means this only fires at unmount — narration started right before navigation away is fine, but the elapsed timer at lines 451-457 is the one doing real work per render. No change required there; just flagging the consistency.
if (narrationAudioRef.current) { narrationAudioRef.current.pause(); narrationAudioRef.current.currentTime = 0; + narrationAudioRef.current = null; } if (narrationObjectUrlRef.current) { URL.revokeObjectURL(narrationObjectUrlRef.current); + narrationObjectUrlRef.current = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/dashboard/page.tsx` around lines 433 - 449, In the useEffect cleanup (the useEffect shown) make the cleanup null-safe by, after clearing the interval (pollTimerRef.current) and cancelling speechSynthesis, pausing and resetting narrationAudioRef.current and then setting narrationAudioRef.current = null, and after URL.revokeObjectURL(narrationObjectUrlRef.current) set narrationObjectUrlRef.current = null; keep the empty dependency array as-is but ensure you reference the existing refs (pollTimerRef, narrationAudioRef, narrationObjectUrlRef) so later code won’t encounter stale non-null references.src/lib/dataStore.ts (1)
110-220: Keyword matching is too broad and stomps on unrelated tasks.
t.includes("github"),t.includes("security"),t.includes("environment"), etc. match well beyond the intended task type. A task titled"Document our security policy"or"Audit GitHub repositories for stale branches"will silently inherit the generic "complete security training" / "get GitHub access" playbook on first read ofgetTasks(), overwriting any author-provided description (see also the related issue inensureStructuredTaskDescription).Consider tightening to whole-word + intent matching, e.g. require both an action verb and the keyword (
/\b(set\s*up|enable|complete|onboard).*\b(security|2fa)\b/), or only apply presets whensourceTitlematches the canonical seed sources. As per coding guidelines, this is a recommended refactor for correctness of normalization, not a stylistic nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dataStore.ts` around lines 110 - 220, presetTaskPlaybook uses broad substring checks (t.includes("github"), etc.) that match unrelated titles; replace each t.includes(...) branch with stricter intent+keyword regex checks or gate by canonical sourceTitle seeds: update the function presetTaskPlaybook to use patterns like /\b(setup|set up|enable|install|complete|onboard|join|get|configure).*\b(github)\b/i (and analogous patterns for security, slack, hr, environment) or require sourceTitle to equal known seeds before returning a preset, and keep the same return payloads for the matching branches (preserve the existing blocks for "hr profile", "slack", "github", "local dev"/"environment", "security").src/lib/prompts.ts (1)
30-46: Prompt contract marksconfidence,limitedSources,sourcesUsedas required, but the TypeScript type marks them optional.The JSON schema described to the model lists
confidence,limitedSources, andsourcesUsedas non-optional fields, yetLessoninsrc/lib/types.ts(lines 80-83) declares them as optional with?:. The dashboard atsrc/app/dashboard/page.tsxrenders defensively (lesson?.confidence ?), so this is non-blocking, but the schema/contract drift will haunt later consumers. Consider either:
- Marking them required in
Lesson(and validating after parsing in/api/lesson/route.ts), or- Annotating them as optional in this prompt schema (
"confidence"?: ...) so the model knows they may be omitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/prompts.ts` around lines 30 - 46, The prompt schema in src/lib/prompts.ts says confidence, limitedSources, and sourcesUsed are required but the TypeScript Lesson type (Lesson in src/lib/types.ts) marks them optional; pick one resolution: either make those fields required on the Lesson type (remove the ? for confidence, limitedSources, sourcesUsed) and add runtime validation in /api/lesson/route.ts to assert presence after parsing, or update the prompt schema in src/lib/prompts.ts to mark "confidence"?, "limitedSources"?, and "sourcesUsed"? as optional so the model may omit them; update any consumers such as src/app/dashboard/page.tsx to match the chosen contract (remove defensive checks if making required or keep them if optional). Ensure the chosen change is consistent across Lesson, the prompt schema, and the API validation.src/lib/lessonRenderStore.ts (2)
24-26: Prefercrypto.randomUUID()for job IDs.
Date.now()+ 6 base36 chars fromMath.random()is collision-prone under burst load and is not cryptographically random. Since the id is also user-visible (URL path param), a UUID is a cleaner default.-import { promises as fs } from "fs"; +import { promises as fs } from "fs"; +import { randomUUID } from "crypto"; @@ -function makeJobId() { - return `lesson-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; -} +function makeJobId() { + return `lesson-${randomUUID()}`; +}(If you adopt this, also update the
JOB_ID_REregex from the path-traversal comment.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/lessonRenderStore.ts` around lines 24 - 26, Replace the collision-prone makeJobId implementation with a UUID-based id by using crypto.randomUUID() inside makeJobId (e.g., return crypto.randomUUID()); also update the JOB_ID_RE used for validating path params to accept UUID v4 format (adjust the regex to match typical UUID hex groups with hyphens) so routes and path-traversal checks remain correct when job IDs are UUIDs.
60-73: Read-modify-write is not atomic.
updateLessonRenderJobreads, mutates, and writes without any locking. If two concurrent updates ever land on the same job (e.g. a future progress callback alongside a status transition), one update will silently overwrite the other. For now the renderer is single-writer per job, so this is mostly future-proofing — consider an atomic "write to temp +fs.rename" pattern at minimum so a crash mid-write doesn't leave a truncated JSON file thatgetLessonRenderJobthen permanently treats as missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/lessonRenderStore.ts` around lines 60 - 73, The updateLessonRenderJob function performs a non-atomic read-modify-write which can be corrupted by concurrent updates or crashes; change updateLessonRenderJob to write to a temporary file (e.g., jobPath(jobId) + ".tmp" or use a unique temp name), fs.writeFile the JSON there, fsync the temp file if available, then atomically replace the original with fs.rename (or fs.promises.rename) and finally return the updated object; keep getLessonRenderJob and jobPath names as-is and ensure error handling cleans up the temp file on failure to avoid leaving partial/truncated JSON files.src/lib/lessonVideoRenderer.ts (3)
79-86:whichis not portable; usecommand -vor just probesaydirectly.
whichis not guaranteed on every Linux distro and is absent on Windows. Sincesayis the only consumer here and it's macOS-only anyway, you could justtry { execFileAsync("say", ["-v", "?"]) }or skip the helper entirely. Low priority since the ElevenLabs path is the primary one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/lessonVideoRenderer.ts` around lines 79 - 86, The commandExists helper currently uses the non-portable "which"; update commandExists (or remove it and inline its use) to probe the actual consumer by attempting to run "say" directly: replace await execFileAsync("which", [command]) with a direct probe like await execFileAsync("say", ["-v", "?"]) (or change callers to try execFileAsync("say", ["-v", "?"]) and handle failure), ensuring you import/keep execFileAsync usage and update the function name/usage accordingly so detection works on macOS without relying on "which".
311-343: LGTM on the orchestration, with two notes.Slide loop, fallbacks, and the
clips.length === 0guard look right. Two small things to consider:
- Slides are rendered serially; for a 10-slide lesson with ElevenLabs that's noticeable latency. Parallelizing per-slide work (with a small concurrency cap) would dramatically shorten wall time without changing correctness, since each slide writes to distinct files.
- If
concatSlideClipsthrows, all the per-slide artifacts underassetDirremain. Wrapping the body intry/finallytormthe asset dir on success (keeping it on failure for debugging) would keep.runbook-data/from growing unboundedly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/lessonVideoRenderer.ts` around lines 311 - 343, renderLessonVideo currently renders slides serially and never cleans up per-slide artifacts on success; update it to run per-slide work in parallel with a bounded concurrency (e.g., use a p-limit or a worker pool) so the per-slide operations (writeSlideTextFile, renderSlideClip, synthesizeSlideSpeech, muxClipWithAudio) for each index i can run concurrently but still write to distinct files in assetDir, then collect resulting clip paths into clips; also wrap the orchestration (the per-slide processing + concatSlideClips call) in a try/finally so that after concatSlideClips succeeds you remove the assetDir (renderAssetDir/job-specific folder) but on error you leave the directory intact for debugging. Ensure you keep existing behavior when audio is missing (push baseClipPath) and preserve the existing clips.length guard in renderLessonVideo.
213-276: Intermediate audio files (mp3,aiff) are never cleaned up.
speech-N.mp3andspeech-N.aiffaccumulate under each job's asset directory along withclip-*.mp4/clip-narrated-*.mp4and the concat list. Over many renders this fills the disk on the host. Considerfs.unlinkafter the WAV is produced, and a job-completion sweep ofassetDironceoutputPathexists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/lessonVideoRenderer.ts` around lines 213 - 276, synthesizeSlideSpeech leaves intermediate files (speech-N.mp3 / .aiff) behind; after successfully writing the WAV, delete the intermediate file(s) (use fs.unlink or fs.rm wrapped in try/catch) in synthesizeSlideSpeech (references: synthesizeSlideSpeech, fs.writeFile, execFileAsync, commandExists) so mp3/aiff are removed whether ElevenLabs or local "say" path was used; additionally add a short job-completion cleanup routine that runs once the final outputPath exists to sweep any remaining speech-*.mp3/.aiff in assetDir (handle and log unlink errors, avoid throwing) to prevent disk accumulation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/lesson/render/`[jobId]/video/route.ts:
- Around line 18-20: The catch block in the video route currently returns a 404
for every error; change it to log the caught error (using the same logger
approach as the sibling [jobId]/route.ts) and only map ENOENT to
NextResponse.json({ error: "Rendered video not found" }, { status: 404 });
otherwise return a 500 response (e.g., NextResponse.json({ error: "Internal
server error" }, { status: 500 })); identify the catch by the catch block in
src/app/api/lesson/render/[jobId]/video/route.ts and update error handling to
inspect err.code === "ENOENT" and log the full error before returning the
response.
- Around line 9-11: The handler uses jobId from context.params directly in
renderOutputPath which allows path traversal; validate jobId (e.g., allow only a
strict pattern like /^[A-Za-z0-9_-]+$/ or your project’s canonical ID format)
and return 400/404 on mismatch, then build the file path with renderOutputPath
and immediately resolve it (path.resolve) and assert the resolved path starts
with path.resolve(OUTPUT_DIR) + path.sep (or equivalent) before calling
fs.readFile; reference the jobId param, the renderOutputPath call, and
OUTPUT_DIR for where to add these checks and the post-join verification.
- Around line 11-17: The current handler reads the entire file with fs.readFile
and returns a full-body NextResponse which can OOM and prevents seeking; replace
that with range-aware streaming: read the incoming "range" header, parse
start/end against fs.stat(outputPath).size, validate ranges and on valid partial
requests return status 206 with headers Accept-Ranges: bytes, Content-Range,
Content-Length and Content-Type, and stream the requested byte slice using
fs.createReadStream(outputPath, { start, end }) (on full requests return 200
with a stream and Accept-Ranges: bytes); keep the existing content-disposition
using jobId and ensure invalid ranges return 416 with appropriate Content-Range.
Use the existing symbols outputPath, jobId, fs.readFile (remove),
fs.createReadStream, NextResponse, and the request.headers.get('range') to
locate where to implement this change.
In `@src/app/api/lesson/render/route.ts`:
- Around line 14-21: The POST handler currently accepts any payload and
immediately calls createLessonRenderJob, which allows unauthenticated/unchecked
abuse; before spawning work call requireHireAccess(body.hireId) (same gate used
by /api/chat) and return 403/401 on failure, and add validation on the lesson
payload (e.g., max slides count and total serialized length) to reject oversized
lessons with 400; additionally throttle concurrent renders by checking/using a
job concurrency limiter or queue (wrap or gate createLessonRenderJob with a
semaphore/queue and reject with 429 when limit reached) so heavy
ffmpeg/ElevenLabs work is capped. Ensure you reference POST,
createLessonRenderJob, and requireHireAccess when implementing these checks and
return appropriate NextResponse statuses for auth/size/concurrency failures.
- Around line 22-35: The fire-and-forget IIFE is unreliable on serverless;
replace it with Next.js's after() background response hook (or integrate an
external queue) so work runs reliably after sending the response: move the logic
that calls setLessonRenderStatus, renderLessonVideo, and the final
setLessonRenderStatus into an after(() => { ... }) callback (or enqueue job and
return immediately), and ensure any errors from setLessonRenderStatus or
renderLessonVideo are caught and logged and the job status is reconciled to
"failed" with the error message (use error instanceof Error ? error.message :
String(error)) so no unhandled rejections leave jobs stuck.
In `@src/app/api/lesson/route.ts`:
- Around line 60-71: The fallback narration currently uses raw normalizedSlides,
so any cleaning/framing done by the slides mapping (asWalkthroughBody,
cleanLessonText, default titles, slice to 14) is not applied; fix by computing
the mapped slide array once (e.g. const slidesNormalized =
normalizedSlides.slice(0,14).map(... ) using the same mapping that calls
asWalkthroughBody and cleanLessonText) and then set slides: slidesNormalized and
narrationScript: lesson.narrationScript || slidesNormalized.map(s =>
`${s.title}. ${s.body}`).join("\n"); this ensures narrationScript uses the
cleaned/framed slide text.
- Around line 281-291: The current logic only calls requireHireAccess(hireId)
when hireId is present, allowing unauthenticated requests that omit hireId to
proceed to buildLessonContext(...) and generateFromGemini(), which likely burns
API quota; move the authentication check to apply unconditionally or explicitly
gate the no-hireId path. Concretely, call requireHireAccess(hireId) (or a new
check that enforces RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS semantics from apiAuth.ts)
before the if (hireId) branch or add an explicit unauthenticated-path guard that
returns a 401/403 like the existing auth error handling, keeping the same
response shape and using the same auth.ok / auth.status logic so
buildLessonContext(...) and generateFromGemini(...) are only reached when
authorized.
- Around line 36-46: The numbered-steps logic uses remaining.replace(/\n/g,
"\n2. ") which hardcodes the "2." label; replace that with proper enumeration by
splitting remaining on '\n' and mapping each line to a numbered prefix that
increments (e.g. remaining.split('\n').map((line,i) => `${i+2}.
${line}`).join('\n')) and use that string in place of the current template;
update the code that builds the "Steps:" block (the return block referencing
remaining and the replace call) to use this mapped/joined value so subsequent
steps are numbered 2., 3., 4., ... correctly.
In `@src/app/api/tasks/route.ts`:
- Around line 20-23: The current early-return in the helper that checks if
(/objective:|steps:|verification:|if blocked:/i.test(description)) can falsely
trigger on inline mentions and causes silently discarding explicit structured
fields; update the condition in the function that processes the incoming
description (the block using the description variable in
src/app/api/tasks/route.ts) to only short-circuit when no structured form fields
were provided (e.g., objective, steps, verification, escalation) OR make the
regex anchor to line starts using a multiline-aware pattern (e.g., match
/^\s*(objective:|steps:|verification:|if blocked:)/im) so the helper keeps
returning raw description only when it truly contains section headers and no
explicit structured fields were sent.
In `@src/app/api/tts/route.ts`:
- Around line 13-40: The route handler lacks auth and hardening: add the same
access check used in src/app/api/chat/route.ts (call requireHireAccess at the
start of the POST handler) to prevent unauthenticated usage; add an upstream
timeout to the fetch by using AbortSignal.timeout(...) (matching the per-attempt
timeouts in src/lib/ai.ts) and pass the signal to fetch; validate the incoming
text length before calling text.slice(0, 4500) and return a 400 JSON via
NextResponse.json when the client-supplied text exceeds your allowed limit
instead of silently truncating; and when handling non-OK responses from
ElevenLabs, avoid returning the raw upstream body — log the err server-side and
return a generic error message (keep the existing NextResponse.json({ error: ...
}, { status: 502 }) pattern but with a sanitized message).
In `@src/app/dashboard/page.tsx`:
- Around line 324-347: pollRenderJob currently always schedules
window.setInterval after awaiting tick(), causing extra polls when the job was
already terminal; modify pollRenderJob so that after await tick() you only set
pollTimerRef.current = window.setInterval(...) if the tick determined the job is
still non-terminal (e.g., check the returned/updated data.status or a boolean
from tick), and update tick to return the latest LessonRenderJob (or a terminal
flag) so you can gate scheduling; additionally add a currentJobIdRef (or
similar) that you set at the start of pollRenderJob and have tick ignore or
avoid calling setRenderJob when its jobId doesn't match currentJobIdRef to
prevent stale ticks from clobbering newer renders, and ensure pollTimerRef is
cleared whenever a terminal status is observed.
In `@src/app/manager/tasks/page.tsx`:
- Around line 342-374: The client textarea bound to form.objective is marked
required (the element with value={form.objective} and onChange updating
form.objective via setForm), but the API only enforces title && (description ||
objective); to resolve the contract drift either remove the required attribute
from the objective textarea so the client matches the backend, or change the
server-side validation to require objective as well (make the API check require
title && objective instead of title && (description || objective)); update
whichever side you choose (client: remove required on the objective textarea;
server: tighten validation in the route that performs title && (description ||
objective)) so both client and API behavior are consistent.
In `@src/lib/ai.ts`:
- Around line 55-101: The inner catch currently throws on per-attempt failures
which aborts the outer models loop; instead, capture the error into
lastGeminiError and skip to the next model so fallback models are tried. In the
catch inside the for (let attempt...) block, replace the throw with logic that
sets lastGeminiError (e.g., stringify error or await response?.text()), breaks
out of the attempt loop, and then continue the outer for (const model of models)
loop (or use a flag/label) so generateFromGemini will try the next model from
models; keep clearing the timeoutId and preserving the response checks that
follow.
In `@src/lib/dataStore.ts`:
- Around line 222-252: ensureStructuredTaskDescription currently returns a
preset playbook unconditionally when presetTaskPlaybook(title, "") finds a
match, which overwrites users' already-structured descriptions; change the logic
in ensureStructuredTaskDescription so it first checks if the provided
description is already structured (use the same
/Objective:|Steps:|Verification:|If blocked:/i test) and returns the raw
description unchanged when structured, and only apply presetTaskPlaybook(title,
"") when the description is empty/unstructured; update the function (referencing
ensureStructuredTaskDescription and presetTaskPlaybook) so normalizeTask callers
get preserved user content instead of being silently replaced by the preset.
In `@src/lib/lessonRenderStore.ts`:
- Around line 51-58: In getLessonRenderJob, don't swallow all errors—only treat
ENOENT as "missing"; change the catch to inspect the thrown error (e.g.,
err.code === "ENOENT") and return null in that case, but rethrow or log and
rethrow other errors (JSON.parse failures, permission errors, etc.) so callers
can return 500s and on-disk corruption isn't hidden; reference
getLessonRenderJob, jobPath and LessonRenderJob when locating the read/parse
block to implement this conditional error handling.
- Around line 20-34: jobId is interpolated directly into paths in jobPath,
renderOutputPath, and renderAssetDir which allows path traversal from URL
params; fix by validating or normalizing jobId before building paths: enforce
the expected pattern (e.g. ^lesson-\d+-[a-z0-9]+$ matching makeJobId) in a
shared helper (e.g. validateJobId) used by
jobPath/renderOutputPath/renderAssetDir (and the callers in route handlers like
src/app/api/lesson/render/[jobId]/route.ts and .../video/route.ts), or resolve
the final path and assert it is inside the intended root (reject/throw if
path.resolve(ROOT, ...) !== pathNormalized or !pathNormalized.startsWith(ROOT));
ensure any invalid jobId results in an error response rather than reading
arbitrary files.
In `@src/lib/lessonVideoRenderer.ts`:
- Around line 224-244: The ElevenLabs fetch in lessonVideoRenderer.ts currently
has no timeout and can hang; fix it by using an AbortController: create an
AbortController before calling fetch, set a timer (e.g., 30s) to call
controller.abort(), pass controller.signal into the fetch options where voiceId,
elevenLabsKey, modelId and body are used, and clear the timer after the fetch
completes; also catch an abort (AbortError) from the fetch/response handling and
throw a clear error so the render job transitions to failed instead of hanging.
- Around line 132-145: The drawtext filter built in lessonVideoRenderer.ts
(inside the useDrawtext branch where relativeTextPath is derived from textPath
and the drawtext array is constructed) must include expansion=none to prevent
ffmpeg from interpreting %{} sequences and backslash escapes in the textfile;
update the drawtext array construction (the variable named drawtext) to add
"expansion=none" as one of the colon-separated filter options so the text is
rendered verbatim.
- Around line 194-211: concatSlideClips fails because inputs mix video-only
files from renderSlideClip with audio-containing files from muxClipWithAudio;
modify the pipeline so every clip has identical streams before calling concat:
in the code path where synthesizeSlideSpeech returns null (or when a clip is
video-only), invoke muxClipWithAudio (or a new helper) to attach a short silent
AAC track (e.g., generate via ffmpeg anullsrc or a precreated silent.aac) to the
video clip so its stream layout matches narrated clips, then pass those
uniformly audio+video files into concatSlideClips; reference functions:
concatSlideClips, renderSlideClip, muxClipWithAudio, synthesizeSlideSpeech.
---
Nitpick comments:
In `@README.md`:
- Around line 48-59: Update README.md to mention ffmpeg install commands for
Linux and Windows in addition to macOS: add brief instructions or examples such
as apt-based install for Debian/Ubuntu and Chocolatey for Windows so non-macOS
contributors know how to install ffmpeg; ensure the text still references the
MP4 export requirement and the existing endpoints (POST /api/lesson/render, GET
/api/lesson/render/:jobId, GET /api/lesson/render/:jobId/video) and the storage
location .runbook-data/lesson-renders/ (which corresponds to renderOutputPath in
src/lib/lessonRenderStore.ts).
In `@src/app/api/tasks/route.ts`:
- Around line 32-40: The current normalization preserves user-supplied numeric
prefixes (using /^\d+\./) which can leave steps out-of-order; update the
normalizedSteps logic so every line is renumbered sequentially: in the chain
that builds normalizedSteps (operating on steps), strip any existing leading
numeric prefix from each line (e.g., remove /^\d+\.\s*/), then always prepend
`${idx + 1}. ` to the cleaned line before joining; reference the variables
normalizedSteps, steps and parts to locate and replace the existing map() that
conditionally keeps /^\d+\./.
In `@src/app/dashboard/page.tsx`:
- Around line 361-381: When handling the fetch response, don't cast the error
branch response to LessonRenderJob; instead parse the JSON into a narrower error
shape (e.g. { error?: string }) when !res.ok, use that error message to populate
setRenderJob and setLessonUiError, and only in the success branch
validate/narrow the parsed body before calling setRenderJob(data as
LessonRenderJob). Update the code around res.json() usage so the error path uses
a separate variable/type for the {error} payload and the success path performs a
runtime shape check (or optional property checks) on the parsed body before
treating it as a LessonRenderJob in setRenderJob.
- Around line 433-449: In the useEffect cleanup (the useEffect shown) make the
cleanup null-safe by, after clearing the interval (pollTimerRef.current) and
cancelling speechSynthesis, pausing and resetting narrationAudioRef.current and
then setting narrationAudioRef.current = null, and after
URL.revokeObjectURL(narrationObjectUrlRef.current) set
narrationObjectUrlRef.current = null; keep the empty dependency array as-is but
ensure you reference the existing refs (pollTimerRef, narrationAudioRef,
narrationObjectUrlRef) so later code won’t encounter stale non-null references.
In `@src/components/ui/ChatMessageBody.tsx`:
- Around line 30-38: The prop type for ChatMessageBody is currently too broad
(text: unknown) and the normalization with normalizedText can produce "[object
Object]" for arrays of objects; change the public prop signature on
ChatMessageBody to a narrower union (e.g. string | string[] | null | undefined)
and update the normalization logic to handle array entries safely by mapping
items to strings (using JSON.stringify for objects, otherwise String(item))
before joining, leaving role and blocks logic unchanged.
In `@src/lib/dataStore.ts`:
- Around line 110-220: presetTaskPlaybook uses broad substring checks
(t.includes("github"), etc.) that match unrelated titles; replace each
t.includes(...) branch with stricter intent+keyword regex checks or gate by
canonical sourceTitle seeds: update the function presetTaskPlaybook to use
patterns like /\b(setup|set
up|enable|install|complete|onboard|join|get|configure).*\b(github)\b/i (and
analogous patterns for security, slack, hr, environment) or require sourceTitle
to equal known seeds before returning a preset, and keep the same return
payloads for the matching branches (preserve the existing blocks for "hr
profile", "slack", "github", "local dev"/"environment", "security").
In `@src/lib/lessonRenderStore.ts`:
- Around line 24-26: Replace the collision-prone makeJobId implementation with a
UUID-based id by using crypto.randomUUID() inside makeJobId (e.g., return
crypto.randomUUID()); also update the JOB_ID_RE used for validating path params
to accept UUID v4 format (adjust the regex to match typical UUID hex groups with
hyphens) so routes and path-traversal checks remain correct when job IDs are
UUIDs.
- Around line 60-73: The updateLessonRenderJob function performs a non-atomic
read-modify-write which can be corrupted by concurrent updates or crashes;
change updateLessonRenderJob to write to a temporary file (e.g., jobPath(jobId)
+ ".tmp" or use a unique temp name), fs.writeFile the JSON there, fsync the temp
file if available, then atomically replace the original with fs.rename (or
fs.promises.rename) and finally return the updated object; keep
getLessonRenderJob and jobPath names as-is and ensure error handling cleans up
the temp file on failure to avoid leaving partial/truncated JSON files.
In `@src/lib/lessonVideoRenderer.ts`:
- Around line 79-86: The commandExists helper currently uses the non-portable
"which"; update commandExists (or remove it and inline its use) to probe the
actual consumer by attempting to run "say" directly: replace await
execFileAsync("which", [command]) with a direct probe like await
execFileAsync("say", ["-v", "?"]) (or change callers to try execFileAsync("say",
["-v", "?"]) and handle failure), ensuring you import/keep execFileAsync usage
and update the function name/usage accordingly so detection works on macOS
without relying on "which".
- Around line 311-343: renderLessonVideo currently renders slides serially and
never cleans up per-slide artifacts on success; update it to run per-slide work
in parallel with a bounded concurrency (e.g., use a p-limit or a worker pool) so
the per-slide operations (writeSlideTextFile, renderSlideClip,
synthesizeSlideSpeech, muxClipWithAudio) for each index i can run concurrently
but still write to distinct files in assetDir, then collect resulting clip paths
into clips; also wrap the orchestration (the per-slide processing +
concatSlideClips call) in a try/finally so that after concatSlideClips succeeds
you remove the assetDir (renderAssetDir/job-specific folder) but on error you
leave the directory intact for debugging. Ensure you keep existing behavior when
audio is missing (push baseClipPath) and preserve the existing clips.length
guard in renderLessonVideo.
- Around line 213-276: synthesizeSlideSpeech leaves intermediate files
(speech-N.mp3 / .aiff) behind; after successfully writing the WAV, delete the
intermediate file(s) (use fs.unlink or fs.rm wrapped in try/catch) in
synthesizeSlideSpeech (references: synthesizeSlideSpeech, fs.writeFile,
execFileAsync, commandExists) so mp3/aiff are removed whether ElevenLabs or
local "say" path was used; additionally add a short job-completion cleanup
routine that runs once the final outputPath exists to sweep any remaining
speech-*.mp3/.aiff in assetDir (handle and log unlink errors, avoid throwing) to
prevent disk accumulation.
In `@src/lib/prompts.ts`:
- Around line 30-46: The prompt schema in src/lib/prompts.ts says confidence,
limitedSources, and sourcesUsed are required but the TypeScript Lesson type
(Lesson in src/lib/types.ts) marks them optional; pick one resolution: either
make those fields required on the Lesson type (remove the ? for confidence,
limitedSources, sourcesUsed) and add runtime validation in /api/lesson/route.ts
to assert presence after parsing, or update the prompt schema in
src/lib/prompts.ts to mark "confidence"?, "limitedSources"?, and "sourcesUsed"?
as optional so the model may omit them; update any consumers such as
src/app/dashboard/page.tsx to match the chosen contract (remove defensive checks
if making required or keep them if optional). Ensure the chosen change is
consistent across Lesson, the prompt schema, and the API validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0e735c7-c524-4f8d-abc2-b5bea1cc0c12
📒 Files selected for processing (20)
.env.exampleREADME.mdsrc/app/api/chat/route.tssrc/app/api/lesson/render/[jobId]/route.tssrc/app/api/lesson/render/[jobId]/video/route.tssrc/app/api/lesson/render/route.tssrc/app/api/lesson/route.tssrc/app/api/tasks/route.tssrc/app/api/tts/route.tssrc/app/dashboard/page.tsxsrc/app/layout.tsxsrc/app/manager/tasks/page.tsxsrc/components/ui/ChatMessageBody.tsxsrc/lib/ai.tssrc/lib/dataStore.tssrc/lib/demoTasks.tssrc/lib/lessonRenderStore.tssrc/lib/lessonVideoRenderer.tssrc/lib/prompts.tssrc/lib/types.ts
Address CodeRabbit findings by securing render/tts endpoints, validating and atomically storing render jobs, adding range-based video streaming, and tightening dashboard/task normalization behavior. Made-with: Cursor
PR #14 referenced buildLessonContext, normalizeLesson, and related helpers after a merge dropped their definitions. Restore the complete lesson API implementation so TypeScript and CI builds succeed. Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…nc video export
Make lesson generation grounded in multi-source retrieval, add richer lesson schema/citations, and expand dashboard with question input, presenter narration, and MP4 export polling. Add async lesson render APIs with ffmpeg-backed pipeline plus local render job store and docs updates.
Made-with: Cursor
Summary by CodeRabbit
Release Notes
New Features
Documentation