Skip to content

Conversation

hahn-kev
Copy link
Collaborator

@hahn-kev hahn-kev commented Sep 3, 2025

Changed the audio recording dialog,
Before:
image

After:
image

above the recording area we display a readonly editor for the related object (example, sense, entry) and we only display the field that we're providing audio for (Definition in this case). In the title of the dialog is the field, and writing system that audio is being recorded for, not very useful here, but if there's multiple audio writing systems then it's very helpful.

Note that we're recording for Thai, but we're also displaying the Thai audio (which has no audio), this is to support the case where you're making a new recording of existing audio, you can easily compare your new recording with the old one, before you commit to replacing it.

Copy link

coderabbitai bot commented Sep 3, 2025

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch audio-recording-context

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the 💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related label Sep 3, 2025
Copy link

github-actions bot commented Sep 3, 2025

UI unit Tests

  1 files  ±0   41 suites  ±0   18s ⏱️ -1s
 90 tests ±0   90 ✅ ±0  0 💤 ±0  0 ❌ ±0 
124 runs  ±0  124 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 0fd4ad1. ± Comparison against base commit ac1ac8a.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Sep 3, 2025

C# Unit Tests

130 tests  ±0   130 ✅ ±0   19s ⏱️ ±0s
 20 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 0fd4ad1. ± Comparison against base commit ac1ac8a.

♻️ This comment has been updated with latest results.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/viewer/src/lib/components/audio/AudioDialog.svelte (1)

55-66: Surface upload errors to users and prevent double-submit

Currently thrown errors bubble with no user feedback. Catch and toast; also disable while submitting.

 async function submitAudio() {
-  if (!selectedFile) throw new Error('No audio to upload');
+  if (!selectedFile) {
+    AppNotification.display($t`No audio to upload`, { type: 'error' });
+    return;
+  }
   submitting = true;
-  try {
+  try {
     const audioId = await uploadAudio();
-    onSubmit(audioId);
+    await onSubmit(audioId);
     close();
+  } catch (err) {
+    const message = err instanceof Error ? err.message : $t`Unknown error`;
+    AppNotification.display($t`Failed to save audio: ${message}`, { type: 'error' });
   } finally {
     submitting = false;
   }
 }

Additionally, update the Save button (outside this hunk) to also disable during submit:

<Button onclick={() => submitAudio()} disabled={submitting || tooBig || !finalAudio} loading={submitting}>
  {$t`Save audio`}
</Button>
🧹 Nitpick comments (22)
frontend/viewer/src/lib/layout/DevToolsDialog.svelte (1)

9-12: Guard API usage and optionally batch-create in parallel

  • Add a quick guard for missing api to avoid a runtime if Dev Tools is opened without a project.
  • Minor: for dev productivity, consider parallelizing creation.
 export async function generateEntries(n: number) {
-  for (let i = 0; i < n; i++) {
-    const entry = defaultEntry();
-    const vWsId = writingSystems.defaultVernacular?.wsId;
-    if (vWsId) entry.citationForm[vWsId] = `*Test ${Math.random().toString(36).substring(2, 7)}`;
-    await projectContext.api.createEntry(entry);
-  }
+  if (!projectContext.maybeApi) return; // or show a toast
+  const tasks: Promise<unknown>[] = [];
+  for (let i = 0; i < n; i++) {
+    const entry = defaultEntry();
+    const vWsId = writingSystems.defaultVernacular?.wsId;
+    if (vWsId) entry.citationForm[vWsId] = `*Test ${Math.random().toString(36).substring(2, 7)}`;
+    tasks.push(projectContext.api.createEntry(entry));
+  }
+  await Promise.all(tasks);
 }

Also applies to: 20-27

frontend/viewer/src/project/browse/BrowseView.svelte (1)

21-24: Right pane renders empty when API is unavailable—show a fallback

If projectContext.maybeApi is falsy and an entry is selected, the pane is empty. Consider a lightweight message.

-          {:else if projectContext.maybeApi}
+          {:else if projectContext.maybeApi}
             <div class="md:p-4 md:pl-4 h-full">
               <EntryView
                 entryId={selectedEntryId.current}
                 onClose={() => (selectedEntryId.current = '')}
                 showClose={IsMobile.value}
               />
             </div>
+          {:else}
+            <div class="flex items-center justify-center h-full text-muted-foreground text-center m-2">
+              <p>{$t`Project API unavailable. Please try again or reload.`}</p>
+            </div>
           {/if}

Also applies to: 106-114

frontend/viewer/src/lib/components/audio/AudioDialog.svelte (1)

14-24: Allow async onSubmit and await it before closing

Let onSubmit be async-capable so parents can persist updates before the dialog closes.

-  let {
+  let {
     open = $bindable(false),
     title = undefined,
-    onSubmit = () => {},
+    onSubmit = () => {},
     children = undefined
   } : {
     open: boolean,
     title?: string,
-    onSubmit?: (audioId: string) => void,
+    onSubmit?: (audioId: string) => void | Promise<void>,
     children?: Snippet
   } = $props();
   try {
     const audioId = await uploadAudio();
-    onSubmit(audioId);
+    await onSubmit(audioId);
     close();
   } finally {
     submitting = false;
   }

Also applies to: 61-62

frontend/viewer/src/lib/components/field-editors/audio-input.svelte (1)

226-230: Explicitly close the dialog on submit (defensive).

If AudioDialog doesn’t auto-close after onSubmit in all paths, the open state could remain true.

 function onAudioDialogSubmit(newAudioId: string) {
   audioId = newAudioId;
   onchange(newAudioId);
+  audioDialogOpen = false;
 }
frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte (3)

38-48: Avoid accidentally rendering the class "true".

If $currentView.fields.sentence.show can be boolean, class={cn(show || 'hidden')} may yield class="true". Prefer an explicit ternary.

Apply:

-  <Editor.Field.Root fieldId="sentence" class={cn($currentView.fields.sentence.show || 'hidden')}>
+  <Editor.Field.Root fieldId="sentence" class={cn($currentView.fields.sentence.show ? undefined : 'hidden')}>

49-58: Same class boolean-guard nit as above.

-  <Editor.Field.Root fieldId="translation" class={cn($currentView.fields.translation.show || 'hidden')}>
+  <Editor.Field.Root fieldId="translation" class={cn($currentView.fields.translation.show ? undefined : 'hidden')}>

61-70: Use Svelte’s class directive for boolean-guard
Replace

class={cn($currentView.fields.reference.show || 'hidden')}

with

class:hidden={!$currentView.fields.reference.show}
frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte (7)

44-54: Field wiring LGTM; apply the class boolean-guard nit.

-  <Editor.Field.Root fieldId="lexemeForm" class={cn($currentView.fields.lexemeForm.show || 'hidden')}>
+  <Editor.Field.Root fieldId="lexemeForm" class={cn($currentView.fields.lexemeForm.show ? undefined : 'hidden')}>

56-65: Citation form block looks good; same class nit.

-  <Editor.Field.Root fieldId="citationForm" class={cn($currentView.fields.citationForm.show || 'hidden')}>
+  <Editor.Field.Root fieldId="citationForm" class={cn($currentView.fields.citationForm.show ? undefined : 'hidden')}>

68-76: Complex forms section LGTM; same class nit.

-  <Editor.Field.Root fieldId="complexForms" class={cn($currentView.fields.complexForms.show || 'hidden')}>
+  <Editor.Field.Root fieldId="complexForms" class={cn($currentView.fields.complexForms.show ? undefined : 'hidden')}>

78-87: Components section LGTM; same class nit.

-  <Editor.Field.Root fieldId="components" class={cn($currentView.fields.components.show || 'hidden')}>
+  <Editor.Field.Root fieldId="components" class={cn($currentView.fields.components.show ? undefined : 'hidden')}>

89-101: Complex form types: consider minor perf hoists.

The labelSelector calls pickBestAlternative per option each render. If options are stable, memoization/hoisting can cut work. Also apply the class nit.

-  <Editor.Field.Root fieldId="complexFormTypes" class={cn($currentView.fields.complexFormTypes.show || 'hidden')}>
+  <Editor.Field.Root fieldId="complexFormTypes" class={cn($currentView.fields.complexFormTypes.show ? undefined : 'hidden')}>

Outside this hunk, you can hoist:

const analysisWs = writingSystemService.viewAnalysis($currentView);
const vernacularWs = writingSystemService.viewVernacular($currentView);

…and reuse analysisWs/vernacularWs where referenced.


104-113: Literal meaning block LGTM; same class nit.

-  <Editor.Field.Root fieldId="literalMeaning" class={cn($currentView.fields.literalMeaning.show || 'hidden')}>
+  <Editor.Field.Root fieldId="literalMeaning" class={cn($currentView.fields.literalMeaning.show ? undefined : 'hidden')}>

115-124: Note block LGTM; same class nit.

-  <Editor.Field.Root fieldId="note" class={cn($currentView.fields.note.show || 'hidden')}>
+  <Editor.Field.Root fieldId="note" class={cn($currentView.fields.note.show ? undefined : 'hidden')}>
frontend/viewer/src/lib/entry-editor/object-editors/LexiconEditorPrimitive.svelte (1)

14-20: Render routing by type guards is clean.

Optional: Provide a tiny fallback (e.g., empty state) for unexpected values to aid debugging in the audio dialog.

 {:else if isExample(object)}
   <ExampleEditorPrimitive example={object} readonly />
 {:else}
-{/if}
+  <!-- no object / unknown type -->
+{/if}
frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte (3)

41-50: Gloss field wiring LGTM; apply the class boolean-guard nit.

-  <Editor.Field.Root fieldId="gloss" class={cn($currentView.fields.gloss.show || 'hidden')}>
+  <Editor.Field.Root fieldId="gloss" class={cn($currentView.fields.gloss.show ? undefined : 'hidden')}>

52-61: Definition field LGTM; same class nit.

-  <Editor.Field.Root fieldId="definition" class={cn($currentView.fields.definition.show || 'hidden')}>
+  <Editor.Field.Root fieldId="definition" class={cn($currentView.fields.definition.show ? undefined : 'hidden')}>

79-91: Semantic domains LGTM; same class nit.

-  <Editor.Field.Root fieldId="semanticDomains" class={cn($currentView.fields.semanticDomains.show || 'hidden')}>
+  <Editor.Field.Root fieldId="semanticDomains" class={cn($currentView.fields.semanticDomains.show ? undefined : 'hidden')}>
frontend/viewer/src/lib/entry-editor/object-editors/subject-context.ts (2)

4-8: Consider generic typing for stronger narrowing.

Making EditorPrimitiveSubject generic preserves the concrete subject type for consumers.

-interface EditorPrimitiveSubject {
-  readonly current: IEntry | ISense | IExampleSentence | undefined
-}
-export const subjectContext = new Context<EditorPrimitiveSubject>('subject-context');
+type Subject = IEntry | ISense | IExampleSentence;
+interface EditorPrimitiveSubject<T extends Subject = Subject> {
+  readonly current: T | undefined;
+}
+export const subjectContext = new Context<EditorPrimitiveSubject>('subject-context');

9-17: Tighten/relax types and drop unnecessary optional call.

subject is required here, so subject?.() is unnecessary. Optionally allow undefined subjects to make the API reusable in wrappers.

-export function initSubjectContext(subject: () => IEntry | ISense | IExampleSentence) {
+export function initSubjectContext(subject: () => IEntry | ISense | IExampleSentence | undefined) {
   const editorPrimitiveSubject: EditorPrimitiveSubject = {
     get current() {
-      return subject?.();
+      return subject();
     }
   };
   subjectContext.set(editorPrimitiveSubject);
   return editorPrimitiveSubject;
 }
frontend/viewer/src/lib/components/editor/field/field-root.svelte (2)

43-46: Prop-to-state bridge: keep, but tiny clarity tweak optional

The $effect correctly syncs the external prop into context state. If you want to avoid any TDZ confusion for readers, destructure props before registering the effect (no functional change).

-const fieldProps = usesFieldRoot(new FieldRootState(fieldLabelId));
-$effect(() => {
-  fieldProps.fieldId = fieldId;
-});
+const fieldProps = usesFieldRoot(new FieldRootState(fieldLabelId));
+// after props destructure below, this will run with the correct value
+$effect(() => {
+  fieldProps.fieldId = fieldId;
+});

57-57: Use Svelte style directive for grid-area
Replace the inline string style so the declaration is omitted when fieldId is undefined:

-<div style="grid-area: {fieldId}" class={cn(...)} {...restProps}>
+<div style:grid-area={fieldId} class={cn(...)} {...restProps}>

All FieldId values (e.g. lexemeForm, definition, sentence, etc.) are camelCase keys and conform to CSS custom-ident rules, so no additional sanitization is needed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ac1ac8a and 00c66bd.

📒 Files selected for processing (15)
  • frontend/viewer/src/lib/DialogsProvider.svelte (0 hunks)
  • frontend/viewer/src/lib/components/audio/AudioDialog.svelte (4 hunks)
  • frontend/viewer/src/lib/components/editor/field/field-root.svelte (2 hunks)
  • frontend/viewer/src/lib/components/editor/field/field-title.svelte (1 hunks)
  • frontend/viewer/src/lib/components/field-editors/audio-input.svelte (4 hunks)
  • frontend/viewer/src/lib/components/field-editors/multi-ws-input.svelte (1 hunks)
  • frontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.svelte (1 hunks)
  • frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte (8 hunks)
  • frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte (4 hunks)
  • frontend/viewer/src/lib/entry-editor/object-editors/LexiconEditorPrimitive.svelte (1 hunks)
  • frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte (5 hunks)
  • frontend/viewer/src/lib/entry-editor/object-editors/subject-context.ts (1 hunks)
  • frontend/viewer/src/lib/layout/DevToolsDialog.svelte (2 hunks)
  • frontend/viewer/src/lib/services/dialogs-service.ts (0 hunks)
  • frontend/viewer/src/project/browse/BrowseView.svelte (2 hunks)
💤 Files with no reviewable changes (2)
  • frontend/viewer/src/lib/DialogsProvider.svelte
  • frontend/viewer/src/lib/services/dialogs-service.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-30T04:53:41.702Z
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1844
File: frontend/viewer/src/lib/entry-editor/ItemListItem.svelte:26-37
Timestamp: 2025-07-30T04:53:41.702Z
Learning: In frontend/viewer/src/lib/entry-editor/ItemListItem.svelte, the TODO comments for unused props `index` and `actions` are intentional reminders for future work to be completed in a separate PR, not issues to be resolved immediately. These represent planned functionality that will be implemented later.

Applied to files:

  • frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte
📚 Learning: 2025-07-24T03:26:59.388Z
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1841
File: frontend/viewer/src/project/browse/filter/OpFilter.svelte:10-17
Timestamp: 2025-07-24T03:26:59.388Z
Learning: In Svelte 5, reactive statements use `$derived()` instead of the Svelte 4 `$:` syntax. For example, to make an array reactive to translation changes, use `const ops = $derived([...])` instead of `$: ops = [...]`.

Applied to files:

  • frontend/viewer/src/lib/components/editor/field/field-title.svelte
📚 Learning: 2025-08-14T12:50:25.135Z
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1906
File: frontend/viewer/src/lib/components/ui/dialog-shared/dialog-shared-root.svelte:3-3
Timestamp: 2025-08-14T12:50:25.135Z
Learning: In the dialog-shared-root.svelte component, the module-level `openDialogs` state is intentionally shared across all component instances to coordinate dialog stacking and overlay behavior across the entire application. This enables proper z-index management where newer dialogs appear on top and only the bottom dialog shows its overlay.

Applied to files:

  • frontend/viewer/src/lib/components/audio/AudioDialog.svelte
🧬 Code graph analysis (1)
frontend/viewer/src/lib/entry-editor/object-editors/subject-context.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IExampleSentence.ts (1)
  • IExampleSentence (10-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: Build UI / publish-ui
  • GitHub Check: Build API / publish-api
  • GitHub Check: check-and-lint
  • GitHub Check: Analyze (csharp)
  • GitHub Check: Build FW Lite and run tests
  • GitHub Check: frontend-component-unit-tests
🔇 Additional comments (19)
frontend/viewer/src/lib/components/editor/field/field-title.svelte (1)

20-22: Confirm single source of truth for label to avoid feedback loops

Setting stateProps.label = label in an effect is fine; just verify useFieldTitle() doesn’t also derive/mutate label internally, which could create a subtle loop. Nice use of Svelte 5 runes per our prior learning.

frontend/viewer/src/lib/components/audio/AudioDialog.svelte (2)

87-90: Good: validate mediaUri before proceeding

Explicitly throwing when mediaUri is missing prevents downstream null/undefined bugs.


136-139: Public API: configurable title and header children look solid

The default title fallback and header snippet injection fit the PR’s “show context while recording” goal.

frontend/viewer/src/lib/components/field-editors/audio-input.svelte (4)

54-59: Imports and new dependencies look correct.

The new dialog and subject-context primitives are wired in cleanly.


66-73: New wsLabel prop: API addition LGTM.

Prop typing matches upstream usage; no issues spotted.


275-284: Conditional rendering of AudioDialog is appropriate.

Gated by supportsAudio and readonly; good UX boundary.


286-291: Actions correctly open the recorder.

Add/Replace routes now bind to the local dialog state as intended.

Also applies to: 344-345

frontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.svelte (1)

80-81: Pass-through of wsLabel is correct.

Prop aligns with AudioInput API; no issues.

frontend/viewer/src/lib/components/field-editors/multi-ws-input.svelte (1)

55-56: Pass-through of wsLabel is correct.

Matches the new AudioInput prop; consistent with rich-multi variant.

frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte (1)

13-13: Subject-context initialization looks correct.

initSubjectContext(() => example) cleanly exposes the active example to consumers. No issues.

Also applies to: 30-30

frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte (1)

16-16: Subject-context initialization for entry is solid.

Also applies to: 36-36

frontend/viewer/src/lib/entry-editor/object-editors/LexiconEditorPrimitive.svelte (1)

1-13: Prop typing and imports look correct for the union use-case.

frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte (2)

15-15: Subject-context initialization for sense is correct.

Also applies to: 34-35


63-77: Select wiring correctly syncs partOfSpeechId.

Minor: If Select can emit rapid changes, consider batching assignment + onFieldChanged inside a microtask to avoid intermediate observers firing (only if you’ve seen jitter). Otherwise, LGTM.

frontend/viewer/src/lib/entry-editor/object-editors/subject-context.ts (1)

19-21: Getter exposure via useSubjectContext is straightforward.

If multiple primitives are nested (Entry → Sense → Example), confirm runed Context scoping behaves as expected (innermost wins) to ensure the audio dialog reads the most specific subject.

frontend/viewer/src/lib/components/editor/field/field-root.svelte (4)

3-15: State class + nominal branding look solid

Good use of a branded class with $state runes to enforce construction and provide reactive fields. No issues spotted.


23-23: Type aliases aligned

Aliases updated to the new state type; consistent with the API shift.

Also applies to: 29-29


40-40: Public props shape is clear

Adding optional fieldId to props aligns with the new layout needs. Looks good.


17-20: No breaking-change risk: all existing calls already pass FieldRootState instances

The only usage of usesFieldRoot is within this file and invokes it with new FieldRootState(...), so no callers pass plain object literals.

Copy link

argos-ci bot commented Sep 3, 2025

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ✅ No changes detected - Sep 8, 2025, 1:53 PM

@imnasnainaec
Copy link
Collaborator

Note that we're recording for Thai, but we're also displaying the Thai audio (which has no audio), this is to support the case where you're making a new recording of existing audio, you can easily compare your new recording with the old one, before you commit to replacing it.

@hahn-kev Can you add a screenshot for what it looks like when there is audio that is going to be replaced? Is it clear that the audio will be replaced (as opposed to having multiple recordings)?

@hahn-kev
Copy link
Collaborator Author

hahn-kev commented Sep 8, 2025

@imnasnainaec the UI doesn't look different in the recording page. But to get there they have to click "Replace Audio", so that should make it clear

@imnasnainaec
Copy link
Collaborator

@hahn-kev Presumably it won't say "No audio" when there is audio. I was wondering how the audio is displayed.

Copy link
Collaborator

@myieye myieye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 👍

I had put some effort into optimizing the context we show, so I put that in a draft PR.
Perhaps Sara will get to decide if that's worth merging too 😄. Obviously I'm also curious to hear any thoughts you have on it.

@hahn-kev
Copy link
Collaborator Author

hahn-kev commented Sep 9, 2025

@imnasnainaec here's what replacing existing audio looks like
image

I'm going to merge this in but I'm still interested in what you think.

@hahn-kev hahn-kev merged commit 3ac6de7 into develop Sep 9, 2025
26 of 27 checks passed
@hahn-kev hahn-kev deleted the audio-recording-context branch September 9, 2025 02:50
@imnasnainaec
Copy link
Collaborator

I'm going to merge this in but I'm still interested in what you think.

Ah, nice. And good use of space having the language abbrev. and corresponding gloss on the same line.

In both the definition screenshot at top and this gloss screenshot, it feels a bit redundant to have the word "Definition" or "Gloss" at the start of both the title and the first line after the title. It appears that the second occurrence is only for the sake of the ? help icon. I guess that's <Editor.Field.Title name={$tGloss} helpId={fieldData.gloss.helpId} />. Perhaps the title could be something like "Add audio gloss for: Tha (A)" or "Record audio definition: Tha (A)"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants