TT-7034 follow the step spell-check setting on mobile Transcribe by gtryus · Pull Request #769 · sillsdev/apm

gtryus · 2026-10-15T15:58:21Z

Summary

With spell check on, the mobile transcription field stays spell-checked both online and offline.
Font data always reports spell check as off, so the step setting is applied when the field renders.

Test plan

PassageDetailTranscribeMobile.cy.tsx: 40 passed

The transcription field stayed unchecked because font data always reports spell check as off, online and offline. Co-authored-by: Cursor <cursoragent@cursor.com> There's now one parser for Transcribe step settings, `parseTranscribeStepSettings` in the new [web/src/crud/transcribeStepSettings.ts](web/src/crud/transcribeStepSettings.ts). Every transcribe-step parse site uses it, and one test file covers it. Typecheck is clean, the full Jest suite passes (1974 passed, 3 skipped), the mobile Cypress spec passes (40/40), and lint has no warnings in the changed files. Nothing is committed. **Before you look at the diff:** your local branch was reset to `f87f35ba`, so `31ddd63c` is now only on `origin`. That reset also removed the project-level spell-check test I added earlier, which was the wrong behaviour anyway. I built on the current `HEAD` and did not pull. **What the parser does** - It accepts a JSON string (online steps) or an object (offline Work Alone steps). - It type-checks the known fields: `artifactTypeId`, `language`, `sisterlanguage`, `font`, `fontSize`, `rtl`, `spellCheck` and `namedRegion`. - `language` can be the `name|bcp47` string or the older object form. - Older steps saved some flags as text: the ASR code was checking `rtl === 'true'`. So `'true'`/`'false'` are read as real booleans. A saved `spellCheck: "false"` therefore means off, rather than falling back to the artifact default. - Any other wrong-typed known field is treated as unset, and unknown keys (the ASR settings) pass through unchanged. - It replaces the old `TranscribeStepSettingsJson` type and the separate ASR settings type, so there's one type. - `resolveStepSpellCheck` still reads only the step's `spellCheck` flag; nothing reads the project setting. **Where it's used** - `crud`: [stepSpellCheck.ts](web/src/crud/stepSpellCheck.ts), [fontChoice.ts](web/src/crud/fontChoice.ts), [transcribeStepAsrSettings.ts](web/src/crud/transcribeStepAsrSettings.ts), [getLwcTranslationAsrSettings.ts](web/src/crud/getLwcTranslationAsrSettings.ts), [useGetAsrSettings.ts](web/src/crud/useGetAsrSettings.ts). - Components: - desktop [Transcriber.tsx](web/src/components/Transcriber.tsx); - [PassageDetailTranscribe.tsx](web/src/components/PassageDetail/PassageDetailTranscribe.tsx); - [PassageDetailTranscribeMobile.tsx](web/src/components/PassageDetail/mobile/transcribe/PassageDetailTranscribeMobile.tsx), six sites; - [BoldClauseTranscriptionEditor.tsx](web/src/components/PassageDetail/lwcTranscription/BoldClauseTranscriptionEditor.tsx); - [PassageDetailLwcTranscription.tsx](web/src/components/PassageDetail/PassageDetailLwcTranscription.tsx); - the step settings editor [TranscribeStepSettings.tsx](web/src/components/StepEditor/TranscribeStepSettings.tsx), 13 sites. - Three local `parseStepSettings` copies are gone. The change is 111 lines added and 186 removed, plus the new module and its test. **Bugs fixed along the way** - `fontChoice` threw on offline Work Alone steps, because it called `JSON.parse` on settings stored as an object. - The step editor crashed on `language.split` when the language was stored in the older object form. **Behaviour change to know about:** when the step settings editor saves, it writes back the parsed settings. A stored `"true"`/`"false"` is rewritten as a real boolean, and any other wrong-typed known field is dropped. **Tests:** all in [transcribeStepSettings.test.ts](web/src/crud/transcribeStepSettings.test.ts), 7 tests. The file failed before the module existed. The text-flag test was added red and went green once the parser read `'true'`/`'false'` as booleans.

nabalone · 2026-10-22T23:24:11Z

+const isObject = (value: unknown): value is Record<string, unknown> =>
+  typeof value === 'object' && value !== null && !Array.isArray(value);
+
+/** Each returns the normalized value, or undefined to drop the field. */
+type Normalize = (value: unknown) => unknown;
+
+const asLanguage: Normalize = (value) =>
+  typeof value === 'string' || isObject(value) ? value : undefined;
+
+const asString: Normalize = (value) =>
+  typeof value === 'string' ? value : undefined;
+
+// Older steps saved some flags as "true" / "false".
+const asBoolean: Normalize = (value) => {
+  if (typeof value === 'boolean') return value;
+  if (value === 'true') return true;
+  if (value === 'false') return false;
+  return undefined;
+};


Should we move these to a more general place so that in the future we can use them for other steps? I added an item to the Priority Refactor List to do a broader centralization of JSON parsing and Orbit decoding. We could wait until then, but if we move them to a more central location now we could start using them elsewhere if opportunity arises.

We could also consider using lodash isPlainObject and isString since lodash is already a dependency, but I'm okay with keeping these for now. I've put in the refactor list that we should consider switching to zod

nabalone · 2026-10-22T23:53:59Z

-      language?: unknown;
-    };
-    const { bcp47: stepLang } = parseStepLanguageField(lgSettings?.language);
+    const lgSettings = parsedSettings;


Can we just use parsedSettings and get rid of lgSettings?

nabalone · 2026-10-23T00:12:53Z

    );
  const asrTip = useMemo(() => {
-    const step = parseStepSettings(stepSettings);
+    const step = parseTranscribeStepSettings(stepSettings);


We call parseTranscribeStepSettings(stepSettings) twice in this file, we could cover both with a memoized parsedSettings like you did in PassageDetailTranscribe.tsx. I also prefer "parsedSettings" or "settings", what does "lg" means in "lgSettings"?

nabalone · 2026-10-23T00:43:01Z

+    [stepSettings]
+  );

  const artifactTypeId = parsedSettings?.artifactTypeId ?? null;


Now that parsedSettings is always an object, we can get rid of the ? and other falsiness guards that don't do anything anymore. Also at:

PassageDetailTranscribeMobile.tsx L547-L549

PassageDetailTranscribeMobile.tsx L998

BoldClauseTranscriptionEditor.tsx L225-L240

TranscribeStepSettings.tsx L250

TranscribeStepSettings.tsx L301

TranscribeStepSettings.tsx L486 (json &&)

getLwcTranslationAsrSettings.ts L40

useGetAsrSettings.ts L159

nabalone · 2026-10-23T00:53:52Z

    let cancelled = false;

-    const lgSettings = parseStepSettings(stepSettings);
+    const lgSettings = parseTranscribeStepSettings(stepSettings);


If you decide to implement my earlier suggestions, can we rename this one to parsedSettings or settings also? And also on Transcriber.tsx L744

nabalone · 2026-10-23T01:27:03Z

Still in the process of reviewing

nabalone · 2026-10-23T20:11:53Z

Since this PR is about following the step spell-check setting: this editor decides spell check with lgSettings?.spellCheck === true (L240) instead of resolveStepSpellCheck, so when the step has no spellCheck saved it skips the artifact default. For Careful Speech that's the same result (default off), but for a Phrase Back Translation bold-clause step the default is on, so this editor shows spell check off while Transcriber and mobile Transcribe show it on. Could we switch it to resolveStepSpellCheck(lgSettings, <artifact slug>, stepLang) so all three surfaces agree? Small thing, but it's one line in a file you're already editing.

nabalone · 2026-10-23T20:11:59Z

Wow, this is a really great cleanup!

nabalone

I think this is a really good fix and direction to go. I added a bunch of comments but they are mostly trivial things that an AI should be able to do all in one sweep; it just looked like your AI may have been trying hard to keep the diff small whereas I think it would be worth it to make some more clarity changes. I can also tell my AI to do them as a follow-up PR if you would prefer.

nabalone · 2026-10-23T20:12:06Z

@@ -49,13 +39,11 @@ export function defaultSpellCheckForArtifact(

 /** Workflow step `tool.settings` is authoritative for Transcriber spell check. */
 export function resolveStepSpellCheck(


How about calling this isStepSpellCheckOn or something like that? When I first saw a usage I could not tell what the function was doing/what it would return. Sorry, I know this is not part of the PR, but if you agree and can do it easily while you are editing here I think it could be helpful

nabalone · 2026-10-23T20:12:06Z

-    return !!settings.spellCheck;
-  }
+  if (settings.spellCheck !== undefined) return settings.spellCheck;
  return defaultSpellCheckForArtifact(artifactTypeSlug, bcp47, availLangs);


How about naming defaultSpellCheckForArtifact isSpellCheckOnByDefault or something like that? Sorry, I know this is not part of the PR, but if you agree and can do it easily while you are editing here I think it could be helpful

nabalone · 2026-10-23T20:12:06Z

+ * offline Work Alone steps store an object. Settings come from persisted Orbit
+ * records, so a known field with an unusable type is treated as unset.
+ */
+export function parseTranscribeStepSettings(


Now that bad settings no longer crash anything and silently become {}, a corrupted step looks exactly like a vernacular step with no settings, and we'd never find out. Could we add some error logging so we hear about it? Suggestions:

In the catch (malformed JSON) and the !isObject(parsed) exit (e.g. "null", [1]), skipping undefined/'' since those just mean "no settings". Including a truncated copy of the raw input would help us see what was stored.

When a known field has a value but normalizes to undefined (wrong type), log the key and its type. Not for "true"/"false" (expected legacy data) and probably not for null. If this turns out noisy it could be Severity.info (breadcrumb only).

Since this is a plain function, it could import bugsnagClient directly like launchAudacity.ts does: logError(Severity.error, bugsnagClient, ...). That also writes to the local log when offline, which covers Work Alone users.

This runs in render memos and per-step loops (e.g. the vernacularSteps filters), so it should only log each distinct bad input once (a module-level Set), or one bad record will flood the logs.

Two related spots:

TranscribeStepSettings.tsx emitSettingsChange: this is the one place bad data becomes lost data, since saving replaces settings we couldn't read. If toolSettings is non-empty but parsed to {}, logging the original string before we overwrite it would let us recover it.

fontChoice.ts L107: the outer tool string is still an unguarded JSON.parse, so a malformed one still throws here even though the inner settings parse is now safe. Could we guard and log it the same way?

nabalone · 2026-10-23T20:12:06Z

+  return undefined;
+};
+
+const fieldNormalizers: Partial<Record<string, Normalize>> = {


Since this PR introduced reading legacy "true"/"false" flags as booleans, should phonetic and selectRoman go in this table too (asBoolean, and on the interface)? They're stored the same way, and their readers still check for the string themselves (transcribeStepAsrSettings.ts L86, L295-L302, L306, TranscribeStepSettings.tsx L250, L487), the same pattern this PR already removed for rtl. With them in the table those checks become === true and stepPhoneticIsSet becomes !== undefined. It would also mean saving in the step editor normalizes these like it now does rtl/spellCheck, instead of leaving mixed forms in the data. Nothing's broken today since every reader handles both forms, so I'm fine with this as a follow-up if you'd rather keep the PR from growing.

nabalone · 2026-10-23T20:12:06Z

-      setInitialValue(json.artifactTypeId);
-      const { languageName, bcp47 } = parseStepLanguageField(json?.language);
-      const slug = slugFromId(json.artifactTypeId);
+      const json = parseTranscribeStepSettings(toolSettings);


How about "settings" or "parsedSettings" instead of "json" for the variable name? It's not JSON anymore at this point, it's the parsed settings object. (Same for the other const json = parseTranscribeStepSettings(...) lines in this file.)

nabalone · 2026-10-23T20:12:06Z

+    it('spellchecks the transcription when the step spell check setting is on', () => {
+      const workflowSteps = [
+        {
+          id: 'step-transcribe',
+          type: 'orgworkflowstep',
+          attributes: {
+            sequencenum: 1,
+            tool: JSON.stringify({
+              tool: 'transcribe',
+              settings: JSON.stringify({ spellCheck: true }),
+            }),
+          },
+        },
+      ];
+
+      mountTranscribeMobile({
+        currentstep: 'step-transcribe',
+        workflowSteps,
+        offline: false,
+      });
+
+      cy.get('#transcriptionText', { timeout: 10000 }).should(
+        'have.attr',
+        'spellcheck',
+        'true'
+      );
+    });
+
+    it('spellchecks the transcription offline when the step spell check setting is on', () => {
+      const workflowSteps = [
+        {
+          id: 'step-transcribe',
+          type: 'orgworkflowstep',
+          attributes: {
+            sequencenum: 1,
+            tool: JSON.stringify({
+              tool: 'transcribe',
+              settings: JSON.stringify({ spellCheck: true }),
+            }),
+          },
+        },
+      ];
+
+      mountTranscribeMobile({
+        currentstep: 'step-transcribe',
+        workflowSteps,
+        offline: true,
+      });
+
+      cy.get('#transcriptionText', { timeout: 10000 }).should(
+        'have.attr',
+        'spellcheck',
+        'true'
+      );
+    });


This looks like it is asking for the flip test, that we don't spellcheck when spellcheck has been set off. But also it seems like spellcheck is not a super important feature, so if you don't think it's worth it or if you think it will take too much time, then okay

gtryus marked this pull request as draft October 15, 2026 16:14

gtryus force-pushed the TT-7034-step-spellcheck branch from 31ddd63 to 9c10239 Compare October 20, 2026 20:08

gtryus force-pushed the TT-7034-step-spellcheck branch from 9c10239 to c7035b6 Compare October 20, 2026 20:15

gtryus requested a review from nabalone October 20, 2026 20:18

gtryus marked this pull request as ready for review October 20, 2026 20:18

nabalone reviewed Oct 22, 2026

View reviewed changes

nabalone reviewed Oct 23, 2026

View reviewed changes

nabalone requested changes Oct 23, 2026

View reviewed changes

		@@ -49,13 +39,11 @@ export function defaultSpellCheckForArtifact(

		/** Workflow step `tool.settings` is authoritative for Transcriber spell check. */
		export function resolveStepSpellCheck(

Conversation

gtryus commented Oct 15, 2026

Summary

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabalone Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabalone Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabalone commented Oct 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabalone commented Oct 23, 2026

Uh oh!

nabalone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nabalone Oct 23, 2026 •

edited

Loading

nabalone Oct 23, 2026 •

edited

Loading