feat: implement video length selection in TopicVideoGenerator#19
feat: implement video length selection in TopicVideoGenerator#19
Conversation
- Added a dialog for users to select video length before generation. - Updated the generateVideoScript function to accept a length parameter. - Modified the script generation logic to adjust based on selected length. - Enhanced database schema to ensure unique subtopic names per parent. - Added a batch script for deleting and regenerating the database and Android build. - Updated EmotionDetector to ensure proper emotion handling. - Improved logging for video generation processes.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds video-length selection end-to-end (UI → agent → Gemini) with dynamic script timing, large DB schema/migration bundle and guarded DB init, subtopic deduplication with ordered inserts and retries, platform-aware MediaPipe face-landmarks loading with asset plugin and setup guide, and assorted build, script, and config updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as TopicVideoGenerator
participant Agent as VideoAgent
participant Gemini as GeminiService
participant External as GeminiAPI
User->>UI: Click "Generate Video"
UI->>UI: Show length-selection dialog
User->>UI: Select duration (e.g., 60s)
UI->>Agent: request generateTopicVideo(topic, subtopics, lengthInSeconds=60)
Agent->>Agent: save lengthInSeconds in state
Agent->>Gemini: generateVideoScript(topic, context, subtopics, tone, lengthInSeconds=60)
Gemini->>Gemini: compute durations & target word counts
Gemini->>External: send prompt with dynamic section durations
External-->>Gemini: return script
Gemini-->>Agent: return script + metadata
Agent-->>UI: deliver script
UI-->>User: display generated script
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/queries/topics.ts (1)
119-151: Consider updating parent topic metadata even when no new subtopics are created.
The early return skips the metadata update block, sobestPractices/commonPitfalls/whyLearn/difficultywon’t refresh on subsequent runs. If this function can be called for regeneration or web‑search refreshes, move the metadata update before the early return or update it unconditionally.components/topic/TopicVideoGenerator.tsx (1)
241-260: Use dynamic script length in status message and update dependency arrayThe
selectedLengthvariable is actively used throughout the component (with options from 10s to 5m), but the status message still hardcodes "Generating 2-second script..." which is misleading. IncludeselectedLengthin both the message template and the useCallback dependency array.Suggested fix
- case 'generating-script': - return 'Generating 2-second script...'; + case 'generating-script': + return `Generating ${selectedLength}s script...`; ... - }, [videoStatus, downloadProgress, isLocalVideo, videoError]); + }, [videoStatus, downloadProgress, isLocalVideo, videoError, selectedLength]);
🤖 Fix all issues with AI agents
In `@components/topic/TopicVideoGenerator.tsx`:
- Around line 379-466: The length selector currently uses Pressable buttons but
should use radio semantics: change the container wrapping the options (the View
elements that hold the Pressable groups) to have accessibilityRole="radiogroup"
and for each option (the Pressable that calls setSelectedLength and reads
selectedLength) set accessibilityRole="radio" and accessibilityState={{ checked:
selectedLength === <value> }} (and update accessibilityLabel/accessible if
needed) so screen readers treat them as mutually-exclusive radios; update all
Pressable instances that setSelectedLength (and any parent View that groups
them) to apply these attributes consistently.
In `@db/schema.ts`:
- Around line 106-110: The new unique index
uniqueIndex("subtopics_parent_name_idx") on (parentTopicId, name) will fail if
duplicates exist; before applying the migration, write a data-cleanup migration
that (1) queries the subtopics table for duplicate groups by parentTopicId and
name, (2) resolves each group by either merging records or choosing a canonical
row and updating references, or renaming duplicates (e.g., append suffix) so
names become unique, and (3) deletes or updates the non-canonical rows and any
related foreign keys; run that cleanup migration (or backfill script) prior to
adding uniqueIndex("subtopics_parent_name_idx") in db/schema.ts to ensure the
unique constraint can be created successfully.
In `@deleting_migration_deleting_android.bat`:
- Around line 1-31: The batch script uses POSIX commands (rm, clear, sleep,
touch, mkdir -p, cp) which will fail in Windows cmd.exe; either rename this file
to a .sh and document it for bash/Git Bash usage, or convert the commands to
Windows cmd equivalents (use del /f, cls, timeout /t, type NUL > for creating
files, rmdir /s /q for folders, mkdir, copy) while preserving the same sequence
(remove public/database.sqlite, recreate it, remove db/migrations, run bun
db:generate and bun db:migrate, remove android, run bun expo prebuild --platform
android, create android/app/src/main/assets, copy face_landmarker.task there,
then bun run android); update the filename extension and any CI/dev docs
accordingly so callers use the correct shell.
In `@EMOTION_DETECTION_SETUP.md`:
- Around line 70-76: Several fenced code blocks in EMOTION_DETECTION_SETUP.md
are missing language identifiers (MD040); update each fence to include an
appropriate language specifier — use "text" for output-only blocks like the
example output (the block containing "✔ Created native directory...") and "bash"
(or other appropriate shell) for command blocks referenced elsewhere (e.g., the
blocks at the ranges noted). Locate the offending fences by searching for
triple-backtick blocks with no language and add the language token immediately
after the opening ```; apply the same change to the other instances mentioned
(around the other ranges/blocks) to satisfy markdownlint.
- Around line 64-68: The phrase "Expo managed" in the documentation text should
be hyphenated as "Expo-managed"; update the sentence that currently reads "Expo
managed projects don't have native `android/` folders by default." to
"Expo-managed projects don't have native `android/` folders by default." and
apply the same hyphenation to any other occurrences of "Expo managed" (e.g.,
within the paragraph describing generating native Android project structure,
applying plugins like `app.config.ts`, linking expo modules and creating the
gradle build system) to keep terminology consistent.
In `@server/queries/topics.ts`:
- Around line 99-101: The code currently uses existingSubtopics.length to assign
new subtopic order which can collide if rows were deleted or orders are
non‑contiguous; instead query the maximum order value from the subtopics table
within the same transaction (using tx and the subtopics model) and set the new
subtopic's order to (maxOrder ?? -1) + 1 (or 0 when no rows), updating the logic
around existingSubtopics and any other spot in this file (the second occurrence
around lines handling insertion at 160-178) to derive order from the max
aggregate rather than the array length.
- Around line 104-117: The current filter only checks against DB existingNames
and doesn't dedupe duplicate titles inside the same explanation.subtopics batch;
update the logic in server/queries/topics.ts (around existingNames,
newSubtopicsToCreate, explanation.subtopics, and normalizedName) to maintain a
single seen set seeded with existingNames and, as you iterate
explanation.subtopics, skip and log any subtopic whose normalized title is
already in seen, otherwise add it to seen and include it in newSubtopicsToCreate
so duplicates in the same AI payload are filtered out before inserting.
🧹 Nitpick comments (1)
server/agents/VideoGen.ts (1)
147-174: Add guardrails forlengthInSeconds.
Right now a non‑finite/negative or extremely large value could produce invalid prompts or excessive cost. Consider validating and capping to a product‑approved range.🔧 Suggested guardrails (adjust limits to match UI options)
if (!apiKey || !avatarId || !voiceId) { const error = 'Missing required HeyGen configuration (API key, avatar ID, or voice ID)'; console.error('❌', error); throw new Error(error); } + + if (!Number.isFinite(lengthInSeconds) || lengthInSeconds <= 0) { + throw new Error('lengthInSeconds must be a positive number'); + } + const MAX_LENGTH_SECONDS = 600; // align with UI / product limits + if (lengthInSeconds > MAX_LENGTH_SECONDS) { + throw new Error(`lengthInSeconds exceeds max allowed (${MAX_LENGTH_SECONDS}s)`); + }
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/queries/topics.ts (1)
243-244: Comment doesn't match code behavior.The comment states "By default, return only 'original' subtopics (not websearch)" but the code returns all results without filtering. Either update the comment to reflect actual behavior, or add the filtering logic if original-only is the intended default.
// By default, return only 'original' subtopics (not websearch) - return result; + return result.filter(st => { + const meta = (st.metadata as Record<string, any>) || {}; + return (meta.source || 'original') === 'original'; + });Or if returning all is intended:
- // By default, return only 'original' subtopics (not websearch) + // Return all subtopics when no source filter is specified return result;components/topic/TopicVideoGenerator.tsx (1)
241-247: Update the status copy now that lengths vary.The message still says “Generating 2-second script…”, which is misleading with 10s–5m presets.
✏️ Suggested tweak
- case 'generating-script': - return 'Generating 2-second script...'; + case 'generating-script': + return 'Generating script...';
🤖 Fix all issues with AI agents
In `@deleting_migration_deleting_android.sh`:
- Around line 1-5: Add a bash shebang to the top of the script so it can be
executed directly: insert "#!/usr/bin/env bash" as the very first line of
deleting_migration_deleting_android.sh (above the existing
rm/clear/echo/sleep/touch commands) to satisfy Shellcheck SC2148 and ensure the
script runs with bash when invoked as ./deleting_migration_deleting_android.sh.
In `@server/queries/topics.ts`:
- Around line 162-168: The current logic in createSubtopics queries
max(subtopics.order) via maxOrderResult and computes startOrder, which can race
when concurrent transactions insert for the same parentTopicId and produce
duplicate order values; to fix, switch to a DB-side sequence or auto-increment
for ordering (use a sequence and fetch NEXTVAL in the insert), or add a unique
constraint on (parentTopicId, order) and implement retry-on-conflict around the
insert loop, or perform the order assignment inside a single serialized
transaction/SELECT FOR UPDATE on the parentTopic row to serialize concurrent
creators; update references in createSubtopics, the maxOrderResult/startOrder
calculation and tx insert logic to use the chosen approach so ordering is
assigned atomically.
♻️ Duplicate comments (1)
components/topic/TopicVideoGenerator.tsx (1)
390-493: Use a singleradiogroupwith a label for all length options.Each row is currently marked as its own
radiogroup, which can make screen readers announce them as separate groups. Wrap all options in oneradiogroupcontainer (with an accessible label), and keep rows as layout-only containers.♿ Suggested structure
- <View className="flex-row gap-3" accessibilityRole="radiogroup"> + <View accessibilityRole="radiogroup" accessibilityLabel="Video length" className="gap-3"> + <View className="flex-row gap-3"> <Pressable onPress={() => setSelectedLength(10)} className={`flex-1 p-3 rounded-lg border-2 ${selectedLength === 10 ? 'border-primary bg-primary/10' : 'border-border bg-card'}`} accessibilityRole="radio" accessibilityState={{ checked: selectedLength === 10 }} accessibilityLabel="10 seconds, ultra short" > <Text className={`font-semibold text-center ${selectedLength === 10 ? 'text-primary' : 'text-foreground'}`}>10s</Text> <Text className="text-xs text-muted-foreground text-center mt-0.5">Ultra short</Text> </Pressable> - </View> + </View> - <View className="flex-row gap-3" accessibilityRole="radiogroup"> + <View className="flex-row gap-3"> ... - </View> + </View> - <View className="flex-row gap-3" accessibilityRole="radiogroup"> + <View className="flex-row gap-3"> ... - </View> + </View> + </View>React Native 0.81 accessibilityRole radiogroup radio accessibilityState checked
| rm public/database.sqlite | ||
| clear | ||
| echo "file removed" | ||
| sleep 2 | ||
| touch public/database.sqlite |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the script declares an interpreter
fd -a '^deleting_migration_deleting_android\.sh$' -x head -n 5 {}Repository: TharunCodes07/SkillSpark
Length of output: 156
Add a shebang so the script is directly executable.
Without a shebang, running this script via ./deleting_migration_deleting_android.sh will fail and Shellcheck flags it as SC2148. Add #!/usr/bin/env bash at the top.
🔧 Proposed fix
+#!/usr/bin/env bash
+
rm public/database.sqlite📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm public/database.sqlite | |
| clear | |
| echo "file removed" | |
| sleep 2 | |
| touch public/database.sqlite | |
| #!/usr/bin/env bash | |
| rm public/database.sqlite | |
| clear | |
| echo "file removed" | |
| sleep 2 | |
| touch public/database.sqlite |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🤖 Prompt for AI Agents
In `@deleting_migration_deleting_android.sh` around lines 1 - 5, Add a bash
shebang to the top of the script so it can be executed directly: insert
"#!/usr/bin/env bash" as the very first line of
deleting_migration_deleting_android.sh (above the existing
rm/clear/echo/sleep/touch commands) to satisfy Shellcheck SC2148 and ensure the
script runs with bash when invoked as ./deleting_migration_deleting_android.sh.
suganth07
force-pushed
the
feat/Emotion-Video
branch
from
c06fa67 to
68d0bab
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/topic/TopicVideoGenerator.tsx (1)
241-260: Status message is hardcoded and misleading.Line 246 displays "Generating 2-second script..." but the video length is now configurable (10s to 5m). The message should reflect the user's actual selection.
Consider storing the selected length when generation starts and using it in the status message:
+ const [generatingLength, setGeneratingLength] = useState<number | null>(null);Then in
handleGenerateVideo:+ setGeneratingLength(lengthInSeconds); setVideoStatus('generating-script');And update the message:
case 'generating-script': - return 'Generating 2-second script...'; + return `Generating ${generatingLength}s script...`;
🤖 Fix all issues with AI agents
In `@components/topic/TopicVideoGenerator.tsx`:
- Around line 390-493: The current implementation places
accessibilityRole="radiogroup" on each row View, creating three separate radio
groups; change to a single radiogroup by moving accessibilityRole="radiogroup"
to the outer container that wraps all Pressable options (the Views containing
the Pressable items) and remove accessibilityRole="radiogroup" from the inner
row Views; keep each Pressable's accessibilityRole="radio", accessibilityState
using selectedLength, and onPress handlers (setSelectedLength) so all nine
options remain mutually exclusive but are announced as one coherent radio group.
In `@server/queries/topics.ts`:
- Around line 162-221: The UNIQUE-constraint retry never triggers in
insertSubtopicWithRetry because the error check looks for camelCase
"parentTopicId" while SQLite reports snake_case column names; update the
isOrderConflict detection in that catch block (within insertSubtopicWithRetry)
to match the actual SQLite message—e.g., normalize errorMessage to lowercase and
check for 'unique constraint failed' plus both 'subtopics.order' and the
snake_case 'subtopics.parent_topic_id' (or use a regex like /parent[_ ]topic[_
]id/), or alternatively detect by SQLite error code if available from tx—so
order conflicts are correctly recognized and retried.
♻️ Duplicate comments (2)
deleting_migration_deleting_android.sh (1)
1-1: Add a bash shebang so the script runs via./....Without a shebang, direct execution can fail or default to
/bin/sh.🔧 Proposed fix
+#!/usr/bin/env bash +db/schema.ts (1)
107-110: Plan a cleanup before applying the new unique indexes.If duplicates already exist for
(parentTopicId, name)or(parentTopicId, order), the migration will fail. A pre-migration dedupe/backfill is needed.
🧹 Nitpick comments (3)
components/topic/TopicVideoGenerator.tsx (1)
10-11: Remove unused imports.
LabelandDialogFooterare imported but never used in this file.-import { Dialog, DialogContent, DialogHeader, DialogTitle, DialogFooter } from '@/components/ui/dialog'; -import { Label } from '@/components/ui/label'; +import { Dialog, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/dialog';deleting_migration_deleting_android.sh (1)
1-9: Make the script fail fast and avoid noisy deletes.Consider
set -euo pipefail(after the shebang) andrm -fso missing files don’t break once-eis enabled.🔧 Proposed fix
+set -euo pipefail -rm public/database.sqlite +rm -f public/database.sqlitedb/schema.ts (1)
107-109: Unique index already covers(parentTopicId, order).The new unique index provides the same lookup path as the non‑unique index, so keeping both adds write overhead. Consider removing the redundant index unless you need it for a different collation or partial index.
♻️ Proposed change
- index("subtopics_order_idx").on(t.parentTopicId, t.order),
| <View className="flex-row gap-3" accessibilityRole="radiogroup"> | ||
| <Pressable | ||
| onPress={() => setSelectedLength(10)} | ||
| className={`flex-1 p-3 rounded-lg border-2 ${selectedLength === 10 ? 'border-primary bg-primary/10' : 'border-border bg-card'}`} | ||
| accessibilityRole="radio" | ||
| accessibilityState={{ checked: selectedLength === 10 }} | ||
| accessibilityLabel="10 seconds, ultra short" | ||
| > | ||
| <Text className={`font-semibold text-center ${selectedLength === 10 ? 'text-primary' : 'text-foreground'}`}>10s</Text> | ||
| <Text className="text-xs text-muted-foreground text-center mt-0.5">Ultra short</Text> | ||
| </Pressable> | ||
|
|
||
| <Pressable | ||
| onPress={() => setSelectedLength(30)} | ||
| className={`flex-1 p-3 rounded-lg border-2 ${selectedLength === 30 ? 'border-primary bg-primary/10' : 'border-border bg-card'}`} | ||
| accessibilityRole="radio" | ||
| accessibilityState={{ checked: selectedLength === 30 }} | ||
| accessibilityLabel="30 seconds, quick" | ||
| > | ||
| <Text className={`font-semibold text-center ${selectedLength === 30 ? 'text-primary' : 'text-foreground'}`}>30s</Text> | ||
| <Text className="text-xs text-muted-foreground text-center mt-0.5">Quick</Text> | ||
| </Pressable> | ||
|
|
||
| <Pressable | ||
| onPress={() => setSelectedLength(45)} | ||
| className={`flex-1 p-3 rounded-lg border-2 ${selectedLength === 45 ? 'border-primary bg-primary/10' : 'border-border bg-card'}`} | ||
| accessibilityRole="radio" | ||
| accessibilityState={{ checked: selectedLength === 45 }} | ||
| accessibilityLabel="45 seconds, brief" | ||
| > | ||
| <Text className={`font-semibold text-center ${selectedLength === 45 ? 'text-primary' : 'text-foreground'}`}>45s</Text> | ||
| <Text className="text-xs text-muted-foreground text-center mt-0.5">Brief</Text> | ||
| </Pressable> | ||
| </View> | ||
|
|
||
| <View className="flex-row gap-3" accessibilityRole="radiogroup"> | ||
| <Pressable | ||
| onPress={() => setSelectedLength(60)} | ||
| className={`flex-1 p-3 rounded-lg border-2 ${selectedLength === 60 ? 'border-primary bg-primary/10' : 'border-border bg-card'}`} | ||
| accessibilityRole="radio" | ||
| accessibilityState={{ checked: selectedLength === 60 }} | ||
| accessibilityLabel="1 minute, short" | ||
| > | ||
| <Text className={`font-semibold text-center ${selectedLength === 60 ? 'text-primary' : 'text-foreground'}`}>1m</Text> | ||
| <Text className="text-xs text-muted-foreground text-center mt-0.5">Short</Text> | ||
| </Pressable> | ||
|
|
||
| <Pressable | ||
| onPress={() => setSelectedLength(90)} | ||
| className={`flex-1 p-3 rounded-lg border-2 ${selectedLength === 90 ? 'border-primary bg-primary/10' : 'border-border bg-card'}`} | ||
| accessibilityRole="radio" | ||
| accessibilityState={{ checked: selectedLength === 90 }} | ||
| accessibilityLabel="1.5 minutes, concise" | ||
| > | ||
| <Text className={`font-semibold text-center ${selectedLength === 90 ? 'text-primary' : 'text-foreground'}`}>1.5m</Text> | ||
| <Text className="text-xs text-muted-foreground text-center mt-0.5">Concise</Text> | ||
| </Pressable> | ||
|
|
||
| <Pressable | ||
| onPress={() => setSelectedLength(120)} | ||
| className={`flex-1 p-3 rounded-lg border-2 ${selectedLength === 120 ? 'border-primary bg-primary/10' : 'border-border bg-card'}`} | ||
| accessibilityRole="radio" | ||
| accessibilityState={{ checked: selectedLength === 120 }} | ||
| accessibilityLabel="2 minutes, medium" | ||
| > | ||
| <Text className={`font-semibold text-center ${selectedLength === 120 ? 'text-primary' : 'text-foreground'}`}>2m</Text> | ||
| <Text className="text-xs text-muted-foreground text-center mt-0.5">Medium</Text> | ||
| </Pressable> | ||
| </View> | ||
|
|
||
| <View className="flex-row gap-3" accessibilityRole="radiogroup"> | ||
| <Pressable | ||
| onPress={() => setSelectedLength(180)} | ||
| className={`flex-1 p-3 rounded-lg border-2 ${selectedLength === 180 ? 'border-primary bg-primary/10' : 'border-border bg-card'}`} | ||
| accessibilityRole="radio" | ||
| accessibilityState={{ checked: selectedLength === 180 }} | ||
| accessibilityLabel="3 minutes, long" | ||
| > | ||
| <Text className={`font-semibold text-center ${selectedLength === 180 ? 'text-primary' : 'text-foreground'}`}>3m</Text> | ||
| <Text className="text-xs text-muted-foreground text-center mt-0.5">Long</Text> | ||
| </Pressable> | ||
|
|
||
| <Pressable | ||
| onPress={() => setSelectedLength(240)} | ||
| className={`flex-1 p-3 rounded-lg border-2 ${selectedLength === 240 ? 'border-primary bg-primary/10' : 'border-border bg-card'}`} | ||
| accessibilityRole="radio" | ||
| accessibilityState={{ checked: selectedLength === 240 }} | ||
| accessibilityLabel="4 minutes, extended" | ||
| > | ||
| <Text className={`font-semibold text-center ${selectedLength === 240 ? 'text-primary' : 'text-foreground'}`}>4m</Text> | ||
| <Text className="text-xs text-muted-foreground text-center mt-0.5">Extended</Text> | ||
| </Pressable> | ||
|
|
||
| <Pressable | ||
| onPress={() => setSelectedLength(300)} | ||
| className={`flex-1 p-3 rounded-lg border-2 ${selectedLength === 300 ? 'border-primary bg-primary/10' : 'border-border bg-card'}`} | ||
| accessibilityRole="radio" | ||
| accessibilityState={{ checked: selectedLength === 300 }} | ||
| accessibilityLabel="5 minutes, full" | ||
| > | ||
| <Text className={`font-semibold text-center ${selectedLength === 300 ? 'text-primary' : 'text-foreground'}`}>5m</Text> | ||
| <Text className="text-xs text-muted-foreground text-center mt-0.5">Full</Text> | ||
| </Pressable> | ||
| </View> |
There was a problem hiding this comment.
Multiple radiogroups break screen reader semantics.
Each row has its own accessibilityRole="radiogroup" (lines 390, 425, 460), creating three separate radio groups instead of one. Screen readers will announce these as independent groups, confusing users since all 9 options are mutually exclusive.
Wrap all options in a single radiogroup:
+ <View accessibilityRole="radiogroup" accessibilityLabel="Video length options">
- <View className="flex-row gap-3" accessibilityRole="radiogroup">
+ <View className="flex-row gap-3">
{/* ... first row options ... */}
</View>
- <View className="flex-row gap-3" accessibilityRole="radiogroup">
+ <View className="flex-row gap-3">
{/* ... second row options ... */}
</View>
- <View className="flex-row gap-3" accessibilityRole="radiogroup">
+ <View className="flex-row gap-3">
{/* ... third row options ... */}
</View>
+ </View>🤖 Prompt for AI Agents
In `@components/topic/TopicVideoGenerator.tsx` around lines 390 - 493, The current
implementation places accessibilityRole="radiogroup" on each row View, creating
three separate radio groups; change to a single radiogroup by moving
accessibilityRole="radiogroup" to the outer container that wraps all Pressable
options (the Views containing the Pressable items) and remove
accessibilityRole="radiogroup" from the inner row Views; keep each Pressable's
accessibilityRole="radio", accessibilityState using selectedLength, and onPress
handlers (setSelectedLength) so all nine options remain mutually exclusive but
are announced as one coherent radio group.
| // Helper function to insert subtopic with atomic order assignment and retry on conflict | ||
| const insertSubtopicWithRetry = async (subtopic: typeof newSubtopicsToCreate[0], maxRetries = 5): Promise<void> => { | ||
| const subtopicId = createId(); | ||
|
|
||
| console.log(`🗄️ [DB] Inserting subtopic ${i + 1}/${explanation.subtopics.length}: "${subtopic.title}"`); | ||
| for (let attempt = 0; attempt < maxRetries; attempt++) { | ||
| try { | ||
| // Query max order atomically within each attempt to avoid race conditions | ||
| const maxOrderResult = await tx | ||
| .select({ maxOrder: max(subtopics.order) }) | ||
| .from(subtopics) | ||
| .where(eq(subtopics.parentTopicId, parentTopicId)); | ||
|
|
||
| const nextOrder = (maxOrderResult[0]?.maxOrder ?? -1) + 1; | ||
|
|
||
| // Attempt insert with the calculated order | ||
| await tx.insert(subtopics).values({ | ||
| id: subtopicId, | ||
| parentTopicId: parentTopicId, | ||
| name: subtopic.title, | ||
| contentDefault: subtopic.explanationDefault, | ||
| contentSimplified: subtopic.explanationSimplified, | ||
| contentStory: subtopic.explanationStory, | ||
| order: nextOrder, | ||
| metadata: JSON.stringify({ | ||
| source: source, | ||
| example: subtopic.example, | ||
| exampleExplanation: subtopic.exampleExplanation, | ||
| exampleSimplified: subtopic.exampleSimplified, | ||
| exampleStory: subtopic.exampleStory, | ||
| keyPoints: subtopic.keyPoints | ||
| }) | ||
| }); | ||
|
|
||
| console.log(`✅ [DB] Created subtopic with 3 content types: ${subtopic.title} (order: ${nextOrder}, source: ${source})`); | ||
| return; // Success, exit retry loop | ||
|
|
||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | ||
|
|
||
| // Check if error is due to unique constraint violation on (parentTopicId, order) | ||
| const isOrderConflict = errorMessage.includes('UNIQUE constraint failed') && | ||
| errorMessage.includes('parentTopicId') && | ||
| errorMessage.includes('order'); | ||
|
|
||
| if (isOrderConflict && attempt < maxRetries - 1) { | ||
| // Retry after a brief delay with exponential backoff | ||
| const delayMs = Math.pow(2, attempt) * 10; // 10ms, 20ms, 40ms, 80ms, 160ms | ||
| console.warn(`⚠️ [DB] Order conflict for "${subtopic.title}", retrying in ${delayMs}ms (attempt ${attempt + 1}/${maxRetries})...`); | ||
| await new Promise(resolve => setTimeout(resolve, delayMs)); | ||
| continue; | ||
| } | ||
|
|
||
| // Either not an order conflict or max retries reached | ||
| console.error(`❌ [DB] Failed to create subtopic ${subtopic.title}:`, error); | ||
| throw error; // Propagate error to outer catch | ||
| } | ||
| } | ||
|
|
||
| throw new Error(`Failed to insert subtopic "${subtopic.title}" after ${maxRetries} attempts`); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the database schema/migration files
find . -type f \( -name "*.ts" -o -name "*.sql" -o -name "*.js" \) | grep -E "(schema|migration|db)" | head -20Repository: TharunCodes07/SkillSpark
Length of output: 134
🏁 Script executed:
# Search for subtopics table definition
rg "subtopics" --type ts --type sql -A 5 -B 1 | head -100Repository: TharunCodes07/SkillSpark
Length of output: 5667
🏁 Script executed:
# Look for drizzle schema definitions
fd -e ts -path "*/schema*" -o -path "*drizzle*"Repository: TharunCodes07/SkillSpark
Length of output: 236
🏁 Script executed:
cat db/schema.tsRepository: TharunCodes07/SkillSpark
Length of output: 14355
🏁 Script executed:
rg "UNIQUE constraint failed" server/ -A 3 -B 3Repository: TharunCodes07/SkillSpark
Length of output: 676
🏁 Script executed:
# Search for other error handling patterns with database operations
rg "error.*message" server/queries/ -A 2 -B 2 | head -50Repository: TharunCodes07/SkillSpark
Length of output: 1173
🌐 Web query:
SQLite UNIQUE constraint failed error message format column name
💡 Result:
SQLite formats the error text as:
-
Single-column UNIQUE/PK violation:
UNIQUE constraint failed: <table>.<column>[1] -
Multi-column (composite) UNIQUE violation:
UNIQUE constraint failed: <table>.<col1>, <table>.<col2>[, ...](comma+space separated; table name repeated for each column) [1]
Retry detection never fires due to snake_case column name mismatch in SQLite error message.
SQLite reports UNIQUE constraint violations as UNIQUE constraint failed: subtopics.parent_topic_id, subtopics.order, but the code checks for camelCase parentTopicId. Since all three conditions are joined with &&, the mismatch causes the condition to always be false, preventing retries entirely.
Proposed fix
- const isOrderConflict = errorMessage.includes('UNIQUE constraint failed') &&
- errorMessage.includes('parentTopicId') &&
- errorMessage.includes('order');
+ const isOrderConflict = errorMessage.includes('UNIQUE constraint failed') &&
+ errorMessage.includes('parent_topic_id') &&
+ errorMessage.includes('order');🤖 Prompt for AI Agents
In `@server/queries/topics.ts` around lines 162 - 221, The UNIQUE-constraint retry
never triggers in insertSubtopicWithRetry because the error check looks for
camelCase "parentTopicId" while SQLite reports snake_case column names; update
the isOrderConflict detection in that catch block (within
insertSubtopicWithRetry) to match the actual SQLite message—e.g., normalize
errorMessage to lowercase and check for 'unique constraint failed' plus both
'subtopics.order' and the snake_case 'subtopics.parent_topic_id' (or use a regex
like /parent[_ ]topic[_ ]id/), or alternatively detect by SQLite error code if
available from tx—so order conflicts are correctly recognized and retried.
…nd strict error handling
suganth07
force-pushed
the
feat/Emotion-Video
branch
from
5140350 to
6a7ab07
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
db/drizzle.ts (1)
58-60: Remove unused exportuseMigrationHelper.This hook is exported from
db/drizzle.tsbut is not imported or used anywhere in the codebase. Theinitializefunction already handles migrations during startup, making this export unnecessary.
🤖 Fix all issues with AI agents
In @.gitignore:
- Around line 44-46: The .gitignore contains redundant/suspicious entries: the
duplicated "android/app/src/main/java/com/expostarter/base/MainActivity.kt" and
" .github/copilot-instructions.md" after an existing "android/" rule; decide
whether you intended to ignore or unignore these files and fix accordingly: if
you meant to keep them ignored, remove the duplicate and the explicit android
path entries (they're already covered by "android/"); if you meant to unignore
them, prefix each path with "!" (e.g.,
"!android/app/src/main/java/com/expostarter/base/MainActivity.kt" and
"!.github/copilot-instructions.md") and remove the duplicate line; ensure no
duplicate lines remain.
- Around line 41-43: The .gitignore currently ignores the entire android/
directory so the negation for !android/app/src/main/assets/face_landmarker.task
is ineffective; update the ignore rules to unignore the path hierarchy leading
to that file (e.g., add negations for android/app/, android/app/src/,
android/app/src/main/, android/app/src/main/assets/ before the specific file
negation) or instead remove the parent android/ ignore and/or use Git LFS or
build-time download for the binary asset if you don't want it committed.
In `@lib/emotion/detectEmotion.ts`:
- Around line 25-44: The early-return in detectEmotion.ts currently returns
emotion: "no_face" when the FaceLandmarks module is missing; change this
sentinel to a distinct value like "unavailable" (or "error") so callers can
differentiate module-unavailable from a true "no_face" detection, keeping
confidence: 0.0 and the same default features shape; update the return in the
FaceLandmarks null-check (the block referencing FaceLandmarks) and any related
tests or callers that expect "no_face" to instead handle the new "unavailable"
value.
🧹 Nitpick comments (5)
app.config.ts (1)
59-62: InconsistentbaseUrlafter rebranding.The app has been renamed to "SkillSpark" throughout the configuration, but
experiments.baseUrlstill references/expo-local-first-template. If this is used for web builds or routing, it should likely be updated to match the new project identity.experiments: { typedRoutes: true, - baseUrl: "/expo-local-first-template", + baseUrl: "/skillspark", },package.json (2)
13-15:build:apkis an exact duplicate ofbuild:android.Both scripts execute identical commands. Consider removing one or differentiating them (e.g., different EAS profiles).
"build:android": "bun run prebuild:copy-assets && eas build --platform android --profile production", "build:android-local": "bun run prebuild:copy-assets && cd android && ./gradlew assembleRelease", - "build:apk": "bun run prebuild:copy-assets && eas build --platform android --profile production",
12-12:prebuild:copy-assetsis not cross-platform.The script uses
mkdir -pandcp, which are Unix commands and will fail on Windows. If Windows support is needed, consider using a cross-platform approach:- "prebuild:copy-assets": "mkdir -p android/app/src/main/assets && cp face_landmarker.task android/app/src/main/assets/", + "prebuild:copy-assets": "node -e \"const fs=require('fs');const p='android/app/src/main/assets';fs.mkdirSync(p,{recursive:true});fs.copyFileSync('face_landmarker.task',p+'/face_landmarker.task')\"",Alternatively, use a tool like
shxorcpy-cli, or document that builds require a Unix environment.db/migrations/meta/0000_snapshot.json (1)
814-837: Redundant indexes onsubtopicstable.There are two indexes on the same columns
(parent_topic_id, order):
subtopics_order_idx(non-unique, lines 814-821)subtopics_parent_order_idx(unique, lines 830-837)The unique index already provides the same lookup capability as the non-unique one. Having both wastes storage and slows down writes.
Consider removing
subtopics_order_idxfrom the schema definition, assubtopics_parent_order_idxalready covers those queries while also enforcing uniqueness.db/migrations/0000_striped_impossible_man.sql (1)
115-118: Redundant index:subtopics_order_idxduplicatessubtopics_parent_order_idx.Both indexes cover
(parent_topic_id, order). The unique index at line 118 already provides the same query optimization as the non-unique index at line 116, while also enforcing uniqueness.Remove
subtopics_order_idxto reduce storage overhead and improve write performance.♻️ Suggested fix
CREATE INDEX `subtopics_parent_idx` ON `subtopics` (`parent_topic_id`);--> statement-breakpoint -CREATE INDEX `subtopics_order_idx` ON `subtopics` (`parent_topic_id`,`order`);--> statement-breakpoint CREATE UNIQUE INDEX `subtopics_parent_name_idx` ON `subtopics` (`parent_topic_id`,`name`);--> statement-breakpoint CREATE UNIQUE INDEX `subtopics_parent_order_idx` ON `subtopics` (`parent_topic_id`,`order`);--> statement-breakpoint
| android/ | ||
| # But keep the model file for emotion detection | ||
| !android/app/src/main/assets/face_landmarker.task |
There was a problem hiding this comment.
Negation pattern is ineffective after ignoring parent directory.
Git ignores the entire android/ directory on line 41, so the negation on line 43 won't work—Git doesn't check children of already-ignored directories. To track a specific file inside an ignored directory, you must unignore the entire path hierarchy:
# Ignore the entire Android directory
android/
# But keep the model file for emotion detection
-!android/app/src/main/assets/face_landmarker.task
+!android/app/
+!android/app/src/
+!android/app/src/main/
+!android/app/src/main/assets/
+!android/app/src/main/assets/face_landmarker.taskAlternatively, consider whether committing this binary asset to the repo is the right approach vs. downloading it during build or using Git LFS.
🤖 Prompt for AI Agents
In @.gitignore around lines 41 - 43, The .gitignore currently ignores the entire
android/ directory so the negation for
!android/app/src/main/assets/face_landmarker.task is ineffective; update the
ignore rules to unignore the path hierarchy leading to that file (e.g., add
negations for android/app/, android/app/src/, android/app/src/main/,
android/app/src/main/assets/ before the specific file negation) or instead
remove the parent android/ ignore and/or use Git LFS or build-time download for
the binary asset if you don't want it committed.
| android/app/src/main/java/com/expostarter/base/MainActivity.kt | ||
| android/app/src/main/java/com/expostarter/base/MainActivity.kt | ||
| .github/copilot-instructions.md |
There was a problem hiding this comment.
Suspicious bare paths without ignore semantics.
Lines 44-46 list paths without a negation (!) or wildcard, yet they appear after an android/ ignore rule. If the intent was to ignore these specific files, they're already covered by android/. If the intent was to unignore them, they're missing the ! prefix. Additionally, line 44 and 45 are duplicates.
Please clarify the intent and remove or fix these entries.
🤖 Prompt for AI Agents
In @.gitignore around lines 44 - 46, The .gitignore contains
redundant/suspicious entries: the duplicated
"android/app/src/main/java/com/expostarter/base/MainActivity.kt" and "
.github/copilot-instructions.md" after an existing "android/" rule; decide
whether you intended to ignore or unignore these files and fix accordingly: if
you meant to keep them ignored, remove the duplicate and the explicit android
path entries (they're already covered by "android/"); if you meant to unignore
them, prefix each path with "!" (e.g.,
"!android/app/src/main/java/com/expostarter/base/MainActivity.kt" and
"!.github/copilot-instructions.md") and remove the duplicate line; ensure no
duplicate lines remain.
| // Check if module is available | ||
| if (!FaceLandmarks) { | ||
| return { | ||
| emotion: "no_face", | ||
| confidence: 0.0, | ||
| features: { | ||
| ear: 0, | ||
| mar: 0, | ||
| brow_height: 0, | ||
| brow_asymmetry: 0, | ||
| tilt_angle: 0, | ||
| rotation_ratio: 0, | ||
| symmetry_ratio: 0, | ||
| energy_concentration: 0, | ||
| jaw_width: 0, | ||
| brow_variance: 0, | ||
| looking_at_camera: false, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Differentiate “module unavailable” from real “no face”
Returning "no_face" when the module is missing can mask integration issues and mislead downstream UI/analytics. Consider a distinct value (e.g., "error" or "unavailable") so callers can handle this separately.
💡 Suggested tweak
- return {
- emotion: "no_face",
+ return {
+ emotion: "error",
confidence: 0.0,
features: {
ear: 0,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check if module is available | |
| if (!FaceLandmarks) { | |
| return { | |
| emotion: "no_face", | |
| confidence: 0.0, | |
| features: { | |
| ear: 0, | |
| mar: 0, | |
| brow_height: 0, | |
| brow_asymmetry: 0, | |
| tilt_angle: 0, | |
| rotation_ratio: 0, | |
| symmetry_ratio: 0, | |
| energy_concentration: 0, | |
| jaw_width: 0, | |
| brow_variance: 0, | |
| looking_at_camera: false, | |
| }, | |
| }; | |
| } | |
| // Check if module is available | |
| if (!FaceLandmarks) { | |
| return { | |
| emotion: "error", | |
| confidence: 0.0, | |
| features: { | |
| ear: 0, | |
| mar: 0, | |
| brow_height: 0, | |
| brow_asymmetry: 0, | |
| tilt_angle: 0, | |
| rotation_ratio: 0, | |
| symmetry_ratio: 0, | |
| energy_concentration: 0, | |
| jaw_width: 0, | |
| brow_variance: 0, | |
| looking_at_camera: false, | |
| }, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@lib/emotion/detectEmotion.ts` around lines 25 - 44, The early-return in
detectEmotion.ts currently returns emotion: "no_face" when the FaceLandmarks
module is missing; change this sentinel to a distinct value like "unavailable"
(or "error") so callers can differentiate module-unavailable from a true
"no_face" detection, keeping confidence: 0.0 and the same default features
shape; update the return in the FaceLandmarks null-check (the block referencing
FaceLandmarks) and any related tests or callers that expect "no_face" to instead
handle the new "unavailable" value.
1 participant
Summary by CodeRabbit
New Features
Documentation
Improvements
Removed
Chores
✏️ Tip: You can customize this high-level summary in your review settings.